HiveBrain v1.2.0
Get Started
← Back to all entries
patterngoMinor

Reverse Polish Calculator in Go

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
calculatorpolishreverse

Problem

I'm very new to Go (this is my first real program), and I was hoping to see if there was anything I was missing about the language or any ways to make it more idiomatic.

calculator.go

package calculator

import (
    "../stack"
    "unicode"
    "strconv"
)

func Calculate(input string) int {
    stack := new(stack.Stack)

    for i := 0; i  1) {
        panic("Input did not fully simplify")
    }

    return stack.Pop()
}

func parseNumber(input string, i *int) int {
    initialIndex := *i

    for unicode.IsDigit(rune(input[*i])) {
        *i++
    }
    num, _ := strconv.Atoi(input[initialIndex : *i])
    return num
}


calculator_test.go

package calculator

import (
 "testing"
)

func TestCalculate(t *testing.T) {
    tests := map[string]int{
     "3 4 +":7,
     "3 4 -":-1,
     "3 4 *":12,
     "3 4 /":0,
     "1 1 + 2 -":0,
     "5 10 * 2 /":25,
    }
    for input, expected := range tests {
        result := Calculate(input)
        if(result != expected) {
            t.Log("Calculate failed. Expected ",expected, ", got ", result)
            t.Fail()
        }
    }
}


stack.go

package stack

type Stack struct {
    i int
    // fixed size
    data [100]int
}

func (s *Stack) Push(i int) {
    if(s.i + 1 >= len(s.data)){
        panic("Out of memory!")
    }

    s.data[s.i] = i
    s.i++
}
func (s *Stack) Pop() int {
    if(s.i - 1 < 0){
        panic("Can't pop when there is no data!")
    }

    s.i--
    return s.data[s.i]
}

func (s *Stack) Size() int {
    return s.i
}


stack_test.go
Not very proud of this. Is there any way I can reduce the duplicated code? Are there any built in assert methods?

```
package stack

import (
"testing"
)

func TestStack(t *testing.T) {
s := new(Stack)

if(s.Size() != 0) {
t.Fail()
}

s.Push(1)

if(s.Size() != 1) {
t.Fail()
}

s.Push(2)

if(s.Size() != 2) {
t.Fail()
}

s.Push(3)

if(s.Size() != 3) {
t.Fail()
}

Solution


  • Don't use relative import paths.



Such as "../stack".
Instead, use full import paths like
(e.g.) "github.com/kyranstar/RPN/stack".
That would be a reasonable path to use if you put your source somewhere like
$GOPATH/github.com/kyranstar/some-repo-name/, since you're a GitHub user.
This is discussed in How to write Go code.

So for example:

$GOPATH/src/github.com
└── kyranstar
└── RPN
├── calculator
│   ├── calculator.go
│   └── calculator_test.go
└── stack
├── stack.go
└── stack_test.go

Note that you can do this even if you never have any intention of making this available on github.
In that case you'd just be using "github.com/kyranstar" as a convient prefix
that won't conflict with anyone else
(e.g. I use "bitbucket.org/myname/…", "github.com/myname/…", and "myname-test/…" as Go import prefixes depending on what I'm doing).

You didn't include anything to build a run-able command.
If you're fine with just go testing your packages, you don't need to.
But if you want to build a standalone non-test program you could either
put a simple package main right in …/kyranstar/RPN/anyname.go
or if the repo might have multiple commands to build, in …/kyranstar/RPN/cmd/name-of-command/anyname.go.
In either case, go build and go install will use the base name of the containing directory as the executable name.

  • Run go fmt (or gofmt, or goimports) on all your Go source.



On your source the changes it makes includes removing extraneous parenthesizes,
it's idiomatic to use if x { rather than if(x) {.

  • Slices vs arrays and your stack package.



It's usually better to use slices rather than fixed sizes arrays (like you do in stack.go)
unless you actually have a fixed size (e.g. perhaps type CoordinateVector [3]float64).

Then stack can become just something like:

package stack

type Stack []int

func (s Stack) Len() int    { return len(s) }
func (s *Stack) Push(i int) { *s = append(*s, i) }
func (s *Stack) Pop() int {
        n := len(*s) - 1
        v := (*s)[n]
        *s = (*s)[:n]
        return v
}


Note, Len() is more common/idiomatic than Size()
(and gorename
can help rename such things for you including all references anywhere in your $GOPATH).
This doesn't have a fixed size like yours does at the slight expense
of re-allocations as it grows.

Note that naming a package stack with a single type Stack causes code using it to stutter,
e.g. s := new(stack.Stack).
There is a blog entry about Package names that addresses this.
For example, perhaps such a stack package could have several types like Int, Int64, Float64, etc
and then callers would use stack.Int, stack.Int64, stack.Float64, etc.

Alternatively, the changed package becomes so small that it is common to just stick it into a stack.go
file in the current (calculator in this case) package and use it as a non-exported package type.

Finally, where you use this you have stack := new(stack.Stack).
Note that you are creating a local variable which shadows the import name.
You've made it difficult/impossible to use anything from the stack package anywhere else in the function
(e.g. to make a second stack for any reason).
If for no other reason then to prevent confusion in people reading the code, it's usually best to avoid such
things.

  • Strings by byte or rune.



Again there is a blog entry that covers this:
Strings, bytes, runes and characters in Go.

You have:

for i := 0; i < len(input); i++ {
            c := rune(input[i])


this loops by bytes instead of by Unicode code points (called runes in Go).
This should normally be:

for i, c := range input {


But then it iterferes with your parseNumber usage.
Sadly, strconv doesn't appear to have a function that will parse
out a number from the start of a string and tell you how many bytes/runes
were parsed.
Since you're only handling digits (and not decimal, negatives, floating point, etc)
it'd be pretty use to just make parseNumber such a function.

  • Return error, don't panic!



You should never panic out of a package for anything other than
critical unrecoverable programming errors.
(For example, the above Stack.Pop will just panic if the caller
miss-uses the stack, it's up to the caller to check Len if they need to.)
You should instead do something like this:

func Calculate(input string) (int, error) {
        // …
        if stack.len() != 1 {
                return 0, errors.New("did not fully simplify")
        }
        return stack.Pop(), nil
}


Note that it's recomended that error strings should not be capitalized
or end in punctuation.
golint
can help catch such things.

  • Testing



Instead of:

t.Log("something")
        t.Fail()


Use t.Error
and/or t.Errorf.

Use a slice instead of a map for testcase data.
A map isn't needed, and it's unordered.
You don't show what input caused the failure which makes it
harder than needed for s

Code Snippets

package stack

type Stack []int

func (s Stack) Len() int    { return len(s) }
func (s *Stack) Push(i int) { *s = append(*s, i) }
func (s *Stack) Pop() int {
        n := len(*s) - 1
        v := (*s)[n]
        *s = (*s)[:n]
        return v
}
for i := 0; i < len(input); i++ {
            c := rune(input[i])
for i, c := range input {
func Calculate(input string) (int, error) {
        // …
        if stack.len() != 1 {
                return 0, errors.New("did not fully simplify")
        }
        return stack.Pop(), nil
}
t.Log("something")
        t.Fail()

Context

StackExchange Code Review Q#84548, answer score: 3

Revisions (0)

No revisions yet.