# Nitpicking my code (7GUIs temperature converter)

**URL:** <https://discourse.elm-lang.org/t/nitpicking-my-code-7guis-temperature-converter/8964>\
**Category:** Learn\
**Created:** [February 25, 2023, 7:31am UTC](https://discourse.elm-lang.org/t/nitpicking-my-code-7guis-temperature-converter/8964 "2023-02-25T07:31:37Z")\
**Posts on this page:** 10\
**Page:** 1

<div class="post-metadata">

**Author:** ![benjamin-thomas](https://yyz1.discourse-cdn.com/flex035/user_avatar/discourse.elm-lang.org/benjamin-thomas/32/3869_2.png) [@benjamin-thomas](https://discourse.elm-lang.org/u/benjamin-thomas)\
**Post date:** [February 25, 2023, 7:31am UTC](https://discourse.elm-lang.org/t/nitpicking-my-code-7guis-temperature-converter/8964/1 "2023-02-25T07:31:37Z")

</div>

EDIT: original title was “Looking for a function similar to Maybe.map2”

* * *

Hello!

I’m implementing a “Temperature Converter” as an exercise, see: [7GUIs](https://eugenkiss.github.io/7guis/tasks).

I’m bothered by this `warnOnInvalidInput` function:

> <https://github.com/benjamin-thomas/7guis/blob/44431637c515e4758779f3396001614e5be39d54/elm/temp_converter/src/Main.elm#L77>

I would like to remove the duplication and collapse the logic such that returning an empty node is specified only once.

Regarding the function I’m looking, I notice that its “shape” is similar to `Maybe.map2`. But I need the inverse logic on the second branch.

```auto
map2 : (a -> b -> value) -> Maybe a -> Maybe b -> Maybe value
map2 func ma mb =
  case ma of
    Nothing ->
      Nothing

    Just a ->
      case mb of
        Nothing ->
          Nothing

        Just b ->
          Just (func a b)

```

I’m interested in improving at recognizing patterns so I’m wondering if such a function exists?

Failing that, would you have a suggestion to improve this function? I’m thinking I may be missing something here.

Thanks 🙂

---

<div class="post-metadata">

**Author:** ![wolfadex](https://yyz1.discourse-cdn.com/flex035/user_avatar/discourse.elm-lang.org/wolfadex/32/2461_2.png) [@wolfadex](https://discourse.elm-lang.org/u/wolfadex)\
**Post date:** [February 25, 2023, 1:47pm UTC](https://discourse.elm-lang.org/t/nitpicking-my-code-7guis-temperature-converter/8964/3 "2023-02-25T13:47:12Z")

</div>

I would leave it as is. If you try and shorten it more I fear you might make it more difficult to read later. It definitely is similar to Maybe.map2, but because you’re inversing the second Maybe I feel like that helper function would be complex to understand.

---

<div class="post-metadata">

**Author:** ![benjamin-thomas](https://yyz1.discourse-cdn.com/flex035/user_avatar/discourse.elm-lang.org/benjamin-thomas/32/3869_2.png) [@benjamin-thomas](https://discourse.elm-lang.org/u/benjamin-thomas)\
**Post date:** [February 26, 2023, 1:24pm UTC](https://discourse.elm-lang.org/t/nitpicking-my-code-7guis-temperature-converter/8964/4 "2023-02-26T13:24:12Z")

</div>

> [@wolfadex](#):
>
> I would leave it as is. If you try and shorten it more I fear you might make it more difficult to read later.

Thanks for the feedback @wolfadex

For sure, readability can suffer by abstracting too much, I’ll keep that in mind 🙂

---

<div class="post-metadata">

**Author:** ![IloSophiep](https://yyz1.discourse-cdn.com/flex035/user_avatar/discourse.elm-lang.org/ilosophiep/32/3489_2.png) [@IloSophiep](https://discourse.elm-lang.org/u/IloSophiep)\
**Post date:** [February 26, 2023, 2:26pm UTC](https://discourse.elm-lang.org/t/nitpicking-my-code-7guis-temperature-converter/8964/5 "2023-02-26T14:26:37Z")

</div>

One simple change to your code would be to assign the “default result” to a variable and reuse that? That would at least indicate your intention of having the same value in these two out of three branches?

---

<div class="post-metadata">

**Author:** ![dta](https://yyz1.discourse-cdn.com/flex035/user_avatar/discourse.elm-lang.org/dta/32/1917_2.png) [@dta](https://discourse.elm-lang.org/u/dta)\
**Post date:** [February 26, 2023, 3:19pm UTC](https://discourse.elm-lang.org/t/nitpicking-my-code-7guis-temperature-converter/8964/6 "2023-02-26T15:19:47Z")

</div>

Rather than trying to combine all of the logic into one operator, I would probably factor out each piece into is own function and use a pipeline. Something like this:

```auto
warnOnInvalidInput : String -> Maybe String -> Html msg
warnOnInvalidInput name ms =
    ms
        |> Maybe.Extra.filter isNotBlank
        |> Maybe.Extra.filter isInvalid
        |> Maybe.Extra.isJust
        |> warningMessage name

warningMessage : String -> Bool -> Html msg
warningMessage name invalidInput =
    if invalidInput then
        p [] [text ("Invalid " ++ name ++ " input")]

    else
        text ""

isNotBlank : String -> Bool
isNotBlank s =
    if String.trim s == "" then
        False

    else
        True

isInvalid : String -> Bool
isInvalid s =
    case String.toFloat s of
        Nothing ->
            True

        Just _ ->
            False

```

---

<div class="post-metadata">

**Author:** ![miniBill](https://yyz1.discourse-cdn.com/flex035/user_avatar/discourse.elm-lang.org/minibill/32/800_2.png) [@miniBill](https://discourse.elm-lang.org/u/miniBill)\
**Post date:** [February 26, 2023, 3:52pm UTC](https://discourse.elm-lang.org/t/nitpicking-my-code-7guis-temperature-converter/8964/7 "2023-02-26T15:52:39Z")

</div>

@dta just a couple tips:

1. `isNotBlank` → the `if` is unneeded, just write `String.trim s /= ""`
2. `isInvalid` → same, just write `String.toFloat s == Nothing`

---

<div class="post-metadata">

**Author:** ![benjamin-thomas](https://yyz1.discourse-cdn.com/flex035/user_avatar/discourse.elm-lang.org/benjamin-thomas/32/3869_2.png) [@benjamin-thomas](https://discourse.elm-lang.org/u/benjamin-thomas)\
**Post date:** [March 1, 2023, 4:53am UTC](https://discourse.elm-lang.org/t/nitpicking-my-code-7guis-temperature-converter/8964/8 "2023-03-01T04:53:13Z")

</div>

Thanks for the feedback.

@IloSophiep I didn’t quite get what you had in mind.

@dta I liked your suggestion but then bugged by the fact that we would then apply `String.toFloat` to the same input twice.

I played with this problem in different contexts, and came to the conclusion (I think) that what bugged me was indicative of a code smell.

It seems that in Elm, when doing contorsions, the solution may be to rethink the `Model`.

I’m much happier with this latest version:

> <https://github.com/benjamin-thomas/7guis/blob/8d09f8691447c2bb81daef793d6cf3b79742f120/elm/temp_converter/src/Main.elm#L40>

The only thing that bothers me now is this function that does too much:

```auto
valueFor : Model -> { celsius : String, fahrenheit : String }
valueFor model =
    case model of
        NoUserInput ->
            { celsius = "", fahrenheit = "" }

        Converted (Celsius celsius) (Fahrenheit fahrenheit) ->
            { celsius = celsius, fahrenheit = fahrenheit }

        BadInputCelsius (Celsius celsius) ->
            { celsius = celsius, fahrenheit = "" }

        BadInputFahrenheit (Fahrenheit fahrenheit) ->
            { celsius = "", fahrenheit = fahrenheit }

```

It returns the current user’s input, or the converted value for the other text field, or blank user inputs on bad data.

I suspect there is a better way to do this so if you can see some improvement, please feel free to share 🙂

---

<div class="post-metadata">

**Author:** ![IloSophiep](https://yyz1.discourse-cdn.com/flex035/user_avatar/discourse.elm-lang.org/ilosophiep/32/3489_2.png) [@IloSophiep](https://discourse.elm-lang.org/u/IloSophiep)\
**Post date:** [March 1, 2023, 8:11am UTC](https://discourse.elm-lang.org/t/nitpicking-my-code-7guis-temperature-converter/8964/9 "2023-03-01T08:11:28Z")

</div>

Sorry, my suggestion simply was to refactor it to

```elm
warnOnInvalidInput : String -> Maybe String -> Html msg
warnOnInvalidInput name ms =
    let
        noErrorNode =
            text ""
        
        errorNode =
            p [] [text ("Invalid " ++ name ++ " input")]
    in
    case Maybe.andThen toNothingIfBlank ms of
        Nothing ->
            noErrorNode
          
        Just s ->
            case String.toFloat s of
                Nothing ->
                    errorNode
          
                Just _ ->
                    noErrorNode

```

Maybe that change is too obvious or not worth it. I’m not even sure i would’ve coded it like that 😅.

---

<div class="post-metadata">

**Author:** ![benjamin-thomas](https://yyz1.discourse-cdn.com/flex035/user_avatar/discourse.elm-lang.org/benjamin-thomas/32/3869_2.png) [@benjamin-thomas](https://discourse.elm-lang.org/u/benjamin-thomas)\
**Post date:** [March 1, 2023, 11:36am UTC](https://discourse.elm-lang.org/t/nitpicking-my-code-7guis-temperature-converter/8964/10 "2023-03-01T11:36:22Z")

</div>

> [@IloSophiep](#):
>
> Maybe that change is too obvious or not worth it. I’m not even sure i would’ve coded it like that 😅.

Oh got ya now 🙂

It’s true, that would have removed the duplication technically, but maybe only shifting the problem 🙂

---

<div class="post-metadata">

**Author:** ![system](https://canada1.discourse-cdn.com/flex035/uploads/elm_lang/original/1X/50a05e53677a2c3b47776d7abd0f113eb50193a1.png) [@system](https://discourse.elm-lang.org/u/system)\
**Post date:** [March 11, 2023, 11:36am UTC](https://discourse.elm-lang.org/t/nitpicking-my-code-7guis-temperature-converter/8964/11 "2023-03-11T11:36:23Z")

</div>

This topic was automatically closed 10 days after the last reply. New replies are no longer allowed.