code inspection

code inspection.md

Yesterday, I reviewed my colleague's code which implemented a few tie-break rules to our search api. the code roughly looks like follow(to save space, I removed a few rules). for each tie-break method, it calls direct to the next rule if the condition is not satisfied.

  private def tieBreakerSortWithinESRelevanceGroups(decoratedSearchRequest:DecoratedSearchRequest, leftListing:ListingInfo, rightListing:ListingInfo):Boolean = {
    def tieBreakOnRoiScore(decoratedSearchRequest:DecoratedSearchRequest, leftListing:ListingInfo, rightListing:ListingInfo):Boolean = {
      (decoratedSearchRequest.intent, leftListing.roiScore, rightListing.roiScore) match {
        case (Some(Intent.TYPE), leftRoi, rightRoi) if (leftRoi == rightRoi) => tieBreakOnSource(decoratedSearchRequest, leftListing, rightListing)
        case (Some(Intent.TYPE), leftRoi, rightRoi) => leftRoi < rightRoi
        case _ => tieBreakOnSource(decoratedSearchRequest, leftListing, rightListing)
      }
    }
    ... //more rules defined.
    ...
    def tieBreakOnListingId(decoratedSearchRequest:DecoratedSearchRequest, leftListing:ListingInfo, rightListing:ListingInfo):Boolean = {
      leftListing.id < rightListing.id
    }
    if (leftListing.score == rightListing.score) 
      tieBreakOnRoiScore(decoratedSearchRequest, leftListing, rightListing)
    else 
      rightListing.score < leftListing.score   // relevance score in descending order
  }

The code is implemented correctly since it meets the business requirement. However from programmer's perspective, I think it could be improved further. But, first of all, what's the problem of the code?

• unit-testabilites: because each rule is encoded inside a big method, unit testing individual rules become unfeasible
• requirement change: if customer want to change the sequence of application of the rules, a lot of code changes occurs therefore
• sharability: those rules are too local to be shared.(maybe there is no requirement for sharing those rules.)

The remediation:

• instead of encapsulating all rules in a big block, we can implement it individually and change the return type to Option[Boolean]: for instance: def tieBreakOnListingId(req:DecoratedSearchRequest, l:ListingInfo, r:ListingInfo):Option[Boolean] inside the method, it will return Some[Boolean] when the rule is applied and None if not.
• compose the rules via scalaz <+> operator to build up a bigger rule: tieBreakOnListingId(req, l, r) <+> tieBreakOnROI(req, l, r) <+> tieBreakOnSomethingelse(req, l, r)