Code review comment for lp:~jtv/gomaasapi/single-wrapper-class

Relieved to see that you got through the review and that it's not too bad! My apologies again for producing such a big branch.

—

> 186 - msg := fmt.Sprintf("Requested %v, got %v.", wantedType,
> obj.Type())
> 187 + msg := fmt.Sprintf("Requested %v, got %T.", wantedType,
> obj.value)
>
> Don't you think that the message here will be a bit confusing? I'd change
> that to
> msg := fmt.Sprintf("Requested type %v, got object %T.", wantedType, obj.value)

The %T prints the value's type, not its contents. The code is asymmetric, in that I'm printing a value (%v) for the wanted type but a type (%T) for the actual type; but that's because the parameters are respectively a string containing the requested type, and an example of the actual type. So the message that comes out will be symmetric: "Requested bool, got string."

—

> 273 +func (obj JSONObject) GetString() (value string, err error) {
> [...]
> 281 +func (obj JSONObject) GetFloat64() (value float64, err error) {
> etc.
>
> I think it's more idiomatic to use pointers as method receivers:
> func (obj *JSONObject) GetString() (value string, err error) {
> func (obj *JSONObject) GetFloat64() (value float64, err error) {
> etc.

I don't think it's particularly idiomatic, or the language wouldn't give us this unusual choice in the first place. It's just one of those make-work decisions the language forces on teams, and which seemed like good ideas in the days when code was usually “owned” by one programmer. It can lead to endless discussion, and I wonder if that is what the Go team have in mind when they say they wanted to make programming fun again... The gift that keeps on giving!

JSONObject is small and meant to be used in an immutable fashion, in which case I think not having the pointer is actually clearer. It rules out most of the worries about which change might affect what instances.

If JSONObject were an interface and the objects had, or might become, mutable operations then it would be different: in that case I think only pointers make sense as receiver types.

And there you have it: three different criteria driving the same decision — mutability, copying cost, and interfaces. The situation pretty much rules out full agreement in everyday situations. :(

—

> 359 + _ = JSONObject(arr[0])
> and
> 382 + _ = JSONObject(mp["key"])
>
> I'm a bit confused here, what is this for exactly and why are you simply
> ignoring the return value?

Well spotted. That was meant to be a type assertion. It has _some_ power to do that in its current form, but it's not very effective. I changed it to:

    var _ JSONObject = arr[0]

(mutatis mutandi of course).

—

> +// a JSONObject, so if an API call returns a list of objects "l," you first
>
> I think you want to write 'a list of objects "l", you first' here.
>
> Same kind of typo here:
> 755 +// in "params." It returns the object's new value as received from
> the API.
> and here:
> 768 +// "params." It returns the object's new value as received from the
> API.

Not a typo. Typography. LaTeX, which is a great school for these things, taught me to move a full stop or comma inside the quotes. I do sometimes break that rule when talking about code or other literal strings where it may lead to confusion.

I realize this may be controversial. It's been a while since I last read 1984, but as I recall Winston Smith, after the régime breaks him, gets put on a committee to argue this very point. That book was written about 65 years ago, so I'm fairly confident without looking that there has been no sudden consensus since that overrules the LaTeX rule. :-)

—

> There is a tiny problem in example/live_example.go now, with this fix, it
> works again: http://paste.ubuntu.com/1636327/

Speaking of controversial commas: I first read this as “There is a tiny problem in example/live_example.go now, with this fix. It works again”! That's the problem with using commas to separate sentences: it leaves room for ambiguity.

—

> 1021 +func (obj JSONObject) MarshalJSON() ([]byte, error) {
> and
> 1028 +func (obj MAASObject) MarshalJSON() ([]byte, error) {
>
> I think it's worth adding a little comment here to explain that this is to
> implement the Marshaler interface from the std lib.

Right you are. And not just that, but type assertions as well!

—

Thanks again,

Jeroen