11
\$\begingroup\$

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?

\$\endgroup\$
  • \$\begingroup\$ Incidentally, this code assumes that the input string contains no character code > 127, otherwise it won’t work. That’s not necessarily a problem though. Just pointing it out. \$\endgroup\$ – Konrad Rudolph Sep 1 '12 at 12:32
14
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ I don't have enough reputation points on this site to post an answer, but a shorter version of the for loop is: ` for k := range(b) { if b[k]<'n' {b[k]+=13} else { b[k]-=13 } }` \$\endgroup\$ – Arnaud A Apr 25 '16 at 20:37
  • \$\begingroup\$ @ArnaudAmzallag That omits the entire sanity checking logic, which makes up the bulk of the code (and the logic), and assumes that the string consists entirely of lowercase letters. \$\endgroup\$ – Konrad Rudolph Apr 25 '16 at 21:44
  • \$\begingroup\$ fair enough. Not sure if shorter now. for k := range(b) { if (b[k] >= 'a' && b[k] <= 'm') || (b[k] >= 'A' && b[k] <= 'M') {b[k]+=13} else { if (b[k] > 'm' && b[k] <= 'z') || (b[k] > 'M' && b[k] <= 'Z') {b[k]-=13} }} \$\endgroup\$ – Arnaud A Apr 26 '16 at 14:54
6
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ Thanks, I was specifically wondering if n is relevant with an error. \$\endgroup\$ – akostadinov Sep 7 '15 at 8:23
3
\$\begingroup\$

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
}
\$\endgroup\$
2
\$\begingroup\$

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
}
\$\endgroup\$
  • \$\begingroup\$ The other algorithms also run in O(n), no need for the lookup table to achieve that. The lookup table may still be faster, of course. \$\endgroup\$ – Konrad Rudolph Sep 16 '14 at 9:02

Not the answer you're looking for? Browse other questions tagged or ask your own question.