dart-archive / angular.dart.tutorial

AngularDart tutorial
MIT License
234 stars 89 forks source link

refactor(Recipe): rename constructor fromJsonMap to fromJson #99

Closed chalin closed 10 years ago

chalin commented 10 years ago
kwalrath commented 10 years ago

I have no opinion about fromJsonMap vs. fromJson. However, I find the literals a bit hard to read and would prefer using a constructor with types, like this:

_recipesCache = new Map<String, Recipe>();

That's as opposed to these:

_recipesCache = new Map();  // used in the original code
_recipesCache = <String, Recipe>{};  // used in your new code

What do you think, @chalin?

chalin commented 10 years ago

It may look a bit odd because the literal is just two characters. IMHO this helps drive home the point that generic parameters should be provided when using literals (as it sets its runtime type). Empty list or map literals abound. Here is a (quick grep) sampling from the several hundred in angular.dart:

./lib/change_detection/dirty_checking_change_detector.dart:1274:  final map =<dynamic, _DuplicateItemRecordList>{};
./lib/change_detection/watch_group.dart:266:        <String, WatchRecord<_Handler>>{},
./lib/core/cache.dart:46:  Map<K, V> _entries = <K, V>{};
./lib/core/scope.dart:610:          ? <String, int>{}
./lib/core_dom/directive_map.dart:16:    final directives = <NgAnnotation, List<Type>>{};
./lib/core_dom/directive_map.dart:48:    final fields = <String, AttrFieldAnnotation>{};
./lib/core_dom/http.dart:372:  var _pendingRequests = <String, async.Future<HttpResponse>>{};
./lib/core_dom/selector.dart:87:  var elementMap = <String, List<_Directive>>{};
./lib/core_dom/selector.dart:88:  var elementPartialMap = <String, _ElementSelector>{};
./lib/core_dom/selector.dart:90:  var classMap = <String, List<_Directive>>{};
./lib/core_dom/selector.dart:91:  var classPartialMap = <String, _ElementSelector>{};
./lib/core_dom/selector.dart:93:  var attrValueMap = <String, Map<String, List<_Directive>>>{};
./lib/core_dom/selector.dart:94:  var attrValuePartialMap = <String, Map<String, _ElementSelector>>{};
./lib/core_dom/selector.dart:125:            .putIfAbsent(name, () => <String, List<_Directive>>{})
./lib/core_dom/selector.dart:130:            .putIfAbsent(name, () => <String, _ElementSelector>{})
./lib/core_dom/selector.dart:278:    var classes = <String, bool>{};

In the end, its your call, but I would follow what they do in the angular.dart code :). Let me know if I need to follow through with adjustments.

kwalrath commented 10 years ago

Instead of following angular.dart code, which might be a little JavaScripty, I'd follow the lead of a long-time Dart programmer like Bob Nystrom—especially since he wrote the Dart style guide. He's recently worked on the barback project (with others), so that seems like something to follow:

https://code.google.com/p/dart/source/browse/branches/bleeding_edge/dart/pkg/barback/lib/src/

The code's not completely consistent, but a quick grep shows that barback uses new Map<...>() 12 times and <...>{} only once.

chalin commented 10 years ago

I agree about the JavaScripty-ness. Done.

kwalrath commented 10 years ago

Looks good. Thanks!