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.
0
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.
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.
5
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.)
3
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.
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.
5