0

I was reviewing some Java code with this line

if ( list.size() == 1 || list.size() <= 4) {...}

I commented that I do not see how that is any different from just doing

if (list.size() <= 4) {...}

he said it matters and needs to be the first. I don't understand. Perhaps if it were something like

if (list.size() == 1 || list.size() <= someVeryCostlyFunction() ) {...}

and the size was expected to be 1 most of the time you might use both, if someVeryCostlyFunction() always returns some positive number >= 1. Otherwise I can't see the difference. We are supposed to check for efficiency.

Tom
  • 16,842
  • 17
  • 45
  • 54
Tony
  • 1,127
  • 1
  • 18
  • 29
  • 3
    `"it matters and needs to be the first"` - This doesn't sound like a compelling answer to the question. I wouldn't put much stock in the actions of people who can't explain why they did those actions. – David Nov 09 '16 at 19:52
  • then you have to compute `list.size()` once for starters ... – Jean-François Fabre Nov 09 '16 at 19:52
  • By "needs to be the first" I meant the first example with the two conditions, and not the second with just the one condition – Tony Nov 09 '16 at 20:04
  • 1
    @Tony Looks to me that the developer just doesn't want to admit that a silly mistake was made. But I am now curious to see how the code is justified. – Nick Div Nov 09 '16 at 22:04
  • @Tony yeah, it would be interesting to know what was the outcome – MaxZoom Nov 11 '16 at 22:14
  • I think you guys are right that he did not want to admit to a mistake. I have done similar stuff in C like instead of declaring int x and passing &x to a function, I declared int *x and passed x, which is of course stupid if it was never initialized. But I digress.... ;-) – Tony Nov 29 '16 at 13:37

1 Answers1

1

The code smells with those two conditions:

if (list.size() == 1 || list.size() <= 4)

Perhaps the author have that in mind:

if there is some number of elements in the list that is 4 or less.

More over the current code allows to satisfy the condition even if the list has zero elements - which is most likely wrong.

Another problem with this condition is use of magic number 4?
What is so important about it and why it is not 5? It should be self documented and distinguished from other 4's that could appear in the code:

int MAX_HANDLED = 4;
if ( list.size() > 0 && list.size() <= MAX_HANDLED )
:
:
int ALL_TIRES = 4;
if (car.getTires() < ALL_TIRES) {
  car.stop();
}

As for the performance, I do not see any significant reason why existing condition should be any faster then yours proposed one (even the second would be faster when list.size > 1). See this question of similar concern.

Community
  • 1
  • 1
MaxZoom
  • 7,619
  • 5
  • 28
  • 44
  • I see now he wanted to be alerted if the size was > 4 but you still don't need the == 1. And you do need a >0 although I guess you will get an assertion error is size == 0: WebElement ele = lists.get(0). Will give an assertion (or size = 0 ind = 0 or something) – Tony Nov 10 '16 at 19:25
  • If there is already an assertion for empty list then you do not need first check. So the check you proposed should be sufficient. – MaxZoom Nov 10 '16 at 19:33