fix(recipe): follow dart:convert.JsonCodec JSON encoding convention

[chalin]

• recipe.dart of Chap. 5:
  • Follow (new?) dart:convert.JsonCodec#encode encoding convention and replace String toJsonString() by Map toJson().
  • Some code cleanup.
• Chap. 6 & 7 made to match updates to Chap. 5.

[kwalrath]

Taking a look now...

[kwalrath]

I'm not sure of the conventions, so I've asked @shailen to take a look.

[chalin]

@shailen, @kwalrath: any updates on this?

[kwalrath]

@shailen was working on something, but I believe he's turned it over for review. So how about it Shailen, do you have a few minutes to spare for this review?

[shailen]

OK, some general comments.

Your toJson() never actually gets called. Try deleting it, and you'll see that the code runs fine. And the Recipe.fromJsonMap() constructor doesn't work with Json data, but with a map. So it should have a signature like the following:

Recipe.fromMap(Map<String, dynamic> map) => ...;

The reason for this is clear when you look at the code in recipe_book.dart that uses Recipe.fromJsonMap():

void _loadData() {
  recipesLoaded = false;
  categoriesLoaded = false;
  _http.get('recipes.json')
    .then((HttpResponse response) {
      print(response);
      for (Map recipe in response.data) {
        recipes.add(new Recipe.fromJsonMap(recipe));
      }
   ...
}

The AngularDart http library already makes sure that the data from reading the JSON file is a list of Map objects. You are no longer dealing with a JSON string at that point.

I would rewrite the Recipe class to have just two constructors, Recipe(...) and Recipe.fromMap(...).

@jbdeboer, do you concur?

[adarshaj]

@shailen The point you raise about toJson() or toJsonString() not being used is correct, but that's because we are not POSTing back anything back to server? (i.e., there's no example of serialization?)

I would still like to keep toJson() or toJsonString(). Because, in future when the tutorial gets updated to demonstrate HTTP POST then you can do something like following:

_http.post('/path/to/post', JSON.encode(recipe)).then(...);
// or
_http.post('/path/to/post', recipe.toJsonString()).then(...);

I think the fromJsonMap makes it very clear that the Map is JsonMap (i.e. map generated by json) and not any arbitrary Map, so the verbosity is justified?

also, I don't understand your last comment, Recipe class currently has only two constructors?

[chalin]

@shailen Let me provide some context: as originally designed (by Tracy Powers?), recipe.dart contained

• toJsonString()
• factory Recipe.fromJsonMap()

The former is provided in support of the exercises of Chapter 6 (not every method that is part of the tutorial code is actually explicitly used in the running of the tutorial, but that does not diminish their pedagogical value). The latter is provided, as you know, to deserialize recipe data as of Chapter 5.

As I mentioned in the commit message, I renamed (and adapted the signature of) toJsonString() by toJson() to conform to the way dart:convert.JsonCodec#encode is meant to be used (which is what @adarshaj has been claiming as well I believe).

The dart:convert.JsonCodec does not define a named JSON type. Instead, (like in JavaScript) a subset of the core Dart types are recognized as "JSON data". That is why, for example, the signature of the encode() method is:

String encode(Object value, {Function toEncodable})

As for the factory Recipe.fromJsonMap() my edits mainly introduce generic types (replacing the parameter type Map by Map<String, dynamic>) to make clear that only Maps with String keys are recognized as JSON data. This factory constructor never accepted a String argument. There is now no serialization or deserialization to/from JSON data and String in this class.

Come to think of it, if any change still remains to be made, it should be to rename the factory to Recipe.fromJson() and drop the Map suffix. This would give us a nice symmetry in the names (toJson() vs fromJson()).

Let me know if you still have concerns.

[chalin]

@adarshaj the exercise at the end of Chapter 6 asks the reader to persist changes to the recipes via an HTTP PUT. Some edits to the Chapter 6 exercise make this clearer (while I had made them yesterday it seems that I forgot to submit the PR --- it is angulardart.org #47).

[chalin]

@shailen, @kwalrath: Sorry, I didn't expect this small change (only +12/-33 lines when ignoring whitespace) to be controversial :). Any update on this? I was hoping to use this new code this week in my class as part of a more global solution using JSON. Thanks.

[kwalrath]

I'm going to commit this change. We can always change the code later.

[chalin]

Thank you Kathy.