Your code sets isConsecutive
to false
and aims to set it to true
when it finds evidence. The problem is that a[i]==a[i+1]
is only partial evidence. So you set it to true
when a[0] == a[1]
, and never change it back if (say) a[1] != a[2]
.
In this case, it would work better if you start with isConsecutive = true
, and in your loop, look for conclusive evidence that it's false:
boolean firstFourEqual(int[] a) {
boolean isConsecutive=true;
for(int i=0;i<3;i++) {
if(a[i]!=a[i+1]) {
isConsecutive=false;
}
}
return isConsecutive;
}
(note that we're iterating 3 times, not 4, because the third test checks the third and fourth element.)
Once you've set isConsecutive
to false, there's no going back, so you might as well leave the loop. Two ways you could do this:
boolean firstFourEqual(int[] a) {
boolean isConsecutive=true;
for(int i=0;i<3;i++) {
if(a[i]!=a[i+1]) {
isConsecutive=false;
break; <-- leave the loop early
}
}
return isConsecutive;
}
Or:
boolean firstFourEqual(int[] a) {
for(int i=0;i<3;i++) {
if(a[i]!=a[i+1]) {
return false;
}
}
return true;
}
Some people think having a return
mid-method is bad style. Others think that having a mutable variable is bad style. My preference is for the last of these examples. The method is short and clear. There's a mid-method return, but the method is so short, it's easy to spot.
It's not always best to start with true
and amend to false
, or vice versa. However, in a situation like this where a hypothesis (e.g. "The first four items are equal") can be instantly disproved by a single piece of evidence ("False because items 3 and 4 are unequal"), it makes sense to default to the result of the hypothesis, changing it to the opposite result if you find evidence otherwise.
Finally, you could do this with streams:
return IntStream.range(0,3)
.mapToObj( x -> a[x] == a[x+1])
.noneMatch( b -> b == false);
All of these assume that the array has as size of at least 4. You would need more code if this can't be guaranteed.