Q: Submodules in wrong way?

I have removed all the calculation functions to a submodule from the Main.
As I tried to optimize or just make the compiled file, it was at least double of the original size. I wonder, is the submodule SunHelper I made somehow against basic or preferred rules of Elm-architecture? Would it be better in the submodule to get rid of referring the module of Main? I guess it would be more complicated, requiring writing more code.

If you run this, it works correctly showing the next equinox of this year with the initial data.
You’ll see the date 2022-09-23 and the time 01:04:27 of the Autumn equinox.

So I wish to you happy Autumn Equinox and Friday :blush:

Extracting a data structure and its associated functions as you did is usually a good thing once they grow to a size that is worth putting in their own module.

What I’d discourage is extracting a Model+Update+View as a “component” because it tends to more wiring and more complexity for no obvious gain. I was worried you did that at the start of the file when I read Model followed by init but it’s not what you did. Yet I’d chose a more descriptive name than Model for your data structure.

Also give a try at elm-format. You can install it and configure your editor to format on save. It’s opinionated, non configurable, and widely used in the elm community so it helps when reading other people’s code when it’s formatted the same way.

1 Like

Thank you for many good ideas!
Ok I’ll reformat the sources although the changes by elm-format are not always good looking, f.ex. too many line feeds. Must also look, how to put together vim editor and elm-format.

If I understand right, you support my idea keeping in Main the model and the parts init, update and view all together. Then I must rethink, how to call the functions - e.g. instead of
sunDeclination model
something else with more arguments like
sunDeclination obliqCorr appLongSun

Usually the main type of a module matches the module name. For example Dict in Dict, List in List, and so on. Its not a hard rule but one I tend to follow unless I find a good reason not to - reasons being that Model seems to be the default name for TEA models, or if a module has no obvious principle type since it contains many types and none of them seems more important or top-level than the others.

Well, I have just renamed Model to Inputdata (and model to inputdata) here and here Additionally, I have formatted the sources with elm-format.
I cannot just find a solution, how to remove completly the model things from the submodule. Thus, I’ll give up and state only, it was not a good idea to split the file in two parts in this case.

Hi, some opinionated thoughts.

Personally, I think that modularising some code is mostly about four things, Naming, Categorisation, Reuse, Encapsulation.

Naming

The smaller a module is, the less chance there is for names to start bumping up against one another. For example in the elm/core library there are several modules and many define a map function: List.map, Dict.map, Platform.Cmd.map, etc. We could just have all of elm/core in one large module, but then we would have to call these functions listMap, dictMap etc. It’s arguable which one is better, however, the former kind promotes consistency, whereas the latter kind it’s very easy to end up with listMap and mapDict.

As a side note, I generally encourage “qualified import”. So for example you have imported a lot of names unqualified, e.g. import String exposing (fromFloat, fromInt, toInt) this means two things. First of all if you have a module of your own that can take some data type to/from Ints/Floats then you either have to break your convention and qualify one of the imports or name your function something like yearFromFloat. If you instead have the general (breakable) convention that you qualify your imports then you just import String and in your code you refer to String.fromInt. To my mind this is better because the name you are using is more descriptive of what is happening. Additionally you don’t have to edit your imports as often. However, there are certainly other opinions available.

Categorisation

This is a dubious benefit, but if I see a name such as Year.fromFloat I know where to look for it, it must be in a Year module. Of course, you should have some editor/IDE support for being able to go directly to the definition so this is relatively dubious. Nonetheless I think a general sense of “orderlyness”/“tidyness” is behind a lot of modularisation (and probably the cause of most premature modularisation).

Reuse

Of course a function is re-usable as well. But if you’re going to use a type plus some related function definitions, in more than one place in your project then a module can communicate the intent of that relatively well.

In Elm, I find this to be a bit less important than in other projects. A common boundary, is between ‘logic’ and user-interface. Such that you could use the same logic for multiple different user-interfaces. Because if you’re using Elm, you’re (mostly) committing to a Web user-interface, so you’re unlikely to use the same logic for, say, a TUI interface. However, you might, in the same project, define some kind of entity with related logic, and then two separate views of that entity, say as part of a list or as a more detailed page all to itself (e.g. blog posts viewed as a list in the home page, but then as a single page for each one).

Encapsulation

I won’t go into this one in detail, but the idea is to restrict the visibility of an underlying data type so that it’s impossible from outwith the defining module to create an invalid value of the given type.

For your project

I’d say you should start by separating the logic from the viewing code. You’ve so far done a pretty reasonable job of this, however you have a bit of a “code smell” in that you have “magic numbers” in the view code. Here is an example from viewDeclination:

 , p [] [ text (" Day Length        = " ++ mnToHrMn (60 * getDayLength inputdata)) ]

How does 60 relate to the ‘day length’? The function getDayLength itself is a little non-descriptive, get day length in whats? seconds? milliseconds? human reaction times? In this case it’s a little worse because 60 is the number of seconds in a minute and also the number of minutes in an hour, so I’m not sure what this 60 represents. Much better if you have a module SunData (or InputData or whatever), with a function getMinutesInDay this means your code now looks like:

 , p [] [ text (" Day Length        = " ++ mnToHrMn (SunData.getMinutesInDay inputData )) ]

Data type in Main/WorkSunCalculator

WorkSunCalculator needs this InputData. So either you define it there, or in a separate module (I would vote for there, don’t do unnecessary modularisation, and when you have a module that contains only a type definition there is a good chance you’re doing something prematurely/unnecessarily).

However, you currently define the exact same type in Main and WorkSunCalculator. You don’t need this, just refer to WorkSunCalculator.InputData in your Main module. If you like you can do the following in Main:

type alias Model = WorkSunCalculator.InputData

This means you can later change the type of Model if you need to store additional information. For example, you might store the current time (perhaps updated by a subscription Time.every).

type alias Model =
    { now : Time.Posix
    , inputData : WorkSunCalculator.InputData
    }

Now, you don’t have to change the WorkSunCalculator module at all. The changes to Main are fairly trivial, wherever you make a call to a function in WorkSunCalculator that accepts an InputData you just have to update the argument e.g. WorkSunCalculator.getMinutesInDay model is now WorkSunCalculator.getMinutesInDay model.inputData. (Note, by keeping WorkSunCalculator module qualified import the places you need to change are easily identifiable, though of course the compiler would identify them for you anyway).

Final unrelated point

You have several places in your view code, where you seem to be intending that a list of things all look the same, but you’re doing so implicitly rather than explicitly, let’s look at your viewDeclination function again:

viewDeclination : InputData -> Html msg
viewDeclination inputdata =
    div [ style "color" "green", style "font-size" "1.4em"]
        [ p [] [ text (" Sun Declination   = " ++ cutDec6 (sunDeclination inputdata) ++ "°") ]
        , p [] [ text (" Day Length        = " ++ mnToHrMn (60 * getDayLength inputdata)) ]
        , p [] [ text (" Civil Twilight    = " ++ mnToHrMn (civTwlMns inputdata -1) ++ locTZ inputdata) ]
        , p [] [ text (morningToNoon inputdata) ]
        ... bunch of similar others

It seems like you want all of these elements to look the same, but it’s unclear from this if this is by accident or design. You can make it explicitly by design:

viewDeclination : InputData -> Html msg
viewDeclination inputdata =
     let
          viewDeclinationItem : String -> Html msg
          viewDeclinationItem content =
             p [] [ text content] 
     in
    div [ style "color" "green", style "font-size" "1.4em"]
        [ viewDeclationItem (" Sun Declination   = " ++ cutDec6 (sunDeclination inputdata) ++ "°") 
        , viewDeclationItem (" Day Length        = " ++ mnToHrMn (60 * getDayLength inputdata))
        , viewDeclationItem (" Civil Twilight    = " ++ mnToHrMn (civTwlMns inputdata -1) ++ locTZ inputdata)
        , viewDeclationItem (morningToNoon inputdata)
        ... bunch of similar others

Or even

viewDeclination : InputData -> Html msg
viewDeclination inputdata =
     let
          viewDeclinationItem : String -> Html msg
          viewDeclinationItem content =
             p [] [ text content]
          items : List String
          items =
             [ " Sun Declination   = " ++ cutDec6 (sunDeclination inputdata) ++ "°"
             , " Day Length        = " ++ mnToHrMn (60 * getDayLength inputdata)
             , " Civil Twilight    = " ++ mnToHrMn (civTwlMns inputdata -1) ++ locTZ inputdata
             ,  morningToNoon inputdata
             ... bunch of similar others
             ]
     in
    div [ style "color" "green", style "font-size" "1.4em"]
       (List.map viewDeclinationItem items)

Depends on whether you are likely to have non-declination items within that list or not.

Even in other places where it’s not quite the same, sometimes factoring out the common parts make the differences stand out:

viewValidation : InputData -> Html msg
viewValidation inputData =
    if {- condition -}
    then
        div [ style "color" "blue" ] [ text "Date entry OK" ]

    else
        div [ style "color" "red" ] [ text "Incorrect month or day" ]

Note how here these are the same other than the color and the actual text, but it takes some scanning and checking to work that out:

viewValidation : InputData -> Html msg
viewValidation inputData =
    let
        viewValidationDiv : String -> String -> Html msg
        viewValidationDiv color content =
            div [ style "color" color ] [ text content ]
    in
    if {- condition -}
    then
        viewValidationDiv "blue" "Data entry OK"
    else
        viewValidationDiv "red" "Incorrect month or day"

Now the differences between the two cases couldn’t be any clearer.

4 Likes

This is replaced so as you commented

Now the new function decHrToTime converts decimal hours of the daylength to timestring as mnToHrMn converts minutes to timestring hh:mm:ss.
items : List String
items =
[ " Sun Declination = " ++ cutDec6 (sunDeclination inputdata) ++ “°”
, " Day Length = " ++ decHrToTime (getDayLength inputdata)

It’s a good idea using the list of row items and to display them with List.map

I have made a separate module CommonModel containing the model InputData.

import CommonModel exposing (InputData)

type alias InputData =
CommonModel.InputData


init : InputData
init =
InputData “2022” “09” “23” “1” “04” “27” “60.0” “25.0” “3”

First I tried to use the model of Main as type alias in the submodule but it caused circulation error because the submodule SunHelper is imported to Main.

Next I tried to expand the file CommonModel with message types but it did’nt work as I removed it from Main. I have impoted it and declared in Main . Is this not valid?
type alias Msg =
CommonModel.Msg

but it has caused errors.

I have used the modules WorkSunCalculator and WorkSunHelper as temporary working versions only (and removed later) as the final ones are Main and Sunhelper uploaded to my github site.

I have improved the viewValidation to be. so as you proposed…
144 viewValidation : InputData → Html msg
145 viewValidation inputData =
146 let
147 validationResult color remark =
148 div [style “color” color] [text remark]
149 in
150 if { conditions}
. . .
159 then
160 validationResult “blue” “Date maybe Ok”
161 else
162 validationResult “red” “Date is incorrect!”
163

Thank you Allan for many good advices!