Refactor OO code to functional

Question

Wanting to refactor this OO looking code that I just came across. author of the code says MySave class was created solely as a placeholder for keys and inserts maps. client of MySave class always creates new instance of MySave per every request and use add() method to populate keys and inserts maps. Then another function will use MySave()'s keys and inserts vals to retrieve those values from corresponding Maps and then send them to persistent layer etc.

Even though there are already flaws even in this OO design (e.g. though current usage is creating MySave object for every new request, following design doesn't prevent from creating only a single class instance and crating threadsafety issues with shared state, and it's leaking shared state via public vals etc). Instead of fixing OO design I would want to convert this into a functional code.

What would your approach be in converting to FP paradigms? I am just starting to learn about Scalaz state monad but does that sound like a good FP alternative to this OO code, or anything more trivial than that can work?

class MySave {

  val keys = mutable.Map[Entity Int]() 
  val inserts = mutable.Map[Entity, String]() 
  def add(d: Entity, value: String): \/[Throwable,Unit] = {
    //this is a side effecting method which either updates `keys` or `inserts` map
   // by simply reassigning `keys` or `inserts` vars e.g. keys += d ->value 
   }



 }

score 2 · Answer 1 · edited Jun 06 '14 at 18:42

This code is a bit impenetrable at the moment, let me see if I can tease out the relevant bits.

Disclaimer: I'm not a Scala programmer, though I have used it a little. Forgive if my syntax is off.

You're saying that the way requests are currently made is something like:

val save = MySave()
save.add(d, x)
save.add(d2, y)
...

SendToPersistentLayerEtCetera(save)

This usage pattern suggests that a (properly used) MySave is isomorphic to a series of Dictionary, String pairs. So your interface might look like:

class MySave {
    val keys = Map[Entity, Int]     # immutable maps
    val inserts = Map[Entity, Keys]
}

def save(inputs : List[Pair[Entity, String]]) : MySave

You can pass MySave to SendToPersistentLayerEtCetera as before, but now MySave cannot be misused. Whether save is implemented functionally or imperatively is irrelevant to the design. If you want to implement save functionally, it looks like a left fold to me (fold the function that inserts a Pair[Entity,String] into a MySave, returning the new MySave).

If you expect there to be a lot of save calls such that the expense of creating a List is too great, I would suggest using a lazy stream, which can be left-folded in constant memory.

you're right with your comment/snippet how save obj is passed to persistentLayer(save) and then inside persistentLayer it does save.keys, save.inserts to get those values.I didn't quite understand your isomorphic comment related to Dictionary. also with your approach add() method will still be side effecting and updating keys and inserts vals? in your def save() you're passing a pair but then when persistantLayer tries to do mySave.keys, mySave.inserts how would that part work? sorry for too many questions but looks like I am missing to understand some details.can you pls elaborate? — user2066049, May 31 '14 at 12:37
This approach combined with the state monad is what you want. — Ivan Meredith, Jun 01 '14 at 08:06

Ivan Meredith · Answer 2 · 2014-06-01T08:25:53.210

To move this code to something more functional, instead of

val save = new MySave()
save.add(d, x)
save.add(d2, y)

You could do something like the following

case class MySave(keys: Map[Dictionary, Int], inserts: Map[Dictionary, String])
def add(mySave: MySave, d: Dictionary, value: String): MySave = {

val save1 = add(MySave(), d, x)
val save2 = add(save1, d2, y)

This is nice and immutable, however it's sucky that you have to pass around the state, what you really want is the state monad.

case class MySave(keys: Map[Dictionary, Int], inserts: Map[Dictionary, String])

def add(d: Dictionary, value: String): State[MySave, Unit] = ...

This would let you do the following...

val prog = for {
  _ <- add(d, x)
  _ <- add(d2, y)
} yield ()

prog.exec(MySave())

And now you no longer have to pass the MySave instance around.

For help with implementing state, or to find out my I recommend trying it out first, then posting a question on SO, or try #scalaz on freenode.

score 0 · Answer 3 · answered May 31 '14 at 04:36

I still don't understand some of the details of your code, but I think there are some clear pointers that can be given. First thing you could do would be to make the whole MySave class immutable like this:

  case class MySave( val keys: Map[Dictionary Int], inserts: Map[Dictionary, String])

Second thing you could do is to modify the add function such that it returns the new state of the MySave object (Maybe as you have noted this could also be done with a State monad but that method is not so clear for me right now). The new function could be something like this:

def add(dd: Dictionary, value: String): \/[Throwable,MySave] = {
nmField match {
     case Some(id) =>
     val validId = checkDictType("numeric", value)
     for {
           k <- validId
     } yield { MySave(keys + (dd -> k) , inserts) }
     case None =>
         for {
           r <- validateDictTypeAndValue(dd, value)
      } yield { MySave( keys, inserts + (dd -> value) ) }
}
}

I guess checkDictType and validateDictTypeAndValue return Either values, right? I'm still not sure if there is some way to make it even more "functional", for example using the State monad as you suggested. In that case you may want to use some Monad Transformer to make it "interoperate" with Either.

Refactor OO code to functional

3 Answers3