Rot13 Reader in Go

Question

For this exercise I have done this:

func between(start, end, value byte) bool{
    if value > end {
        return false
    } else if value < start {
        return false
    }
    return true
}
func (r rot13Reader) Read(p []byte) (n int, err error) {
    s, err := r.r.Read(p)
    if err != nil {
        return s, err
    }
    for i,v := range p {
        if between(97,122, v) {
            new := v + 13
            if new > 122 {
                new -= 26
            }
            p[i] = new
        } else if between(65, 90, v) {
            new := v + 13
            if new > 90 {
                new -= 26
            }
            p[i] = new
        }
    }

    return s, err

It works correctly, but it feels like too much code for something like this. Can I improve this code?

Answer 1

I don’t know Go but here are a few general things that I noticed:

func between(start, end, value byte) bool {
    if value > end {
        return false
    } else if value < start {
        return false
    }
    return true
}

As a general pattern, don’t write if condition return true else return false or any permutation thereof. Instead, return the condition directly. In your case, you need to rewrite the condition ever so slightly:

func between(start, end, value byte) bool {
    return ! (value > end) && ! (value < start)
}

Which is the same as (and which can also derived intuitively from the expected semantics of between):

func between(start, end, value byte) bool {
    return value >= start && value <= end
}

In general, only use boolean constants true and false to initialise variables (or parameters). This is their only use.

---

In your main code, you are essentially doing the same thing twice, once for upper-case and once for lower-case letters. Avoid redundancy and make these cases into one:

is_upper := between(65, 90, v)
is_lower := between(97, 122, v)

if is_upper || is_lower {
    new := v + 13
    if (is_upper && new > 90) || (is_lower && new > 122) {
        new -= 26
    }
    p[i] = new
}

But this code is still cryptic: what are these weird numbers? Substitute them by named constants or use character constants (since it’s clear what 'a' means).

Furthermore, you can get rid of between since Go has functions to test whether a byte is an upper-case or lower-case letter (requires the package unicode):

is_upper := unicode.IsUpper(rune(v))
is_lower := unicode.IsLower(rune(v))

Finally, the logic of rot-13 transforming a character can be simplified by the use of the modulus operation:

c := (c + 13) % 26

Obviously, this assumes that c is a value from 0 to 25; so the real logic should be something along these lines: ^{(Seriously, Go? No conditional operator?!)}

if is_upper || is_lower {
    a := byte('a')
    if is_upper { a = byte('A') }
    p[i] = (v - a + 13) % 26 + a
}

The last line here first puts the value of v into the range 0–25 by subtracting a, then does the rot-13 transformation, and then translates it back into a letter by adding the value of a back.

Notice how much shorter the logic of this code has become: the whole loop body is now only seven lines long.

Answer 2

This improves slightly on Keeth's answer by making the source of the magic numbers more clear.

func (r *rot13Reader) Read(p []byte) (n int, err error) {
    n, err = r.r.Read(p)
    for i := 0; i < n; i++ {
        c := p[i]
        switch {
        case 'a' <= c && c <= 'z':
            p[i] = (c-'a'+13)%26 + 'a'
        case 'A' <= c && c <= 'Z':
            p[i] = (c-'A'+13)%26 + 'A'
        default:
        }
    }
    return
}

I want to point out in particular that we never need to check "err" within this code, particularly not before processing all of the bytes that we did receive! http://golang.org/pkg/io/#Reader explicitly says "Callers should always process the n > 0 bytes returned before considering the error err. Doing so correctly handles I/O errors that happen after reading some bytes and also both of the allowed EOF behaviors." In yasar's original code, checking the state of err early skips the proper processing of the n bytes that were produced before the error happened.

Answer 3

I used similar logic to the original, but made it simpler using modulus operation:

func (self rot13Reader) Read(p []byte) (n int, err error) {
    n, err = self.r.Read(p)
    for i,v := range p {
        switch {
        case v > 64 && v < 91:
            p[i] = (v - 65 + 13) % 26 + 65
        case v > 96 && v < 123:
            p[i] = (v - 97 + 13) % 26 + 97
        }
    }
    return
}

Answer 4

I used a lookup table, keeping the reader code short and uncoding the cipher in O(n).

var rot13lot = map[byte]byte{
    'A': 'N',
    /* ... */
    'z': 'm',
}

func (rot13r rot13Reader) Read(p []byte) (n int, err error) {
    n, err = rot13r.r.Read(p)
    for i := 0; i < n; i++ {
        rot, isRot := rot13lot[p[i]]
        if isRot {
            p[i] = rot
        }
    }
    return
}