1
\$\begingroup\$

I am newbie in scala don't know how to refactor to make it better because currently it kinds of boring when read it. I have 2 classes BidWinPostData and BidEventPostData. They are kind similar the different between of 2 is in modifier when call getSelectorAndModifier.

I am using it at

bidEventRDD.foreach { line =>
        println("bidEventRDD => " + line)
        implicit val formats = Serialization.formats(NoTypeHints)
        val bidEventPostList = parse(line).extract[List[BidEventPostData]]
        for (event <- bidEventPostList) {
          val selector, modifier = BidEventPostData.getSelectorAndModifier(event)
          if (event.isPerCampaign || event.isPerChannel) {
            aggregationCol.findAndUpdate(selector, modifier, upsert = true)
          }
          else if (event.isDailyWin) {
            dailyWinCol.findAndUpdate(selector, modifier, upsert = true)
          }
        }
      }
      bidWinRDD.foreach { line =>
        implicit val formats = Serialization.formats(NoTypeHints)
        println("bidWinRDD => " + line)
        val bidWinList = parse(line).extract[List[BidWinPostData]]
        for (event <- bidWinList) {
          val selector, modifier = BidWinPostData.getSelectorAndModifier(event)
          if (event.isPerCampaign || event.isPerChannel) {
            aggregationCol.findAndUpdate(selector, modifier, upsert = true)
          }
          else if (event.isDailyWin) {
            dailyWinCol.findAndUpdate(selector, modifier, upsert = true)
          }
        }

      }

This is 2 classes

case class BidWinPostData(width: Option[Int], height: Option[Int], cid: String, host: Option[String],
                          per_channel: Option[Boolean], per_campaign: Option[Boolean],
                          totalBudgetSpent: Long, totalAuctionSpent: Long, totalWin: Long,
                          startOfDayTs: Option[Long], wid: String){
  def isValidEvent: Boolean = {
    this.wid != null || this.cid != null
  }

  def isPerCampaign: Boolean = {
    this.per_campaign != null && this.per_campaign.nonEmpty
  }

  def isPerChannel: Boolean = {
    this.per_channel != null && this.per_channel.nonEmpty
  }

  def isDailyWin: Boolean = {
    this.startOfDayTs != null && this.startOfDayTs.nonEmpty
  }
}

object BidWinPostData {
  def getSelectorAndModifier(event: BidWinPostData): (BSONDocument, BSONDocument) = {
    var selector = BSONDocument()
    var modifier = BSONDocument()
    if (event.isValidEvent) {
      if (event.isPerCampaign) {
        if (event.wid != null) {
          selector = BSONDocument(
            "per_campaign" -> true,
            "wid" -> event.wid,
            "cid" -> event.cid)
        }
        else {
          selector = BSONDocument(
            "per_campaign" -> true,
            "cid" -> event.cid)
        }
        modifier = BSONDocument("$inc" -> BSONDocument(
          "totalBudgetSpent" -> event.totalBudgetSpent,
          "totalAuctionSpent" -> event.totalAuctionSpent,
          "totalWin" -> event.totalWin))      }
      else if (event.isPerChannel) {
        if (event.wid != null) {
          selector = BSONDocument(
            "width" -> event.width.get,
            "height" -> event.height.get,
            "host" -> event.host.get,
            "per_channel" -> true,
            "wid" -> event.wid,
            "cid" -> event.cid)
        }
        else {
          selector = BSONDocument(
            "width" -> event.width.get,
            "height" -> event.height.get,
            "host" -> event.host.get,
            "per_channel" -> true,
            "cid" -> event.cid)
        }
        modifier = BSONDocument("$inc" -> BSONDocument(
          "totalBudgetSpent" -> event.totalBudgetSpent,
          "totalAuctionSpent" -> event.totalAuctionSpent,
          "totalWin" -> event.totalWin))

      }
      else if (event.isDailyWin) {
        if (event.wid != null) {
          selector = BSONDocument(
            "timestamp" -> event.startOfDayTs.get,
            "widgetid" -> event.wid,
            "campaignid" -> event.cid)
        }
        else {
          selector = BSONDocument(
            "timestamp" -> event.startOfDayTs.get,
            "campaignid" -> event.cid)
        }
        modifier = BSONDocument("$inc" -> BSONDocument("total_win" -> event.totalWin))
      }
    }
    (selector, modifier)
  }
}


case class BidEventPostData(width: Option[Int], height: Option[Int], cid: String, host: Option[String],
                            per_channel: Option[Boolean], per_campaign: Option[Boolean], auction: Long, startOfDayTs: Option[Long],
                            wid: String) {
  def isValidEvent: Boolean = {
    this.wid != null || this.cid != null
  }

  def isPerCampaign: Boolean = {
    this.per_campaign != null && this.per_campaign.nonEmpty
  }

  def isPerChannel: Boolean = {
    this.per_channel != null && this.per_channel.nonEmpty
  }

  def isDailyWin: Boolean = {
    this.startOfDayTs != null && this.startOfDayTs.nonEmpty
  }
}

object BidEventPostData {
  def getSelectorAndModifier(event: BidEventPostData): (BSONDocument, BSONDocument) = {
    var selector = BSONDocument()
    var modifier = BSONDocument()
    if (event.isValidEvent) {
      if (event.isPerCampaign) {
        if (event.wid != null) {
          selector = BSONDocument(
            "per_campaign" -> true,
            "wid" -> event.wid,
            "cid" -> event.cid)
        }
        else {
          selector = BSONDocument(
            "per_campaign" -> true,
            "cid" -> event.cid)
        }
        modifier = BSONDocument("$inc" -> BSONDocument("totalAuction" -> event.auction))
      }
      else if (event.isPerChannel) {
        if (event.wid != null) {
          selector = BSONDocument(
            "width" -> event.width.get,
            "height" -> event.height.get,
            "host" -> event.host.get,
            "per_channel" -> true,
            "wid" -> event.wid,
            "cid" -> event.cid)
        }
        else {
          selector = BSONDocument(
            "width" -> event.width.get,
            "height" -> event.height.get,
            "host" -> event.host.get,
            "per_channel" -> true,
            "cid" -> event.cid)
        }
        modifier = BSONDocument("$inc" -> BSONDocument("totalAuction" -> event.auction))

      }
      else if (event.isDailyWin) {
        if (event.wid != null) {
          selector = BSONDocument(
            "timestamp" -> event.startOfDayTs.get,
            "widgetid" -> event.wid,
            "campaignid" -> event.cid)
        }
        else {
          selector = BSONDocument(
            "timestamp" -> event.startOfDayTs.get,
            "campaignid" -> event.cid)
        }
        modifier = BSONDocument("$inc" -> BSONDocument("auction_count" -> event.auction))

      }
    }
    (selector, modifier)
  }
\$\endgroup\$
1
\$\begingroup\$

To be honest, this code contains quite a few bad practices.

An easy, minor one is naming inconsistency: per_channel, totalBudgetSpent. Go for the latter, as it is the common convention is scala/java world.

The most striking problem is that it contains business logic that is very tied to the specific data representation:

The getSelectorAndModifier contains a lot of business logic in a non-type-safe way, when type-safety is the major argument in favor of Scala. The output of this logic should be some strongly typed case class with the desired data. This class should then be converted to a BSONDocument in a straight-forward manner.

There is indeed a degree of duplication between the two classes but the refactoring needs domain knowledge: Is the case that a BidWin has-a BidEvent or the opposite then one should contain the other as an argument? Is it one-to-one? Are they unrelated but are sharing some information that could be recorded in a BidInfo class which they would inherit? These answers are not directly related to the data representation, which may be structured around performance compromises.

Lastly, the isPerCampaign, isPerChannel, isDailyWinmethods strongly suggest the existence of a hierarchy. Having these cases represented as different classes buys you both code clarity and type-safety. Again the decision of creating a hierarchy should not be tied to the representation of the data. In fact, Scala's match-case syntax is a very elegant way of moving from hierarchies back to flat representations or the opposite.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.