3
\$\begingroup\$

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))
      }
}
\$\endgroup\$
  • \$\begingroup\$ You could also put removeExpiredCookies to CookieManager's (along with the types) companion object. This way, it'd be obvious that it doesn't use or modify any CookieManager's state. \$\endgroup\$ – Petr Pudlák Jul 9 '13 at 21:15
  • \$\begingroup\$ Actually putting the types in the companion object seems to hide them - I guess I can call them out at CookieManager.CookieJar \$\endgroup\$ – JasonG Jul 10 '13 at 1:57
  • \$\begingroup\$ Yes I think you're right - putting things in the companion object appears to be much nicer. The methods are side effect free so anything that is side effect free can be static. It makes the class very nice. \$\endgroup\$ – JasonG Jul 10 '13 at 2:20
2
\$\begingroup\$

(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)
  }
}
\$\endgroup\$
  • \$\begingroup\$ I think this is a good improvement \$\endgroup\$ – JasonG Jul 10 '13 at 13:28
  • \$\begingroup\$ Implicits can be used as well - I'm going to edit your post to demonstrate how implicit can be used to further get the code cleaner. \$\endgroup\$ – JasonG Jul 10 '13 at 16:34
  • \$\begingroup\$ @JasonG I'd say in this case implicits go against clean code. Generally, implicits make often code harder to read - you have to search, from where an implicit comes from. I'd definitely not use it in this case. I'd rather make removeExpiredCookies a member of the class and let it use cookieJar variable directly, if you want to take this approach. \$\endgroup\$ – Petr Pudlák Jul 10 '13 at 16:48
  • \$\begingroup\$ Okay I'll revert that \$\endgroup\$ – JasonG Jul 10 '13 at 17:04
2
\$\begingroup\$

What I see in your code are side effects which are completely orthogonal to readability. Why are you purging the cookie jar whenever you fetch a cookie? If you are doing it to save having a separate Purge method, that's lazy in the worst way. What is ostensibly a read-only method is modifying the object. Why? What do you gain apart from not having to think about cookie management (this is lazy in a bad way)? What you lose is the ability to inspect and manage expired cookies (there may be many valid reasons for wanting to do this). Why hide a surprise side effect like this?

If you want getCookie only to return current cookies, have it fetch from a filtered cookiejar. You can achieve this simply by changing that method to this:

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

Create a separate purgeJar method which actually removes expired cookies. Know when you are modifying the object.

Personally, I'd rather give the calling code the choice about how to filter the jar. If you

  1. add a set of useful filters to the CookieManager class (including a noExpiredCookies filter)
  2. allow for the creation of new cookieJar filters
  3. add a filter parameter to getCookie
  4. have that parameter default to noExpiredCookies

You get the current behaviour while offering much more flexibility.

Really, though, for the best of readability, minimisation of unexpected side-effects and flexibility, I recommend you create a CookieJar class which knows how to do all the basic things with cookies and a CookieManager class which knows how to enforce policy on cookies (e.g. ensuring that only current cookies are returned by default, choosing a cookie purging policy etc.)

\$\endgroup\$
  • 1
    \$\begingroup\$ Really good advice about the side effects of purging - I agree with that 100% and have to bring that attentiveness to my day to day. Thanks for the review! \$\endgroup\$ – JasonG Aug 22 '13 at 17:39
  • \$\begingroup\$ Hey I wanted to come back and thank you again - I just reviewed and refactored a co-workers code today and I pulled out the same piece of advice where there was a side-effect stuffed somewhere. I pulled it out and put it into the top level space where a message was being handled. Much better now -easier to understand etc. \$\endgroup\$ – JasonG Aug 26 '13 at 21:05
  • \$\begingroup\$ No problem :) The people who have to deploy your code and make it scale will be very grateful to you. For them, it is usually very important to know which code changes state (and thus requires write access to some persistent store) and which code does not (and so can make do with a read-only cache/replica). \$\endgroup\$ – itsbruce Aug 27 '13 at 9:22

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.