Modelling employees in Go as a basic OOP exercise

I am learning Go and have the following working code:

$GOPATH/src/mvctest/main.go

package main

import (
    "fmt"
    "mvctest/model"
)

func main() {
    // Initialise the data model
    employees := model.NewPeople("Employee")

    // Populate the model
    employees.Add(model.NewPerson("Joe", 99))
    employees.Add(model.NewPerson("Sue", 45))
    employees.Add(model.Person{Name: "Tom", Age: 22}) // break encaps a bit

    // Test retrieval from model
    fmt.Printf("You have %d employees.\n", employees.Count())

    for i := 0; i < employees.Count(); i++ {
        f, err := employees.Person(i)
        if err != nil {
            fmt.Printf("Unable to get employee %d because %v", i, err)
            return
        }
        fmt.Printf("Employee %d is %s\n", i+1, f)
    }

    // Test error handling for impossible values
    test(employees, -1)
    test(employees, 42)

}

// Tests whether a person can be retrieved using an index value
func test(ppl *model.People, i int) *model.Person {
    p, err := ppl.Person(i)
    if err != nil {
        fmt.Printf("Unable to get %s %d because %v\n", ppl.Group(), i, err)
        return nil
    }
    return p
}

$GOPATH/src/mvctest/model/model.go

```
package model

import (
"errors"
"fmt"
)

// ------------------------------------------------------

type Person struct {
Name string
hatsize int
Age int
}

func (p *Person) String() string {
return fmt.Sprintf("%s aged %d.", p.Name, p.Age)
}

func NewPerson(name string, age int) Person {
return Person{name, 0, age}
}

// ------------------------------------------------------

type People struct {
group string
list []Person
}

func (ppl *People) SetGroup(g string) {
ppl.group = g
}

func (ppl *People) Group() string {
return ppl.group
}

func (ppl *People) Add(p Person) {
ppl.list = append(ppl.list, p)
}

func (ppl *People) Count() int {
return len(ppl.list)
}

func (ppl People) Person(i int) (Person, error) {

Using a NewType() constructor-like function is the idiomatic way in Go. See Effective Go: Constructors and composite literals. Also relevant: Effective Go: Package names:


The importer of a package will use the name to refer to its contents, so exported names in the package can use that fact to avoid stutter. (Don't use the import . notation, which can simplify tests that must run outside the package they are testing, but should otherwise be avoided.) For instance, the buffered reader type in the bufio package is called Reader, not BufReader, because users see it as bufio.Reader, which is a clear, concise name. Moreover, because imported entities are always addressed with their package name, bufio.Reader does not conflict with io.Reader. Similarly, the function to make new instances of ring.Ring—which is the definition of a constructor in Go—would normally be called NewRing, but since Ring is the only type exported by the package, and since the package is called ring, it's called just New, which clients of the package see as ring.New. Use the package structure to help you choose good names.

And if you want to iterate over a whole "collection" (e.g. slice, array, map, channel), the For statements with range clause is the clearest way. "Unfortunately" your People type does not expose such values, so it's not possible to use for range here.

It can still be improved to call People.Count() once:

for i, count := 0, employees.Count(); i < count; i++ {
    f, err := employees.Person(i)
    // ...
}

Other improvements

Your NewPeople() could be simply written as:

func NewPeople(group string) *People {
    return &People{group: group}
}

In People.Person() it's redundant to call errors.New() and fmt.Sprintf(), as the fmt package also has an fmt.Errorf() function doing just that:

func (ppl *People) Person(i int) (*Person, error) {
    if n := len(ppl.list); i = n {
        return nil, fmt.Errorf("%s index %d is out of range %d-%d.",
            ppl.group, i, 0, n-1)
    }
    return &ppl.list[i], nil
}

Also it's just a matter of taste, but I find it easier to read if you use the max index instead of the length (which is max+1):

if max := len(ppl.list) - 1; i  max {
    return nil, fmt.Errorf("%s index %d is out of range %d-%d.",
        ppl.group, i, 0, max)
}

Also it's a good habit for a specific type to be consistent. Either use pointers in all places, or non-pointers. In case of Person, you mix both: People.Add() expects Person, but People.Person() returns *Person.

StackExchange Code Review Q#157637, answer score: 2