A Cipher with 2 methods for encryption/decryption

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 Char):String = {
var newStr = ""
for(c = 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:St

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

• Create an endlessly repeating Stream from the key (fast and efficient). Then you can just iterate through both in step (for example, using zipped)

• for (i






• 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.

StackExchange Code Review Q#74183, answer score: 4