Code review comment for lp:~gz/gomaasapi/gomaasapi

Revision history for this message
Roger Peppe (rogpeppe) wrote :

https://codereview.appspot.com/7228069/diff/1/jsonobject.go
File jsonobject.go (right):

https://codereview.appspot.com/7228069/diff/1/jsonobject.go#newcode159
jsonobject.go:159: func (obj jsonString) GetBool() (bool, error)
         { return failBool(obj) }
On 2013/02/05 12:26:31, fwereade wrote:
> This bit is awfully dense and repetitive. I think a few standalone
Get* funcs
> would work a bit better here, as mentioned above:

> func (jsonString) Type() string { return "string" }
> func (jsonFloat64) Type() string { return "float64" }
> ...

> func GetString(obj JSONObject) (string, error) {
> if obj.Type() == "string" {
> return string(obj), nil
> }
> return "", failConversion("string", obj)
> }

> func GetFloat64(obj JSONObject) (float64, error) {
> if obj.Type() == "float64" {
> return float64(obj), nil
> }
> return "", failConversion("float64", obj)
> }

> ...

if that's the case, why bother with the JSONObject type at all?
why not:

func GetString(obj interface{}) (string, error) {
     if s, ok := obj.(string); ok {
         return s, nil
     }
     return "", failConversion("string", obj)
}

or even a single function:

// Get sets the value of into (which must be a pointer
// to one of float64, string, map[string]interface{},
// bool or MaasObject) to the value of jsonObj.
func Get(jsonObj interface{}, into interface{}) error {
     var ok bool
     switch into := into.(type) {
     case *string:
         *into, ok = jsonObj.(string)
     case *float64:
         *into, ok = jsonObj.(float64)
     etc
     }
     if !ok {
         return failConversion(reflect.TypeOf(into).Elem().String(),
jsonObj)
     }
     return nil
}

but all this seems like it's trying to do something
similar to launchpad.net/juju-core/schema without
the nice generality that that package provides.

https://codereview.appspot.com/7228069/

« Back to merge proposal