Skip to content
This repository was archived by the owner on Feb 22, 2018. It is now read-only.

fix(DateFilter): fix a wrong type #579

Closed
wants to merge 3 commits into from
Closed

fix(DateFilter): fix a wrong type #579

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1fda346
fix(DateFilter): fix a wrong type
technohippy Feb 18, 2014
8446b0e
fix(DateFilter): set cache
technohippy Feb 18, 2014
40ef0ee
refactor(DateFilter): code reviewed
technohippy Feb 18, 2014
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 5 additions & 5 deletions lib/filter/date.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ class DateFilter {
'shortTime': 'h:mm a',
};

Map<num, NumberFormat> nfs = new Map<num, NumberFormat>();
var dfs = <String, DateFormat>{};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


/**
* [date]: Date to format either as Date object, milliseconds
Expand All @@ -52,13 +52,13 @@ class DateFilter {
if (date is String) date = DateTime.parse(date);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since the main public service of this class is delivered through the call() method, it should have type annotations. I would suggest

String call(dynamic date, [String format = r'mediumDate'])

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • I'm all for (2)
  • I think dynamic is useless in your latest comment,
  • You must be right for dfs (i.e., the other less likely option would be extensibility by inheritance)
  • I think there are lots of place where the _ is missing for private variables...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use of dynamic is no more, no less useless than use of, say, @overrides. (But again, this discussion is for another time.)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Drop the r (raw) qualifier from the format default. In this case, it is truly useless :).

if (date is num) date = new DateTime.fromMillisecondsSinceEpoch(date);
if (date is! DateTime) return date;
var nf = nfs[format];
if (nf == null) {
var df = dfs[format];
if (df == null) {
if (MAP.containsKey(format)) {
format = MAP[format];
}
nf = new DateFormat(format);
df = new DateFormat(format);
}
return nf.format(date);
return df.format(date);
}
}