This repository has been archived by the owner. It is now read-only.

Refactor Input #138

Merged
merged 13 commits into from Jan 26, 2017

Conversation

Projects
None yet
3 participants
@rogeriochaves
Copy link
Contributor

rogeriochaves commented Jan 19, 2017

This is part of #122

I believe it was really a good idea to take this approach, things are simpler now imo. Some problems I've found:

  • Language was on inputs model and update, causing a need for it to sync with the language of the parent (reimbrusements), now only the view receives it when rendering
  • Instead of the TranslationId, the already translated version of the label text was the one getting passed around, also causing the need to sync if language changed for example, or always translate when adding a label
  • There was no types but the primitive ones, it was mostly String passing back and forth, which made me confused of the kind of values some functions were getting
  • To know which field should be translated to what, the code was doing a correlation between 2 arrays, which led to the creation of some TranslationIds that were not fields
  • No only inputs, but all other parts have its own Mdl model, Mdl msg and a case to update Mdl

What I did:

  • Remove language and mdl from input
  • Pass the parent (reimbrusements) model to the input view, so it can get language and mdl from there
  • Removed Html.map so now this view produces Html Msg of the parent type
  • Since now it is only doing that, I was able to rename it to Search, because in fact this part is only doing Search, and everything made more sense
  • Transform the fields labels into an union type called Label
  • Move mapping of translations to Fields file, so there is a direct mapping between a Label and a TranslationSet
  • Create a Field type, which means a Label + Value, which is a String, but maybe we can improve that later (some fields need date, other number, etc, this could be described in the types)

So, for me, it looks like the code is simpler now, as Search doesn't have to manage translation and mdl, translation is only done at rendering time.

It is easier to understand, since we have less primitives and more types, we can look at function signature and understand what values it takes and what it do as not everything is a String, also creating more safety to the programmer. Then I've figured that not every value need to map to a string key, only the ones that will get on the url, the searchable ones.

Also, Inputs is too technical of a name, Search clarifies better what this part of the code do imo.

A few lessons learned:

  • Don't save transformed data on the state together with the original one, they lead to sync problems, and your model ceases to be the single source of truth. For example, when we store the translated version together with the TranslationId, we start need to sync them. Instead, only save the original values on the state and transform them only when using, like when rendering. I've seend that happening on redux apps as well.
  • Add new types when possible (but don't exaggerate!), you'll get more help and safety from the compiler, and more readability in the code
  • Name things according to the domain, and think about functions reuse later, when you need
  • Elm has the best refactor experience of all time, of all time!

What you think? I believe if you look commit by commit it is easier to understand the changes, let me know if you don't agree on some of those refactorings.

Cheers

@rogeriochaves rogeriochaves force-pushed the rogeriochaves:issue-122 branch from b523542 to f117cbc Jan 19, 2017

@rogeriochaves

This comment has been minimized.

Copy link
Contributor Author

rogeriochaves commented Jan 19, 2017

TODO: fix tests :x

@coveralls

This comment has been minimized.

Copy link

coveralls commented Jan 19, 2017

Coverage Status

Changes Unknown when pulling 223d493 on rogeriochaves:issue-122 into ** on datasciencebr:master**.

@rogeriochaves rogeriochaves force-pushed the rogeriochaves:issue-122 branch from 223d493 to f596cf9 Jan 19, 2017

@coveralls

This comment has been minimized.

Copy link

coveralls commented Jan 19, 2017

Coverage Status

Changes Unknown when pulling f596cf9 on rogeriochaves:issue-122 into ** on datasciencebr:master**.

@rogeriochaves

This comment has been minimized.

Copy link
Contributor Author

rogeriochaves commented Jan 19, 2017

Done, I haven't add any new tests, but at least I believe I haven't reduced coverage either

@cuducos
Copy link
Member

cuducos left a comment

Wow! This is way better than what we had. Many many thanks!

I added two minor inline comments, once we can rule them out this is good to merge!

label


getLabelTranslation : Language -> Field -> String

This comment has been minimized.

@cuducos

cuducos Jan 23, 2017

Member

Do we really need to move translations out of the Internationalization module? I know Internationalization.elm is a long file that we could break down, but somehow having all translations in the same module is a good thing (even if we break this module down in submodules).

This comment has been minimized.

@rogeriochaves

rogeriochaves Jan 25, 2017

Author Contributor

I agree, I scratched my head with the same point, but decided to give it a try to see how it looked. I too I like having them centralized, I'll move back.

>>> toUrl [ ( "year", "2016" ), ( "foo", "bar" ) ]
"#/year/2016"
>>> toUrl [ ( "foo", "bar" ) ]

This comment has been minimized.

@cuducos

cuducos Jan 23, 2017

Member

This comments were here because they are tests (elm-doc-test). Any special reason not to test the function with invalid query parameters?

Is there an implicit assumption that they mean no harm so I shouldn't be worried? That could be true, this might be just my OCD hahaha…

This comment has been minimized.

@rogeriochaves

rogeriochaves Jan 25, 2017

Author Contributor

as fair as I remember, there was no way in the code that this function would have been called with an invalid value, or that wouldn't be a problem, I don't remember now. In fact, maybe we couldn't guarantee that just by looking at this function, maybe the ideal would be having an integration test.

But I'll look again and get back here.

This comment has been minimized.

@rogeriochaves

rogeriochaves Jan 26, 2017

Author Contributor

So, I've changed the code to reduce confusion - or maybe cause more :x

I noticed that the toUrl function was only getting called when we had full control of the variables that were being passed to it, therefore we didn't need that isSearchable check. But that wasn't obvious neither guaranteed without an integration test, cause maybe another function could end up using this one, without passing the right params.

I also noted that toUrl was always getting called together with toQuery, we had to call one to then pass as argument to the other.

So what I did was moving the toUrl function to Search/Update.elm, since it is only used to build search urls, and now it take the fields as arguments, not query, and return a clean url, so it is no longer possible to pass weird params to it and get a malformed url.

And the page case is just a string appending

This comment has been minimized.

@cuducos

cuducos Jan 26, 2017

Member

69b91c0 🍾 Awesome ; )

@cuducos

This comment has been minimized.

Copy link
Member

cuducos commented Jan 25, 2017

Done, I haven't add any new tests, but at least I believe I haven't reduced coverage either

The coverage just targets Python backend. I see little usage for coverage in Elm as a lot can be taken for granted just by the fact it compiled ; )

@rogeriochaves

This comment has been minimized.

Copy link
Contributor Author

rogeriochaves commented Jan 25, 2017

@cuducos yeah, I meant, the imaginary coverage for Elm hahahaha

@rogeriochaves rogeriochaves force-pushed the rogeriochaves:issue-122 branch from f596cf9 to 80c3fb4 Jan 26, 2017

@coveralls

This comment has been minimized.

Copy link

coveralls commented Jan 26, 2017

Coverage Status

Changes Unknown when pulling 80c3fb4 on rogeriochaves:issue-122 into ** on datasciencebr:master**.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Jan 26, 2017

Coverage Status

Changes Unknown when pulling aa75264 on rogeriochaves:issue-122 into ** on datasciencebr:master**.

@rogeriochaves rogeriochaves force-pushed the rogeriochaves:issue-122 branch from a869836 to 203f210 Jan 26, 2017

@coveralls

This comment has been minimized.

Copy link

coveralls commented Jan 26, 2017

Coverage Status

Changes Unknown when pulling 203f210 on rogeriochaves:issue-122 into ** on datasciencebr:master**.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Jan 26, 2017

Coverage Status

Changes Unknown when pulling 203f210 on rogeriochaves:issue-122 into ** on datasciencebr:master**.

@rogeriochaves rogeriochaves force-pushed the rogeriochaves:issue-122 branch from 203f210 to 70be535 Jan 26, 2017

@rogeriochaves rogeriochaves force-pushed the rogeriochaves:issue-122 branch from 70be535 to 6d9e509 Jan 26, 2017

@coveralls

This comment has been minimized.

Copy link

coveralls commented Jan 26, 2017

Coverage Status

Changes Unknown when pulling 6d9e509 on rogeriochaves:issue-122 into ** on datasciencebr:master**.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Jan 26, 2017

Coverage Status

Changes Unknown when pulling 6d9e509 on rogeriochaves:issue-122 into ** on datasciencebr:master**.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Jan 26, 2017

Coverage Status

Changes Unknown when pulling 69b91c0 on rogeriochaves:issue-122 into ** on datasciencebr:master**.

@cuducos

This comment has been minimized.

Copy link
Member

cuducos commented Jan 26, 2017

I think that after 69b91c0 we're LGTM… or are you planning to refactor something else?

@rogeriochaves

This comment has been minimized.

Copy link
Contributor Author

rogeriochaves commented Jan 26, 2017

@cuducos yes, I am, but not in this branch, refactors are good to be done in pieces to avoid conflicts (:

merge it away!

@cuducos cuducos merged commit b7b3ade into okfn-brasil:master Jan 26, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls First build on master at 100.0%
Details
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.