10
\$\begingroup\$

I learnt everywhere that using if statement is a bad practice when it's possible to avoid them. I'm trying to learn how to make clean code, it seems also that some design patterns could be helpful so I'm wondering if it's possible to refactor this piece of code in order to remove if statement from, here is the code demonstrating this:

 public class Printer {

    private boolean on=false;
    private Job currentJob=null;
    private String name;

    public String getName(){
        return name;
    }

    public void setName(String name) {
        this.name=name;
    }

    public void setOn(boolean _on){
        this.on=_on;
    }

    public boolean getOn(){
        return this.on;
    }

    public void setCurrentJob(Job _currentJob){
        this.currentJob=_currentJob;
    }

    public Job getCurrentJob(){
        return this.currentJob;
    }

    private boolean getOnStart(){
        setOn(true);
        return getOn();
    }

    public boolean start(){
        setOn(true);
        return on;
    }

    public boolean stop(){
        setOn(false);
        return !on;
    }

    public boolean suspend(){
        if (!isPrinting()) {
            throw new IllegalStateException("Error");
        }
            currentJob.setState(Job.WAINTING);
            return true;
    }

    public boolean resume(){
        if (this.currentJob==null && currentJob.getState()!=0) {
            throw new IllegalStateException("Error");
        }
            currentJob.setState(Job.PRINTING);
            return true;
    }

    public boolean cancel(){
        if (this.currentJob==null && currentJob.getState()!=0) {
            throw new IllegalStateException("Error");
        }
            currentJob = null;
            return true;
    }
    public boolean print(Job aJob){
        if (isAvailable()){
            currentJob=aJob;
            aJob.setPrinter(this);
            aJob.setState(Job.PRINTING);
            return true;
        }
        System.err.println("Error");
        return false;
    }

    public boolean printingCompleted(){
        if (isPrinting()){
            currentJob.setPrinter(null);
            currentJob.setState(Job.COMPLETED);
            currentJob=null;
            return true;
        }
        System.err.println("Error");
        return false;
    }

    public void setSpooler(Spooler spool){
        spool.join(this);
    }

    public boolean isAvailable(){
        return on && currentJob==null;
    }

    public boolean isPrinting(){
        return on && currentJob!=null;
    }
}
\$\endgroup\$
  • 1
    \$\begingroup\$ Welcome to CR! Possibly related to the code, how is currentJob used outside of these four methods? And do you have concerns with multi-threading, or is everything run on a single thread in your application? Entirely optional, but I think it might be helpful for reviewers if you can also show how these methods are being called... for example, are there other checks done before calling instance.print(aJob)? \$\endgroup\$ – h.j.k. May 8 '15 at 9:32
  • \$\begingroup\$ currentJob is just a variable that contains an object Job witch means the thing to print \$\endgroup\$ – Ryan Ben May 8 '15 at 9:38
  • 2
    \$\begingroup\$ I just put the entire code hope it could helps \$\endgroup\$ – Ryan Ben May 8 '15 at 9:41
12
\$\begingroup\$

I learnt everywhere that using if statement is a bad practice when it's possible to avoid them.

Just out of curiosity, how would you do this without an if-statement? I wonder where you heard this. In code, the moment you need to check for a condition (of a variable or the result of a method), you have to use an if-statement.

Spacing:

Just for readability, leave spaces between assignments and checks in conditions:

currentJob=aJob;
//or
return on && currentJob!=null;

becomes:

currentJob = aJob;
//and
return on && currentJob != null;

Naming:

A name like aJob is bad practice. Give meaningful names to parameters/variables. Change it for example to printJob. Although the class is Job, the method probably expects a "print-job", making printJob a meaningful name.

Writing output:

Try to apply separation of concerns. In this case this means that you shouldn't write output to a screen in a method that merely returns a boolean. You could refactor it like this:

public boolean print(Job printJob) {
    if (isAvailable()) {
        currentJob = printJob;
        printJob.setPrinter(this);
        printJob.setState(Job.PRINTING);
        return true;
    }
    return false;
}

//Usage elsewhere in the code:
Job newPrintJob = new Job(); //dummy Job instance for demo
if (print(newPrintJob)) {
    //continue code
}
else {
    System.err.println("Error printing...");
}
\$\endgroup\$
  • \$\begingroup\$ Some Smalltalkers would vehemently disagree that aJob is a bad name, and changing it to printJob really doesn't add info in either case. \$\endgroup\$ – cHao May 8 '15 at 15:45
  • \$\begingroup\$ I appreciate your comment but this is Java and as it is close to C#, I'd recommend following that style instead of Smalltalk. And, regardless of any language, good variable names make 'better' code. Just my 2 cents of course... \$\endgroup\$ – Abbas May 8 '15 at 16:41
  • 1
    \$\begingroup\$ Good class names are even more important. :) If the Job class represents a print job (which, judging by the API, it clearly does), then it seems pretty clear that the class should be called PrintJob. At that point, the variable could be named printJob, aJob, or even job, and you'd still know exactly what it is. \$\endgroup\$ – cHao May 9 '15 at 1:06
  • \$\begingroup\$ With that I completely agree. Better would be a subclass from the Job class and indeed name it PrintJob, making the parameter name rather irrelevant. But in this case, with the Job class, a clear parameter name makes more sense. :) \$\endgroup\$ – Abbas May 9 '15 at 8:43
8
\$\begingroup\$

General observations

Job.WAINTING looks like a typo.

You don't need to mangle parameter names like _currentJob. You can write this.currentJob = currentJob; just fine.

It is a convention that predicates (functions that return a boolean) should be named with a prefix like is… or has… or can…. So, getOn() should be renamed isOn(). The boolean return values for start() and stop() aren't useful; those should return void instead.

isAvailable() is using poor terminology. In operations research, "availability" means the time during which a piece of equipment is not broken. I suggest isIdle() instead for what you mean.

getOnStart() is a function that doesn't make sense. Furthermore, it's a private function that isn't called, so it's dead code.

The name of the setSpooler() method suggests that a printer can only belong to one spooler at a time. However, your code does not enforce that. You should either add that enforcement or eliminate the method.

if statements and error handling

Your if statements aren't excessive. However, the indentation is inconsistent, and the error handling that you perform using the if statements seems wrong.

The suspend(), resume() and cancel() methods should either…

  • Have a void return type, and throw an exception, or
  • Return boolean, but not throw exceptions. In my opinion, returning a boolean is more appropriate: if there's no current job to suspend, that should be a no-op rather than a crisis to handle.

In contrast, print(Job) should throw an exception if the printer is busy, because the printer has declined to accept the job, and throwing an exception would make it hard for the caller to neglect to check the return code and accidentally discard a job.

If you do throw an exception, use a more informative message than "Error".

This code, being part of a model, shouldn't write anything to System.err. Let the views and controllers take care of that kind of output.

\$\endgroup\$
5
\$\begingroup\$

There are a few things that stand out, but we don't know much about the Job class, so aside from remarking that you should never use integers to represent states (as in currentJob.getState() != 0), there isn't much to say.

However you are mostly interested in if reduction, possibly through the use of a design pattern.

This is an example that could benefit from using the State pattern. This is one of the original GOF patterns that (in my opinion) you can use quite safely without risking to overengineering your code. Still, it's unlikely that you could decrease the number of if statements, however reducing the number of ifs is not the point and shouldn't be your goal.

To exemplify, let us make the state a property of the printer instead than one of the job.

In a nutshell, to represent the state you shouldn't use an enumeration or an integer, but instead you should use a full-fledged object, that implements a State interface:

public class State {
    public State tick() { return this; }
    public State suspend() { return this; }
    public State resume() { return this; }
    public State cancel() { return this; }
    public State print(Job aJob) { return this; }
}

Instead of if statement, Java polymorphism is used to correctly execute a command.

The method tick is not part of your original specification but I added it to the printer to give as a way to advance a job; after 100 ticks a document is considered printed.

The printer itself just delegates all command handling to the inner state object, using an idiom in which the result of a call to state is re-assigned as the new state.

public class Printer {
    private state = new Idle();

    public State tick() {
        state = state.tick();
    }

    public State suspend() {
        state = state.suspend();
    }

    public State resume() {
        state = state.resume();
    }

    public State cancel() {
        state = state.cancel();
    }

    public State print(Job aJob) {
        state = state.print(aJob);
    }
}

Here is a possible implementation. Note that there are no if statements, except for the one that checks for job completion.

public class Idle extends State {
    public State print(Job aJob) {
        return new Printing(aJob);
    }
}

public class Paused extends State {
    private Job currentJob;
    private int percentage;

    public Paused(Job aJob, int perc) {
        currentJob = aJob;
        percentage = perc;
    }

    public State resume() {
        return new Printing(currentJob, percentage);
    }
}

public class Printing extends State {
    private Job currentJob;
    private int percentage;

    public Printing(Job aJob) {
        this(aJob, 0);
    }

    public Printing(Job aJob, int perc) {
        currentJob = aJob;
        percentage = perc;
    }

    public State tick() {
        ++percentage;

        if (percentage == 100) {
            return new Idle();
        }
        else {
            return this;
        }
    }

    public State suspend() {
        return new Paused(currentJob, percentage);
    }

    public State cancel() {
        return new Idle();
    }
}

Note that if you don't override a State method in a subclass, when the subclass object receives the corresponding command the default behaviours is to ignore it. This may not be desirable, and in some instances you may want to throw exceptions in the base class.

public class State {
    public State tick() { return this; }
    public State suspend() { throw new RuntimeException(""); }
    public State resume() { throw new RuntimeException(":-("); }
    public State cancel() { throw new RuntimeException(":-("); }
    public State print(Job aJob) { throw new RuntimeException(":-("); }
}
\$\endgroup\$
  • \$\begingroup\$ This is Java we're talking about. Enums are full-fledged objects. \$\endgroup\$ – cHao May 8 '15 at 15:51
2
\$\begingroup\$

Bug

if (this.currentJob==null && currentJob.getState()!=0) { ... }

Uh-oh, code, meet NullPointerException on calling currentJob.getState()...

What is a Job?

This is going beyond the scope of this review, but I do hope you consider this:

What is the interaction between the lifecycle of a Job instance and your Printer instance here?

The answer to this question is important in understanding where actions are done. Following which, you can potentially refactor both your Job and Printer classes at once. For example, consider what you are doing currently when you tell your Printer instance to cancel():

public boolean cancel(){
    // ommitting buggy if statement for brevity
    currentJob = null;
    return true;
}

However, is the current Job instance (currentJob) supposed to continue running? Or is your use case actually to call currentJob.cancel(), where its implementation is something like:

public class Job {

    ...

    public boolean cancel() {
        if (printer == null) {
            return false;
        }
        result = printer.cancel();
        printer = null;
        // do something else about its internal state too?
        return result;
    }

    ...

}

Also, it is potentially troubling that both setPrinterJob() and print() can re-assign currentJob, and what will this mean for 'un-assigned' Job instances? Since they still have a reference to your Printer instance, well at least if they were referenced through print(), will they be able to 'talk' with the Printer?

The third example I want to use to conclude this section is when a printing is completed:

public boolean printingCompleted(){
    if (isPrinting()){
        currentJob.setPrinter(null);
        currentJob.setState(Job.COMPLETED);
        currentJob=null;
        return true;
    }
    System.err.println("Error");
    return false;
}

Ok, so a Job instance is also completed when both its printer reference and state are updated? What happens when only one of them is updated, does that render the instance's internal state invalid? Also, what happens to it afterwards? Can I re-print or submit to another Printer? Does it just get GC-ed?

Summary

The code inside Printer is only one side of the story, you need to look at Job as well as understand your use cases in order for you to do any further refactoring properly.

What is a Job's state?

On a related note, and as briefly touched upon by @damix911's answer, a Job's state can probably be better represented with enums. Even in your code's current state (pun unintended), you are using integer values (0) and what appears to be class-level constants Job.PRINTING inconsistently, which makes it even harder to understand what does either of them really mean. This is of course assuming both the getters and setters are not performing any kind of mappings for accepting from a set of states and indicating them.

\$\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.