Feedback on my project

Hey guys,

I’m new here, a friend recommended me this Elm group, and I’m glad to be part of it.
Quick introduction: I’m working professionally with Elm for 2 and a half years, learned a bit of Haskell and Scala, and I’m enjoying the functional universe.

I recently applied for a company hiring a senior Elm developer, and I got the feedback that my code was a bit amateur, and they were expecting to see more of the Elm “power” in its type and more complexity on the modelling structure. I would like to have a feedback about it and how I can take my Elm skills to a next level. I’m following other projects and trying to improve myself, but would be very good to hear from the experts.

It would be awesome if you could take a few minutes to take a look and I would be very help if you could give me your feedback on how to improve (if it is really shitty, don’t need to be too hard XD)

Thank you!

From what I’ve seen the assignment does look like it is worded to explore this, rather than the multipage spa with routes setup.

One way you can approach it is to just focus on the types and functions to be able to represent all the business logic and requirements, without wiring up routes and pages, etc.

Once you have a solid modelling of the requirements with data and functions, I think it would be a lot easier to reason through the assignment and solution and talk about alternative designs.

1 Like

I see the relevant model for the exercise seems to be on Page.Call.

From an initial look, it seems pretty flat so a lot of the requirements are very loosely enforced by the type system and instead will need extensive runtime logic in the functions.

Here is my approach from a few minutes and looking at your assingment.txt which I found kind of strange and underspecified, just the types:

module Main exposing (..)


type alias State =
    { client : Client
    }


type Client
    = Participant
    | Presenter ScreenSharingPreference PresentingStatus


type ScreenSharingPreference
    = VNCFirst
    | WebRTCFirst


type PresentingStatus
    = NotAsked
    | Waiting
    | Started ScreenSharingState
    | Paused ScreenSharingState
    | Error String


type ScreenSharingState
    = VNC VNCState
    | WebRTC WebRTCState


type VNCState
    = Mobile
    | Desktop
        { windows : List Window
        , screens : List Screen
        , modal : Maybe SelectWindowOrScreenModal
        , selected : Maybe SelectedWindowOrScreen
        }


type alias WebRTCState =
    { selectedScreen : Maybe Screen, screens : List Screen }


type SelectWindowOrScreenModal
    = Screens
    | Windows


type SelectedWindowOrScreen
    = SelectedWindow Window
    | SelectedScreen Screen


type Window
    = Window


type Screen
    = Screen


type Msg
    = StartClicked
    | ServerAcknowledgedTechnology (Result String ScreenSharingState)
    | StopClicked
    | AvailableWindowsAndScreens (List Window) (List Screen)
    | SelectScreen
    | ScreenSelected Screen
    | SelectWindow
    | WindowSelected Window
    | PauseClicked

You can go even further ensuring certain things with the type system, like using zip lists for the screen/window selections to avoid impossible states with that, and I’m sure there are other things but it is tradeoffs in the end.

Are you familiar with the making impossible states impossible talk?

1 Like

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:

  • I cannot tell what the purpose of the Session module is. I can see it exists, and it wraps up the navigation key within a constructor for Guest and extracts that. I assume you have in mind that you would have different kinds of sessions, but I’m not sure why you would store the navigation key there especially if your function navKey : Session -> Nav.Key is total, so you’re assuming other session kinds will have the nav key as well? Finally you have fromGuest here, what’s the point? It just aliases the constructor. I think you have some kind of idea of ‘hiding’ going on here, you’re trying to hide implementation or something, but why? It feels like you’ve heard “hiding is good” and so you’ve applied it wherever you could, rather than should.
  • Another example in Home.elm:
       toSession : Model -> Session
       toSession model =
            model.session
    
    I’m sorry, just why?

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.

1 Like

This code is based quite directly on rtfeldman/elm-spa-example.

So for example the Session which doesn’t seem to have purpose, exists because this code doesn’t feature logged in users like elm-spa-example does, but Session was still kept around even though it doesn’t really have use in this new context.

1 Like

Hey @joakin thank you very much for using your time to review my project.

I was aware of the my mistakes, and your approach is exactly where I wanted to go.

Just justifying myself, I thought a whole complete prototype solution would have more impact and I ended losing my focus, and most of the feedback from other comments I was aware, but I had to deliver because I was already out of time.

My goal here is to learn from my mistake and improve. I just saw the talk " Making Impossible States Impossible" talk, and it’s really good. I try to catch up see more talks about Elm, I think i need them.

Btw, what other sources for Elm knowledge do you consume?

Once again, thank you for your time!

Hey @allanderek, thank you so much for taking your time to review my project, that’s really awesome that you dove deep into it.

Your perspective is very helpful. Some things you pointed out I was aware of, and I wasted so much time on interface and trying to please them, that at the end I haven’t had the eye or the patience to spot and fix. That makes me happy (being aware), I wasn’t so bad after all, it was just a matter of time management and focus more than Elm knowledge.

Once again, I really appreciate that you have used yoru time to help me.

:slight_smile:

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