Improve developer experience

[jeluard]

This document details steps to improve codebase intelligibility. Codebase should be as simple and intuitive as possible to understand and navigate.
It goes in pair with a best practices document that will follow.

status: RFC

6 phases

To reduce potential for regression and simplify implementation and subsequent merge into develop this refactoring can be split in 5 phases.

Phase #1: introduce new hierarchy

Reorganize folders / rename file to match new hierarchy and match re-frame semantic.

src/status-im/
├── components
├── data_store
├── platforms
├── protocol
├── translations
├── ui
│   └── screens
│       ├── account
│       │   ├── login
│       │   └── recover
│       │       ├── core.cljs
│       │       ├── db.cljs
│       │       ├── events.cljs
│       │       ├── subs.cljs
│       │       └── views.cljs
│       ├── contacts
│       │   └── new_contact
│       │       └── views.cljs
│       └── navigation.cljs
└── utils

and

resources
├── default_contacts.json
├── default_contacts_groups.json
├── images/
 | └── contacts/
├── bots/
 | ├── browse/
 | ├── ...
 | └── console/
└── vendors/
└── js/

Phase #2: consolidate namespaces

Merge utils namespaces and reduce files number to improve code understandability.
Consolidate android and ios shared code.

Phase #3: improve modularization

Make sure we don't have unwanted dependencies between namespaces (i.e. re-frame usage in non ui/ namespaces)
Might require spliting files

Phase #4: normalize app-db

Make sure app-db is properly normalized, speced and documented.

From @janherich

Cases which I find problematic:

Data under top level keys :chat-animations, :chat-loaded-callbacks, :chat-ui-props and :chats should me merged together, as they are all indexed by chat-id, I propose top level key :chat-by-id
Messages should be kept at top level key :message-by-id, every occurrence where message is referenced (for example [:chat-by-id "console" :messages] will contain collection of references to particular messages, like [:message-by-id "intro-message"] we can then either lookup data manually in subscriptions by using reference path as simple (get-in db ...) argument, or maybe even provide some automatic denormalisation where all references (links) will be replaced be referenced data in subscriptions, I'm not sure how that's feasible in reagent subscription model.

Phase #5: follow re-frame patterns

Reorganize ui code by views and follow re-frame patterns

Phase #6: apply code convention

Fix code convention according to best practices.

[flexsurfer]

For the phase №6 we can consider the following guides
https://github.com/bbatsov/clojure-style-guide
http://tonsky.me/blog/readable-clojure/

[GustavoNunes]

Nice @jeluard, thanks!
I think we need another level between ui and screens, for the "modules" (contacts, drawer, wallet, discover). Something like this, what do you think?
It is possible that some screens don't even have their specific subs and events, they only use shared ones.

├── components
├── data_store
├── platforms
├── protocol
├── translations
├── ui
│   ├── lifecycle.cljs
│   ├── wallet
│   │  ├── core.cljs
│   │  ├── db.cljs
│   │  ├── events.cljs
│   │  ├── subs.cljs
│   │  └── screens
│   │      ├── some-screen
│   │      │   ├── core.cljs
│   │      │   ├── db.cljs
│   │      │   ├── events.cljs
│   │      │   ├── subs.cljs
│   │      │   └── views.cljs
│   │      └── another-screen
│   │          ├── core.cljs
│   │          ├── db.cljs
│   │          ├── events.cljs
│   │          ├── subs.cljs
│   │          └── views.cljs
│   └── contacts
│       ├── core.cljs
│       ├── db.cljs
│       ├── events.cljs
│       ├── subs.cljs
│       └── screens
│           ├── some-screen
│           │   ├── core.cljs
│           │   ├── db.cljs
│           │   ├── events.cljs
│           │   ├── subs.cljs
│           │   └── views.cljs
│           └── another-screen
│               ├── core.cljs
│               ├── db.cljs
│               ├── events.cljs
│               ├── subs.cljs
│               └── views.cljs
└── utils

[flexsurfer]

hey @GustavoNunes, tbh in your case, screens package looks superfluous, @jeluard's option looks better. we can open only screens and can have shared files for all screens

[julienfantin]

I'm not familiar with the data model, but assuming that all entities are maps that can be identified by a single key I'd kindly suggest taking a look at https://github.com/vimsical/subgraph documentation is still sparse but we've used it in production for months and it's greatly simplified our data model. Happy to answer any questions here or in the repo's issues.

A simple practice that's had a big impact on our codebase was to use fully-qualified keywords for subscription and handler ids. This lets the compiler work for you, preventing small regressions and typos. It also makes very explicit the structure of the dependencies.

[jeluard]

@julienfantin Thanks for the tips! We definitively need some more detailed spec for the data model. @janherich already started working on it and I am sure he will make good use of your points.

[flexsurfer]

I usually follow the next steps
1)introduce new hierarchy (#1495)
2)add letsubs macro in views
3) apply code convention for views (#1495)
4)move nav/preload-data! to navigation.cljs
5)rewrite events using fx and cofx
6)apply code convention for events (#1495)
7)rewrite subs using re-frame best practice (db can be used only in extraction subscriptions)
8)use ns keys in app-db
9)write specs (in db.cljs, move from spec.cljs)
10)write tests (using re-frame-test)

[goranjovic]

What do you think about adding "write docstrings for important functions" to the to-do list? I usually find them useful in REPL. It kind of falls under the blanket term "good practices", but it might be good to emphasize it.

It's annoying to write those, I know, but we'll spend more time reading that code than writing it!