Parallel task with synchronized arraylist

Question

My homework was to create project using parallelization that all should be proper. However, I made my project but my profesor mentioned something is wrong in my code "please look at array list, something is not ok, maybe synchronization?".

I would like ask you community to help me and point what could be wrong. I think it might be problem with not covering by synchronize brackets my array list, am I right?

import java.util.ArrayList;
import java.util.Collections;
import java.util.List;
import java.util.logging.Level;
import java.util.logging.Logger;

    /**
    * My project finds all dividors for specific number
    *It must use threads, so I made them. First I start them (first loop) 
    *then join them (second loop). My project must have that loops.
    *Problem might be with not synchronizing methods array list...
    */

    public class Main {

        private final static int NUMBER = 100;
        private final static List<Integer> dividors = new ArrayList<Integer>();

        public static void main(String[] args) {
            new Main().doStuff();
        }
        private int sqr;
        private int sqrp1;

        private void doStuff() {

            sqr = (int) Math.sqrt(NUMBER);
            sqrp1 = sqr + 1;

            Thread[] t = new Thread[sqrp1];

        //starting tasks
            for (int i = 1; i < sqrp1; i++) {
                final int it = i;

                if (NUMBER % i == 0) {
                    final int e = i;

                    t[i] = new Thread(new Runnable() {
                        @Override
                        public void run() {
                            System.out.println("sta"+e);
                            if (!checkContains(e)) {
                                addElement(e);
                            }

                            final int dividednumber = NUMBER / e;

                            if (!checkContains(dividednumber)) {
                                addElement(dividednumber);
                            }
                        }
                    });
                    t[i].start();
                }
            }

        //calling join for tasks
            for (int i = 1; i < sqrp1; i++) {
                final int it = i;

                if (NUMBER % i == 0) {
                    try {
                        System.out.println("sto"+i);
                        t[i].join();
                    } catch (InterruptedException ex) {
                        Logger.getLogger(Main.class.getName()).log(Level.SEVERE, null, ex);
                    }
                }
            }


            System.out.println("xxx");

            Collections.sort(dividors);
            Integer[] arrayDividors = dividors.toArray(new Integer[0]);

            for (int i = 0; i < arrayDividors.length; i++) {
                System.out.println(arrayDividors[i]);
            }
        }

        private synchronized void addElement(int element) {
            dividors.add(element);
        }

        private synchronized boolean checkContains(int element) {
            return dividors.contains(element);
        }
    }

---

Am I right changing this part, is it ok now?

t[i] = new Thread(new Runnable() {
    @Override
    public void run() {
        System.out.println("waiting " + e);
        synchronized (this) {
            System.out.println("entering " + e);

            if (!checkContains(e)) {
                addElement(e);
            }

            final int dividednumber = NUMBER / e;

            if (!checkContains(dividednumber)) {
                addElement(dividednumber);
            }
            System.out.println("leaving " + e);
        }
    }
});

Answer 1

You need to turn this into a single atomic operation.

 if (!checkContains(dividednumber)) {
     addElement(dividednumber);
 }

Imagine you have two threads.

 T1:  if (!checkContains(dividednumber)) { // false
 T2:  if (!checkContains(dividednumber)) { // false
 T1:      addElement(dividednumber); // adds number
 T2:      addElement(dividednumber); // adds same number

If you have one addElementWithoutDuplicates, this won't happen.