Elevator program

  • I've been working on this elevator program for quite some time now and finally finished it and made it work and I'm pretty proud of myself, but I'd like to see ways how to optimize my code so I can learn for the future. By optimizing, I mean making the code look better, maybe by using fewer lines of code.



    import java.awt.geom.*;

    public class elevator
    {

    static int floor = 0, choice1, person = 0;

    public static void main(String args[])
    {

    floor = ((int) (Math.random() * 10 + 1));

    System.out.println("The elevator is now on floor " +floor);
    System.out.print("Which floor are you at now (0-10) where 0 = basement: ");
    choice1 = Keyboard.readInt();

    if(floor == choice1)
    {
    System.out.println("Enter the elevator");
    }

    else if(floor > choice1)
    {
    ElevatorDown();
    }

    else if(floor < choice1)
    {
    ElevatorUp();
    }

    System.out.println("To which floor would you want to go (0-10) where 0 = basement");
    choice1 = Keyboard.readInt();

    if(floor > choice1)
    {
    ElevatorDown();
    }

    else if(floor < choice1)
    {
    ElevatorUp();
    }

    }

    public static void ElevatorUp()
    {
    System.out.println("The elevator is on it's way up...");

    for (person = choice1; choice1>=floor; floor++)

    System.out.println(floor);

    System.out.println("The elevator has arrived");
    }

    public static void ElevatorDown()
    {
    System.out.println("The elevator is on it's way down...");
    for (person = choice1; choice1<=floor; floor--)

    System.out.println(floor);

    System.out.println("The elevator has arrived");
    }
    }

    Maybe you could post the complete code of your class.

    And please post `Keyboard`, too.

    I've posted my whole code now and I uploaded the Keyboard.class to speedyshare, not sure if there's a better place to upload it. http://speedy.sh/pGMx8/Keyboard.class

    Well, because I don't have the source file to the Keyboard.class but I uploaded what I could read from the class file onto pastebin. http://pastebin.com/MMyQvAA3 I hope that's enough, thanks

    This still doesn't work: the imports from `java.awt.geom.*` are unused while `Keyboard` is not imported.

    I'm afraid I don't follow you, it's working great on my end. All I've to do is adding the Keyboard.class file to my bin folder in Eclipse.

    Not an optimisation as such; but almost all Java programmers begin class names with capital letters and method names with lower case letters. This is actually a standard that was recommended by Sun (when they were still Sun), and it will make your code more readable to just about everyone who ever sees it.

    And what exactly is the `person` variable for? It looks to me like you assign it, but never actually use it. Could you remove this variable?

    You never actually check the value of the floor, yet you say you limit it from 0 to 10. You should have some "checkInputIsValid" method

    Making the code look better (more readable) and using less lines of code are usually opposite aims in Java.

  • import java.awt.geom.*;

    public class elevator


    Class names are typically capitalized



    {

    static int floor = 0, choice1, person = 0;


    Prefer local variables to statics like this. Person doesn't appear to be used.



        public static void main(String args[])
    {

    floor = ((int) (Math.random() * 10 + 1));


    You've got an extra pair of parens that you don't need.



            System.out.println("The elevator is now on floor " +floor);
    System.out.print("Which floor are you at now (0-10) where 0 = basement: ");
    choice1 = Keyboard.readInt();

    if(floor == choice1)
    {
    System.out.println("Enter the elevator");
    }

    else if(floor > choice1)
    {
    ElevatorDown();
    }

    else if(floor < choice1)
    {
    ElevatorUp();
    }


    Rather then keeping your choice in a static variable. Pass it as a parameter to your ElevatorUp()/ElevatorDown() Movement functions.



            System.out.println("To which floor would you want to go (0-10) where 0 = basement");
    choice1 = Keyboard.readInt();

    if(floor > choice1)
    {
    ElevatorDown();
    }

    else if(floor < choice1)
    {
    ElevatorUp();
    }


    This is repeated from before. Write an ElevatorMove() function that decides whether to call ElevatorUp or ElevatorDown.



        }

    public static void ElevatorUp()
    {
    System.out.println("The elevator is on it's way up...");

    for (person = choice1; choice1>=floor; floor++)


    Declare for loop variables in the loop not some other random place. And person? What does person have to do with it? In fact person=choice1 does nothing. I'd make this a while loop.



                System.out.println(floor);

    System.out.println("The elevator has arrived");
    }

    public static void ElevatorDown()
    {
    System.out.println("The elevator is on it's way down...");
    for (person = choice1; choice1<=floor; floor--)

    System.out.println(floor);

    System.out.println("The elevator has arrived");
    }


    These two functions are similiar. They can be combined.
    }



    My reworking of your code:



    import java.awt.geom.*;

    public class elevator
    {

    static int floor;

    public static void main(String args[])
    {

    floor = (int) (Math.random() * 10 + 1);

    System.out.println("The elevator is now on floor " +floor);
    System.out.print("Which floor are you at now (0-10) where 0 = basement: ");
    int current_floor = Keyboard.readInt();

    if(floor == current_floor)
    {
    System.out.println("Enter the elevator");
    }
    else
    {
    MoveElevator(current_floor);
    }


    System.out.println("To which floor would you want to go (0-10) where 0 = basement");
    int target_floor = Keyboard.readInt();

    MoveElevator(target_floor);
    }

    public static void MoveElevator(int target_floor)
    {
    int direction;
    if( target_floor > floor )
    {
    System.out.println("The elevator is on it's way up...");
    direction = 1;
    }else{
    System.out.println("The elevator is on it's way down...");
    direction = -1;
    }

    while(target_floor != floor)
    {
    floor += direction;
    System.out.println(floor);
    }

    System.out.println("The elevator has arrived");
    }
    }

  • You should separate the main method from the Elevator class, so that your Elevator class truly represents an elevator! You could have, let's say, a Program class, that'll call your Elevator class.



    Second, your methods and fields shouldn't be static. Usually, when your class has a state (such as the current floor), your object shouldn't be static. This way, you could have 2 (or more) Elevator instances, and they won't collide together.



    The elevator shouldn't start at a random floor, imagine a real elevator. At it's first use, there's a big possibility that it starts at the bottom floor (at least, I guess, I'm no elevator expert!). Let's start this elevator at floor 0 (the basement).



    So right now, I assume your elevator is used by only one person, because a real elevator might not come and get you if it isn't going in the direction you want to go.



    Your class name should be capitalized and your method named should be camelCased so :



    Elevator instead of elevator



    elevatorUp instead of elevatorUp



    elevatorDown instead of elevatorDown



    Also, the Java convention specifies that your brackets should be "egyptian style". Which means :



    //Good
    if{
    }

    //Less good
    if
    {
    }


    There goes the resume of my explanations :



    public class Elevator {
    private int floor = 0, choice1, person = 0;

    public void callFrom(int floor){
    System.out.println("Elevator is coming");
    goTo(floor);
    }

    public void goTo(int floor){
    if(floor == choice1) {
    System.out.println("Enter the elevator");
    }
    else if(floor > choice1) {
    elevatorDown();
    }

    else if(floor < choice1){
    elevatorUp();
    }
    }

    private void elevatorUp() {
    System.out.println("The elevator is on it's way up...");

    for (person = choice1; choice1>=floor; floor++)

    System.out.println(floor);

    System.out.println("The elevator has arrived");
    }

    private void elevatorDown() {
    System.out.println("The elevator is on it's way down...");
    for (person = choice1; choice1<=floor; floor--)

    System.out.println(floor);

    System.out.println("The elevator has arrived");
    }
    }


    I wasn't sure about the goTo and callFrom, but I thought it was important to split them since if you ever want to make your elevator even more complex (dealing with multiple calls etc..) you'll need to have them splitted.


    This is the best answer "for the future" (quoting the OP). Other answers demonstrate better Java this one included, but this answer is the only one addressing better software.

  • In object oriented programming, you typically ask yourself: which objects do I operate with, what can I do with them, and what are their properties? In your case:




    • There is an elevator, and there could possibly be many elevators, which are all similar.

    • An elevator always has a current floor where it currently is.

    • That current floor can change when the elevator is in action.

    • One typical action is to move up.

    • Another typical action is to move down.

    • When a person wants to use the elevator, the elevator moves to the floor where the person is. In general, the elevator moves to a certain floor.



    And here is the code, based on the description above:



    public class Elevator {

    private int currentFloor;

    public void run() {
    currentFloor = 1 + ((int) (Math.random() * 10));

    System.out.println("The elevator is now on floor " + currentFloor);
    System.out.print("Which floor are you at now (0-10) where 0 = basement: ");
    int personFloor = Keyboard.readInt();
    moveTo(personFloor);
    System.out.println("Enter the elevator");

    System.out.println("To which floor do you want to go (0-10) where 0 = basement");
    int destinationFloor = Keyboard.readInt();
    moveTo(destinationFloor);
    System.out.println("Leave the elevator");
    }

    private void moveTo(int destinationFloor) {
    if (destinationFloor == currentFloor) {
    /* nothing to do */
    } else if (destinationFloor > currentFloor) {
    moveUpTo(destinationFloor);
    } else {
    moveDownTo(destinationFloor);
    }
    }

    private void moveUpTo(int destinationFloor) {
    System.out.println("The elevator is on its way up ...");
    while (currentFloor < destinationFloor) {
    currentFloor++;
    System.out.println(currentFloor);
    }
    System.out.println("The elevator has arrived");
    }

    private void moveDownTo(int destinationFloor) {
    System.out.println("The elevator is on its way down ...");
    while (currentFloor > destinationFloor) {
    currentFloor--;
    System.out.println(currentFloor);
    }
    System.out.println("The elevator has arrived");
    }

    public static void main(String[] args) {
    new Elevator().run();
    }

    }


    By the way, the elevator is on its way, not on it's way.


  • This is my code review. I don't read others much of others answers, it's likely they have point almost the same issues.




    1. Name conventions: you have to pay attention of Java widely used name conventions: Java code conventions



    In this case, your class name should start with uppercases and your methods with lowercases.




    1. is this import used?




      • import java.awt.geom.*;


    2. your implementation is to much procedural. You do all in the main method. You should put the logic in Elevator class and call it via main method, after instantiating the Elevator object. You should
      avoid static members, too.


    3. your implementation have to much ifs, that let your code bigger and polluted. see anti-if.


    4. elevatorUp and elevatorDown methods are almost the same. you should avoid code duplication. See DRY.




    I made an re-implementation of your elevator simulator. I change the methods elevatorUp and elevatorDown to
    one unique method, to avoid code repetition and to put off some ifs.



    public class Elevator {
    private static final String DOWN = "down";
    private static final String UP = "up";
    private final Random rand = new Random();
    private int floor = rand.nextInt(10) + 1;

    public void simulateElevator() {
    System.out.println("The elevator is now on floor " + floor);
    System.out.print("Which floor are you at now (0-10) where 0 = basement: ");
    int destinationFloor = readDestinationFloor();
    int movement = getMovementDirection(destinationFloor);
    if (movement == 0) {
    System.out.println("Enter the elevator");
    } else {
    moveElevator(destinationFloor, movement);
    }
    }

    private int readDestinationFloor() {
    return Keyboard.readInt();
    }

    /* determine if movement needed is to up or to down */
    private int getMovementDirection(int destinationFloor) {
    int dif = destinationFloor - floor;
    if (dif == 0) {
    return 0;
    }
    return dif / Math.abs(dif);
    }


    private String getMovementName(int movement) {
    return movement < 0 ? DOWN : UP;
    }

    private void moveElevator(int destination, int moveDirection) {
    String movementName = getMovementName(moveDirection);
    System.out.println("The elevator is on it's way ".concat(movementName).concat("..."));
    while (floor != destination) {
    floor += moveDirection;
    System.out.println(floor);
    }
    System.out.println("The elevator has arrived");
    }

    public static void main(String[] args) {
    new Elevator().simulateElevator();
    }
    }

License under CC-BY-SA with attribution


Content dated before 7/24/2021 11:53 AM

Tags used