fix(DateFilter): fix a wrong type

[technohippy]

not NumberFormat but DateFormat

[vicb]

LGTM
Note: I like to write this as var dfs = <num, DateFormat>{}; to avoid having to change on both side.

[technohippy]

Thanks. I've done so.

[vicb]

👍

[chalin]

I am assuming that no test was broken by this change. In which case this indicates that test(s) were missing :). Consider adding test(s).

[chalin]

Since this file is being reviewed, note that the MAP declaration @ line 25 should be marked

• final and the
• literal as const.

Give that this is a filter class (hence its main service is offered via the call() method), my guess is that MAP should be private: MAP -> _MAP.

I would suggestion the following declaration

static final Map<String, String> _MAP = const <String, String> { ... }  // (1)

but I know that @vicb will object to the type annotations :) ... preferring

static final _MAP = const <String, String> { ... }  // (2).

Until I have finished preparing my blog post to explain why type annotations are useful when they contain generics, I can live with (2) :).

[chalin]

I don't understand the purpose of dfs. It is empty and never assigned to. Given that this is an @NgFilter class it will only be instantiated by the dependency injector and access to this instance will be limited to use via an AngularDart pipe. Hence, clients will not have access to dfs.

If I were to guess a use for dfs it would be as a cache. In which case it should be populated with values just before line 61 after a value is obtained for df; i.e.,

dfs[format] = df;

In this case dfs should be made private: _dfs.

[vicb]

and @chalin I think I got what you mean for non-final vars... would be happy to get an eraly preview of your post though !

[chalin]

Great, thanks.

[chalin]

One more thing. In this article by Gilad Bracha, Emulating Functions in Dart, Gilad suggests that "As a matter of good style, have the class implement the Function interface". We should follow this recommendation here: i.e., add implements Function to the class declaration header.

[technohippy]

Done. Thanks!

[chalin]

Sorry, @vicb was correct about one thing. The style guide recommends "DO annotate with Object instead of dynamic to indicate any object is accepted". Please change the call() parameter type from dynamic to Object.

[chalin]

@vicb, since I was writing something up for my class, I decided turn it into a blog post (this is not the one on types I was referring to earlier). Comments are welcome. (I'll advertise for greater feedback after I hear from you.)

[vicb]

I actually was referring to the lines above that

In most places in Dart, a type annotation can be omitted, in which case the type will automatically be dynamic. Thus, omitting the type annotation entirely is semantically equivalent but more terse.

[chalin]

@vicb caught one more thing: the return type of call should be dynamic (or, as Victor would prefer it, there should be no return type annotation.)

[technohippy]

done.