5

I would like to know, is it considered a common practice to use constructions like |=, &&, ||, != altogether in the single line of code?

E.g.

hasErrors |= vi2!=null && vi2.hasErrors() || vi.hasErrors();

What can be done to make the code more readable?

The programming language is Java, if it matters.

|improve this question
20

Personally, I'd consider either

hasErrors = hasErrors || (vi2!=null && vi2.hasErrors()) || vi.hasErrors();

or

hasErrors = ...;
hasErrors |= (vi2!=null && vi2.hasErrors());
hasErrors |= vi.hasErrors();

more readable, i.e., I feel that it takes me slightly less time to read and understand the meaning.

Note, however, that the three options are not semantically equivalent: In Java, |= does not short-circuit. Thus, if hasErrors() is a costly operation and you do not need its side effects, the first of my options is probably the most efficient.

|improve this answer
  • 3
    Your first option has also the advantage of expliciting operator precedences, which is something I never remember for operators other than the basic +, -, * and /. – mouviciel Dec 20 '12 at 8:21
  • 2
    @mouviciel Very good point. Code which requires you to remember operator precedences by definition harder to read than code which doesn't! – vaughandroid Dec 20 '12 at 10:46
  • It would also improve readability to wrap the statement in a well named method call. I find any boolean operation needing more than 2 operators becomes difficult to read. – Jay Lindquist Dec 20 '12 at 14:41
3

IMO you can introduce more variables ,

 hasVi2Errors = vi2!=null && vi2.hasErrors(); 
 hasViErrors = vi.hasErrors(); 
 hasErrors = hasErrors || hasVi2Errors || hasViErrors;

Advantage is following through is easy, as in each condition all are ORs or all are ANDs. Also if in future you need to add more condition like vi!= NULL, you can change only one line, by keeping the solution still readable.

|improve this answer
  • 3
    -1 IMO, adding more variables make code less readable, especially if they are poorly named as x, x2, x3. And it adds the possibility of introducing errors because of code-complete to the wrong variable. – Andy Dec 20 '12 at 9:28
  • 1
    It's worth noting, as @Heinzi's answer does, that your suggested replacement is not semantically equivalent to the original code. – vaughandroid Dec 20 '12 at 10:24
  • @Baqueta - Then having such constructs in single line, is more harmful for code readability. – Manoj R Dec 20 '12 at 11:03
  • @Andy - Please explain "code-complete to the wrong variable"? – Manoj R Dec 20 '12 at 11:06
  • @ManojR I mean the IDE-feature to complete a variable's name upon typing. You are often kind to hit enter for the first entry in the list. In your example, if you type "has", the IDE suggests 3 variables - than you got a 33% chance to be right. – Andy Dec 20 '12 at 12:10
0

If it is short expressions, it is okay to put a few statements like those in one line (or even break in the next lines).

However, I'd rather create a method for that purpose, e.g.

private boolean hasErrors() {
    return hasErrors || vi2!=null && vi2.hasErrors() || vi.hasErrors();
}

Then a call would look like if(hasErrors()) {...}. It is easy to read and easy to spot where to change the code, if necessary. Furthermore, it is reusable and easily testable in unit-tests. (Especially the latter point is very charming to me.)

|improve this answer
  • 1
    Turning a single line statement into a method is over-engineering and does not augment readability imo. – André Dec 20 '12 at 13:48
  • @André Well, it depends... I added some explanation why I like this approach. However, it may not always be suitable. – Andy Dec 20 '12 at 14:18
  • 2
    if you used hasErrors() incorrectly throughout your code, you could be surprised by the results - the function doesn't really tell me what it's checking for errors. Also, if you intend to use the new function only once, why create a function call at all? – CokoBWare Dec 20 '12 at 17:08
0

First off, the bitwise-or-and-assign |= is simply what you're doing with the expression. It's usually an assignment operator =, and a reader has to catch that this line is different, but it's fundamentally the goal of what you're doing. So it's perfectly fine to mix it with other logical operators found in the expression.

As for question of putting it all on one line, I think it's a matter of length. One or two operators on one line works if the variable names are short and there's nothing complex:

hasErrors |= vi2.hasErrors() || vi.hasErrors() || vi3.hasErrors();

I'm not a real hard-ass when it comes to 80 length lines, but you don't want to go past that limit too far:

isfuzzywuzzy |= themodulewithcats.fuzzycats.hairlegth > 5 || themodulewithcats.shaggycats.hairlength > 7 || genericContainerclass.Isuckatnamingconventions.feelsbadman();

This is better displayed by breaking it into multiple lines

isfuzzywuzzy |= themodulewithcats.fuzzycats.hairlegth > 5 || 
                themodulewithcats.shaggycats.hairlength > 7 || 
                genericContainerclass.Isuckatnamingconventions.feelsbadman();

Ideally aligning with the start of the expression, but auto-formatting never catches that so it's kind of a pain.

And if you have a mix of operators in the expression, or anything that makes it more complex, it's best to split it out and put parameters around things that need to be explicit. Breaking the line on the logical-or works best because of it's low order of operation.

hasCrazyErrors |= hasErrors > someObscureCode() || 
                  (vi2!=null && vi2.hasErrors()) || 
                  vi.hasErrors();

And while don't like implied brackets and long lines, all this is different if you're dealing with a large set of such instructions:

if(Bit1) errors |= vi2.hasErrors() || vi.hasErrors() || vi3.hasErrors(); else errors = 0;

Looks horrible right? Simply hard to read. But if you put enough of that together you get something where the eye can easily diff and tell what the hell is going on. (This is ultimately an example of breaking Don't Repeat Yourself, but it comes up a lot in embedded programming.)

if(Bit1) errors |= vi1.hasErrors() || vi.hasErrors() || vi3.hasErrors(); else errors = 0;
if(Bit2) errors |= vi2.hasErrors() || vi.hasErrors() || vi4.hasErrors(); else errors = 0;
if(Bit3) errors |= vi3.hasErrors() || vi.hasErrors() || vi5.hasErrors(); else errors = 0;
if(Bit4) errors |= vi4.hasErrors() || vi.hasErrors() || vi6.hasErrors(); else errors = 0;
if(Bit5) errors |= vi5.hasErrors() || vi.hasErrors() || vi7.hasErrors(); else errors = 0;

And remember, if you're working on a team, the convention everyone uses is better than your own personal convention. Uniformity is more important.

|improve this answer
-2

Personally i think it is fine to use these constructions on one line, if use brackets to separate statements, for example :

    private boolean hasErrors()
    {
      return ( ( hasErrors |= vi2 != null ) && ( vi2.hasErrors() ) || ( vi.hasErrors() ));
    }

This is so you can see exactly what each statement is doing and how they are connected.It is much easier for spotting errors.

|improve this answer
  • Taking a single line statement and hiding it into a method while adding a bunch of parentheses does not make something more readable imo. – André Dec 20 '12 at 13:46
  • If you wanted to have it in a one line statement then yes it would be more readable. You can see exactly what is a statement and how the statements are connected. I would not advise it for very long statements where they can easily be separated but for one line statements it is better to have parenthesis than nothing it all. – CodeCompileHack Dec 20 '12 at 13:49
  • If hasErrors or vi were local to the original function, you now have to make them module/class level. Which makes them visible to other functions. Not recommended here! – Andrew Dec 20 '12 at 14:49
  • Sorry i was simply just answering this in the case of if you had a single line with multiple constructions and statements how could they make it more readable, that's how i interpreted the question. I was simply stating that brackets could just make it easier to read if you were writing code in this way. – CodeCompileHack Dec 20 '12 at 14:56
  • I respectfully disagree. Over use of brackets adds clutter. – Philip Dec 20 '12 at 15:48

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.