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? I am absolute beginner in go, as you might guess.

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

For example, Exercise: Rot13 Reader,

func (r rot13Reader) Read(p []byte) (n int, err error) {
    n, err = r.r.Read(p)
    const (
        rot   = 13
        lcase = 0x20
    )
    for i := 0; i < n; i++ {
        c := p[i] | lcase
        if 'a' <= c && c <= 'z' {
            if c <= 'z'-rot {
                p[i] += rot
            } else {
                p[i] -= rot
            }
        }
    }
    return n, err
}