Nitpicking my code (7GUIs temperature converter)

Learn
1

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

Hello!

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

3

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.

4

Thanks for the feedback @wolfadex

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

5

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?

6

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 =
    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
7

@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
8

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:

9

Sorry, my suggestion simply was to refactor it to

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 :sweat_smile:.

10

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:

11

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