caesar cipher decoder and encoder - go

Question

I implemented my first algorithm in golang - the caesar cipher. Is there something i could do more efficiently? I am quite new to go and any improvement suggestions are welcome.

package main
import "fmt"
// +1 encoding, -1 decoding
func main() {
 var text string
 var choice int
 var shift int
 fmt.Print("Text: ")
 fmt.Scanf("%s", &text)
 fmt.Print("+1 encoding, -1 decoding: ")
 fmt.Scanf("%d", &choice)
 fmt.Print("Shift: ")
 fmt.Scanf("%d", &shift)
 fmt.Println(cipher(text, choice, shift))
}
func cipher(text string, choice int, shift int) string{
 chars := []rune(text)
 var result string
 for i := 0; i < len(chars); i++ {
 if chars[i] >= 'a' && chars[i] <= 'z' || chars[i] >= 'A' && chars[i] <= 'Z' {
 dchar := chars[i] + rune(shift*choice)
 if dchar >= 'a' && dchar <= 'z' || dchar >= 'A' && dchar <= 'Z' {
 result += string(dchar)
 } else {
 result += string(dchar + rune(-26 * choice))
 }
 } else {
 result += string(chars[i])
 }
 }
 return result
}

Answer 1

Your API does not appear to be a thoughtful design:

func cipher(text string, choice int, shift int) string

A good API design follows the shape of the problem, not the implementation. The problem:

• Wikipedia: Caesar cipher

• Wikipedia:Encryption

• Wikipedia: Inverse function

The general shape of the Caesar cipher is an encryption inverse function.

For the Caesar cipher, an encryption package API,

package caesar
func Encrypt(plain string, key int) string
func Decrypt(cipher string, key int) string

with the encryption inverse function form

plain = caesar.Decrypt(caesar.Encrypt(plain, key), key) 

---

Your cipher implementation function is not correct. For example, it fails the Wikipedia: Caesar cipher example:

Example:

func main() {
 var plainText = `THE QUICK BROWN FOX JUMPS OVER THE LAZY DOG`
 fmt.Println(plainText)
 var cipherText = `QEB NRFZH YOLTK CLU GRJMP LSBO QEB IXWV ALD`
 fmt.Println(cipherText)
 var key = -3
 fmt.Println(key)
 fmt.Println(cipher(cipher(plainText, +1, key), -1, key))
}

Output:

THE QUICK BROWN FOX JUMPS OVER THE LAZY DOG
QEB NRFZH YOLTK CLU GRJMP LSBO QEB IXWV ALD
-3
THE QUI&K %ROWN FOX JUMPS OVER THE L$ZY DOG

---

Your cipher implementation function is orders of magnitude too expensive. Your cipher function versus reasonably efficient package caesar functions:

name time/op
Cipher-8 9.32μs ± 0%
Caesar-8 502ns ± 0%
name alloc/op
Cipher-8 2.98kB ± 0%
Caesar-8 192B ± 0%
name allocs/op
Cipher-8 172 ± 0%
Caesar-8 4.00 ± 0%

Amongst other things, your frequent use of immutable string concatenation (+=) is expensive.

---

Here is a revised version of your code that addresses my code review issues.

package main
import "fmt"
func caesar(text string, key int) string {
 shift := key % 26
 c := make([]byte, len(text))
 for i, b := range []byte(text) {
 lower := b | 0x20 // ASCII lower case
 if 'a' <= lower && lower <= 'z' {
 base := int('A')
 if b == lower {
 base |= 0x20 // ASCII lower case
 }
 b = byte(base + (int(b)-base+shift+26)%26)
 }
 c[i] = b
 }
 return string(c)
}
func Encrypt(plain string, key int) string {
 return caesar(plain, key)
}
func Decrypt(cipher string, key int) string {
 return caesar(cipher, -key)
}
func main() {
 Plaintext := `THE QUICK BROWN FOX JUMPS OVER THE LAZY DOG`
 fmt.Println(Plaintext)
 Ciphertext := `QEB NRFZH YOLTK CLU GRJMP LSBO QEB IXWV ALD`
 fmt.Println(Ciphertext)
 Key := -3
 fmt.Println(Key)
 fmt.Println(Encrypt(Plaintext, Key))
 fmt.Println(Decrypt(Encrypt(Plaintext, Key), Key))
}

https://go.dev/play/p/7sDXyIcVFpc

THE QUICK BROWN FOX JUMPS OVER THE LAZY DOG
QEB NRFZH YOLTK CLU GRJMP LSBO QEB IXWV ALD
-3
QEB NRFZH YOLTK CLU GRJMP LSBO QEB IXWV ALD
THE QUICK BROWN FOX JUMPS OVER THE LAZY DOG

Answer 2

I'm going to focus on the cipher function itself for feedback

First, Go includes a range function that lets you iterate/loop over objects and collections directly. If you use range with a string it will return the index and rune so you don't need to worry about using char[i] so many times:

for index, char := range <string> { ... }

Second, you repeat the same check a couple different if statements which is a sign you can likely break that out into a separate function so you only have to write the logic once. I would also reorder it like A <= x <= B so that it's simpler to understand at a glance:

func isLetter(char rune) bool {
 return 'a' <= char && char <= 'z' || 'A' <= char && char <= 'Z'
}

For this case though the Go standard library already contains a unicode package with an isLetter(r rune) function we can use instead of implementing our own.

Lastly, for readability and better comprehension of what your code is trying to achieve you can decompose your algorithm into smaller, clearly named functions. In this case the two main things you're doing are 1. shifting the character and 2. wrapping it back into a specific range if it got shifted out of that. Since each step is relatively straight forward I'd keep them together but if they were more complex you could split them into separate shiftChar and wrapChar functions.

Putting it all together a cleaner version of the cipher function could look something like:

func cipher(text string, direction int, shift int) string {
 var result string
 for _, char := range text {
 result += string(shiftLetterWithWrap(char, direction, shift))
 }
 return result
}
func shiftLetterWithWrap(char rune, direction int, shift int) rune {
 if unicode.IsLetter(char) {
 char += rune(shift * direction)
 
 if !unicode.IsLetter(char) {
 char += rune(direction * -26)
 }
 }
 return char
}

• Coupcoup: That recreates your original logic but there's presumably a bug if you enter a shift value greater than 26. Ways you could handle that include adding either input validation or changing the shift code to char += rune(shift * direction % 26)