4
\$\begingroup\$

To work on my Java skills, I decided to create a structure for a pool that I work at. Lifeguards get shifted when their time at their position is up, and managers periodically check the pH level of the pool. I would like feedback on the following items please.

  • Structure: Is how the current project is built acceptable? Is it coded in the most efficient way possible?
  • Methods
  • advancePosition Is using a switch statement the most efficient way to change the position of the guards? Is using an enum a good practice for this type of system?
  • checkpHLevel Is the logic hard to understand? Are there any ways I can improve upon this methods to get rid of bad habits? To the scientists here: I haven't had that much experience with working with pH levels, so is it correct to increase the pH level with Alkaline's and decrease the level with Acids?
  • admitPatron Can this method be implemented in any way that can make it more efficient?

Pool.java

package Vets;

public class Pool {

    private final String name;
    private final int capacity;
    private int patronCount;
    private double pHLevel;
    private boolean isClosed;

    private ArrayList<Patron> patrons;
    private ArrayList<Staff> staff;

    public Pool(String name, int capacity) {
        this.name = name;
        this.capacity = capacity;
        this.patronCount = 0;
        this.pHLevel = 7.45;
        this.isClosed = false;

        this.patrons = new ArrayList<Patron>();
        this.staff = new ArrayList<Staff>();
    }

    //Class methods
    public boolean admitPatron(Patron patron) {
        if((this.patronCount + 1) <= this.capacity) {
            this.patrons.add(patron);
            this.patronCount++;
            return true;
        }
        return false;
    }

    public void rotateLifeguards() {
        for(Lifeguard lifeguard : this.staff) {
            lifeguard.advancePosition();
        }
    }

    public void addAcids(double amount) {
        this.pHLevel -= amount;
    }

    public void addAlkalis(double amount) {
        this.phLevel += amount;
    }

    //Getters
    public String getName() { return this.name; }
    public int getCapacity() { return this.capacity; }
    public int getPatronCount() { return this.patronCount; }
    public double getpHLevel() { return this.pHLevel; }
    public boolean isClosed() { return this.isClosed; }
    public ArrayList<Patron> getPatrons() { return this.patrons; }
    public ArrayList<Staff> getStaff() { return this.staff; }

    //Custom getters
    public double getOptimumLevel() { return 7.45; }

}

Staff.java

package Vets;

public class Staff {

    private final String name;

    public Staff(String name) {
        this.name = name;
    }

    //Getters
    public String getName() { return this.name; }

}

Lifeguard.java

package Vets;

public class Lifeguard extends Staff {

    private Position position;

    public Lifeguard(String name, Position position) {
        super(name);
        this.position = position;
    }

    //Class methods
    public void advancePosition() {
        switch(this.position) {
            case ONE: this.position = Position.TWO; break;
            case TWO: this.position = Position.BREAK_ONE; break;
            case BREAK_ONE: this.position = Position.THREE; break;
            case THREE: this.position = Position.FOUR; break;
            case FOUR: this.position = Position.BREAK_TWO; break;
            case BREAK_TWO; this.position = Position.ONE; break;
        }
    }

    //Getters and setters
    public Position getPosition() { return this.position; }

}

Manager.java

package Vets;

public class Manager extends Staff {

    public Manager(String name) {
        super(name);
    }

    //Class methods
    public boolean checkpHLevel(Pool pool) {
        if(pool.getpHLevel() != pool.getOptimumLevel()) {
            int amountToAdd = pool.getOptimumLevel() - pool.getpHLevel();
            if(amountToAdd > 0.0) {
                pool.addAcids(amountToAdd);
            } else {
                pool.addAlkalis(amountToAdd);
            }
        }
    }

}

Patron.java

package Vets;

public class Patron {

    private final String name;

    public Patron(String name) {
        this.name = name;
    }

}

Position.java

package Vets;

public enum Position {

    ONE, TWO, BREAK_ONE, THREE, FOUR, BREAK_TWO

}

Here is a picture of the structure of the program

Program Structure

\$\endgroup\$
4
\$\begingroup\$

Is how the current project is built acceptable? Is it coded in the most efficient way possible?

That depends on your definition of effiency. I guess you mean in it OO-Terms, i.e. maintainability, encapsulation, decoupling, etc.. In that terms, yes, your program has a good encapsulation and a good overall structure. I do not know the exact purpose of Staff.java, as far as I see it is designed for inheritance and not for direct instantiation so you could consider making it abstract. Furthermore Patron.java is not able to do any actions after initialization as it does not have any public methods besides a constructor, so the class just does nothing. As it has a field name a getter would be suitable. If it really surves no purpose other than to count the number of patrons you could just replace it by an int in Pool.java.

advancePosition Is using a switch statement the most efficient way to change the position of the guards? Is using an enum a good practice for this type of system?

Yes, using an enum is definitely the best way of modeling a finite set of states, as it provides type safety and the range of values is clear. The switch statement could be replaced by a Map in order to keep things clean, e.g.:

// initialize this map in a constructor or better use it as constant
Map<Position, Position> nextPosition = new Hashmap<>();
nextPosition.put(Position.ONE, Position.TWO);
// etc
// ...
// then in advancePosition()
// getting them is then as simple as:
position = nextPosition.get(position);

checkpHLevel Is the logic hard to understand? Are there any ways I can improve upon this methods to get rid of bad habits? To the scientists here: I haven't had that much experience with working with pH levels, so is it correct to increase the pH level with Alkaline's and decrease the level with Acids?

The logic itself is understandable, but your condition can be expressed in a more readable way. As you want to add acids or alcalines I suggest changing int amountToAdd = pool.getOptimumLevel() - pool.getpHLevel(); to something more readable like boolean isAcid = pool.getpHLevel() < pool.getOptimumLevel();. This way it is immediately clear what you are trying to achieve, add acids when it's to alcaline and add alcalines when it's too acid. Your ph-level question: an acid ph-value ranges from 0-7 and alcaline from 7-14 so you add acids to an acid environment (not good :p)

admitPatron Can this method be implemented in any way that can make it more efficient?

Your if-condition could be simplified to if(patronCount < capacity).

Some overall hints for your code. You got some magic numbers i.e., numbers that are not directly understandable e.g., this.pHLevel = 7.45;. Why is it 7.45? In case of that it is common to use constants as you can give them names like private static final int optimumPhLevel = 7.45. If you then use it everyone knows that your ph-level is the optimum level and 7.45 doesn't really matter anymore. Furthermore you can write this.patrons = new ArrayList<>(); instead of this.patrons = new ArrayList<Patron>();. This is called the diamond operator.

\$\endgroup\$
3
\$\begingroup\$

@vajk already mentioned most important points, but I will add comments on some things that I see.

patronCount is redundant, you can simply use patrons.size().

Both arrays that are Pool members can be made final.

phLevel is a double meaning you have to work with floating point. You have to consider which values are used and perhaps better to use integer, since no actual meter will have such low/high range of possible values.

checkPhLevel and getOptimumPhLevel are compared in a wrong way, since checkPhLevel is a double you need to compare range of values instead of exact values, this comes out from previous comment about phLevel being a double.

checkPhLevel method does not say what it actually does, meaning it has a side effect. Therefore either split it in two methods where one checks ph level and returns boolean and second method adds required chemical to the pool or rename it to something like balansePoolPhLevel.

checkPhLevel can be simplified to:

double actualLevel = pool.getpHLevel();
double desiredLevel = pool.getOptimumLevel()

if(Math.abs(actualLevel - desiredLevel) < SOME_DELTA_YOU_THING_IS_PRECISE_ENOUGH) {
  return;
}

int amountToAdd = actualLevel - desiredLevel;

amountToAdd > 0 
  ? pool.addAcids(amountToAdd) 
  : pool.addAlkalis(-amountToAdd); // I think you need to invert it otherwise you are adding it to phLevel in pool class

A good thing to consider would be creating another constructor where you can pass arrays used in Pool class therefore it would be much easier to test, but that's only when you write tests.

admitPatron method can be refactored:

if(this.patronCount >= this.capacity) {
   return false;
}

this.patrons.add(patron);
this.patronCount++;

return true;

But instead of doing two things in admitPatron. First being to check if item can be added and second add the item in a list. You could change class API to boolean isPatronLimitReached() and then void addPatron(patron). isPatronLimitReached() simply return boolean if max value is reached and addPatron(patron) add a patron and if list is full throw an error.

I'm bit confused about rotateLifeguards method since you are casting Staff to Lifeguard. Staff also is parent class of Manager and actually can be Staff object itself therefore you could be casting to the wrong class therefore it is not very safe. I don't write Java, but you should not cast Staff instead have a separate list of Lifeguards and Managers. Also you are not using managers anywhere in shown code.

Patron class can be just a string, no reason to keep it as class.

advancePosition method could be split in two parts one which get next position and second part where you assign new position to current position:

public void advancePosition() {
  this.position = this.getNextPosition();
}

Position getNextPosition() {
  switch(this.position) {
    case Position.ONE:
      return Position.Two;
    ...
  }
}
\$\endgroup\$
0
\$\begingroup\$

This looks like a fairly common exercise (in futility?) A common scenario that can't be represented here is be a lifeguard who is promoted to a manager but still does a few shifts saving lives.

The position should be a property of staff. Checking pH should be a task that has a requirement for managerial position from whoever is assigned to it. It has to be performed regularly so there should be a work schedule. Might as well throw the lifeguard shifts in the schedule too with clocking in and out too. It gets complicated very quickly.

What I am trying to say is that attempts to represent organization hierarchies or taxonomies as a class structure always fail quickly when they encounter real life. Usually this is done with vehicles or animals but this works as an example too. The correct way is to represent roles as properties. :)

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