What takes precedence: readability or elimination of side effects?

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


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

class CookieManager {
type ExpiryDate = Long

val currentDate = System.currentTimeMillis / 1000L
jar.filter(_._2._2 > currentDate)
}

}
}


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

val currentDate = System.currentTimeMillis / 1000L
}

removeExpiredCookies() //bracket to indicate side effect
}
}

• 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. – Petr Pudlák Jul 9 '13 at 21:15
• Actually putting the types in the companion object seems to hide them - I guess I can call them out at CookieManager.CookieJar – JasonG Jul 10 '13 at 1:57
• 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. – JasonG Jul 10 '13 at 2:20

(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 {

}
}

type ExpiryDate = Long

val currentDate = System.currentTimeMillis / 1000L
jar.filter(_._2._2 > currentDate)
}
}

• I think this is a good improvement – JasonG Jul 10 '13 at 13:28
• 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. – JasonG Jul 10 '13 at 16:34
• @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. – Petr Pudlák Jul 10 '13 at 16:48
• Okay I'll revert that – JasonG Jul 10 '13 at 17:04

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] = {
}


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