HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavaModerate

Simple FIFO and LIFO ferry simulator

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
simplefifoferryandsimulatorlifo

Problem

I am trying to make a ferry simulator with one ferry that works like a stack and another like a queue. I have a working class for stacks and queues with pop/push; I am trying to figure out how to simply this code for the ferry class.

```
import java.util.Scanner;

public class Ferry {

static Stack ferry1 = new Stack();
static Queue ferry2 = new Queue();
Stack ferryStack = new Stack();
static int car1, car2, car3, car4, car5, car6, car7, car8, car9, car10, car11, car12, car13, car14, car15, car16, car17, car18, car19, car20;
static int getTag(){

Scanner tag = new Scanner(System.in);
int carID;

System.out.print("Enter in a tag number.");
carID = tag.nextInt();
return carID;
}
public static Queue ferry2Loading(){
car1 = getTag();
car2 = getTag();
car3 = getTag();
car4 = getTag();
car5 = getTag();
car6 = getTag();
car7 = getTag();
car8 = getTag();
car9 = getTag();
car10 = getTag();
car11 = getTag();
car12 = getTag();
car13 = getTag();
car14 = getTag();
car15 = getTag();
car16 = getTag();
car17 = getTag();
car18 = getTag();
car19 = getTag();
car20 = getTag();

ferry2.queuePush("car1");
ferry2.queuePush("car2");
ferry2.queuePush("car3");
ferry2.queuePush("car4");
ferry2.queuePush("car5");
ferry2.queuePush("car6");
ferry2.queuePush("car7");
ferry2.queuePush("car8");
ferry2.queuePush("car9");
ferry2.queuePush("car10");
ferry2.queuePush("car11");
ferry2.queuePush("car12");
ferry2.queuePush("car13");
ferry2.queuePush("car14");
ferry2.queuePush("car15");
ferry2.queuePush("car16");
ferry2.queuePush("car17");
ferry2.queuePush("car18");
ferry2.queuePush("car19");
ferry2.queuePush("car20");

Solution

A single ferry?!

Why is this Ferry class (mostly) static? There is such thing as multiple ferries. And, since this information that you are putting inside the Ferry class isn't going to be useful to anything but this one class, then you should turn this into a class that is instantiated and stores data in it's own instance.

As Quill noted in a comment:

ferry1Loading, ferry2Loading: doesn't that imply more than one ferry?

You should refactor your class so you are instantiating two separate ferries and loading them, rather than creating these as fields inside the class itself.

Scanners everywhere!

static int getTag(){

    Scanner tag = new Scanner(System.in);
    int carID;

    System.out.print("Enter in a tag number.");
    carID = tag.nextInt();
    return carID;
}


Each time this method is run, a new Scanner is instantiated. This is wasteful and unnecessary. You should create a single Scanner, stick it in a field, and only use that Scanner to get input (make static if you do not follow the above advice)

private static Scanner input;
...
public static void main(String[] args) {
    input = new Scanner(System.in);
}


If you follow the recommendation above about making this into a class that is instantiated, then you can, in the constructor, have an option for specifying what stream the Scanner should read from. That would allow for easier unit testing in the future.

DRY DRY DRY DRY...

Don't repeat yourself.

car1 = getTag();
    car2 = getTag();
    car3 = getTag();
    car4 = getTag();
    car5 = getTag();
    car6 = getTag();
    car7 = getTag();
    car8 = getTag();
    car9 = getTag();
    car10 = getTag();
    car11 = getTag();
    car12 = getTag();
    car13 = getTag();
    car14 = getTag();
    car15 = getTag();
    car16 = getTag();
    car17 = getTag();
    car18 = getTag();


All this code and the other code blocks like this are ugly looking. You have soo many statements just like this, with the only change being the variable/field acted upon.

You should, to fix this, change all these carX fields into a single array of cars. Then, when you are doing repeated operations like this, you can simply iterate over the array.

Here is an example:

private int[] cars; /* set to new int[20] in constructor */
....
for(int i = 0; i < cars.length; i++) {
    cars[i] = getTag();
}


Follow this same idea of using loops for the rest of your repeated code.

Code Snippets

static int getTag(){

    Scanner tag = new Scanner(System.in);
    int carID;

    System.out.print("Enter in a tag number.");
    carID = tag.nextInt();
    return carID;
}
private static Scanner input;
...
public static void main(String[] args) {
    input = new Scanner(System.in);
}
car1 = getTag();
    car2 = getTag();
    car3 = getTag();
    car4 = getTag();
    car5 = getTag();
    car6 = getTag();
    car7 = getTag();
    car8 = getTag();
    car9 = getTag();
    car10 = getTag();
    car11 = getTag();
    car12 = getTag();
    car13 = getTag();
    car14 = getTag();
    car15 = getTag();
    car16 = getTag();
    car17 = getTag();
    car18 = getTag();
private int[] cars; /* set to new int[20] in constructor */
....
for(int i = 0; i < cars.length; i++) {
    cars[i] = getTag();
}

Context

StackExchange Code Review Q#108334, answer score: 10

Revisions (0)

No revisions yet.