2
\$\begingroup\$

I was wondering if I am doing any useless conversions byte←→int, for example:

byte((int(ch-'A')+shift)%26 + 'A')

Converting ch-'A' to an int is because the shift argument can be negative (to implement the decode function). I couldn't figure out a simpler way to negate the operation using bytes.

package main

import (
    "bufio"
    "flag"
    "fmt"
    "os"
)

func main() {
    shift := flag.Int("shift", 13, "Cipher shift")
    decode := flag.Bool("decode", false, "Decode input")

    flag.Parse()

    scanner := bufio.NewScanner(os.Stdin)
    for scanner.Scan() {
        if *decode {
            fmt.Println(Decode(scanner.Text(), *shift))
        } else {
            fmt.Println(Encode(scanner.Text(), *shift))
        }
    }
}

func Encode(s string, shift int) string {
    return cipher(s, shift)
}

func Decode(s string, shift int) string {
    return cipher(s, -shift+26)
}

func cipher(s string, shift int) string {
    var line string
    for _, ch := range []byte(s) {
        if ch >= 'A' && ch <= 'Z' {
            ch = byte((int(ch-'A')+shift)%26 + 'A')
        } else if ch >= 'a' && ch <= 'z' {
            ch = byte((int(ch-'a')+shift)%26 + 'a')
        }
        line += string(ch)
    }
    return line
}
\$\endgroup\$
3
\$\begingroup\$

Flags

It's not very problematic for very short programs like this, but better get good habits: flags should be outside of all code blocks; so that you detect immediately if you have a flag naming conflict between different files.

var (
    shiftF = flag.Int("shift", 13, "Cipher shift")
    decodeF = flag.Bool("decode", false, "Decode input")
)

func main() {
    …
}

Code organization

You're checking for the value of the -decode flag every time you scan a line, and recomputing -shift+26 every time as well: not very efficient, nor super readable. As you noticed, the only difference between encoding and decoding is the value of shift. Why not do something like:

func main() {
    flag.Parse()

    // we could also directly change the value of the flag, but I find it
    // less readable — it's better to treat flag values as immutable.
    var shift int
    if *decode {
        shift = 26 - *shiftF
    } else {
        shift = *shiftF
    }

    scanner := bufio.NewScanner(os.Stdin)
    for scanner.Scan() {
        cipher(scanner.Text(), shift)
    }
}

Type conversions

bufio.Scanner holds bytes internally: the doc for Scanner.Bytes says:

The underlying array may point to data that will be overwritten by a subsequent call to Scan. It does no allocation.

while Scanner.Text indicates that it returns:

a newly allocated string.

So here, you allocate a string to copy bytes; and then you allocate a new slice of type []byte to copy this string, which is also an expensive operation.

So you're saving two conversions by not dealing with strings at all:

func main() {
    …
    for scanner.Scan() {
        fmt.Printf("%s\n", cipher(scanner.Bytes(), shift))
    }
}

func cipher(bytes []byte, shift int) []byte {
    var line []byte
    for _, b := range bytes {
        if b >= 'A' && b <= 'Z' {
            b = byte((int(b-'A')+shift)%26 + 'A')
        } else if b >= 'a' && b <= 'z' {
            b = byte((int(b-'a')+shift)%26 + 'a')
        }
        line = append(line, b)
    }
    return line
}

Note the slight change in the way you print the result: using fmt.Println on a []byte variable will print the numeric value of each byte, something like [116 104 101 32 103 97 109 101], so you have to use fmt.Printf to tell it to print it like a string.

You're also wondering whether the conversions from byte to int are inefficient: kind of, yes. Arithmetic operations work on bytes, so you could do instead:

shiftB := byte(shift)
b = ((b-'A')+shiftB)%26 + 'A'

However, this is just a simple type conversion: the performance gain from it will be much lower than when avoiding memory allocations, so you shouldn't worry about it too much =)

Optimizations

Initializing a []byte variable and appending char by char to it is not great: the slice is going to be re-sized every time it hits its maximum default size. It's even worse in your code: you're initializing a string variable and appending char by char to it, which will reallocate the string each time. In cipher, you know in advance which size line is going to be: the same as the original slice. So you can initialize directly with the correct allocated capacity:

    line := make([]byte, 0, len(bytes))

okay, now let's go even further: calling append at each iteration means that under the hood, there will be a check to see if the slice over capacity. We know it's not going to happen, because the sizes are the same: why not always allocate the exact amount of memory that we need and write at the right place directly?

func cipher(bytes []byte, shift int) []byte {
    shiftB := byte(shift)
    line := make([]byte, len(bytes))
    for i, b := range bytes {
        if b >= 'A' && b <= 'Z' {
            b = ((b-'A')+shiftB)%26 + 'A'
        } else if b >= 'a' && b <= 'z' {
            b = ((b-'a')+shiftB)%26 + 'a'
        }
        line[i] = b
    }
    return line
}

We can go even further by noticing that the bytes argument won't be used after we call cipher. So we can re-use this slice instead of allocating a new one!

func cipher(bytes []byte, shift int) []byte {
    shiftB := byte(shift)
    for i, b := range bytes {
        // shift b…
        bytes[i] = b
    }
    return bytes // optional: callers could simply reuse the argument
}

Final words

The code we obtain is more readable, definitely more efficient; and has the same behavior as yours. However, there are still two potential issues with it:

  • Encoding: we're considering every byte of the input separately, as if it were ASCII; so it will probably not do what you want if the input is in UTF-8.
  • Input size: if the input has very long lines, then using bufio.NewScanner is a bad idea; see for example this SO question.
\$\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.