code review 157068: Add json.Marshal to json package

Add json.Marshal to json package

[hoisie, 2009-11-18 18:44:46]

Hello rsc,

I'd like you to review the following change.

[rsc, 2009-11-18 18:59:05]

Thanks for doing this.

Please run

    hg gofmt
    hg upload 157068

and let me know when you've done this.
The code is not in the standard Go style,
and it will be easier to see revisions if
we get that out of the way first.

Also please follow the instructions at 
http://golang.org/doc/contribute.html#copyright
to create an AUTHORS/CONTRIBUTORS change list.

Thanks.

[hoisie, 2009-11-18 19:44:26]

Just ran gofmt and re-uploaded. Enjoy :)

[rsc, 2009-11-19 06:15:50]

This is a good start, but there are some structural things
that I think you can do differently to get much better
performance.  See below.

Thanks.

http://codereview.appspot.com/157068/diff/1013/1014
File src/pkg/json/struct.go (right):

http://codereview.appspot.com/157068/diff/1013/1014#newcode11
src/pkg/json/struct.go:11: "reflect";
sort this list

http://codereview.appspot.com/157068/diff/1013/1014#newcode312
src/pkg/json/struct.go:312: type JsonMarshalError struct {
There's no need for the word Json, because it is 
already in the package name: this would be a
json.JsonMarshalError.  It should just be
json.MarshalError.

Better:

type MarshalError string

func (e MarshalError) String() string { return string(e) }

http://codereview.appspot.com/157068/diff/1013/1014#newcode322
src/pkg/json/struct.go:322: field_typ := typ.FieldByIndex([]int{i});
Go style is to write fieldTyp (probably fieldType actually)
instead of field_typ.

This function call can be simplified to typ.Field(i);

http://codereview.appspot.com/157068/diff/1013/1014#newcode326
src/pkg/json/struct.go:326: value, err :=
typeToJson(reflect.Typeof(field_val.Interface()), field_val.Interface());
With a writer (see comment at bottom) this would be

if err := fmt.Fprintf(w, "%q:", key); err != nil {
    return err;
}
if err := write(w, val); err != nil {
    return err;
}

http://codereview.appspot.com/157068/diff/1013/1014#newcode386
src/pkg/json/struct.go:386: func typeToJson(typ interface{}, val interface{})
(json_value string, err os.Error) {
I suggest calling this

func write(w io.Writer, val reflect.Value) os.Error

The typ interface{} is unnecessary: you can ask val.Type()
if you need the type, and a type switch on value is 
probably good enough anyway.

http://codereview.appspot.com/157068/diff/1013/1014#newcode387
src/pkg/json/struct.go:387: switch v := typ.(type) {
This type switch can be on val, and then you avoid the type
conversions in the body of each case:

switch v := val.(type) {
case *reflect.BoolValue:
    fmt.Fprint(w, v.Get());
case *reflect.FloatValue:
    fmt.Fprint(w, v.Get());
case *reflect.Float32Value:
    fmt.Fprint(w, v.Get());
...
}

In fact, you can use the Interface() method to merge all the
simple cases into a single default case:

switch v := val.(type) {
default:
    fmt.Fprint(w, v.Interface());
case *reflect.StringType:
    ...
case *reflect.ChanType:
    ...
case *reflect.MapType:
    ...
case *reflect.ArrayType:
    ...
case *reflect.SliceType:
    ...
case *reflect.StructType:
    ...
case *reflect.PtrType:
    ...
}

http://codereview.appspot.com/157068/diff/1013/1014#newcode454
src/pkg/json/struct.go:454: func Marshal(i interface{}) (string, os.Error) {
The fact that json.Unmarshal takes a string was a mistake.
(json was one of the earliest packages to be written.)
This function should be

func Marshal(w io.Writer, val interface{}) os.Error 

and should write to the writer instead of creating a string.
All those string additions keep allocating larger and larger
strings, which will make the run-time quadratic.  In contrast,
writing to a writer doesn't have to reallocate at all (if writing
to a file, for example) and lets the writer make decisions
about allocation.

[hoisie, 2009-11-19 10:45:45]

OK I made a few changes tonight:

1) updated the Marshal method to use a writer instead of converting everything
to a string. 

2) Just pass around reflect.Value, because you don't really need the
reflect.Type

3) Simplified MarshalError

4) Random cleanup

[rsc, 2009-11-19 23:03:11]

Looking pretty good.  These comments are mostly
ways to make the Go code a little more idiomatic.

http://codereview.appspot.com/157068/diff/1019/8
File src/pkg/json/struct.go (right):

http://codereview.appspot.com/157068/diff/1019/8#newcode313
src/pkg/json/struct.go:313: type MarshalError string
It looks like all the marshalling errors are types.
You can give the information directly by doing

type MarshalError struct {
    T reflect.Type
}

func (e *MarshalError) String() string {
    return "json cannot encode value of type " + e.T.String()
}

http://codereview.appspot.com/157068/diff/1019/8#newcode317
src/pkg/json/struct.go:317: func jsonWriteArrayOrSlice(w io.Writer, val
reflect.ArrayOrSliceValue) os.Error {
writeArrayOrSlice

You're in package json already: no identifier needs to start with json.  

writeMap writeValue etc.

http://codereview.appspot.com/157068/diff/1019/8#newcode321
src/pkg/json/struct.go:321: v := val.Elem(i);
You can shorten this to

if err := writeValue(w, val.Elem(i)); err != nil {
    return err
}

http://codereview.appspot.com/157068/diff/1019/8#newcode327
src/pkg/json/struct.go:327: if i < (val.Len() - 1) {
no need for parens here

http://codereview.appspot.com/157068/diff/1019/8#newcode343
src/pkg/json/struct.go:343: return MarshalError("Json Marshal can only accept
string keys in a map")
Instead of doing this test once per map key,
you can check the map's key type at the top of the function:

key := val.Type().(*reflect.MapType).Key();
if _, ok := key.(*reflect.StringType); !ok {
    return &MarshalError{val.Type()}
}

Then the code here can be just 

fmt.Fprintf(w, "%q:", keys[i].(*reflect.StringValue).Get());

http://codereview.appspot.com/157068/diff/1019/8#newcode349
src/pkg/json/struct.go:349: val := val.Elem(keys[i]);
can merge as above

http://codereview.appspot.com/157068/diff/1019/8#newcode355
src/pkg/json/struct.go:355: if i < (len(keys) - 1) {
no need for parens

http://codereview.appspot.com/157068/diff/1019/8#newcode367
src/pkg/json/struct.go:367: structType := val.Type().(*reflect.StructType);
More idiomatic would be to call this typ.

http://codereview.appspot.com/157068/diff/1019/8#newcode370
src/pkg/json/struct.go:370: fieldType := structType.Field(i);
This isn't the fieldType, just the field structure.
Can be rolled into next line too:

fmt.Fprintf(w, "%q:", typ.Field(i).Name);

http://codereview.appspot.com/157068/diff/1019/8#newcode375
src/pkg/json/struct.go:375: if i < (val.NumField() - 1) {
no need for parens

http://codereview.appspot.com/157068/diff/1019/8#newcode384
src/pkg/json/struct.go:384: func jsonWriteValue(w io.Writer, val interface{})
(err os.Error) {
This should be val reflect.Value, and
then you won't need the .(reflect.Value) below.

http://codereview.appspot.com/157068/diff/1019/8#newcode388
src/pkg/json/struct.go:388: fmt.Fprintf(w, `"%s"`, stringVal);
This should be %q instead of "%s".
Even %q isn't quite right, but it's closer.

http://codereview.appspot.com/157068/diff/1019/8#newcode397
src/pkg/json/struct.go:397: case *reflect.ChanValue:
case *reflect.ChanValue,
    *reflect.InterfaceValue,
    *reflect.PtrValue,
    *reflect.UintptrValue,
    *reflect.UnsafePointerValue:
    return &MarshalError{val.Type()}

http://codereview.appspot.com/157068/diff/1019/9
File src/pkg/json/struct_test.go (right):

http://codereview.appspot.com/157068/diff/1019/9#newcode163
src/pkg/json/struct_test.go:163: buffer :=
bytes.NewBufferString("");	//make([]byte, 100));
var buf bytes.Buffer;
err := Marshal(&buf, val);
s := buf.String();

is a bit simpler.

http://codereview.appspot.com/157068/diff/1019/9#newcode176
src/pkg/json/struct_test.go:176: runJsonMarshalTest(t, true, `true`);
Instead of a long sequence of function calls, the Go idiom
is to turn this sequence into pure data -- a table -- and then
have a loop here that runs over the table.

There's no example of the Go idiom in this package's tests,
but look at http://golang.org/src/pkg/fmt/fmt_test.go
or http://golang.org/src/pkg/strconv/itoa_test.go for
examples.

[hoisie, 2009-11-20 04:02:18]

Updated the patch to include all the changes. 

* Changed error format
* removed 'json' prefix from function names
* Use %q instead of %s
* writeValue takes reflect.Value
* misc cleanup and tightening
* reworked tests into a table format

[rsc, 2009-11-20 04:14:44]

Really close to done!

http://codereview.appspot.com/157068/diff/12/1024
File src/pkg/json/struct.go (right):

http://codereview.appspot.com/157068/diff/12/1024#newcode391
src/pkg/json/struct.go:391: case *reflect.ChanValue:
Go is different from C here.  The code you've written declares
a case *reflect.ChanValue with no body.  It doesn't fall through
to the next case.

What you want is a single case statement with a comma-separated
list of elements:

case *reflect.ChanValue, 
    *reflect.InterfaceValue,
    *reflect.PtrValue, ...:

http://codereview.appspot.com/157068/diff/12/1024#newcode394
src/pkg/json/struct.go:394: case *reflect.UintptrValue:
A Uintptr is just an integer like any other uint.
It can get printed as part of default.

http://codereview.appspot.com/157068/diff/12/1025
File src/pkg/json/struct_test.go (right):

http://codereview.appspot.com/157068/diff/12/1025#newcode178
src/pkg/json/struct_test.go:178: marshalTest{map[string]string{"one": "one"},
`{"one":"one"}`},
please change "one": "one" to "one": "1"

also, add a new test of a non-string value type:

marshalTest{map[string][]int{"3": []int{1,2,3}}, `{"3":[1,2,3]}`},

[hoisie, 2009-11-20 04:29:54]

Good catch on the map case :)

Re-uploaded, and I added those tests.

[rsc, 2009-11-20 04:45:04]

*** Submitted as http://code.google.com/p/go/source/detail?r=0f043e6b7545 ***

Add json.Marshal to json package

R=rsc
CC=golang-dev
http://codereview.appspot.com/157068

Committer: Russ Cox <rsc@golang.org>