Incorrect logic in my loop

Question

public static void main(String[] args) {
        Integer[] courses1 = {1,2,3};
        Integer[] courses2 = {0,1};

        Integer[] longestArr;
        Integer[] shortestArr;
        ArrayList commonCourses = new ArrayList();

        if ( courses1.length > courses2.length)
        {
            longestArr = courses1; shortestArr = courses2;
        }    
        else
        {    
            longestArr = courses2; shortestArr = courses1;
        }

        for ( int i : longestArr)
        {
            for ( int j : shortestArr)
            {
                if (i == j);
                   commonCourses.add(i); 
            }
        } 
        Collections.sort(commonCourses);
        System.out.println(commonCourses.size());


}

No matter what the values of course1 and courses2, the arrayList commonCourses always has a size of one more the the total number of elements in both arrays when it is supposed to only contain the elements that are in both arrays.

I have 2 questions why is ever element from the 2 arrays being added to the arrayList and why is the size of the arrayList always one more than the total number of elements?

If you are wondering why I have courses1 and courses2 declared at the start, this is a problem from talentBuddy.co that I'm testing on eclipse. I have tested with different starting conditions and the same thing always happens.

My new slimmed down solution

public static void main(String[] args) {
        Integer[] courses1 = {1};
        Integer[] courses2 = {0};

        ArrayList <Integer>commonCourses = new ArrayList<Integer>();

        for ( int i : courses1)
        {
            for ( int j : courses2)
            {
                if (i == j)
                   commonCourses.add(i); 
            }
        } 
        Collections.sort(commonCourses);
        System.out.println(commonCourses.size());     
}

You can do this with an single loop. No need for nested loops — RisingSun, Aug 14 '15 at 18:29
I thought that if I used the shortest array in the outside loop it would have an array out of bounds exception but using the enhanced for loop I see that cannot happen — Jerry Murphy, Aug 14 '15 at 18:31

score 3 · Answer 1 · answered Aug 14 '15 at 18:24

3

Your problem is the semicolon in this line:

if (i == j);

The if clause ends here and the following line is always executed.

answered Aug 14 '15 at 18:24

Sleafar

1,486
8
10

score 2 · Accepted Answer · edited May 23 '17 at 12:24

2

The semi-colon at the end of the if-condition is your problem. For a detailed explanation on why this happens, see this StackOverflow post. In short, the semi-colon causes the compiler to ignore the conditional at runtime.

for ( int j : shortestArr)
{
    if (i == j);
    //         ^  Remove this semi-colon
    commonCourses.add(i); 
}

Using curly-braces to format your if-statements helps prevent this from happening:

if(condition){
    doSomething();
}

edited May 23 '17 at 12:24

Community

1
1

answered Aug 14 '15 at 18:25

CubeJockey

2,209
8
24
31

2

The suggestion in that post you linked gives good advice, always uses braces and this will likely never happen. – Jerry Murphy Aug 14 '15 at 18:29
I disagree that adding braces would prevent this from happening. His coding style is start-brace on next line, and adding the braces but leaving the extraneous semi-colon would change nothing. Now, using a good IDE (e.g. Eclipse) would very likely warn you that your code is suspicious, and THAT will prevent errors like that. – Andreas Aug 14 '15 at 18:54
@Andreas I used Eclipse to debug his code, and there was no warning thrown. Likely after some smart customization though, it would recognize the issue ahead of time, but not by default. – CubeJockey Aug 14 '15 at 18:57
@Trobbins I just pasted the original code into Eclipse and got multiple warnings in editor. Code compiles and runs without error, but editor shows warnings in yellow. Ignoring the warnings about generics leaves the warning I was referring to, which is marked on the offending semi-colon: "Empty control-flow statement". – Andreas Aug 14 '15 at 18:58
Check your settings: Windows > Preferences > Java > Compiler > Error/Warnings, section "Potential programming problems" > "Empty statement" and make sure it's set to "Warning". Better yet, if you have a little time, change all "Ignore" to "Warning" (leave "Error"), then ignore only the few warnings you don't care about (e.g. "Non-externalized strings"). – Andreas Aug 14 '15 at 19:04

Alexandru Severin · Answer 3 · 2015-08-14T18:29:17.887

1

You have a misused semicolon after the if condition.

if (i == j);

edited Aug 14 '15 at 18:29

answered Aug 14 '15 at 18:25

Alexandru Severin

6,021
11
48
71

1

[No, `i` is definitely not the index](https://ideone.com/71aYYt). Otherwise, things like `String s : stringArray` wouldn't work at all. And auto-unboxing converts `Integer` to `int`, which makes them `==`-comparable. – Siguza Aug 14 '15 at 18:27

score 1 · Answer 4 · answered Aug 14 '15 at 18:28

 for ( int j : shortestArr)
 {
      if (i == j);
              // ^ REMOVE THIS SEMICOLON
           commonCourses.add(i); 
 }

The problem is that extra semicolon. Your chunk of code is functionally identical to this right now:

 for ( int j : shortestArr)
 {
      if (i == j)
      {
      }

      commonCourses.add(i); 
 }

score 1 · Answer 5 · answered Aug 14 '15 at 18:35

I have never worked in a firm where they did not require parenthesis after if:

if (...) {
   ...
}

And the reason quite evident.

Let me show the most abstracted solution:

Courses are sets of unique course numbers
Common courses is an intersection set
In java one uses abstract interfaces like Set for the variable, but can implement that set with different implementing classes

One wants a sorted set, TreeSet is such an implementation

Set<Integer> courses1 = new HashSet<>();
Collections.addAll(courses1, 1, 2, 3);

SortedSet<Integer> courses2 = new HashSet<>();
Collections.addAll(courses2, 0, 1);

SortedSet<Integer> commonCourses = new TreeSet<>(courses1);
commonCourses.retainAll(courses2);

I haven't come across sets yet (or worked in a firm!) but I will look into them it looks like an elegant solution. — Jerry Murphy, Aug 14 '15 at 18:39

Incorrect logic in my loop

5 Answers5