4
\$\begingroup\$

I have four methods like these (here are only two of them):

  def checkLeft(clickedIndex: Int): Option[Int] = {
    val leftIndex = clickedIndex - 1
    if (leftIndex >= 0 && clickedIndex % field.width != 0 && isEmptyCell(leftIndex))
      Some(leftIndex)
    else
      None
  }

  def checkRight(clickedIndex: Int): Option[Int] = {
    val rightIndex = clickedIndex + 1
    if (rightIndex < field.size && clickedIndex + 1 % field.width != 0 && isEmptyCell(rightIndex))
      Some(rightIndex)
    else
      None
  }

They all have similar structures. How I can reduce code duplication here?

\$\endgroup\$
  • \$\begingroup\$ Does bottomIndex depend on clickedIndex? And what something1 depends on? \$\endgroup\$ – Petr Pudlák Jul 22 '13 at 16:44
  • \$\begingroup\$ @PetrPudlák I edited question with real code. Please review it \$\endgroup\$ – MyTitle Jul 22 '13 at 16:49
  • \$\begingroup\$ Pretty sure in the second block you'll want parens around clickedIndex + 1 before the % operator. \$\endgroup\$ – Carl Manaster Jul 22 '13 at 17:02
2
\$\begingroup\$

For the LEFT case, you're checking that an index is above the low value of zero; for the RIGHT case, you're checking that it's below the high value of field.size. You can't really reconcile these except by combining them into a method that checks both boundaries - and there's nothing wrong with that. Now you've got

if (inBounds(theIndex) && isEmptyCell(theIndex) && ...)
    Some(theIndex)

That ... is questionable to me because you seem to be comparing a different index (clickedIndex or clickedIndex + 1) depending on LEFT or RIGHT.

I think there are errors here that you need to reconcile before going too far down the path of duplication elimination.

\$\endgroup\$
2
\$\begingroup\$

One way to do this would be with a trait or abstract class defining the basic behaviour of a check. Then you extend that for the actual checks you want.

trait indexCheck {
  def adjustedIndex( index: Int): Int = index
  def newIndexCondition(index: Int): Boolean = true
  def oldIndexCondition(index: Int): Boolean = true
  def apply( clickedIndex: Int): Option[Int] = {
    val returnIndex = adjustedIndex(clickedIndex)
    if (newIndexCondition(returnIndex) && oldIndexCondition(clickedIndex) && isEmptyCell(returnIndex))
      Some(returnIndex)
    else None
  }
}

Then you create checkLeft and checkRight classes, overriding methods as necessary

class checkLeft extends indexCheck {
  override def adjustedIndex(index: Int): Int = index - 1
  override def newIndexCondition(index: Int): Boolean = {
     index >= 0
  }
  override def oldIndexCondition(index: Int): Boolean = {
      index % field.width != 0
  }
}

aCheck = new checkLeft
aCheck(clickedIndex) // The magic apply method is used here.

My example above is probably a little over-specific. It would be more flexible just to have a conditionList onto which could be pushed a sequence of functions returning booleans. But you get the idea.

By the way, I used traits in case you wanted to be able to create classes which did radically different things but also implemented checks. However, if you made indexCheck a class rather than a trait, you could do things like building maps of anonymous classes, like this:

val checks:Map[String,indexCheck] = Map(
  "left" -> new indexCheck {
    override def adjustedIndex(index: Int): Int = index - 1
    override def newIndexCondition(index: Int): Boolean = {
      index >= 0
    }
    override def oldIndexCondition(index: Int): Boolean = {
      index % field.width != 0
    }
  },
  "right" -> new indexCheck {
    override def adjustedIndex(index: Int): Int = index + 1
    override def newIndexCondition(index: Int): Boolean = {
      index < field.size
    }
    override def oldIndexCondition(index: Int): Boolean = {
      Index + 1 % field.width != 0
    }
  }
)

Which gives you a map of anonymous classes, which you can use like this:

checks("left")(SomeValue)
checks("right")(SomeOtherValue)

or whatever.

Another way to do this would be through higher order functions. That is, methods can return functions just like any other value/object. So you could have a method that took various parameters (some of them would have to be closures) and returned a function that would implement a specific check - a check factory, if you like. I'd need to know more about the parent object to give a useful demo of that.

\$\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.