patterngoModerate
Rot13 Reader in Go
Viewed 0 times
rot13readerstackoverflow
Problem
For this exercise I have done this:
It works correctly, but it feels like too much code for something like this. Can I improve this code?
func between(start, end, value byte) bool{
if value > end {
return false
} else if value 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, errIt works correctly, but it feels like too much code for something like this. Can I improve this code?
Solution
I don’t know Go but here are a few general things that I noticed:
As a general pattern, don’t write
Which is the same as (and which can also derived intuitively from the expected semantics of
In general, only use boolean constants
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:
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
Furthermore, you can get rid of
Finally, the logic of rot-13 transforming a character can be simplified by the use of the modulus operation:
Obviously, this assumes that
The last line here first puts the value of
Notice how much shorter the logic of this code has become: the whole loop body is now only seven lines long.
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) % 26Obviously, 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.
Code Snippets
func between(start, end, value byte) bool {
if value > end {
return false
} else if value < start {
return false
}
return true
}func between(start, end, value byte) bool {
return ! (value > end) && ! (value < start)
}func between(start, end, value byte) bool {
return value >= start && value <= end
}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
}is_upper := unicode.IsUpper(rune(v))
is_lower := unicode.IsLower(rune(v))Context
StackExchange Code Review Q#15256, answer score: 15
Revisions (0)
No revisions yet.