You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@harmony.apache.org by Jesse Wilson <je...@google.com> on 2009/10/26 22:57:24 UTC
Idiomatic Java: inverted conditions
Harmony Team,
Continuing along with a theme, there's another C/C++ism in our Java code
that frustrates me. Our Java code frequently inverts conditions from their
natural language form. From HttpURLConnectionImpl:
if (null == resHeader) {
resHeader = new Header();
}
...or even more surprising, from HttpURLConnection:
if (0 < chunkLength) {
throw new IllegalStateException(Msg.getString("KA003"));
}
I find myself having to slow down to interpret what the code intends. I
can't mentally parse "if 0 is less-than chunkLength" nearly as efficiently
as the equivalent "if chunkLength is greater than 0" condition. From a quick
survey of the code base, it looks like we use a mix of the two forms.
If anyone thinks I should avoid flipping of these conditionals back to their
normal form, please let me know. In my logging patch, I flipped several
"null == foo" checks and I found it made the code easier to read.
Thanks,
Jesse
Re: Idiomatic Java: inverted conditions
Posted by Oliver Deakin <ol...@googlemail.com>.
Jesse Wilson wrote:
> Harmony Team,
>
> Continuing along with a theme, there's another C/C++ism in our Java code
> that frustrates me. Our Java code frequently inverts conditions from their
> natural language form. From HttpURLConnectionImpl:
>
> if (null == resHeader) {
>
> resHeader = new Header();
>
> }
>
>
> ...or even more surprising, from HttpURLConnection:
>
> if (0 < chunkLength) {
>
> throw new IllegalStateException(Msg.getString("KA003"));
>
> }
>
>
> I find myself having to slow down to interpret what the code intends. I
> can't mentally parse "if 0 is less-than chunkLength" nearly as efficiently
> as the equivalent "if chunkLength is greater than 0" condition. From a quick
> survey of the code base, it looks like we use a mix of the two forms.
>
> If anyone thinks I should avoid flipping of these conditionals back to their
> normal form, please let me know. In my logging patch, I flipped several
> "null == foo" checks and I found it made the code easier to read.
>
+1 to making these changes - increasing readability helps everyone to
work in the codebase and there is no need for these inverted conditions.
Regards,
Oliver
> Thanks,
> Jesse
>
>
--
Oliver Deakin
Unless stated otherwise above:
IBM United Kingdom Limited - Registered in England and Wales with number 741598.
Registered office: PO Box 41, North Harbour, Portsmouth, Hampshire PO6 3AU
Re: Idiomatic Java: inverted conditions
Posted by Chris Gray <ch...@kiffer.be>.
On Tue, 27 Oct 2009 14:21:14 +0000, Tim Ellison <t....@gmail.com>
wrote:
> On 27/Oct/2009 13:35, Xiao-Feng Li wrote:
>> On Tue, Oct 27, 2009 at 6:51 PM, Tim Ellison <t....@gmail.com>
>> wrote:
>>> On 26/Oct/2009 21:57, Jesse Wilson wrote:
>>>> Continuing along with a theme, there's another C/C++ism in our Java
>>>> code
>>>> that frustrates me. Our Java code frequently inverts conditions from
>>>> their
>>>> natural language form.
>>> I'm sure we all have our own horror stories. The ones that make me
>>> cringe are structured like this,
>>>
>>> public void foo(Object bar) {
>>>
>>> if (bar != null) {
>>> ...
>>> <some long method, typically /too/ long>
>>> ...
>>> return result;
>>> }
>>>
>>> throw IllegalArgumentException();
>>>
>>> }
>>>
>>> Grrr.
>>>
>>> Tim
>>>
>>
>> LOL. The code examples from Jesse, Alexey and Tim are all interesting.
>> When I saw those styles, I usually just assumed the authors must have
>> their strong justifications for that, since sometimes I saw the code
>> from some seasoned programmers and they refused to give an
>> explanation. :) I might guess the original intention of the authors
>> is to help the (unwise) compiler to produce expected efficient code.
>> For example, with the code above, the author may expect the compiler
>> be silly and instruct it to produce fall-through code for the
>> most-frequently-taken branch. Well, with modern
>> microprocessor/compiler, this kind of code is (almost) no longer
>> needed.
>
> I agree Xiao-Feng, and that is a poor reason to write code in this style
> anyway. I would even suggest that coding for the compiler and coding
> for the next person who has to read the code are usually complimentary,
> such as marking fields as final, and using the most restrictive possible
> method scope, etc. Of course, these details are usually swamped by
> higher-level algorithm choice anyway.
Some of these are indeed C-isms - e.g. "if (constant == variable)" because
if you mistakenly typed "if (constant = variable)" the C compiler would
give you an error whereas with "if (variable = constant)" it would not. (In
fact gcc will issue a warning for this pattern, so even this is moot). But
others are based on well-intentioned rules such as "conditionals should be
positive, i.e. if the conition is true this should indicate that everything
is OK", or "show the normal path of excution first and the abnormal paths
afterward". These rules sound fine until you try to put them into practice.
As for the "one return point at the end" rule, I personally don't see a
big difference between
if (bar == null) return null;
and
if (bar == null) throw new IllegalArgumentException();
So for me early reaturns are OK in this kind of context, especially it
there is enough whitespace around that they are not likely to be
overlooked.
Greets
Chris Gray
--
Chris Gray /k/ Embedded Java Solutions
Re: Idiomatic Java: inverted conditions
Posted by Tim Ellison <t....@gmail.com>.
On 27/Oct/2009 13:35, Xiao-Feng Li wrote:
> On Tue, Oct 27, 2009 at 6:51 PM, Tim Ellison <t....@gmail.com> wrote:
>> On 26/Oct/2009 21:57, Jesse Wilson wrote:
>>> Continuing along with a theme, there's another C/C++ism in our Java code
>>> that frustrates me. Our Java code frequently inverts conditions from their
>>> natural language form.
>> I'm sure we all have our own horror stories. The ones that make me
>> cringe are structured like this,
>>
>> public void foo(Object bar) {
>>
>> if (bar != null) {
>> ...
>> <some long method, typically /too/ long>
>> ...
>> return result;
>> }
>>
>> throw IllegalArgumentException();
>>
>> }
>>
>> Grrr.
>>
>> Tim
>>
>
> LOL. The code examples from Jesse, Alexey and Tim are all interesting.
> When I saw those styles, I usually just assumed the authors must have
> their strong justifications for that, since sometimes I saw the code
> from some seasoned programmers and they refused to give an
> explanation. :) I might guess the original intention of the authors
> is to help the (unwise) compiler to produce expected efficient code.
> For example, with the code above, the author may expect the compiler
> be silly and instruct it to produce fall-through code for the
> most-frequently-taken branch. Well, with modern
> microprocessor/compiler, this kind of code is (almost) no longer
> needed.
I agree Xiao-Feng, and that is a poor reason to write code in this style
anyway. I would even suggest that coding for the compiler and coding
for the next person who has to read the code are usually complimentary,
such as marking fields as final, and using the most restrictive possible
method scope, etc. Of course, these details are usually swamped by
higher-level algorithm choice anyway.
Regards,
Tim
Re: Idiomatic Java: inverted conditions
Posted by Xiao-Feng Li <xi...@gmail.com>.
On Tue, Oct 27, 2009 at 6:51 PM, Tim Ellison <t....@gmail.com> wrote:
> On 26/Oct/2009 21:57, Jesse Wilson wrote:
>> Continuing along with a theme, there's another C/C++ism in our Java code
>> that frustrates me. Our Java code frequently inverts conditions from their
>> natural language form.
>
> I'm sure we all have our own horror stories. The ones that make me
> cringe are structured like this,
>
> public void foo(Object bar) {
>
> if (bar != null) {
> ...
> <some long method, typically /too/ long>
> ...
> return result;
> }
>
> throw IllegalArgumentException();
>
> }
>
> Grrr.
>
> Tim
>
LOL. The code examples from Jesse, Alexey and Tim are all interesting.
When I saw those styles, I usually just assumed the authors must have
their strong justifications for that, since sometimes I saw the code
from some seasoned programmers and they refused to give an
explanation. :) I might guess the original intention of the authors
is to help the (unwise) compiler to produce expected efficient code.
For example, with the code above, the author may expect the compiler
be silly and instruct it to produce fall-through code for the
most-frequently-taken branch. Well, with modern
microprocessor/compiler, this kind of code is (almost) no longer
needed.
Thanks,
xiaofeng
--
http://people.apache.org/~xli
Re: Idiomatic Java: inverted conditions
Posted by Tim Ellison <t....@gmail.com>.
On 26/Oct/2009 21:57, Jesse Wilson wrote:
> Continuing along with a theme, there's another C/C++ism in our Java code
> that frustrates me. Our Java code frequently inverts conditions from their
> natural language form.
I'm sure we all have our own horror stories. The ones that make me
cringe are structured like this,
public void foo(Object bar) {
if (bar != null) {
...
<some long method, typically /too/ long>
...
return result;
}
throw IllegalArgumentException();
}
Grrr.
Tim
Re: Idiomatic Java: inverted conditions
Posted by sebb <se...@gmail.com>.
On 26/10/2009, Alexey Petrenko <al...@gmail.com> wrote:
> The construction which kills me is:
>
> <if "some string".equals(str)> instead of <if str.equals("some string")>
>
> But unfortunately the first form is better then second one :)
Surely it's only better if the "str" variable can be null?
i.e.
if ("some string".equals(str)) // works even if str == null
is simpler than
if (str != null && str.equals("some string"))
However, if "str" is known to be non-null, then use
if (str.equals("some string"))
In the first case, I suggest including the comment to avoid problems
if someone tries to swap the variable and string.
> Alexey
>
> 2009/10/27 Jesse Wilson <je...@google.com>:
>
> > Harmony Team,
> >
> > Continuing along with a theme, there's another C/C++ism in our Java code
> > that frustrates me. Our Java code frequently inverts conditions from their
> > natural language form. From HttpURLConnectionImpl:
> >
> > if (null == resHeader) {
> >
> > resHeader = new Header();
> >
> > }
> >
> >
> > ...or even more surprising, from HttpURLConnection:
> >
> > if (0 < chunkLength) {
> >
> > throw new IllegalStateException(Msg.getString("KA003"));
> >
> > }
> >
> >
> > I find myself having to slow down to interpret what the code intends. I
> > can't mentally parse "if 0 is less-than chunkLength" nearly as efficiently
> > as the equivalent "if chunkLength is greater than 0" condition. From a quick
> > survey of the code base, it looks like we use a mix of the two forms.
> >
> > If anyone thinks I should avoid flipping of these conditionals back to their
> > normal form, please let me know. In my logging patch, I flipped several
> > "null == foo" checks and I found it made the code easier to read.
> >
> > Thanks,
> > Jesse
> >
>
Re: Idiomatic Java: inverted conditions
Posted by Alexey Petrenko <al...@gmail.com>.
The construction which kills me is:
<if "some string".equals(str)> instead of <if str.equals("some string")>
But unfortunately the first form is better then second one :)
Alexey
2009/10/27 Jesse Wilson <je...@google.com>:
> Harmony Team,
>
> Continuing along with a theme, there's another C/C++ism in our Java code
> that frustrates me. Our Java code frequently inverts conditions from their
> natural language form. From HttpURLConnectionImpl:
>
> if (null == resHeader) {
>
> resHeader = new Header();
>
> }
>
>
> ...or even more surprising, from HttpURLConnection:
>
> if (0 < chunkLength) {
>
> throw new IllegalStateException(Msg.getString("KA003"));
>
> }
>
>
> I find myself having to slow down to interpret what the code intends. I
> can't mentally parse "if 0 is less-than chunkLength" nearly as efficiently
> as the equivalent "if chunkLength is greater than 0" condition. From a quick
> survey of the code base, it looks like we use a mix of the two forms.
>
> If anyone thinks I should avoid flipping of these conditionals back to their
> normal form, please let me know. In my logging patch, I flipped several
> "null == foo" checks and I found it made the code easier to read.
>
> Thanks,
> Jesse
>
Re: Idiomatic Java: inverted conditions
Posted by Mark Hindess <ma...@googlemail.com>.
In message <a4...@mail.gmail.com>,
Jesse Wilson writes:
>
> Harmony Team,
>
> Continuing along with a theme, there's another C/C++ism in our Java
> code that frustrates me. Our Java code frequently inverts conditions
> from their natural language form. From HttpURLConnectionImpl:
>
> if (null == resHeader) {
>
> resHeader = new Header();
>
> }
Even in C this is largely unnecessary since we compile most code with:
-Wall -Werror
which should catch inadvertent assignments.
> ...or even more surprising, from HttpURLConnection:
>
> if (0 < chunkLength) {
>
> throw new IllegalStateException(Msg.getString("KA003"));
>
> }
This is definitely unnecessary.
> I find myself having to slow down to interpret what the code
> intends. I can't mentally parse "if 0 is less-than chunkLength" nearly
> as efficiently as the equivalent "if chunkLength is greater than 0"
> condition. From a quick survey of the code base, it looks like we use
> a mix of the two forms.
+1. I agree it significantly impedes the reading of code.
> If anyone thinks I should avoid flipping of these conditionals back to
> their normal form, please let me know. In my logging patch, I flipped
> several "null == foo" checks and I found it made the code easier to
> read.
Again I'd rather separate patches but in this case it is trivial enough
that it is probably okay to make them in other patches.
-Mark.