Nitpicking my code (7GUIs temperature converter)

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


I’m implementing a “Temperature Converter” as an exercise, see: 7GUIs.

I’m bothered by this warnOnInvalidInput function:

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.

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

    Just a ->
      case mb of
        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 :slight_smile:

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.

Thanks for the feedback @wolfadex

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

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?

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:

warnOnInvalidInput : String -> Maybe String -> Html msg
warnOnInvalidInput name 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") ]

        text ""

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


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

        Just _ ->

@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

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:

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

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 :slight_smile:

Sorry, my suggestion simply was to refactor it to

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

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

Oh got ya now :slightly_smiling_face:

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

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