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
}
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.

-
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. –  Konrad Rudolph Sep 1 '12 at 12:32

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.

-

``````func (r rot13Reader) Read(p []byte) (n int, err error) {
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
}
``````
-