Hi,
To be honest I don’t have the inclination to do an in-depth structure analysis, so I cannot tell you if your overall approach to the problem is reasonable or not. Looking briefly through your code my general impression was that it was pretty neat and tidy. There weren’t many examples of code I couldn’t understand. I thought I’d highlight a couple of places I would tidy up, but note that a lot of these are pretty close to a personal preference, rather than objective improvements. And as always with code reviews they often appear a bit negative merely because reviews tend to highlight questionable/wrong/bad code, rather than pointing out places that are good.
Unused Root
My first gripe is Route.Root and Route.Home why do you have both? Root is never parsed and both are formatted to the same string. It seems like Root is not required, this is a great example of wasting the time of someone who has to understand your code (including your later self).
Over-engineered Structure
Some of the structure seems over-engineered to me. It feels almost like auto-generated code. A great example of this is Page.Blank, you have a whole module dedicated to:
module Page.Blank exposing (view)
import Html.Styled exposing (Html, text)
view : { title : String, content : Html msg }
view =
{ title = ""
, content = text ""
}
Where is this used? Once in Main.elm:
case model of
Redirect _ ->
viewPage Page.Other (\_ -> Ignored) Blank.view
This is just obscuring this from the user. It might be excusable if you ever thought you might use Page.Blank elsewhere, but really is that likely? Seems like the only place you’ll use this is for viewPage. Better:
case model of
Redirect _ ->
let
blank =
{ title = ""
, content = ""
}
in
viewPage Page.Other (\_ -> Ignored) blank
All the relevant information in one place. I have no need for a module here.
Other places include:
Cart before Horse
Sorry for the sub-title here, I’m not sure what to call this. I had a colleague who frequently lost sight of the point of parameterisation. So he would frequently have functions that went something like:
myFunction : (Int -> Msg) -> Int -> ....
myFunction toMsg index ... =
let
message =
toMsg index
.....
I think the function parameter toMsg originally served a purpose in that index would come from within the function, but then a refactor had the index coming as a parameter as well, so my comment was always why not just take Msg in as the parameter? It’s silly to pass in a function to calculate something and the argument to that function, unless it’s a heavy to compute function and you can sometimes avoid the computation, but even then you probably want to refactor somehow. Anyway, you have a different tendency but one that remind me of this. I think the best example is Api.Endpoint.request. Here you are trying to ‘wrap’ Http.request:
{-| Http.request, except it takes an Endpoint instead of a Url.
-}
request :
{ body : Http.Body
, expect : Http.Expect a
, headers : List Http.Header
, method : String
, timeout : Maybe Float
, url : Endpoint
, withCredentials : Bool
}
-> Http.Request a
request config =
Http.request
{ body = config.body
, expect = config.expect
, headers = config.headers
, method = config.method
, timeout = config.timeout
, url = unwrap config.url
, withCredentials = config.withCredentials
}
This is really noisy, there is a lot of code here that doesn’t achieve anything. You kind of acknowledge that with your comment above. This leads to a use like:
Endpoint.request
{ method = "GET"
, url = url
, expect = Http.expectJson decoder
, headers = []
, body = Http.emptyBody
, timeout = Nothing
, withCredentials = True
}
What’s wrong with just:
Http.request
{ method = "GET"
, url = Endpoint.unwrap url
, expect = Http.expectJson decoder
, headers = []
, body = Http.emptyBody
, timeout = Nothing
, withCredentials = True
}
or just:
Http.get
{ url = Endpoint.unwrap url
, expect = Http.expectJson decoder
}
So again there is a lot of code that isn’t really achieving anything. This is makes it more difficult to read.
Power of Elm
Some of your code that actually produces Html reads like a template, you’re not really using the fact that you have a full functional language, eg. in Home.elm:
, div [ css [ paddingTop <| px 10 ] ]
[ a [ css scenarioStyle, Route.href (Route.Call Nothing) ]
[ text "Scenario 1" ]
, a [ css scenarioStyle, Route.href (Route.Call <| Just "guest") ]
[ text "Scenario 2" ]
, a [ css scenarioStyle, Route.href (Route.Call <| Just "webRTC") ]
[ text "Scenario 3" ]
, a [ css scenarioStyle, Route.href (Route.Call <| Just "error") ]
[ text "Scenario 4" ]
]
, div [ css [ paddingTop <| px 10 ] ]
[ p [] [ text "Scenario 1: presenter and VNC, mobile = false" ]
, p [] [ text "Scenario 2: guest and VNC, mobile = true" ]
, p [] [ text "Scenario 3: presenter and webRTC, mobile = false " ]
, p [] [ text "Scenario 4: error with first technology and retry with the second" ]
]
You’ve got a lot of similar code here, I’d factor it out a bit, but other people prefer to read it like Html. I think I spend enough time in Elm that this is easier for me to read:
let
scenarioParagraph s =
p [] [ text s ]
scenarioParagraphs =
[ "Scenario 1: Prens ..."
, "Scenario 2: guest ..."
, ....
]
|> List.map scenarioParagraph
scenarioLink (mCall, s) =
a [ css scenarioStyle, Route.href <| Route.Call mCall ] [ text s ]
scenarioLinks =
[ (Nothing, "Scenario 1")
, (Just "guest", "Scenario 2")
, ....
]
|> List.map scenarioLink
section =
div [ css [paddingTop <| px 10]
in
...
, section scenarioLinks
, section scenarioParagraphs
Doing this makes it a bit more obvious that you probably intended a scenarioParagraph to match up with a scenarioLink? If so I’d be tempted to store them together and then separate them with a List.unzip.
So I’m sure there are other places if I looked around more, and I’m sure that some/all of my points here are debatable. But at least someone else’s perspective can be helpful sometimes.
