On Fri, 09 Dec 2011 13:15:47 -0000, Timon Gehr <timon.g...@gmx.ch> wrote:
On 12/09/2011 01:36 PM, Regan Heath wrote:
4)
if (std.ascii.toLower(p.front) == 'n' &&
(p.popFront(), std.ascii.toLower(p.front) == 'f') &&
(p.popFront(), p.empty))
'if' statements with side effects are yuck. I prefer the check for error
and bail style but you could use multiple layers of 'if' instead..
if (std.ascii.toLower(p.front) != 'n') //error handling
p.popFront();
if (std.ascii.toLower(p.front) != 'f') //error handling
p.popFront();
if (!p.empty) //error handling
Your '//error handling' shortcut hides relevant information.
What information? With context I could be more specific about what to do
for each/all.
5)
enforce((p.popFront(), !p.empty && std.ascii.toUpper(p.front) == 'A')
&& (p.popFront(), !p.empty && std.ascii.toUpper(p.front) == 'N'),
new ConvException("error converting input to floating point"));
This is blatant enforce abuse IMO..
p.popFront();
if(p.empty || std.ascii.toUpper(p.front) != 'A'))
throw new ConvException("error converting input to floating point"));
p.popFront();
if(p.empty || std.ascii.toUpper(p.front) != 'N'))
throw new ConvException("error converting input to floating point"));
This is blatant code duplication.
Of the throw line? Sure, and you can re-write with nested 'if' if you
prefer. I prefer the code duplication tho.
7)
if (c == '\"' || c == '\\')
put(w, '\\'), put(w, c);
else
put(w, c);
More {}; avoidance..
if (c == '\"' || c == '\\')
{
put(w, '\\');
put(w, c);
}
else
{
put(w, c);
}
No comma operator necessary to avoid {}:
if(x == '"' || c == '\\') put(w, '\\');
put(w, c);
Yeah, it was a /quick/ re-write without additional re-factoring (aside
from removal of the comma).
8)
return (++mi.m_cRefs, cast(HXModule)mi);
less (), one more ; and <enter>..
++mi.m_cRefs;
return cast(HXModule)mi;
() would not be necessary.
True, but if you're going to use the comma operator, enclosing it in () at
makes it more obvious you've done so. I tend to use extra () when using
the ?: operator for this reason.
9)
return (++mi.m_cRefs, hModule);
as above..
++mi.m_cRefs;
return hModule;
10)
return
c <= 0x7F ? 1
: c <= 0x7FF ? 2
: c <= 0xFFFF ? 3
: c <= 0x10FFFF ? 4
: (assert(false), 6);
*Much* clearer with the rewrite..
assert(c <= 0x10FFFF);
return
c <= 0x7F ? 1
: c <= 0x7FF ? 2
: c <= 0xFFFF ? 3
: c <= 0x10FFFF ? 4
: 6;
This is a *much* better rewrite:
assert(c <= 0x10FFFF);
return
c <= 0x7F ? 1
: c <= 0x7FF ? 2
: c <= 0xFFFF ? 3
: 4;
Again, I was missing context.. is the return value of 6 not required in
release mode?
None of the above look significantly harder without the comma operator
and quite a few are far clearer (to me) without it.
Yah, many of those occurences in Phobos are mostly unnecessary.
:)
--
Using Opera's revolutionary email client: http://www.opera.com/mail/
