# A little code review

## Warning ⚠️

Many things happen to be idiomatic or accepted in different platforms. So there is no good/bad neccessarily in everything we'll discuss.

For instance, while multiple inheritance is considered evil in many languages, it might make sense here. Or inheriting from interfaces with implementations, coupled with a token...

> Maybe things discussed here don't apply. We should understand the tools we're using (does our framewok allow for X or will it be a struggle? e. g Rails + Data mapper)

## Dependency injection 💉

* Instead of using the IDE targets it'd could be better to inject those classes.

#if DEBUG
var localRepositories = LocalRepositoriesFake()
var services = ServicesFake()
var repositories = RepositoriesFake()

#else
var localRepositories = LocalRepositoriesReal()
var services = ServicesReal()
var repositories = RepositoriesReal()

* Both Android and iOS, `cache policy` is being passsed as a param in many functions, while it could just be injected to the classes.
* Another interesting abstraction to consider: Auth Service to inject in the repositories.

## Improve error flow (mobile) 💥

* The ideal would be to map infrastructure and domain errors.
* Throwing is a nice way to handle this, instead of if..elses.
* Once cought, the exception can be used for various purposes, such as calling analytics services, mapping to a proper response to the user...
* We should not have a FlowState of type ErrorHttp, because the ViewModel should't know about Http

## Domain models 📝

Avoid anemic models. It has some disadvantages:

* Less encapsulation
* Business logic is spread throught the code
* Low cohesion

Don't group too many concepts in a multi purpose domain model, since it might lead to many null fields (listings and details might have different models, e.g.)

Repositories should return domain models. In some cases we've seen repositories that look more like view models (scene/models). This leads to the same as above, many nulls when we could have many view models that derive from a domain model.

## Use cases 🧳

iOS and Android implementations could benefit from moving orchestration logic to Use Cases.

For instance, `Home` class in Android could benefit from this.

Use cases also would improve testing.

## Encapsulation 💊

### Utility classes or services
Here domain info is loaded directly using ES6 import system. Another abstraction would have made it easier to reason about what is going on and change in the future in case we wanted to import this from a place other than a json file (which IMO doesn't belong to the infrastructure, but is part of the domain data)

import * as configurationData from '../data/config.json'

export class MyModuleBaseUseCase {
  dataSource: ConfigData

  constructor(...) {
    this.dataSource = configurationData
  }
}

vs

import ConfigurationStore from '../data/ConfigurationStore'

export class MyModuleBaseUseCase {
  dataSource: ConfigData

  constructor(...) {
    this.dataSource = ConfigurationStore.load(...)
  }
}

vs

import ConfigurationStore from '../data/ConfigurationStore'

export class MyModuleBaseUseCase {
  dataSource: ConfigData

  constructor(
    @Inject(CONFIG) public configurationStore: ConfigurationStore) {
    this.dataSource = configurationStore.load(...)
  }
}

Other example: navigation service in iOS so no calls are directly made to `navigationController`.

### Dependencies
Encapsulate direct dependencies to external services

Example:
mapbox/supercluster is used directly.

const index = new Supercluster({
  radius: DEFAULT_RADIUS,
  maxZoom: DEFAULT_MAX_ZOOM,
}).load(features)

Could we encapsulate it so that our cluster implementation is not that tightly coupled with that of the external library?

Same thing with FirebaseCrashlytics, for example.

### Function parameters
Some functions have too many parameters. For example:

function fetchOpportunityList(date: state, type, sort, offset, limit, completionCb)

Could become:

function fetchOpportunityList(queryParams, completionCb)

If parameters have some certain degree of cohesion, they can be wrapped into objects that help mitigate param spread and make function interfaces more flexible.

## Polymorphism 👨‍👩‍👧‍👦

Try to work with interfaces, especially for external services, so we can make fake implementations for testing.

Example: RemoteConfig (iOS) is a very well extracted service, but it'd be better if it implemented an interface and there was a FakeRemoteConfig for testing purposes.

## Naming 🧐

Express the real intent of the code.
Examples:

* This function is not really humanizing the input:

protected humanizeValue(value: string | null | undefined) {
  if (value === null || value === undefined || value === '') {
    return this.i18n.translate('shared.null', { lang: this.lang })
  }

  return Promise.resolve(value)
}

* getLocalVisit is a function that navigates or shows alerts (calls to navigateToPendingDocVisitDetail).

Some ideas:
* Stick to the estabilished conventions. E.g: `result.CONFIRMED = [...]` is a variable, but named as a constant.
* `Managers` is usually not a good word. What are they? Services? Thisname might lead to God Objects.
* Give DTOs a meaningful name: `ProductDTO` vs `ProductCreationResponseDTO`
* Give services meaningful names: If the `visitService` acts as an HTTP client, maybe a better name is `visitClient`
* Watch out for unneccessary prefixes, suffixes, and the such. E.g.: Is suffix “byFlow” neccesary?

## Consistency 🧩

### Return values

In most repositories

async findByUuid(uuid: string): Promise<TypeX | null> {
  const xEntity = await this.xRepository.findOne(...)
  
  if (!xEntity) {
    return null
  }

  return xEntity.toDomainObject()
}

but some return `undefined`

async findByUuid(uuid: string, ...): Promise<TypeY | undefined> {
  const queryBuilder = ...
  const yEntity = await queryBuilder.getOne()
  
  if (!yEntity) {
    return undefined
  }

  return yEntity.toDomainObject()
}

In most places this is irrelevant

const c = await this.xRepository.findByUuid(xUuid)
if (!x) {
  throw new XNotFoundError(xUuid)
}

but it's a good sign of care for consistency to be observant of these things.

> **Note**: An improvement of this would be to use a findByUuidOrFail that throws, but that's another discussion 😉

### Consistency accross projects

3 projects that use repositories have different naming conventions
* `XxxRepositoryInterface` + `XxxRepositoryFake` + `XxxRepository`
* `XxxRepository` (then mocked, so no fake)
* `XxxRepository` + `XxxRepositoryFake` + `XxxRepositoryReal`

We favour the naming:
* InstallationAgentRepository -> Interface
* InstallationAgentRepositoryRetrofit -> Real implementation
* InstallationAgentRepositoryFake -> For testing

### File placement
* Sesion (JTW representation) should not live in /Managers
* Make sure domain/application don't get mixed up

## Maintainability 🔗

Always think about the next programmer that is going to inherit your code (probably your future self):

* is it easy to reason about?
* does it pass the [squint test](https://robertheaton.com/2014/06/20/code-review-without-your-eyes/)?
* does it need to be documented?

In general:
* We should keep classes and files as small as possible.
* Private methods can help with readability:

const responses = {} as any
myCoolDTO.form_response.answers.forEach(
  //... 10+ lines of indented code
)

/// this could be moved to a private method

const responses = extractResponses(myCoolDTO)

* Don't indent too much (nested code reads worse)
* A 1000 line file probably means there are some abstractions lurking in the dark, waiting to be discovered.
* Examples: `DashboardFinderUseCase`, `PatchOnsiteEngine`, ...

## Testing 🧪

* Test useful things. Don't unit test fake repositories, for instance.
* Order of execution should not affect tests
* We should not expose the internals in tests (`visitdocument` e2e mocks `findOne`, e.g)
* A useful abstraction would be a test client (`client.login`…)
* We should cover repositories with complex queries (installerrepo doesn't have)
* In general, things that are implementation specific should be injected (repositories, routers, ...)
* Try to make them easy to understand
* We should try to approach e2e testing from the final user's perspective. In that regard, what keys/texts should we use? i18n?

Open discussion: Mocks vs test/fake repositories

## Performance 📈

Now that we reached a point of great complexity, it'd be nice to pause for a second and dedicate some time to measi¡uring performance.

We should check:
* Hot spots where performance could improve
* SQL performance: is there room for improvement? Change relations? indexes?
* Do we use measuring tools?
* Android and iOS performance measurements
* Should we think of cache/index systems (Redis? Elasticsearch?)
* Snapshot performance improvement: WIP

Proposal: performance tech talk

## Language specific
Know your language and avoid bad practices such as:
* !!
* Too many ternary operators
* Too many allocations/deallocations/memory leaks
* ...

## Notes and misc
* We're in the middle of a big change: visits + technical services -> services. This will need careful study both in the modeling and in the migration.
* API Versioning (when breaking changes happen)
* Duplicated code -> extract into services
* Countries table contains data that maybe doesn't belong there (technicalServiceBaseRate, e.g.) -> maybe for a little more complex infrastructure to reflect county + taxes + rates?
* Design no longer has the notes module but the backend does and IM use it.
* We have a lot of design debt
* Various refactors are needed in frontend (maps, icons, UI updates, datepickers, handle types in UI tables)

## TODO

Se está delegando el hasheo de la contraseña de la infrastructura, en vez de hacerlo a nivel de un estático .create en el Admin.

Se está agregando la lógica del reseteo de contraseña a un servicio externo, en vez de un método sobre el propio modelo del admin. creo que han sacado a un servicio una cosa que no hace falta y complica la lógica y tiene efectos secundarios del tipo guardar 2 veces

In `code/src/mails/domain/Mailer.ts` we should use ISP