5
\$\begingroup\$

I am looking to make the following code more efficient since it it rather lengthy. However I need to include in the code constructor classes that include accessor methods for the container number, height, radius 1, radius 2, and a method that computes the liquid that the container can hold. So I am looking to leave these concepts in there but make it more efficient if it is possible.

public class PoolCleanRequest {
final static double PI = 3.14;
private String num;
private double height;
private String height1;
private String height2; 
private String radius1;
private String radius2;
private double r1;
private double r2;
private String rad2;
private String rad3;
public PoolCleanRequest(String num1){
    num = num1;
}
public void setHeight(double h){
    height=h;
    height1=Double.toString(h);
    height2=Double.toString(h);
}
public void setR1(double rr){
    r1 = rr;
    radius1=Double.toString(rr);
    radius2=Double.toString(rr);
}
public void setR2(double rrr){
    r2 = rrr;
    rad2=Double.toString(rrr);
    rad3=Double.toString(rrr);
}
public String getNum(){
    return num;
}
public String getHeight1(){
   return height1;
}
public String getHeight2(){
   return height2;
}
public String getRadius1(){
    return radius1;
}
public String getRadius2(){
    return radius2;
}
public String getRad1(){
    return rad2;
}
public String getRad3(){
    return rad3;
}
public double getPoolCost(){
    return ((PI*height/3)*(r1*r1+r2*r2+r1*r2))/1000; 
}
public static void main( String[] args ) {
    PoolCleanRequest pool1 = new PoolCleanRequest("1.0");
    PoolCleanRequest pool2 = new PoolCleanRequest("2.0");
    PoolCleanRequest height1 = new PoolCleanRequest("20.0");
    PoolCleanRequest height2= new PoolCleanRequest("15.0");
    PoolCleanRequest radius1 = new PoolCleanRequest("3.0");
    PoolCleanRequest radius2 = new PoolCleanRequest("5.0");
    PoolCleanRequest rad1 = new PoolCleanRequest("2.0");
    PoolCleanRequest rad3 = new PoolCleanRequest("4.0");
    pool1.setHeight(20.0);
    height1.setHeight(20.0);
    height2.setHeight(15.0);
    radius1.setR1(3.0);
    radius2.setR1(5.0);
    rad3.setR2(4.0);
    rad1.setR2(2.0);
    pool1.setR1(3.0);
    pool1.setR2(5.0);
    pool2.setHeight(15.0);
    pool2.setR1(2.0);
    pool2.setR2(4.0);
    System.out.println("Container Num: " + pool1.getNum());
    System.out.println("H: " + height1.getHeight1());
    System.out.println("R1: " + radius1.getRadius1());
    System.out.println("R2: " + radius2.getRadius2());
    System.out.println("Litres: " + pool1.getPoolCost());
    System.out.println();
    System.out.println("Container Num: " + pool2.getNum());
    System.out.println("H: "+ height2.getHeight2());
    System.out.println("R1: " + rad1.getRad1());
    System.out.println("R2: " + rad3.getRad3());
    System.out.println("Litres: " +  pool2.getPoolCost());
  }
}

This code gives the correct following output:

Container Num: 1.0
H: 20.0
R1: 3.0
R2: 5.0
Litres: 1.0257333333333334

Container Num: 2.0
H: 15.0
R1: 2.0
R2: 4.0
Litres: 0.43960000000000005
\$\endgroup\$
1
\$\begingroup\$

Obviously something like

Container Num: 1.0
H: 20.0
R1: 3.0
R2: 5.0
Litres: 1.0257333333333334

belongs to each other, so why don't you encapsulate it in an object like i.e.

public class Container {

    private final static double PI = 3.14;

    private final double number;
    private final double height;
    private final double radius1;
    private final double radius2;

    public Container(double num, double height, double r1, double r2) {
        this.number = num;
        this.height = height;
        this.radius1 = r1;
        this.radius2 = r2;
    }

    public double calculateVolume() {
        return ((PI*this.height/3) * (this.radius1*this.radius1 
                                      + this.radius2*this.radius2 
                                      + this.radius1*this.radius2)) / 1000;
    }

    // getters

    public double getNumber() {
        return this.number;
    }

    public double getHeight() {
        return this.height;
    }

    public double getRadius1() {
        return this.radius1;
    }

    public double getRadius2() {
        return this.radius2;
    }
}

You can further reduce the getter overhead by utilizing Project Lombok, where you can specify a @Getter annotation at the top of the Container class which will therefore autogenerate the getters for you automatically

Your main method can then be updated to something like:

public static void main(String ... args) {
    Container container1 = new Container(1.0, 20, 3.0, 5.0);
    Container container2 = new Container(2.0, 15.0, 2.0, 4.0);

    printContainerInfo(container1);
    printContainerInfo(container2);
}

private static void printContainerInfo(Container container) {
    System.out.println("Container "
                       + "Num: " + container.getNumber() + "\n"
                       + "H: " + container.getHeight() + "\n"
                       + "R1: " + container.getRadius1() + "\n"
                       + "R2: " + container.getRadius2() + "\n"
                       + "Litres: " + container.calculateVolume() + "\n");
}

As your requirement was:

need to include in the code constructor classes that include accessor methods for the container number, height, radius 1, radius 2, and a method that computes the liquid that the contain can hold

Container does provide all of the required accessor methods and also a computation method to return the liquid size. While the accessor methods are not used directly within the main method (code constructor?), they will be indirectly invoked via the printContainerInfo(Container) method though. I hope that this still fulfills the requirements. I'd refactor the printContainerInfo(Container) method usually to the Container class' toString() method but as you obviously need to invoke the getters from the main method I did not do it that way.

To avoid getting confused which argument has to be specified at which position you may also use a builder pattern to create your container.

\$\endgroup\$
0
5
\$\begingroup\$

The first problem that jumps out at me is that many of your variables are "stringly typed". This class is a calculator, right? Why are you storing your numbers as strings rather than as doubles?

Why does the constructor take a num parameter, and what does it mean?

There is no need to redefine π. Just use Math.PI.

Why is there a getPoolCost() method? Your code would be more flexible if you offered a method to calculate the volume, and a separate method to calculate the cost based on the volume.

I don't think that there is much of a reason to define a bunch of setters and getters. Just define one function that accepts all of the dimensions as parameters, and be done with it.

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