2
\$\begingroup\$

I am working with synchronization for the first time. I wrote some code that I have to implement in my current work. I wanted to know if I am doing it right or am I making a mistake.

Whenever a new list of data is downloaded, NewDataArrived() is called. This method then calls the ListData() that returns the list of previously saved data. There is another class running in parallel that uses the ListData(). What I want to do is whenever the List is being updated, the Thread class should just wait until the list is updated.

public class ListData() {

/*
*
*




*/
public List < name_holder > getList() {
    synchronized(LOCK_LIST) {
        if (newDataArrived)
            LOCK_LIST.notifyAll();

        return full_list;
    }
}


void NewDataArrived() {

    newDataArrived = true;
    parallelDownload p = new parallelDownload();
    p.start();
    getList().addAll(my_list);
    newDataArrived = false;


}

class parallelDownload extends Thread {
    private Object o = new Object();

    @Override
    public void run() {
        synchronized(o) {
            for (name_holder n: getList()) {
                while (newDataArrived) {
                    try {
                        o.wait();
                    } catch (InterruptedException e) {
                        e.printStackTrace();
                    }
                }
            }
        }
    }
}
}
\$\endgroup\$
4
  • \$\begingroup\$ Welcome to Code Review! If I might recommend one very quick step to make your code formatting/indentation better, is to just copy the code into a code beautifier (a JavaScript or PHP beautifier works fine for Java as well - I often use jsbeautifier.org for Java code) \$\endgroup\$
    – Phrancis
    Mar 17 '16 at 19:38
  • \$\begingroup\$ You explained what you are trying to do but I still don't understand. What is parallelDownload supposed to do? The newDataArrived variable isn't accessible in parallelDownload. What is getList()? Sorry, but this code looks rather messy and confusing and it's hard to make sense of it. \$\endgroup\$ Mar 18 '16 at 1:03
  • \$\begingroup\$ Solomonoff's Secret .. The parallel download gets the information from the list and uses that information to perform network related things. I am sorry, i forgot to put than in the code above. \$\endgroup\$ Mar 18 '16 at 1:32
  • \$\begingroup\$ Please avoid updating your code after you've received answers. This invalidates said answers. Read What to do after receiving answers on the help page for more information. \$\endgroup\$
    – Quill
    Mar 18 '16 at 1:36
1
\$\begingroup\$

Coding Conventions

First of all: You should follow common naming conventions for Java. It makes it harder to read your code if you don't -- or to put it differently: It is easier to help if the code does follow common conventions. Maybe have a look at Google's Java Style or at least:

  • write methods in lowerCamelCase style and
  • classes in UpperCamelCase style.

Furthermore it would be good to provide a compilable minimal example.

Threadsafe List

From what I understood you want to have a list which is capable of being accessed and modified from multiple threads. If so, you may have a look at this stackoverflow discussion.

Java Concurrency

Some general things:

  1. You don't need to instantiate an object for your synchronized blocks. If you do not specify an object, the current object is used. That should be sufficient for your use cases.
  2. Don't start a thread directly. Rather use an ExecutorService instead.
  3. Don't subclass Thread, but rather use a Runnable (also see here).
  4. Very good book on the whole topic: Java Concurrency in Practice.
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.