patternjavaModerate
Modelling a printer
Viewed 0 times
printermodellingstackoverflow
Problem
I learnt everywhere that using
```
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("Er
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("Er
Solution
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:
becomes:
Naming:
A name like
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:
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...");
}Code Snippets
currentJob=aJob;
//or
return on && currentJob!=null;currentJob = aJob;
//and
return on && currentJob != null;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...");
}Context
StackExchange Code Review Q#90131, answer score: 12
Revisions (0)
No revisions yet.