How to write less code for a long if-else statement in Scala (code review)

I have the following code:

  def upd开发者_如何学编程ateProgrammcounter(element: Name) {
    val level = findSecurityLevelOfNameInSymboltable(element.toString.trim)
    var pc = new HashSet[String]
    println("LEVEL: " + level)
    if (pc.isEmpty && level != "N") {
      // the pc is empty so add the level
      pc += level
    } else if (pc.contains("H") && level == "L") {
      // giving high information to low is not allowed
      errorReject()
    } else if (pc.contains("L") && level == "L") {
      // nothing to change because the pc is the same
      println("okay")
    } else if (pc.contains("H") && level == "H") {
      println("okay")
    } else if (pc.contains("L") && level == "H")
      // we have to raise the pc
    pc += level
    else if (level == "N") {
      println("We have a not valid operation!")
    } else {
      println("Something went terribly wrong!")
    }
  }

Do you see some level of enhancement I can do, so that this big chunk of code looks just better. Thanks for your advise, Matthias.

---

One common trick is to wrap the two values (pc and level) into a tuple, giving you a single entity that you can pattern match on:

(pc,level) match {
  case (a,b) if condition => ...
  case _ => ...
}

The check pc.isEmpty immediately after creating an empty pc suggests that you've cropped the code for stackoverflow, so the approach may not help you here. But I'll try it anyway:

def updateProgrammcounter(element: Name) {
  val level = findSecurityLevelOfNameInSymboltable(element.toString.trim)
  var pc = new HashSet[String]
  println("LEVEL: " + level)

  (pc, level) match {
    case (pc, level) if pc.isEmpty && level != "N" => pc += level
    case (pc, "L") if pc.contains("H") => errorReject()
    case (pc, "L") if pc.contains("L") => println("okay")
    case (pc, "H") if pc.contains("H") => println("okay")
    case (pc, "H") if pc.contains("L") => pc += level
    case (_,  "N") => println("We have a not valid operation!")
    case _ => println("Something went terribly wrong!")
  }
}

This is obviously nonsence, as half of those conditions are impossible, but it is a literal translation of the code you originally supplied. I'm also unhappy with the side effects and mutation, but didn't want to change too much in one sitting!

If you can add back in the code you removed, or give a better idea of what you're trying to achieve then I'll happily update my answer :)

UPDATE Prompted by Daniel, here's the next step; rearranging and optimising the cases:

(pc, level) match {
  case (_,  "N") => println("We have a not valid operation!")
  case (pc, level) if pc.isEmpty => pc += level
  case (pc, level) if pc.contains(level) => println("okay")
  case (pc, "L") if pc.contains("H") => errorReject()
  case (pc, "H") if pc.contains("L") => pc += level
  case _ => println("Something went terribly wrong!")
}

---

You can certainly rearrange this into levels, then pc:

def updateProgrammcounter(element: Name) {
    val level = findSecurityLevelOfNameInSymboltable(element.toString.trim)
    var pc = new HashSet[String]
    println("LEVEL: " + level)
    if (level == "L") {
    } else if (level == "H") {
        // All the "H" stuff in here
    } else {
       // Put the "N" case at the top here - it looks like your fall through default.
    }
  }

For the other items, perhaps you can use a Map with pc as the key and behavior functions as the values.

This might suggest that you could have L and H updater objects that you could look up using the level.

You can also combine level and pc into the key in a Map and lookup what you'd like to do.

Your instinct is good: when you see long if/then/else, you should think about polymorphism or lookups to simplify it. Think how you could modify the flow by adding objects rather than rewriting code (open/closed principle).