Simulating a printer

Question

I'm trying to recreate a printer in Java. I'm fairly new to programming so I'm using huge if/else blocks inside a single function to dictate the logic of the program. I'm noticing this is creating a mass of code inside the same function.

I was wondering if there was a more eloquent/efficient way of doing this Printer class.

Logic for the printer isn't too important, but just to show anyway: one is a double sided printer one isn't, and logic is in charge of checking toner levels and making sure pages printed are in line with printer being double sided or not.

package com.company;

public class Printer {
private String name;
private double tonerLevel = 100;
private int ammountOfPaper;
private int numberOfPagesPrinted;
private boolean isDoubleSided;

public Printer(String name, double tonerLevel, int ammountOfPaper, boolean isDoubleSided) {
    this.name = name;
    if(tonerLevel >= 0 && tonerLevel <= 100) {
        this.tonerLevel = tonerLevel;
    }
    this.ammountOfPaper = ammountOfPaper;
    this.isDoubleSided = isDoubleSided;
}

private boolean isOutOfToner(double numberToPrint) {
    if((tonerLevel - (numberToPrint / 2) < 0)) {
        return true;
    }
    else {
        return false;
    }
}



private boolean isOutOfPaper(double numberToPrint) {
    if(((ammountOfPaper - numberToPrint) < 0)) {
        return true;
    }
    else {
        return false;
    }
}

private boolean twoSideNoPaperEven(double numberToPrint) {
    if((ammountOfPaper - ((int) numberToPrint / 2)) < 0 ) {
        return true;
    }
    else {
        return false;
    }
}

private boolean twoSideNoPaperOdd(double numberToPrint) {
    if(((ammountOfPaper - ((int) numberToPrint / 2)) - 1) < 0) {
        return true;
    }
    else {
        return false;
    }
}

public void printPages(double numberToPrint) {

    if(isDoubleSided == false) {
        if(tonerLevel == 0) {
            System.out.println("Out of toner");
        }
        if(ammountOfPaper == 0) {
            System.out.println("Out of Paper");
        }
        if(isOutOfToner(numberToPrint) && (tonerLevel != 0)) {
            double difference = tonerLevel * 2;
            numberToPrint = difference;
            ammountOfPaper -= numberToPrint;
            System.out.println("Will run out of toner after this print, able to print " + (int) numberToPrint +
                    " pages");
            tonerLevel = 0;
        }
        if(isOutOfPaper(numberToPrint) && (ammountOfPaper != 0)) {
            double different = ammountOfPaper - numberToPrint;
            numberToPrint = numberToPrint + different;
            System.out.println("Will run out of paper after this print, printing " + (int) numberToPrint + " pages");
            ammountOfPaper = 0;
        }
        else if(!isOutOfToner(numberToPrint) && (!isOutOfPaper(numberToPrint))) {
            ammountOfPaper -= numberToPrint;
            tonerLevel = tonerLevel - (numberToPrint / 2);
            showPages(numberToPrint);
        }

    }
    else if(isDoubleSided = true) {
            if (numberToPrint % 2 == 0) {
                if(tonerLevel == 0) {
                    System.out.println("Out of Toner");
                }
                if(ammountOfPaper == 0) {
                    System.out.println("Out of Paper");
                }
                if(twoSideNoPaperEven(numberToPrint) && (ammountOfPaper != 0)) {
                    ammountOfPaper -= numberToPrint / 2;
                    System.out.println("There is no Paper");
                }
                else if(!twoSideNoPaperEven(numberToPrint)) {
                    tonerLevel = tonerLevel - (numberToPrint / 2);
                    ammountOfPaper -= numberToPrint / 2;
                    showPages(numberToPrint);
                }
            } else {
                if(tonerLevel == 0) {
                    System.out.println("Out of Toner");
                }
                if(ammountOfPaper == 0) {
                    System.out.println("Out of Paper");
                }
                if(twoSideNoPaperOdd(numberToPrint) && (ammountOfPaper != 0)) {
                    System.out.println("There is no paper");
                    ammountOfPaper = (ammountOfPaper - ((int) numberToPrint / 2)) - 1;
                    ammountOfPaper = 0;
                }
                else if(!twoSideNoPaperOdd(numberToPrint)) {
                    tonerLevel = tonerLevel - (numberToPrint / 2);
                    ammountOfPaper = (ammountOfPaper - ((int) numberToPrint / 2)) - 1;
                    showPages(numberToPrint);
                }
            }
        }

    }

public void showPages(double numberToPrint) {
    System.out.println("Printing " + (int) numberToPrint + " Pages, paper remaining is: " + this.ammountOfPaper
            + " Toner level is: " + this.tonerLevel);
}

public void refillToner() {
    tonerLevel = 100;
}
public void refillPaper(int paper) {
    if(paper > 50) {
        System.out.println("Cannot put in more paper");
    }
    else {
        this.ammountOfPaper += paper;
    }
}

public int getAmmountOfPaper() {
    return ammountOfPaper;
}

public double getTonerLevel() {
    return tonerLevel;
}

public void setTonerLevel(double tonerLevel) {
    this.tonerLevel = tonerLevel;
}

public void setAmmountOfPaper(int ammountOfPaper) {
    this.ammountOfPaper = ammountOfPaper;
}

Answer 1

I'd just like to add something that I haven't seen mentioned by others (or maybe I skipped it), but take for example this piece of code:

public void printPages(double numberToPrint) {
    // ...

    if(tonerLevel == 0) {
        System.out.println("Out of toner");
    }
    if(ammountOfPaper == 0) {
        System.out.println("Out of Paper");
    }

    if(isOutOfToner(numberToPrint) && (tonerLevel != 0)) {
        double difference = tonerLevel * 2;
        numberToPrint = difference;
        ammountOfPaper -= numberToPrint;
        System.out.println("Will run out of toner after this print, able to print " + (int) numberToPrint +
                " pages");
        tonerLevel = 0;
    }
    if(isOutOfPaper(numberToPrint) && (ammountOfPaper != 0)) {
        double different = ammountOfPaper - numberToPrint;
        numberToPrint = numberToPrint + different;
        System.out.println("Will run out of paper after this print, printing " + (int) numberToPrint + " pages");
        ammountOfPaper = 0;
    }
    else if(!isOutOfToner(numberToPrint) && (!isOutOfPaper(numberToPrint))) {
        ammountOfPaper -= numberToPrint;
        tonerLevel = tonerLevel - (numberToPrint / 2);
        showPages(numberToPrint);
    }

    // ...
}

You're checking your parameters twice, when you could return after a faulty state of the printer has been acknowledged (i.e. out of paper; we surely can't print without paper). So you could do something like this:

private boolean canPrint(int numberToPrint) {
    if (tonerLevel == 0) {
        System.out.println("Out of toner");
        return false;
    } else if (ammountOfPaper == 0) {
        System.out.println("Out of Paper");
        return false;
    } else if (!hasEnoughToner(numberToPrint)) {
        // Maybe print a message to let the user know why you're not printing
        return false;
    } else if (!hasEnoughPaper(numberToPrint)) {
        // Maybe print a message to let the user know why you're not printing
        return false;
    }

    return true;
}

private void print(int numberToPrint) {
    if (canPrint(numberToPrint)) {
        ammountOfPaper -= numberToPrint;
        tonerLevel = tonerLevel - (numberToPrint / 2);
        showPages(numberToPrint);
    }
}

I have avoided the parts of code where you mention when you will run out of paper or toner after the print, because I am in a hurry, but you get the point.

Basically, once you know the state of something, you shouldn't have to come back to that. If the state is fatal (i.e. no paper), you can return from your method, as there is nothing that you can really print. This way, you can check your conditions only once and be done with them.

Hope this helps. :)

Answer 2

There are several things I would do to increase readability of your code.

The first would be to create methods that query some field. It will allow the code to be more human readable and self documenting.

So for example you would have methods such as:

private boolean isOutOfPaper() {
    return amountOfPaper == 0;
}

private boolean isOutOfToner() {
    return tonerLevel == 0;
}

These would replace if(amountOfPaper == 0) with if(isOutOfPaper()) and if(tonerLevel == 0) with if(isOutOfToner())

The second would be to move the functionality of how the printer handles each bad event (ie. no paper, no toner) into their own methods.

so for example you would have methods:

private void handleNoPaper() {
    System.out.println("Out of Paper");
}

private void handleNoToner() {
    System.out.println("Out of Toner");
}

The code in your main method will now be much easier to read and understand when it looks like this:

if(isOutofToner()) {
    handleNoToner();
}

if(isOutOfPaper()) {
    handleNoPaper();
}

(As I'm writing this I noticed you do have isOutOfPaper(int numOfPages) method. However that method name is a little misleading because it is checking if there is enough paper to print the current job rather than if it is out of paper now. I would rename that method to hasEnoughPaperToPrint(int numOfPages) and of course the same applies to your current isOutOfToner(int numOfPages) method)

The 3rd thing you could do is to utilize inheritance and polymorphism by creating subclasses of Printer and making the Printer class abstract.

So for example you would have:

public abstract class Printer {
    private String name;
    private double tonerLevel = 100;
    private int amountOfPaper;
    private int numberOfPagesPrinted;

    public abstract void printPages();

    //rest of class ommitted
}

public class DoubleSidedPrinter extends Printer {
    public void printPages() {
        //print as double sided. No check necessary.
    }
}

public class OneSidedPrinter extends Printer {
    public void printPages() {
        //print as one sided. No check necessary.
    }
}

This eliminates some of the checking needed to see if the printer is double sided.

Answer 3

One little thing you can do is related to methods like

private boolean isOutOfToner(double numberToPrint) {
    if((tonerLevel - (numberToPrint / 2) < 0)) {
        return true;
    }
    else {
        return false;
    }
}

that you can easily rewrite them as

private boolean isOutOfToner(double numberToPrint) {
    return (tonerLevel - (numberToPrint / 2) < 0);
}

with exactly the same behaviour.

Answer 4

package com.company;

public class Printer {

The internals of a class definition are typically indented.

private String name;

Do you actually use the name at all?

private double tonerLevel = 100;
private int ammountOfPaper;

Why is there a default for tonerLevel but not amountOfPaper? Also, it might be better to call it pagesOfPaper so that its clear what the units are.

If you are keeping track of a percentage, I actually recommend that you keep the value between 0 and 1, with 0 being 0% and 1 being 100%. Math tends to work out more straightforwardly that way.

private int numberOfPagesPrinted;
private boolean isDoubleSided;


public Printer(String name, double tonerLevel, int ammountOfPaper, boolean isDoubleSided) {
    this.name = name;
    if(tonerLevel >= 0 && tonerLevel <= 100) {
        this.tonerLevel = tonerLevel;
    }

Generally, its a bad idea to automatically correct input. If you receive invalid inputs, its a bug in the code using this class and you should throw an exception to get somebodies' attention.

    this.ammountOfPaper = ammountOfPaper;
    this.isDoubleSided = isDoubleSided;
}

private boolean isOutOfToner(double numberToPrint) {

As others have noted, the name of the function is a bit confusing, it doesn't check if the printer is out of toner, but if it can print a certain number of pages.

    if((tonerLevel - (numberToPrint / 2) < 0)) {
        return true;
    }
    else {

This is an unconventially else style. Typically its either

    } else {

or

    } 
    else
    {

I recommend against mixing the style.

        return false;
    }
}

private boolean twoSideNoPaperEven(double numberToPrint) {

Why does this function take a double?

    if((ammountOfPaper - ((int) numberToPrint / 2)) < 0 ) {
        return true;
    }
    else {
        return false;
    }
}

public void printPages(double numberToPrint) {

Why double? does your printer allow fractional printing?

    if(isDoubleSided == false) {

Firstly, don't check == false. Instead say if(!isDoubleSided). It is already a boolean, there is no need to compare it to false.

Secondly, the logic of this function is more complicated then it needs to be. Everything is repeated three times. They key to avoid the duplication is to seperate your calculations from your output. Like this:

// calculate the number of physical pages we have toner to print
int tonerPages = tonerLevel * isDoubleSided ? 1 :  2;
// calculate the number of physical pages we need.
int physicalPagesRequired = isDoubleSided ? (int) Math.ceil((double) numberToPrint / 2) : numberToPrint;

// calculate the number of physical pages that will actually be printed.
int actualPagesPrinted = Math.min(tonerPages, physicalPagesRequired, amountOfPaper);

// Update printer state.
tonerLevel -= actualPagesPrinted / 2;
amountOfPaper -= actualPagesPrinted;

Now, when it comes to printing, check the calculated values to determine what needs to be printed. If tonerLevel == 0, you've run out of toner. If amountOfPaper == 0, you've run out of paper. If actualPagesPrinted != physicalPagesRequired, you didn't manage to print everything.

        if(tonerLevel == 0) {
            System.out.println("Out of toner");
        }
        if(ammountOfPaper == 0) {
            System.out.println("Out of Paper");
        }

If you aren't required to have a particular order to the output, I'd group the toner prints together and the paper prints together.

        if(isOutOfToner(numberToPrint) && (tonerLevel != 0)) {
            double difference = tonerLevel * 2;
            numberToPrint = difference;
            ammountOfPaper -= numberToPrint;
            System.out.println("Will run out of toner after this print, able to print " + (int) numberToPrint +
                    " pages");
            tonerLevel = 0;

This logic is subtly incorrect, what if you run out of pages before you run out of toner?

        }

        if(isOutOfPaper(numberToPrint) && (ammountOfPaper != 0)) {
            double different = ammountOfPaper - numberToPrint;
            numberToPrint = numberToPrint + different;
            System.out.println("Will run out of paper after this print, printing " + (int) numberToPrint + " pages");
            ammountOfPaper = 0;
        }
        else if(!isOutOfToner(numberToPrint) && (!isOutOfPaper(numberToPrint))) {
            ammountOfPaper -= numberToPrint;
            tonerLevel = tonerLevel - (numberToPrint / 2);
            showPages(numberToPrint);
        }

    }
    else if(isDoubleSided = true) {
            if (numberToPrint % 2 == 0) {
                if(tonerLevel == 0) {
                    System.out.println("Out of Toner");
                }
                if(ammountOfPaper == 0) {
                    System.out.println("Out of Paper");
                }
                if(twoSideNoPaperEven(numberToPrint) && (ammountOfPaper != 0)) {

You could have easily reduced this duplication, simply by having this function check the even/oddness of number to print instead of requiring this code to call different functions.

                    ammountOfPaper -= numberToPrint / 2;
                    System.out.println("There is no Paper");
                }
                else if(!twoSideNoPaperEven(numberToPrint)) {
                    tonerLevel = tonerLevel - (numberToPrint / 2);
                    ammountOfPaper -= numberToPrint / 2;
                    showPages(numberToPrint);
                }
            } else {
                if(tonerLevel == 0) {
                    System.out.println("Out of Toner");
                }
                if(ammountOfPaper == 0) {
                    System.out.println("Out of Paper");
                }
                if(twoSideNoPaperOdd(numberToPrint) && (ammountOfPaper != 0)) {
                    System.out.println("There is no paper");
                    ammountOfPaper = (ammountOfPaper - ((int) numberToPrint / 2)) - 1;
                    ammountOfPaper = 0;
                }
                else if(!twoSideNoPaperOdd(numberToPrint)) {
                    tonerLevel = tonerLevel - (numberToPrint / 2);
                    ammountOfPaper = (ammountOfPaper - ((int) numberToPrint / 2)) - 1;
                    showPages(numberToPrint);
                }
            }
        }

    }

public void showPages(double numberToPrint) {
    System.out.println("Printing " + (int) numberToPrint + " Pages, paper remaining is: " + this.ammountOfPaper
            + " Toner level is: " + this.tonerLevel);
}

Why is this a seperate method, whereas none of the other printing was?

public void refillToner() {
    tonerLevel = 100;
}
public void refillPaper(int paper) {
    if(paper > 50) {
        System.out.println("Cannot put in more paper");
    }
    else {
        this.ammountOfPaper += paper;
    }
}

Why are you enforcing this rule here, when you didnt' enforce it in the constructor.

public int getAmmountOfPaper() {
    return ammountOfPaper;
}

Its amount not ammount

public double getTonerLevel() {
    return tonerLevel;
}

public void setTonerLevel(double tonerLevel) {
    this.tonerLevel = tonerLevel;
}

public void setAmmountOfPaper(int ammountOfPaper) {
    this.ammountOfPaper = ammountOfPaper;
}

Since you have methods to reload the ink and toner, you probably shouldn't have these methods. Generally, its makes sense to either have smart method that encode logic, or dumb ones that let you set them. Both is usually not useful.