A shared counter is not incrementing as expected on concurrent server

Question

I'm trying to store the number of times any client requests a description from a ServerProtocol class.

Currently, the counter will increment from zero each time a new client joins. Any ideas?

Counter class:

public class Counter {

private int counter;

public synchronized int get() {
    return counter;
}

public synchronized void set(int n) {
    counter = n;
}

public synchronized void increment() {
    set(get() + 1);
}
}

Snippet from ServerProtocol class:

 case OPTIONS:
            if (theInput.equals("1")) {
                theOutput = "computer program description here  -- Another? Y or N";
                counter.increment();
                System.out.println(counter.get());
                state = ANOTHER;

The println method above is printing the current value of counter into the terminal in a Server Class:

ServerProtocol Class:

public class ServerProtocol {

private static final int TERMS = 0;
private static final int ACCEPTTERMS = 1;
private static final int ANOTHER = 2;
private static final int OPTIONS = 3;
private int state = TERMS;

public String processInput(String theInput) {
    String theOutput = null;

    Counter counter = new Counter();

    switch (state) {
        case TERMS:
            theOutput = "Terms of reference.  Do you accept? Y or N";
            state = ACCEPTTERMS;
            break;
        case ACCEPTTERMS:
            if (theInput.equalsIgnoreCase("y")) {
                theOutput = "1. computer program 2. picture 3. e-book";
                state = OPTIONS;
            } else if (theInput.equalsIgnoreCase("n")) {
                theOutput = "Bye.";
            } else {
                theOutput = "Invalid Entry -- Terms of reference.  Do you accept? Y or N";
                state = ACCEPTTERMS;
            }
            break;
        case ANOTHER:
            if (theInput.equalsIgnoreCase("y")) {
                theOutput = "1. computer program 2. picture 3. e-book";
                state = OPTIONS;
            } else if (theInput.equalsIgnoreCase("n")) {
                theOutput = "Bye.";
            } else {
                theOutput = "Invalid Entry -- Another? Y or N";
                state = ACCEPTTERMS;
            }
            break;
        case OPTIONS:
            if (theInput.equals("1")) {
                theOutput = "computer program description here  -- Another? Y or N";
                counter.increment();
                counter.get();
                state = ANOTHER;

            } else if (theInput.equals("2")) {
                theOutput = "picture description here -- Another? Y or N";
                state = ANOTHER;

            } else if (theInput.equals("3")) {
                theOutput = "e-book description here -- Another? Y or N";
                state = ANOTHER;

            } else {
                theOutput = "Invalid Entry -- 1. computer program 2. picture 3. e-book";
                state = OPTIONS;
            }
            break;
        default:
            System.out.println("Oops");
    }

    return theOutput;
}
}

Answer 1

Not sure if I get what you're asking, but if you want there to only be 1 counter, you can make it static. This should ensure that there is only 1 copy that gets incremented. Does that help?

Edit: you can read about static variables here.

Answer 2

The counter instance inside your serverprotocol method is a local variable. So everytime you call the method processInput a new instance of counter is created with zero value. Thats the reason.

Answer 3

Are you calling processInput more than once?

Counter isn't static; every time you initialize it (e.g. Counter counter = new Counter()), the count value will be reinitialized to 0. Either make it static, or make sure that it only gets initialized once.

Answer 4

You didn't specify life-cycle of ServerProtocol so I don't know if it's created every time client calls the server e.g. it's servlet in Google App Engine.

If not you need at least move definition of Counter counter from method to class. So Counter counter becomes class member.

PS. Making counter static in current code will work but it's uncool from design prospective.