markphelps / optional

Optional is a library of optional Go types
https://godoc.org/github.com/markphelps/optional
MIT License
210 stars 19 forks source link

Calling Get() on empty optional crashes #6

Closed roudy16 closed 5 years ago

roudy16 commented 5 years ago

Calling Get() on an empty optional leads to a nil pointer dereference. It seems like it would be okay to return a zero value with a non-nil error in the case that Get() fails. I'm willing to do this work if you approve of my suggested fix, I also understand if crashing is the desired behavior. In the case that you want the crash the Get() method doesn't seem like it needs to call Present().

Crashing example:

package main

import (
    "fmt"
    "github.com/markphelps/optional"
)

func main() {
    valOpt := optional.Int64{}
    val, err := valOpt.Get() // crash here
    if err == nil {
        fmt.Println("Got expected nil err for empty optional")
    }
    fmt.Printf("Got value: '%d' from empty optional\n", val)
}

Suggested fix for int64 type:

func (i Int64) Get() (int64, error) {
    if !i.Present() {
        var zero int64
        return zero, errors.New("value not present")
    }
    return *i.value, nil
}
markphelps commented 5 years ago

Ahh good catch! Yes I think it makes sense to return the zero value. I don't think panicking is a good UX. A PR would be greatly appreciated 🙇

The only thing I worry about is that the point of using optional types is to be able differentiate between a default value and a value that wasn't set.. but.. it's common knowledge in the Go community to always check the result of error first.. so I think this is ok.

roudy16 commented 5 years ago

Should be fixed in #7