HiveBrain v1.2.0
Get Started
← Back to all entries
patternMinor

What takes precedence: readability or elimination of side effects?

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
whatreadabilitysideeffectseliminationprecedencetakes

Problem

I have a method:

def removeExpiredCookies(jar: CookieJar): CookieJar = {
    val currentDate = System.currentTimeMillis / 1000L
    jar.filter(_._2._2 > currentDate)
  }


You use said method like so.

var cookieJar = new CookieJar
cookieJar = removeExpiredCookies(cookieJar)


That method has no side effects. But it's used inside a class:

class CookieManager {
      type ExpiryDate = Long
      type CookieJar = scala.collection.immutable.HashMap[String, (Cookie, ExpiryDate)]

      var cookieJar = new CookieJar

      private def removeExpiredCookies(jar: CookieJar): CookieJar = {
        val currentDate = System.currentTimeMillis / 1000L
        jar.filter(_._2._2 > currentDate)
      }

      def getCookie(domain: String): Set[Cookie] = {
        cookieJar = removeExpiredCookies(cookieJar)
        cookieJar.find(_._1 endsWith (domain))
      }
}


Is it better to write the method such as above or to have a more succinct and readable code but with side-effecting methods? I'm guessing as this is encapsulated inside a class, this is actually "better" for maintainability but it is not as pure functional programming. It's an imperative style. However, the strongest argument against this is that it's more difficult to test as the tests are not as isolated.

class CookieManager {
      type ExpiryDate = Long
      type CookieJar = scala.collection.immutable.HashMap[String, (Cookie, ExpiryDate)]

      var cookieJar = new CookieJar

      private def removeExpiredCookies {
        val currentDate = System.currentTimeMillis / 1000L
        cookieJar = cookieJar.filter(_._2._2 > currentDate)
      }

      def getCookie(domain: String): Set[Cookie] = {
        removeExpiredCookies() //bracket to indicate side effect
        cookieJar.find(_._1 endsWith (domain))
      }
}

Solution

(I'm making my comment into an answer.) You can separate everything that is static (not using the state of instances of CookieManager) into a companion object (and import the object at the beginning of the class for convenience). This way, it is clear what functions use this directly or indirectly (by accessing class variables). This often helps me make my code clearer, and prevents stupid mistakes such as confuse class variables with function arguments etc.

class CookieManager {
  import CookieManager._

  var cookieJar = new CookieJar

  def getCookie(domain: String): Set[Cookie] = {
    cookieJar = removeExpiredCookies(cookieJar)
    cookieJar.find(_._1 endsWith (domain))
  }
}

object CookieManager {
  type ExpiryDate = Long
  type CookieJar = scala.collection.immutable.HashMap[String, (Cookie, ExpiryDate)]

  private def removeExpiredCookies(jar: CookieJar): CookieJar = {
    val currentDate = System.currentTimeMillis / 1000L
    jar.filter(_._2._2 > currentDate)
  }
}

Code Snippets

class CookieManager {
  import CookieManager._

  var cookieJar = new CookieJar

  def getCookie(domain: String): Set[Cookie] = {
    cookieJar = removeExpiredCookies(cookieJar)
    cookieJar.find(_._1 endsWith (domain))
  }
}

object CookieManager {
  type ExpiryDate = Long
  type CookieJar = scala.collection.immutable.HashMap[String, (Cookie, ExpiryDate)]

  private def removeExpiredCookies(jar: CookieJar): CookieJar = {
    val currentDate = System.currentTimeMillis / 1000L
    jar.filter(_._2._2 > currentDate)
  }
}

Context

StackExchange Code Review Q#28297, answer score: 2

Revisions (0)

No revisions yet.