2
\$\begingroup\$

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

\$\endgroup\$
1
\$\begingroup\$

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.

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