1
\$\begingroup\$

does someone has any opinions about this SafeBox StateMachine?

import java.util.Scanner;

public class SafeBoxStateMachine {

    public enum SafeBox {

        ACCEPT{     
            private Scanner scan;
            private boolean askPassword(SafeBoxStateMachine safebox){

                System.out.println("Please type a password:");
                scan = new Scanner(System.in);
                String buffer = scan.nextLine();

                if (buffer.equals(safebox.password) == false){
                    ++safebox.counter;
                    return false;
                }
                else {
                    return true;
                }   
            }
            public void process(SafeBoxStateMachine safebox){

                boolean res = askPassword(safebox);

                if (res == false) 
                    System.out.println("try number:" + safebox.counter);

                if (safebox.counter == 3){

                    safebox.state = SafeBox.PENELTY;                
                }

                if (res == true)
                {
                    safebox.state = SafeBox.SUCCESS;        
                }   
            }           
        }, 
        PENELTY{
            public void process(SafeBoxStateMachine safebox){
                System.out.println("7 seconds penalty!");
                try{
                    Thread.sleep(7000); 
                }
                catch(InterruptedException ex) {
                    Thread.currentThread().interrupt();
                }   
                safebox.counter = 0;
                safebox.state = SafeBox.ACCEPT;
            }       
        }, 
        SUCCESS{
            public void process(SafeBoxStateMachine safebox){

                System.out.println("Success!");
                safebox.stop = true;
            }

        };

        abstract public void process(SafeBoxStateMachine safebox);  
    }

    int counter = 0;    
    private SafeBox state = SafeBox.ACCEPT; 
    String password;
    boolean stop = false;

    SafeBoxStateMachine (String password){
        this.password = password;
        counter = 0;
    }

    public void run(){
        while (stop == false)           
            this.state.process(this);
    }

    public void setPass(String pass){
        this.password = pass;
    }

    public static void main(String[] args) {

        SafeBoxStateMachine pass = new SafeBoxStateMachine("hello");

        pass.run();

    }

}
\$\endgroup\$
2
\$\begingroup\$

Make fields immutable when possible

The setPass method is never used in the posted code. As such, you can make the password field immutable:

private final String password;

Reuse Scanner

ACCEPT.askPassword will recreate the Scanner scan field from System.in every time it's called. This is unnecessary, it would be better to create the scanner only once. That also means that you can make scan a final field.

Use boolean expressions directly

Instead of code like this:

if (res == false)
// ...

if (res == true)
// ...

Use boolean expressions directly like this:

if (!res)
// ...

if (res)
// ...

Do this everywhere where you used something == true or false.

Combine related if statements

In this code there are two conditions on the value of res:

if (res == false) 
    System.out.println("try number:" + safebox.counter);

if (safebox.counter == 3){

    safebox.state = SafeBox.PENELTY;                
}

if (res == true)
{
    safebox.state = SafeBox.SUCCESS;        
}

The code could be reorganized to make it clearer and to avoid one unnecessary condition:

if (safebox.counter == 3) {
    safebox.state = SafeBox.PENELTY;
}

if (res) {
    safebox.state = SafeBox.SUCCESS;
} else {
    System.out.println("try number:" + safebox.counter);
}

And since safebox.counter is only incremented in case of failure, the penalty condition can be moved inside the else branch of the second if statement:

if (askPassword(safebox)) {
    safebox.state = SafeBox.SUCCESS;
} else {
    System.out.println("try number:" + safebox.counter);
    if (safebox.counter == 3) {
        safebox.state = SafeBox.PENELTY;
    }
}
\$\endgroup\$
  • \$\begingroup\$ Hi Janos. Yet again another great review - fixed it all in accordance to your recommendations. they all make sense. tnx! \$\endgroup\$ – Arigatonic Dec 26 '16 at 8:26

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.