# Code Review: Password Form

**URL:** <https://discourse.elm-lang.org/t/code-review-password-form/5192>\
**Category:** Learn\
**Created:** [February 17, 2020, 7:58am UTC](https://discourse.elm-lang.org/t/code-review-password-form/5192 "2020-02-17T07:58:44Z")\
**Posts on this page:** 11\
**Page:** 1

<div class="post-metadata">

**Author:** ![codepadawan](https://avatars.discourse-cdn.com/v4/letter/c/a4c791/32.png) [@codepadawan](https://discourse.elm-lang.org/u/codepadawan)\
**Post date:** [February 17, 2020, 7:58am UTC](https://discourse.elm-lang.org/t/code-review-password-form/5192/1 "2020-02-17T07:58:44Z")

</div>

Thanks to assistance in [this thread](https://discourse.elm-lang.org/t/how-to-reason-about-code-to-avoid-equality-checks/5183), 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](https://github.com/codepadawanelm/elm-guide/blob/master/Password%20Form.elm).

---

<div class="post-metadata">

**Author:** ![paulh](https://yyz1.discourse-cdn.com/flex035/user_avatar/discourse.elm-lang.org/paulh/32/5022_2.png) [@paulh](https://discourse.elm-lang.org/u/paulh)\
**Post date:** [February 17, 2020, 9:49am UTC](https://discourse.elm-lang.org/t/code-review-password-form/5192/2 "2020-02-17T09:49:16Z")

</div>

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.

---

<div class="post-metadata">

**Author:** ![Libbum](https://yyz1.discourse-cdn.com/flex035/user_avatar/discourse.elm-lang.org/libbum/32/1311_2.png) [@Libbum](https://discourse.elm-lang.org/u/Libbum)\
**Post date:** [February 17, 2020, 10:15am UTC](https://discourse.elm-lang.org/t/code-review-password-form/5192/3 "2020-02-17T10:15:49Z")

</div>

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

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

```

To

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

```

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

```auto
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:

```auto
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.

---

<div class="post-metadata">

**Author:** ![Titus\_Anderson](https://avatars.discourse-cdn.com/v4/letter/t/e56c9b/32.png) [@Titus\_Anderson](https://discourse.elm-lang.org/u/Titus_Anderson)\
**Post date:** [February 18, 2020, 12:32am UTC](https://discourse.elm-lang.org/t/code-review-password-form/5192/4 "2020-02-18T00:32:55Z")

</div>

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

> [@Libbum](#):
>
> You could also harden the types here, by introducing some aliases into your model:
> 
> ```auto
> 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.

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

```auto
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.

---

<div class="post-metadata">

**Author:** ![Libbum](https://yyz1.discourse-cdn.com/flex035/user_avatar/discourse.elm-lang.org/libbum/32/1311_2.png) [@Libbum](https://discourse.elm-lang.org/u/Libbum)\
**Post date:** [February 18, 2020, 6:57am UTC](https://discourse.elm-lang.org/t/code-review-password-form/5192/5 "2020-02-18T06:57:42Z")

</div>

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

---

<div class="post-metadata">

**Author:** ![codepadawan](https://avatars.discourse-cdn.com/v4/letter/c/a4c791/32.png) [@codepadawan](https://discourse.elm-lang.org/u/codepadawan)\
**Post date:** [February 19, 2020, 7:44am UTC](https://discourse.elm-lang.org/t/code-review-password-form/5192/6 "2020-02-19T07:44:52Z")

</div>

> [@Libbum](#):
>
> 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
> 
> ```auto
> pwIsUpper : Model -> Bool
> pwIsUpper model =
> String.any isUpper model.password || String.any isUpper model.password
> 
> ```
> 
> To
> 
> ```auto
> pwIsUpper : String -> Bool
> pwIsUpper password =
> String.any isUpper password
> 
> ```

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`?

---

<div class="post-metadata">

**Author:** ![Libbum](https://yyz1.discourse-cdn.com/flex035/user_avatar/discourse.elm-lang.org/libbum/32/1311_2.png) [@Libbum](https://discourse.elm-lang.org/u/Libbum)\
**Post date:** [February 19, 2020, 9:12am UTC](https://discourse.elm-lang.org/t/code-review-password-form/5192/7 "2020-02-19T09:12:15Z")

</div>

Indeed. I’ve made an [ellie-app](https://ellie-app.com/85xmpG82TsMa1) for you that works with some changes.

Points of interest:

```elm
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`.

* * *

```elm
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:

```elm
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.

* * *

```elm
onInput (UpdatePassword << Password)

```

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

```auto
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.

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

```

The [function composition](https://package.elm-lang.org/packages/elm/core/latest/Basics#(%3C%3C)) `<<` is syntactic sugar for the anonymous function above.

---

<div class="post-metadata">

**Author:** ![sparksp](https://yyz1.discourse-cdn.com/flex035/user_avatar/discourse.elm-lang.org/sparksp/32/2789_2.png) [@sparksp](https://discourse.elm-lang.org/u/sparksp)\
**Post date:** [February 19, 2020, 2:49pm UTC](https://discourse.elm-lang.org/t/code-review-password-form/5192/8 "2020-02-19T14:49:20Z")

</div>

> [@Titus\_Anderson](#):
>
> ```elm
> type Password = Password String
> type VerifyPassword = VerifyPassword String
> 
> ```

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?

> [@Libbum](#):
>
> ```auto
> 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.

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:

```elm
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:

```elm
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)

```

---

<div class="post-metadata">

**Author:** ![Libbum](https://yyz1.discourse-cdn.com/flex035/user_avatar/discourse.elm-lang.org/libbum/32/1311_2.png) [@Libbum](https://discourse.elm-lang.org/u/Libbum)\
**Post date:** [February 19, 2020, 9:31pm UTC](https://discourse.elm-lang.org/t/code-review-password-form/5192/9 "2020-02-19T21:31:55Z")

</div>

> [@sparksp](#):
>
> 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?

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.

---

<div class="post-metadata">

**Author:** ![codepadawan](https://avatars.discourse-cdn.com/v4/letter/c/a4c791/32.png) [@codepadawan](https://discourse.elm-lang.org/u/codepadawan)\
**Post date:** [February 20, 2020, 7:25am UTC](https://discourse.elm-lang.org/t/code-review-password-form/5192/10 "2020-02-20T07:25:14Z")

</div>

> [@paulh](#):
>
> You could look at your validation checks. Do you need to check against `model.passwordAgain` as well as `model.password` ?

> [@paulh](#):
>
> 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.

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.

> [@Libbum](#):
>
> 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).

Updated across all the functions.

> [@paulh](#):
>
> You have a typo in `pwIsUpper` , you are checking `model.password` twice.

Nice catch.

> [@paulh](#):
>
> You could write `viewSubmitButton` like so (just a suggestion):

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

> [@paulh](#):
>
> Personally, I wouldn’t do the validation check on `model.passwordAgain` - only on `model.password` .

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.

> [@Libbum](#):
>
> 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.

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.

> [@Libbum](#):
>
> Indeed. I’ve made an [ellie-app](https://ellie-app.com/85xmpG82TsMa1) for you that works with some changes.

Thank you very much 🙂

> [@Libbum](#):
>
> One additional thing for `pwIsEmpty` : there is a `String.isEmpty` function - you don’t need to do an explicit `String.length password == 0` check.

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

> [@sparksp](#):
>
> 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:

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.

---

<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 1, 2020, 7:25am UTC](https://discourse.elm-lang.org/t/code-review-password-form/5192/11 "2020-03-01T07:25:19Z")

</div>

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