# Feedback on my project

**URL:** <https://discourse.elm-lang.org/t/feedback-on-my-project/5915>\
**Category:** Learn\
**Created:** [June 11, 2020, 2:23pm UTC](https://discourse.elm-lang.org/t/feedback-on-my-project/5915 "2020-06-11T14:23:29Z")\
**Posts on this page:** 8\
**Page:** 1

<div class="post-metadata">

**Author:** ![danilol](https://yyz1.discourse-cdn.com/flex035/user_avatar/discourse.elm-lang.org/danilol/32/3342_2.png) [@danilol](https://discourse.elm-lang.org/u/danilol)\
**Post date:** [June 11, 2020, 2:23pm UTC](https://discourse.elm-lang.org/t/feedback-on-my-project/5915/1 "2020-06-11T14:23:29Z")

</div>

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.

> **[danilol/snapcall](https://github.com/danilol/snapcall)**
>
> Snapview code challenge. Contribute to danilol/snapcall development by creating an account on GitHub.

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!

---

<div class="post-metadata">

**Author:** ![joakin](https://yyz1.discourse-cdn.com/flex035/user_avatar/discourse.elm-lang.org/joakin/32/171_2.png) [@joakin](https://discourse.elm-lang.org/u/joakin)\
**Post date:** [June 11, 2020, 3:44pm UTC](https://discourse.elm-lang.org/t/feedback-on-my-project/5915/2 "2020-06-11T15:44:05Z")

</div>

> [@danilol](#):
>
> they were expecting to see more of the Elm “power” in its type and more complexity on the modelling structure

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.

---

<div class="post-metadata">

**Author:** ![joakin](https://yyz1.discourse-cdn.com/flex035/user_avatar/discourse.elm-lang.org/joakin/32/171_2.png) [@joakin](https://discourse.elm-lang.org/u/joakin)\
**Post date:** [June 11, 2020, 5:09pm UTC](https://discourse.elm-lang.org/t/feedback-on-my-project/5915/3 "2020-06-11T17:09:58Z")

</div>

I see the relevant model for the exercise seems to be on [`Page.Call`](https://github.com/danilol/snapcall/blob/master/src/Page/Call.elm#L68).

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:

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

---

<div class="post-metadata">

**Author:** ![allanderek](https://yyz1.discourse-cdn.com/flex035/user_avatar/discourse.elm-lang.org/allanderek/32/1360_2.png) [@allanderek](https://discourse.elm-lang.org/u/allanderek)\
**Post date:** [June 12, 2020, 12:52pm UTC](https://discourse.elm-lang.org/t/feedback-on-my-project/5915/4 "2020-06-12T12:52:01Z")

</div>

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:

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

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

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

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

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

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

```auto
Endpoint.request
        { method = "GET"
        , url = url
        , expect = Http.expectJson decoder
        , headers = []
        , body = Http.emptyBody
        , timeout = Nothing
        , withCredentials = True
        }

```

What’s wrong with just:

```auto
Http.request
        { method = "GET"
        , url = Endpoint.unwrap url
        , expect = Http.expectJson decoder
        , headers = []
        , body = Http.emptyBody
        , timeout = Nothing
        , withCredentials = True
        }

```

or just:

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

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

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

---

<div class="post-metadata">

**Author:** ![malaire](https://avatars.discourse-cdn.com/v4/letter/m/b782af/32.png) [@malaire](https://discourse.elm-lang.org/u/malaire)\
**Post date:** [June 12, 2020, 1:07pm UTC](https://discourse.elm-lang.org/t/feedback-on-my-project/5915/5 "2020-06-12T13:07:55Z")

</div>

This code is based quite directly on [rtfeldman/elm-spa-example](https://github.com/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.

---

<div class="post-metadata">

**Author:** ![danilol](https://yyz1.discourse-cdn.com/flex035/user_avatar/discourse.elm-lang.org/danilol/32/3342_2.png) [@danilol](https://discourse.elm-lang.org/u/danilol)\
**Post date:** [June 17, 2020, 1:46pm UTC](https://discourse.elm-lang.org/t/feedback-on-my-project/5915/6 "2020-06-17T13:46:05Z")

</div>

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!

---

<div class="post-metadata">

**Author:** ![danilol](https://yyz1.discourse-cdn.com/flex035/user_avatar/discourse.elm-lang.org/danilol/32/3342_2.png) [@danilol](https://discourse.elm-lang.org/u/danilol)\
**Post date:** [June 17, 2020, 1:54pm UTC](https://discourse.elm-lang.org/t/feedback-on-my-project/5915/7 "2020-06-17T13:54:18Z")

</div>

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.

🙂

---

<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:** [June 27, 2020, 1:54pm UTC](https://discourse.elm-lang.org/t/feedback-on-my-project/5915/8 "2020-06-27T13:54:24Z")

</div>

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