HackerRank Caesar Cipher, not using map, filter, reduce, regex

Question

Julius Caesar protected his confidential information by encrypting it in a cipher. Caesar's cipher rotated every letter in a string by a fixed number, K, making it unreadable by his enemies. Given a string,S , and a number, K , encrypt S.

Note: The cipher only encrypts letters; symbols, such as -, remain unencrypted.

func cipher(messageToCipher: String, k: Int)-> String{ var eMessage = "" let arr = messageToCipher.characters.map { String($0) } for ch in arr { for code in String(ch).utf8 { if (65<=code && code<=90) || (97<=code && code<=122) { if k > 26{ let value = k % 26 == 0 ? k / 26 : k % 26 var pCode = ((Int(code)) + (value)) if (pCode > 122 && (97<=code && code<=122)) || (pCode > 90 && (65<=code && code<=90)) { pCode = pCode - 26 } let s = String(UnicodeScalar(UInt8(pCode))) eMessage = eMessage + s }else { var pCode = Int(code) + k if (pCode > 122 && (97<=code && code<=122)) || (pCode > 90 && (65<=code && code<=90)) { pCode = pCode - 26 } let s = String(UnicodeScalar(UInt8(pCode))) eMessage = eMessage + s } } else{ eMessage = eMessage + ch } } } return eMessage } print(cipher(messageToCipher: "abc", k: 2) ) 

I solved this way and it was passed all test cases.

More information on this problem: Caesar Cipher

Answer 1

Enumerating UTF-8 characters: Your code uses two nested loops to encrypt letters (based on the UTF-8 code):

var eMessage = "" let arr = messageToCipher.characters.map { String($0) } for ch in arr { for code in String(ch).utf8 { if (65<=code && code<=90) || (97<=code && code<=122) { // ... translate `code` and append to `eMessage` ... } else{ eMessage = eMessage + ch } } } 

This is unnecessary complicated and has an unwanted side effect if the message contains non-ASCII characters (which are encoded as 2 or more UTF-8 code units):

print(cipher(messageToCipher: "ä € 😀 🇧🇩", k: 2) ) // ää €€€ 😀😀😀😀 🇧🇩🇧🇩🇧🇩🇧🇩🇧🇩🇧🇩🇧🇩🇧🇩 

This could be solved by adding a break in the else-case. The better solution is to enumerate the UTF-8 code units directly:

var encoded: [UInt8] = [] for code in messageToCipher.utf8 { if (65<=code && code<=90) || (97<=code && code<=122) { // ... translate `code` and append to `encoded` ... } else{ encoded.append(code) } } return String(bytes: encoded, encoding: .utf8)! 

Code duplication: There is some duplicate code in

 if k > 26{ let value = k % 26 == 0 ? k / 26 : k % 26 var pCode = ((Int(code)) + (value)) if (pCode > 122 && (97<=code && code<=122)) || (pCode > 90 && (65<=code && code<=90)) { pCode = pCode - 26 } let s = String(UnicodeScalar(UInt8(pCode))) eMessage = eMessage + s }else { var pCode = Int(code) + k if (pCode > 122 && (97<=code && code<=122)) || (pCode > 90 && (65<=code && code<=90)) { pCode = pCode - 26 } let s = String(UnicodeScalar(UInt8(pCode))) eMessage = eMessage + s } 

which is not necessary. The if-part works for the case k <= 26 as well.

Exceptional key values: The special handling of the case k % 26 == 0 causes unexpected results:

print(cipher(messageToCipher: "abc xyz ABC XYZ", k: 26) ) // abc xyz ABC XYZ print(cipher(messageToCipher: "abc xyz ABC XYZ", k: 52) ) // cde zab CDE ZAB 

and is not needed. Negative key values are not handled at all:

print(cipher(messageToCipher: "abc xyz ABC XYZ", k: -2) ) // _`a vwx ?@A VWX print(cipher(messageToCipher: "abc xyz ABC XYZ", k: -200) ) // Fatal error: Negative value is not representable 

Both issues can be solved by computing the remainder of k modulo 26, as a number in the range 0...25:

var value = k % 26 if value < 0 { value += 26 } 

In addition, this computation can be done once, before entering the loop.

Other issues:

• I would use some different argument/variable names, e.g. message instead of messageToCipher (it is clear from the context and the function name that this string should be encrypted), or key instead of k.

• There are unnecessary parentheses, e.g. in

  var pCode = ((Int(code)) + (value)) 

• The usage of white space is not consistent, e.g. in

  if (pCode > 122 && (97<=code && code<=122)) || (pCode > 90 && (65<=code && code<=90)) { 

Summarizing the suggestions so far, we have

func cipher(message: String, key: Int) -> String { var offset = key % 26 if offset < 0 { offset += 26 } var encoded: [UInt8] = [] for code in message.utf8 { if (65 <= code && code <= 90) || (97 <= code && code <= 122) { var pCode = Int(code) + offset if (pCode > 122 && (97 <= code && code <= 122)) || (pCode > 90 && (65 <= code && code <= 90)) { pCode -= 26 } encoded.append(UInt8(pCode)) } else { encoded.append(code) } } return String(bytes: encoded, encoding: .utf8)! } 

Make it functional: If the encryption of a single character is moved to a separate function then one can apply this to the UTF-8 view with map():

func cipher(message: String, key: Int) -> String { var offset = key % 26 if offset < 0 { offset += 26 } func utf8encrypt(code: UInt8) -> UInt8 { // ... compute and return encrypted character ... } return String(bytes: message.utf8.map(utf8encrypt), encoding: .utf8)! } 

Further suggestions:

• Use a switch-statement with range patterns instead of the if-conditions:

  switch code { case 65...90: // ... case 97...122: // ... default: // ... } 

• Use constants instead of the literal numbers, e.g.

  let letter_A = 65 let letter_Z = 90 let letter_a = 97 let letter_z = 122 

so that a future reader of your code does not have to guess what the numbers stand for.