My first app - getting to an elm mindset

[Carl]

I have written an app (in my first fp lang)

It’s hosted at 9birthdays.com with public source.

I’d appreciate any constructive feedback on the source. I’ve still got a c/python/swift mindset though changes have begun while writing this app but there are, no doubt, areas I could tackle differently.

so far, it’s been truly delightful!

thanks, all!

[runarfu]

Looks great! If you want to make sure that your examples (e.g. in Ordinal) are correct you could consider using GitHub - stoeffel/elm-verify-examples. Not really a feedback on your code, just a suggestion. I’m using it extensively and find it perfect to document intent, usage and correctness.

[Carl]

thanks, I’ll check it out

[pit]

Nice, Carl! Two remarks:

• In Elm we normally don’t use snake case (calculate_birthday) but camel case. There aren’t many names like this in your code, though.
• It seems that the logic in isBirthdayToday works only for Earth. At least I wasn’t able to enter a date with today as a birthday on other planets.

[Carl]

snake case: I kept slipping into this format. I’ll fix those that got past my radar. I did look for a style rule when I started but didn’t read the text around the example base_height

this gives me an idea for an elm-review rule to catch these habits

isBirthdayToday: thanks for the report. I’ll check it out

[allanderek]

Some of these opinions are not commonly held within the Elm community so I will try to flag those up.
Also note, that your code is pretty reasonable to me, these are all pretty superficial comments.

The Main module

This doesn’t do anything. It’s just 21 lines of code with no purpose. It does actually have a useful comment, but the fact that it’s hidden in this module as opposed to the App module (or move all the App module code into Main if you really want to call the module Main) means that it is less likely to ever be read. So this module’s one purpose appears to be to hide a comment.

Import aliases

Possibly not a common Elm opinion

You have: import Browser.Dom as Dom, but this is only used in one place:

newTopPage : Url.Url -> Cmd Msg
newTopPage url =
    Task.perform (\_ -> ChangeUrl url) (Dom.setViewport 0 0)

Furthermore, this is not in any heavy logic, where a longer name would hurt readability.
It’s not so much that this alias is unhelpful, or mis-directing, but more that it hints that you’re doing things by default without thinking whether it serves a useful purpose.
So personally I’d just use import Browser.Dom and then (Browser.Dom.setViewport 0 0).

The Browser.Navigation as Nav alias is more interesting, because you use Nav. 8 times or so. Nevertheless all of these are pretty simple use case so I personally probably wouldn’t bother with the alias. Note that all the uses case are not in the real meat of your application, it’s mostly the boilerplaty stuff you need to do.

A lot of exposed imported functions

Also possibly not a common Elm opinion

I wrote a blog post about this. I think in the documentation it’s pretty common to expose functions because somehow it perhaps looks nicer in the actual documentation. However, it has the issue that names are often better designed to be either exposed or qualified, not both. For example if you have a module that renders some widget, you can either call the main view function view or viewWidget. I’d prefer the former, and then I call Widget.view, I feel like the latter is re-inventing qualified names without any compiler help. The blog post above links to a post on the Haskell mailing list which discusses this in more depth.

Note, as my blog post notes, the Html (and Html.Styled) modules may be exceptions to this, because if you expose all the functions then your Elm code more resembles the actual Html that will be produced.

Some local hiding

You have a lot of names that are defined at the top level, and then only used in one place. But to find this out I have to do a search for that name. Why not show this explicitly in your code? An example, init and initModel, you have:

initModel : Url.Url -> Nav.Key -> Model
initModel url key =
    { userBirthdate = fallbackBirthdate
    , birthdays = dummyBirthdaysReplacedByAppStarted
    , today = dummyTodayReplacedByAppStarted
    , key = key
    , url = url
    }


init : () -> Url.Url -> Nav.Key -> ( Model, Cmd Msg )
init _ url key =
    ( initModel url key
    , Task.perform AppStarted Date.today
    )

Now, initModel isn’t used anywhere else, so you can quickly make this:

init : () -> Url.Url -> Nav.Key -> ( Model, Cmd Msg )
init _ url key =
	let
		initModel : Url.Url -> Nav.Key -> Model
		initModel url key =
    		{ userBirthdate = fallbackBirthdate
    		, birthdays = dummyBirthdaysReplacedByAppStarted
    		, today = dummyTodayReplacedByAppStarted
    		, key = key
    		, url = url
    		}
	in
    ( initModel url key
    , Task.perform AppStarted Date.today
    )

Doing this, Elm will complain about the shawdowed variable names url and key but this also highlights that you don’t really need initModel to be a function at all, so this simplifies to:

init : () -> Url.Url -> Nav.Key -> ( Model, Cmd Msg )
init _ url key =
	let
		initModel : Model
		initModel =
    		{ userBirthdate = fallbackBirthdate
    		, birthdays = dummyBirthdaysReplacedByAppStarted
    		, today = dummyTodayReplacedByAppStarted
    		, key = key
    		, url = url
    		}
	in
    ( initModel 
    , Task.perform AppStarted Date.today
    )

To me this is pretty good result, you’ve added information to the reader (that initModel isn’t used anywhere else) and you’ve managed to simplify your code. Now, you can see that init itself, is only used in one place. Go ahead and place that within the definition of app. (While we’re at it, why app and not application? It’s not like this name is going to be used in many places).

application : Program () Model Msg
application =
	let
		init : () -> Url.Url -> Nav.Key -> ( Model, Cmd Msg )
		init _ url key = 
			let
				initModel : Model
				initModel =
    				{ userBirthdate = fallbackBirthdate
    				, birthdays = dummyBirthdaysReplacedByAppStarted
    				, today = dummyTodayReplacedByAppStarted
    				, key = key
    				, url = url
    				}
			in
    		( initModel 
    		, Task.perform AppStarted Date.today
    		)
	in
    Browser.application
        { init = init
        , view = view
        , update = update
        , subscriptions = always Sub.none
        , onUrlChange = UrlChanged
        , onUrlRequest = LinkedClicked
        }

Certainly some people are against overly long function definitions, and with some good arguments regarding code readability. But a lot of your code seems to go in the opposite direction making everything a top-level definition.

[Carl]

Thanks for taking so much time to review. It all sounds very useful on my first reading.