[Intl] Cleanup some code

[Intl] Cleanup some code

BUG=
R=alanknight@google.com

Committed: https://code.google.com/p/dart/source/detail?r=32217

[Alan Knight, 2014-01-28 01:33:45]

Thanks. A few quibbles, but on the whole good cleanups. A lot of that code
predates the existence of things like cascades, so it's good to revisit it.

https://chromiumcodereview.appspot.com/140843002/diff/1/pkg/intl/lib/date_for...
File pkg/intl/lib/date_format.dart (right):

https://chromiumcodereview.appspot.com/140843002/diff/1/pkg/intl/lib/date_for...
pkg/intl/lib/date_format.dart:258: if (utc)dateFields.utc = true;
Shouldn't there be a space between the end of the condition and the statement?

https://chromiumcodereview.appspot.com/140843002/diff/1/pkg/intl/lib/date_for...
pkg/intl/lib/date_format.dart:260: _formatFields.forEach((_) => _.parse(stream,
dateFields));
I normally treat _ as meaning "a parameter that's never used so it doesn't need
a name". Naming it that way and then using it seems wrong.

https://chromiumcodereview.appspot.com/140843002/diff/1/pkg/intl/lib/date_for...
pkg/intl/lib/date_format.dart:575: return parsed..add(matched);
I don't think I like this one. It eliminates the line, but it's too easy to miss
the cascade and be confused about what's going on.

https://chromiumcodereview.appspot.com/140843002/diff/1/pkg/intl/lib/date_sym...
File pkg/intl/lib/date_symbols.dart (right):

https://chromiumcodereview.appspot.com/140843002/diff/1/pkg/intl/lib/date_sym...
pkg/intl/lib/date_symbols.dart:87: "NAME": NAME,
I think the style guide would indent the map elements only two. Also, at this
point, couldn't this just be a lambda?

https://chromiumcodereview.appspot.com/140843002/diff/1/pkg/intl/lib/extract_...
File pkg/intl/lib/extract_messages.dart (right):

https://chromiumcodereview.appspot.com/140843002/diff/1/pkg/intl/lib/extract_...
pkg/intl/lib/extract_messages.dart:115: return !(node.target.token.toString() !=
"Intl");
Can we delete both "not"s and simplify this?

https://chromiumcodereview.appspot.com/140843002/diff/1/pkg/intl/lib/extract_...
pkg/intl/lib/extract_messages.dart:407: return !(node.target.token.toString() !=
"Intl");
Again, I think we can just remove both "not"s

https://chromiumcodereview.appspot.com/140843002/diff/1/pkg/intl/lib/generate...
File pkg/intl/lib/generate_localized.dart (right):

https://chromiumcodereview.appspot.com/140843002/diff/1/pkg/intl/lib/generate...
pkg/intl/lib/generate_localized.dart:109: result..write("  ")
I think the cascades should all line up and this one should be on a separate
line to match with the others.

https://chromiumcodereview.appspot.com/140843002/diff/1/pkg/intl/lib/generate...
pkg/intl/lib/generate_localized.dart:117: result.writeAll([entries.join(",\n"),
"\n  };\n}"]);
This to me seems less clear than having it as two separate lines. It's too easy
to get lost in a maze of twisty little punctuation characters.

https://chromiumcodereview.appspot.com/140843002/diff/1/pkg/intl/lib/intl.dart
File pkg/intl/lib/intl.dart (right):

https://chromiumcodereview.appspot.com/140843002/diff/1/pkg/intl/lib/intl.dar...
pkg/intl/lib/intl.dart:176: static bool _localeExists(localeName)
=>DateFormat.localeExists(localeName);
Missing a space after the =>

https://chromiumcodereview.appspot.com/140843002/diff/1/pkg/intl/lib/number_f...
File pkg/intl/lib/number_format.dart (right):

https://chromiumcodereview.appspot.com/140843002/diff/1/pkg/intl/lib/number_f...
pkg/intl/lib/number_format.dart:703: _StringIterator(String input): input =
input, length = input.length;
Why move the assignment of input out of the constructor arguments? 

Also, what's the point of having the length field? If the compiler can't
optimize the input.length calls away for a final string, we need new compiler
people :-)

[vicb, 2014-01-28 07:52:44]

Alan,

Thanks for the review, glad to hear it could be helpful (it also helps me
learning Dart).

I've integrated most of your feedback, only a couple of remarks.

https://chromiumcodereview.appspot.com/140843002/diff/1/pkg/intl/lib/date_for...
File pkg/intl/lib/date_format.dart (right):

https://chromiumcodereview.appspot.com/140843002/diff/1/pkg/intl/lib/date_for...
pkg/intl/lib/date_format.dart:258: if (utc)dateFields.utc = true;
fixed

https://chromiumcodereview.appspot.com/140843002/diff/1/pkg/intl/lib/date_for...
pkg/intl/lib/date_format.dart:260: _formatFields.forEach((_) => _.parse(stream,
dateFields));
This is how I start using "_" but I must admit is slowly became my equivalent
for for's "i". Anyway changed for "f".

https://chromiumcodereview.appspot.com/140843002/diff/1/pkg/intl/lib/date_for...
pkg/intl/lib/date_format.dart:575: return parsed..add(matched);
I would prefer to keep this one. To me this is a Dart pattern devs will get used
to with time. It is also widely used elsewhere in Dart internal libs.
Thought ?

https://chromiumcodereview.appspot.com/140843002/diff/1/pkg/intl/lib/date_sym...
File pkg/intl/lib/date_symbols.dart (right):

https://chromiumcodereview.appspot.com/140843002/diff/1/pkg/intl/lib/date_sym...
pkg/intl/lib/date_symbols.dart:87: "NAME": NAME,
Indentation fixed.

Not easy to change for a lambda as the method return a map.

https://chromiumcodereview.appspot.com/140843002/diff/1/pkg/intl/lib/extract_...
File pkg/intl/lib/extract_messages.dart (right):

https://chromiumcodereview.appspot.com/140843002/diff/1/pkg/intl/lib/extract_...
pkg/intl/lib/extract_messages.dart:115: return !(node.target.token.toString() !=
"Intl");
I did hesitate and thought there might be tricky edge cases when I first went
over the code but obviously it's equivalent, fixed.

https://chromiumcodereview.appspot.com/140843002/diff/1/pkg/intl/lib/extract_...
pkg/intl/lib/extract_messages.dart:407: return !(node.target.token.toString() !=
"Intl");
fixed

https://chromiumcodereview.appspot.com/140843002/diff/1/pkg/intl/lib/generate...
File pkg/intl/lib/generate_localized.dart (right):

https://chromiumcodereview.appspot.com/140843002/diff/1/pkg/intl/lib/generate...
pkg/intl/lib/generate_localized.dart:109: result..write("  ")
fixed

https://chromiumcodereview.appspot.com/140843002/diff/1/pkg/intl/lib/generate...
pkg/intl/lib/generate_localized.dart:117: result.writeAll([entries.join(",\n"),
"\n  };\n}"]);
I've changed because I (personal preferences) find it more readable and I know
that some script language have expense method calls (ie context switch) and this
version would be more efficient.
Thoughts ?

https://chromiumcodereview.appspot.com/140843002/diff/1/pkg/intl/lib/intl.dart
File pkg/intl/lib/intl.dart (right):

https://chromiumcodereview.appspot.com/140843002/diff/1/pkg/intl/lib/intl.dar...
pkg/intl/lib/intl.dart:176: static bool _localeExists(localeName)
=>DateFormat.localeExists(localeName);
fixed

https://chromiumcodereview.appspot.com/140843002/diff/1/pkg/intl/lib/number_f...
File pkg/intl/lib/number_format.dart (right):

https://chromiumcodereview.appspot.com/140843002/diff/1/pkg/intl/lib/number_f...
pkg/intl/lib/number_format.dart:703: _StringIterator(String input): input =
input, length = input.length;
"input" was moved out of the constructor args because it was used in the
initializer list.

I have reverted the addition of "length" as it could probably be automatically
optimized now with the input being final.

[Emily Fortuna, 2014-01-29 01:18:14]

https://chromiumcodereview.appspot.com/140843002/diff/1/pkg/intl/lib/date_for...
File pkg/intl/lib/date_format.dart (right):

https://chromiumcodereview.appspot.com/140843002/diff/1/pkg/intl/lib/date_for...
pkg/intl/lib/date_format.dart:575: return parsed..add(matched);
On 2014/01/28 07:52:45, vicb wrote:
> I would prefer to keep this one. To me this is a Dart pattern devs will get
used
> to with time. It is also widely used elsewhere in Dart internal libs.
> Thought ?
> 

I'm inclined to agree with Alan on this one. We use it in cases of
instantiation:
var list = new Set.from(oldSet)..add(newItem);

but for things as important as the return value of a function, I support being
more obvious.

[Alan Knight, 2014-01-29 01:27:57]

https://chromiumcodereview.appspot.com/140843002/diff/1/pkg/intl/lib/date_sym...
File pkg/intl/lib/date_symbols.dart (right):

https://chromiumcodereview.appspot.com/140843002/diff/1/pkg/intl/lib/date_sym...
pkg/intl/lib/date_symbols.dart:87: "NAME": NAME,
On 2014/01/28 07:52:45, vicb wrote:
> Indentation fixed.
> 
> Not easy to change for a lambda as the method return a map.

Why isn't that easy?  foo() => { "hello" : 1, "world" : 2}; is perfectly
allowable syntax.

https://chromiumcodereview.appspot.com/140843002/diff/1/pkg/intl/lib/generate...
File pkg/intl/lib/generate_localized.dart (right):

https://chromiumcodereview.appspot.com/140843002/diff/1/pkg/intl/lib/generate...
pkg/intl/lib/generate_localized.dart:117: result.writeAll([entries.join(",\n"),
"\n  };\n}"]);
On 2014/01/28 07:52:45, vicb wrote:
> I've changed because I (personal preferences) find it more readable and I know
> that some script language have expense method calls (ie context switch) and
this
> version would be more efficient.
> Thoughts ?
> 

Method calls should not be expensive at all in Dart. If anything, allocating a
new List just to access the two entries would be more expensive. I can see it
being more readable in some circumstances, but with so many special characters
in the strings I think it's better to have them on separate lines.

https://chromiumcodereview.appspot.com/140843002/diff/1/pkg/intl/lib/number_f...
File pkg/intl/lib/number_format.dart (right):

https://chromiumcodereview.appspot.com/140843002/diff/1/pkg/intl/lib/number_f...
pkg/intl/lib/number_format.dart:703: _StringIterator(String input): input =
input, length = input.length;
On 2014/01/28 07:52:45, vicb wrote:
> "input" was moved out of the constructor args because it was used in the
> initializer list.
> 
> I have reverted the addition of "length" as it could probably be automatically
> optimized now with the input being final. 

Interesting, I didn't realize that wasn't permitted.

[vicb, 2014-01-29 08:15:13]

Thanks for the feedback. I've integrated the changes.

https://codereview.chromium.org/140843002/diff/1/pkg/intl/lib/date_format.dart
File pkg/intl/lib/date_format.dart (right):

https://codereview.chromium.org/140843002/diff/1/pkg/intl/lib/date_format.dar...
pkg/intl/lib/date_format.dart:575: return parsed..add(matched);
reverted

https://codereview.chromium.org/140843002/diff/1/pkg/intl/lib/date_symbols.dart
File pkg/intl/lib/date_symbols.dart (right):

https://codereview.chromium.org/140843002/diff/1/pkg/intl/lib/date_symbols.da...
pkg/intl/lib/date_symbols.dart:87: "NAME": NAME,
I misunderstood... I was think of replacing the map itself with a closure.
fixed.

https://codereview.chromium.org/140843002/diff/1/pkg/intl/lib/generate_locali...
File pkg/intl/lib/generate_localized.dart (right):

https://codereview.chromium.org/140843002/diff/1/pkg/intl/lib/generate_locali...
pkg/intl/lib/generate_localized.dart:117: result.writeAll([entries.join(",\n"),
"\n  };\n}"]);
reverted

[Alan Knight, 2014-01-29 18:41:34]

LGTM with one quibble.

https://codereview.chromium.org/140843002/diff/1/pkg/intl/lib/date_format.dart
File pkg/intl/lib/date_format.dart (right):

https://codereview.chromium.org/140843002/diff/1/pkg/intl/lib/date_format.dar...
pkg/intl/lib/date_format.dart:575: return parsed..add(matched);
On 2014/01/29 08:15:14, vicb wrote:
> reverted

It doesn't look reverted. It looks like you moved the cascade up onto the
previous line. I'm saying I like it as three statements.

[vicb, 2014-01-29 22:05:12]

oh god, so you've spotted it :)

I've just *really* reverted in my last commit.

[Alan Knight, 2014-01-31 17:51:49]

lgtm

[Alan Knight, 2014-01-31 21:36:20]

Committed patchset #5 manually as r32217 (presubmit successful).