A Cipher with 2 methods for encryption/decryption

Question

I'd like advice on how this could be written more efficiently, idiomatically, or just anything that could be improved upon.

I'd also like to know if there's an alternative to my forString method; I couldn't find anything similar in the library (and Scalex seems to be down, so I couldn't search).

There are 2 provided ways of encrypting a message:

val message:String = "Hello World!"
val key:String = "Key"
val encrypted = Cipher(message).simpleOffset(5)
println(encrypted) "Mjqqt%\twqi&"
val decrypted = Cipher(encrypted).simpleOffset(-5)
println(decrypted) "Hello World!"
val encrypted2 = Cipher(message).encryptWith(key)
println(encrypted2) "5l?Yv;Dv?Yk<"
val decrypted2 = Cipher(encrypted2).decryptWith(key)
println(decrypted2) "Hello World!"

and the code:

class Cipher(message:String) {
 val cHARMIN = 32 //Lower bound
 val cHARMAX = 126 //Upper bound
 //"Wraps" a character, so it doesn't go under cHARMIN, or over cHARMAX
 //Takes an Int so the Char can't underflow prior to being passed in
 private def wrap(n:Int):Char =
 if (n > cHARMAX) (n - cHARMAX + cHARMIN).toChar
 else if (n < cHARMIN) (n - cHARMIN + cHARMAX).toChar
 else n.toChar
 private def forString(str:String)(f:Char => Char):String = {
 var newStr = ""
 for(c <- str) { newStr += f(c) }
 newStr
 }
 //Creates a repeated key that's at least as long as the message to be encrypted
 private def getRepKey(key:String):String =
 if (key.length >= message.length) key
 else key * (message.length / key.length + 1)
 //Modifies a letter in the message, based on the respective letter in the key,
 // and the given binary operator
 private def modifyWithStringWith(f:(Int,Int) => Int)(strKey:String):String = {
 var newString = ""
 (message zip getRepKey(strKey)).foreach { case (c1,c2) =>
 newString += wrap( f(c1,c2) ) }
 newString
 }
 //Offsets each character in a String by a given offset
 def simpleOffset(offset:Int):String =
 forString(message) { (c:Char) =>
 wrap((c + offset).toChar)
 }
 //Encrypts/Decrypts using another String as the key
 def encryptWith(key:String):String =
 modifyWithStringWith(_+_)(key)
 def decryptWith(key:String):String =
 modifyWithStringWith(_-_)(key)
}
object Cipher {
 def apply(msg:String):Cipher = new Cipher(msg)
}

• itsbruce: Um, Scala Strings have map. No need at all for your forString method. docs.scala-lang.org/overviews/collections/strings.html
• Carcigenicate: @itsbruce That doesn't return a String though.
• itsbruce: Yes, it does. Read the documentation. scala-lang.org/api/current/…
• Carcigenicate: @itsbruce Weird. I was getting back an IndexedSeq (or something like that), and it wasn't readily convertible to a String. Thanks
• itsbruce: There also seem to be some unseen (untested?) errors in your code. Your wrap function fails if a character has a numeric value higher than 220 or lower than -63. I don't think its logic is right even within those bounds. Are you aware that the third line of your simpleOffset method is useless?

Answer 1

forString

As discussed already in comments, your forString method is not needed because map is available to Scala Strings. But if you do need to construct such a method, your design is not best practice. Specifically, you create a local var, append to it repeatedly from within a for block and return that.

This block

 var newStr = ""
 for(c <- str) { newStr += f(c) }
 newStr

can be written simply as

 for (c <- str) yield f(c)

Updating an external var from within a for block is a stateful, potentially error-prone, very non-FP practice which you should use with care. It is not needed here. Leave the compiler to construct and return the string - it will be more efficient and your code is cleaner as a result.

(It will be more efficient because in the for-yield block, the compiler knows it is creating a sequence one item at a time and begins each step with a ready pointer to the end of the collection. In contrast, when your code explicitly appends to the end of a sequence each time, it has to traverse the entire sequence each time. No handy reference to the end of the string is cached. Appending to strings is expensive.)

wrap

Both the original version and the modulo alternative in your answer use if... else if... then chains. These are a bad smell. All three of your options depend upon the value of the input parameter, but nothing about an if-elseif-then chain forces you to cite that parameter in each successive condition. Pattern matching would at least ensure that the same object is being assessed for each action. That would look something like this:

n match {
 case _ if (n > cHARMAX) => (cHARMIN + (n - cHARMAX) % cRANGE)
 case _ if (n < cHARMIN) => (cHARMAX - (cHARMIN - n) % cRANGE)
 case _ => n
}

However, although I am still not sure why you have this function in there, I think this is what you are trying to do:

n match {
 case _ if (n > cHARMAX) => cHARMIN + ((n - cHARMIN) % cRANGE)
 case _ if (n < cHARMIN) => cHARMAX - ((cHARMIN - n) % cRANGE)
 case _ => n
}

Key length

Repeating the key until it is as long as the message is a waste of resources (more so, the longer the message becomes). Just leave the key in its original size. There are several ways to iterate through the message and (effectively or literally) use modulo arithmetic to fetch the corresponding character from the key.

1. Create an endlessly repeating Stream from the key (fast and efficient). Then you can just iterate through both in step (for example, using zipped)
2. for (i <- message.indices) yield myFunction(message(i), key(i % key.length)) (reasonably fast and efficient)
3. Use zipWithIndex and map to turn the message into a list of tuples (each character bundled with its index) and then iterate through that, using modulo arithmetic on the index to fetch the right key character (less efficient, slower).

Option 1 is best

Cipher class design

The most curious thing about your Cipher class is that its Apply method takes a message as a parameter. This is not good OO design and it also inverts the meaning of the word (OK, cipher can mean an encrypted text but more commonly it means one particular encryption method with one particular key choice). It would make more sense for a cipher object to have the key (and possibly some other parameters like, for example, the range of valid characters) as a field. So you could create a cipher instance containing the key "MyAmazingSecretCode" and then use that object to encrypt or decrypt as many messages as you want. As things stand, Cipher is mostly a java-like utility class containing static methods, offering no way to maintain any state.

Having the two different encryption options in one class is also less good. Are you going to rewrite this class every time you add another variant on the encryption method?

Consider, instead, a basic Cipher class or trait or even interface, which describes the key interface methods (setting the secret, encrypting, decrypting). If it is a class or trait, you could also design an "encryption strategy" trait from which specific encryption methods (your simple offset and your substitution key, as two examples) could be derived. You could then mix the appropriate strategy trait into the Cipher class to provide a working cipher. If you choose to make Cipher an interface, then your two strategies could be separate classes which each incorporate that interface. Which to choose depends on larger design considerations.

Answer 2

Style

When you declare a type of a function, value, or variable, add a space between the : and the type.

// this
def func(...): T = {...}
val xs: List[T] = List[T](...)
// not this
def func(...):T = {...}
val xs:List[T] = List[T](...)

Code

Below is another interpretation of how you might write your code. I've separated all your original parts into a set of case classes and one trait. For example, simpleOffset is now called Caesar and it extends the Cipher trait. I imagine you were aware of it, but simpleOffset was essentially a Caesar cipher. Likewise, all the logic you had embedded in the rest of your program, particularly how you were dealing with the the key string and the wrap function resulted in something close to a Vigenere cipher. In order to align the key with the input text I choose to utilize a method on the list class, sliding, which works as follows:

val xs = List(1, 2, 3, 4)
val ys = xs.sliding(2, 2).toList
// ys == List(List(1, 2), List(3, 4))

Well, that about sums up the changes I would make. If you have any questions, don't hesitate to comment! :)

case class PlainText(msgStr: String) 
case class CipherText(msgStr: String) 
case class Key(keyStr: String)
trait Cipher {
 val max = 126
 def E(p: PlainText): CipherText
 def D(c: CipherText): PlainText
}
case class Caesar(shift: Int) extends Cipher {
 def op(xs: List[Char], f: (Char) => Int): String = {
 val xss = xs map(c => f(c))
 (xss map(i => i.toChar)).mkString
 }
 def E(p: PlainText): CipherText = {
 val str = op(p.msgStr.toList, (c: Char) => c + shift % max)
 CipherText(str)
 }
 def D(c: CipherText): PlainText = {
 val str = op(c.msgStr.toList, (c: Char) => c - shift % max)
 PlainText(str)
 }
}
case class Vigenere(k: Key) extends Cipher {
 val blockSize = k.keyStr.length()
 def op(xs: List[Char], f: (Char, Char) => Int): String = {
 val ys = xs.sliding(blockSize, blockSize).toList
 val zs = (ys map(_ zip k.keyStr)).flatten
 (for {
 (m, k) <- zs
 result = f(m, k).toChar
 } yield result).mkString
 }
 def E(p: PlainText): CipherText = {
 val msgStr = op(p.msgStr.toList, (m: Char, k: Char) => m + k % max)
 CipherText(msgStr)
 }
 def D(c: CipherText): PlainText = {
 val msgStr = op(c.msgStr.toList, (m: Char, k: Char) => m - k % max)
 PlainText(msgStr)
 }
}

Answer 3

Using %, wrap no longer breaks:

val cRANGE = cHARMAX - cHARMIN
private def wrap(n:Int):Char = (
 if (n > cHARMAX) (cHARMIN + (n - cHARMAX) % cRANGE)
 else if (n < cHARMIN) (cHARMAX - (cHARMIN - n) % cRANGE)
 else n
 ).toChar

• itsbruce: shame about the if... else if... else...
• Carcigenicate: I was trying to emulate guards. How would you arrange it?
• itsbruce: Funny you should mention guards. Proper pattern matching with real guards would be better than the if else if chain with pseudo-guards. However, this is actually one simple arithmetical function with only slight variations and would be better written that way. Will try to find time for an answer tonight.
• itsbruce: Is this function supposed to limit the encrypted text to that range of characters, or is valid input only allowed from that range? Because the function affects both, the way this code is designed.