Intro to Code Review - raisercostin/software-wiki GitHub Wiki

Intro to Code Review

Untangled: Code Review

Code changes are needed.
Issue - Is good to have the why (reason for change) in an agreed issue (not as a comment in a change/pull/merge request, nor in a comment in a chat app as such information is lost).
Test - The most formal definition of the why is a new test that fails on the current code.
Design Direction - This concept pays respect to the various forces that influence the shape of the code. You can envision a solution but this is not possible to reach in one step. So then at least you clarify where you want to go and then proceed with first step that adds already value. There is a chance the design will never complete as various forces will show up: other stake holders, other more important changes, new risks and events. The Design Direction is not visible and should be explained in the why. But consider that not necessary all steps will be taken to fully implement the change. Add more value first. See the book The Nature of Software Development.
Several principles come in handy:
- SRP (Single Responsibility Principle) for the responsibility of the change request - See Code-Changes.
- OCP (Open Closed Principle) should guide any non trivial changes.
  - Pre redesign is a refactoring based change that tries to keep everything as before. Behaviorally this is as a no-change. Unfortunately we don't have tools now that asses if a change is pure refactoring. Here we must trust the proposal that only such changes where done. All the tests should still pass. If they are high level there should be no reason for them to change.
  - Post change redesign is a refactoring at the end of the passing test where the quality of design is improved. Attention: big post change redesign could suffer from over design and a break of YAGNI (You aren't gonna need it) principle.
  - A bug fix is an intentional break of OCP possible when all the clients are under control or can be assessed related to the impact. Even then a gradual change with a feature flag is best. This should happen with minimal coding that pass the test.
  - New features is not breaking OCP as are extensions.
- More complex changes are breaking the OCP and needs more careful investigation.

Code review resouces

Concrete advices

CR1: An exception should be handled only once. Don't log it and throw again.
CR2: Don't swallow exception. At least log it at trace level
- java: Worst case scenario for handling is logger.trace("Swallowed error",ex);
CR3: Don't convert an exception to a message and lose the stacktrace

Advices

Random list of things to review. See also Coding Style and Java Coding Style

always use ConstructorInjection - no object build in phases
on collections
- do not compare size with zero use isEmpty
  - bad: collection.size() == 0
    - this will need to create any intermediary collection. (for example will produce entire stream)
  - good: collection.isEmpty()
    - this will short circuit any collection
- collection.size() >= 1 => collection.nonEmpty() or !collection.isEmpty()
- collection.size() != 0 => collection.nonEmpty() or !collection.isEmpty()
- collection.size() > 0 => collection.nonEmpty() or !collection.isEmpty()
- filtering and counting instead of directly counting will store a collection (what about size() on a iterator?) collection.filter(predicate).size() => collection.count(predicate)
```
public int activeMarketsCount() {
  return RichIterable.ofVavr(this.markets)
    .filter(m -> m.isActiveForOffer())
    .size();
}
```
ideally lambda expressions that do not need anything from context should be private static final fields.

json serialization

can happen on fields that are Iterable<T>

@AllArgsConstructor
@NoArgsConstructor
public static class OfferBetslipMarketModel {
  public String name;
  public String code;
  public String marketTypeName;
  public Iterable<OfferOutcomeModel> outcomeItems;
}

can happen on fields that are Iterator<T> (iterable only once)

log and operations done on log
- search log.debug( with +
- sample: log.debug("feedCache matches premarkets " + livematch.asMap().keySet());
CPU is 100% from a bug or too many live games
- switch XML processing from jaxb to Jackson
- switch reading collections from java directly to vavr
  - convert jaxb to jackson annotations
  - add tests for preplayOdds for sports
buffering: from multiple calls to same URL all except the last should be discarded to process only the latest
use json response instead of XML. might be more reliable and uniform
remove conversion between vavr and standard
- asJava
- toJava
@lombok.ToString has performance issues because is easy to call toString on objects. @ToString(onlyExplicitlyIncluded = true) should be a default. Attention ToString is automatically included for any @Data, @Value maybe others.
Do not convert java collections to vavr just to iterate once. Use Iterator.ofAll(...) that returns a Traversable. If the collection needs to be kept a toStream() can be called that is evaluated lazy.
search Vavr.list( and try to use native vavr
fix most //TODO
check ignored update feedTimestamp 2020-12-12T13:47:15Z/hb:6 is older than current 2020-12-12T18:15Z/hb:0
vavr instead of collection.filter() is better to collection.iterator().filter. The second doesn't create an intermediate collection
in vavr doing map/filter/... operations on the iterator is faster. You can convert it to the desired datastructure very late or at least to stream since you're not sure.
Vavr deserializer converts java.List to Seq or anything else. For big file is best to keep it native to java. After that we can get iterators to them as needed.
- Feed and *Update collections are a good candidate for this since they will be just converted to updates and domain
- https://www.daniellas.tech/posts/speed-of-stream.html
attention iterator.size will consume it

use Vavr.sortedIterator

/**Having an iterator by sorting we want to get a new iterator not a vavr collection that is more heavy:time & memory.*/
public static <T> Iterator<T> sortedIterator(Iterator<T> original, Comparator<T> comparator) {
  return Iterator.ofAll(original.toJavaStream().sorted(comparator).iterator());
}

jackson vs gson - https://www.ericdecanini.com/2020/10/13/benchmarking-gson-vs-jackson-vs-moshi-2020/
All money in UX should have two decimals #.##
All money in backend as BigDecimal (not double, float, Double, float, Long, long, Integer, int, String)
All time stored as OffsetDateTime (not ZonedDateTime, Local, ... )
In tables standard fields:
- id - generated
- code - String - business unique id (ideally, not mandatory). Could also be userCode, leagueCode.
- created
- updated
- whom
In Model fields code fields should be prefixed by the type of data for example: userCode, leagueCode, guildCode, sportCode, matchCode
- This is translated in json and you don't have the type information
UI should consume *Model classes not *Domain

Library Usage

Always use constructor injection. This also solves problems on instantiating hierarchies of objects and checks that they are not having cycles.

For example this will generate a constructor based on the fields that are final or @NonNull. The other field will be injected later. Notice also additional constructor.

@Service
@RequiredArgsConstructor(onConstructor_ = @Autowired)
@NoArgsConstructor(access = AccessLevel.PRIVATE, force = true)
@lombok.experimental.FieldDefaults(makeFinal = true, level = AccessLevel.PRIVATE)
public class XUserApi {
  public final PlayerProfileService playerProfileService1;
  @org.checkerframework.checker.nullness.qual.NonNull
  public PlayerProfileService playerProfileService2;
  PlayerProfileService playerProfileService3; //automatically private final
  @Autowired
  @Value("${app.guest.username}")
  @NonFinal //to not be included in RequiredArgConstructor
  private String guestUsername;
  //for tests
  public XUserApi(String guestUsername, boolean guestAllow) {
    this();
    this.guestUsername = guestUsername;
    this.guestAllow = guestAllow;
  }
}