Code Review: Password Form

Learn
1

Thanks to assistance in this thread, I was able to rethink my code to make it more consistent. I did not end up including the List functions, as I am not able to clearly reason my way through them yet.

As an aspiring elm programmer, I would highly appreciate it if you could review my code from the perspective of a team leader or tech lead and advise where I could improve if this was my given task and the code was to be used in production.

The reasoning I went through to code the solution the way I did was as follows:

  1. If the code was to be called more than once, place it in a function, so that it is only defined in one place.

  2. Make similar functions have the same type signature, so that one general helper function (viewDrawSymbol) can call the individual helper functions (pwIsLength) consistently.

  3. Use pw for password, to shorten the code.

  4. Elm gave me the confidence to code this in a novel way, where each criteria of the password is checked as the user types.

Please have a look at this code.

2

You could look at your validation checks. Do you need to check against model.passwordAgain as well as model.password?

Personally, I wouldn’t do the validation check on model.passwordAgain - only on model.password. The reason being:

  1. Your messages to the user only state ‘Password must …’
  2. You have pwIsMatched in place, so when model.password passes all the validation, you know that model.passwordAgain does too if this function returns True.

If you must check both, comparing both with an || means the user will get a check mark if just model.passwordAgain passes, which doesn’t make sense to me. It may better to use && to check both pass the validation.

You have a typo in pwIsUpper, you are checking model.password twice.

You could write viewSubmitButton like so (just a suggestion):

viewSubmitButton : Model -> Html msg
viewSubmitButton model =
    let
         disabled_ =
             not (pwLength model && pwIsUpper model && pwIsLower model && pwIsDigit model && pwIsMatched model)
    in
    button [ disabled disabled_ ] [ text "Submit" ]

Otherwise all looks good to me.

3

I’d agree that changing the logic to only compare against the first password (other than for the matching test) would simplify your entire codebase.

With that you can minimise how many times you’re throwing around the entire model, thus narrowing the scope of your functions (this is, generally speaking, something you wish to do all of the time. Saves you from using the wrong value accidentally).

From

pwIsUpper : Model -> Bool
pwIsUpper model =
    String.any isUpper model.password || String.any isUpper model.password

To

pwIsUpper : String -> Bool
pwIsUpper password =
    String.any isUpper password

You could also harden the types here, by introducing some aliases into your model:

type alias Model =
    { name : String
    , password : Password
    , passwordAgain : VerifyPassword
    }

type alias Password = String
type alias VerifyPassword = String

...

pwIsUpper : Password -> Bool

which would return a compile error if you accidentally sent model.passwordAgain into this function.

This particular naming convention would collide with your current message values, but a rule of thumb with message naming could also be invoked here. You want to name your messages in a way that you explicitly understand what the message does. So UpdatePassword or even UserInputUpdatePassword could be good choices here.

I’d also look at expanding some of your variable names:

viewInput : String -> String -> String -> (String -> msg) -> Html msg
viewInput t p v toMsg = ...

It’s not so obvious what t p v refer to without digging into the rest of your code. Having more verbose variables helps with readability.

My personal taste here though is that viewInput is a redundant helper that abstracts away what you’re doing. It’s not saving you any keystrokes, adds some boilerplate and adds cognitive overhead. Others may argue about that point, and certainly this may be a helpful function if your view gets massive, but for the moment I’d drop this function.

4

I just wanted to reply to this part, because it’s not entirely correct.

pwIsUpper can be called with passwordAgain as a parameter since they are both Strings; you could even call it with the name field. Actually hardening the types requires creating new custom types similar to the following:

type alias model =
    { name: String
    , password: Password
    , passwordAgain: VerifyPassword
    }

type Password = Password String
type VerifyPassword = VerifyPassword String

...

pwIsUpper : Password -> Bool
pwIsUpper (Password pw) = ...

The (Password pw) deconstructs the Password custom type, extracting the String into pw. This will prevent calls to pwIsUpper with any other strings, but it does require more work to make use of the password field.

5

Yes, indeed - thanks for picking up that oversight Titus.

6

That is how I had it initially, but then viewDrawSymbol didn’t work for pwIsMatched, as each function type is String -> Bool, except pwIsMatched which is Model -> Bool

How would I fix viewDrawSymbol?

7

Indeed. I’ve made an ellie-app for you that works with some changes.

Points of interest:

viewDrawSymbol : (a -> Bool) -> a -> Html Msg

Rather than having (Model -> Bool) -> Model ... we allow an any type here such that we can also send in (Password -> Bool) -> Password ..., then in view, we can call both:

viewDrawSymbol pwLength model.password
and
viewDrawSymbol pwIsMatched model.

pwIsEmpty : Password -> Bool
pwIsEmpty (Password password) =
    String.isEmpty password

Since we use the opaque type type Password = Password String, we must denature that type to get at the String portion. This is how you do that, there are a few let ... in blocks that do the same for you.
Building a Password type is similar, you can see that in the init constructor:

Model "" (Password "") (PasswordAgain "")

This may feel odd to do at this point, perhaps a little overkill, but it is seriously handy once you have larger models to play with and the possibility of sending the wrong values into a function.

One additional thing for pwIsEmpty: there is a String.isEmpty function - you don’t need to do an explicit String.length password == 0 check.

onInput (UpdatePassword << Password)

This is also something interesting to learn about. Let’s take a look at some type signatures:

onInput : (String -> msg) -> Attribute msg
update : Msg -> Model -> Model
type Msg
    = ...
    | UpdatePassword Password
      ...
type Password = Password String

Html.input has a value of type String. We need to make that a Password so that UpdatePassword can work. We can’t do that initially, since onInput is expecting a String!

One method would be to use an anonymous function to sort all this out.

onInput (\string -> UpdatePassword (Password string))

The function composition << is syntactic sugar for the anonymous function above.

8

I’m not sure what the benefit of having two separate types here is? password and passwordAgain both represent a password, do we really need a closed type for both to show this? Would it be acceptable to just use the Password type for both fields?

I like this adaptation, but it could be taken a step further. psIsMatched does not need the whole model, you could show this by using an extensible record:

pwIsMatched : { a | password: Password, passwordAgain: PasswordAgain } -> Bool

Or you could pass a tuple of (Password, PasswordAgain). Although this leads to a more lengthy call, it is very explicit about what information pwIsMatched needs, and you can do the destructuring inline, as in:

pwIsMatched : (Password, PasswordAgain) -> Bool
pwIsMatched (Password password, PasswordAgain passwordAgain) =
    password == passwordAgain && not (String.isEmpty password)

-- Then call viewDrawSymbol with:
viewDrawSymbol pwIsMatched (model.password, model.passwordAgain)
9

That’s certainly just taste for the most part, yeah. No issues if one chose to do set both password values against the same type. Would certainly simplify some of the logic.

Although it does depend on exactly how you approach the view code then. Say you do only have one Password type for password and passwordAgain, all of your checks must pass before the submit button is enabled - since the passwords at this stage will be verified the same through pwIsMatched, then it is indeed irrelevant which of the two variables are sent into the rest of the check functions.

However, say you swap your viewDrawSymbol calls to use model.passwordAgain. Now, as a user types in their password to the first field, none of the indicators show up as a tick mark, feedback is only shown to the user once they start entering in the second password. Forcing a separate type makes sure this UX pattern is made impossible.

10

I figured, if the user clicks on the passwordAgain field instead of the password field, the form shouldn’t stop validating. This has subsequently been changed to avoid the issue entirely.

Updated across all the functions.

Nice catch.

Fantastic! I’ve used it to remove the if then else blocks from viewSubmitButton and viewConfirmPassword.

This was very helpful. It lead me to ask how I could reduce code complexity and represent that in the UI at the same time? I decided to go with a disabled passwordAgain field that only becomes active once the password field validates successfully.

That helper function comes directly from the Elm Guide code I was following, so I left it in. I agree. I had no idea what was going on when I first encountered it. I have removed it.

Thank you very much :slight_smile:

Updated. There is only one equality check left, which is required, which is a major improvement over my first attempt.

I removed the name field, as it was not used anywhere else. As the model only contains the two password fields, I’ll use it for now. I’ll revisit this when I create forms with more fields.

I was surprised that Elm did not have a String.isSpecial function. I looked at how elm implemented isUpper and wrote my own. Now the form checks for special characters too.

Opaque types, custom types per field, anonymous functions, function composition: I had to leave all of that for now. Thank you for all the help and for showing me the road ahead.

11

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