1
\$\begingroup\$

This is my solution for a problem in my workbook. It says to create a program that accepts input from a user to compare the areas of two rectangles. Is it good or bad practice to call functions in a constructor as soon as it is initialized?

Such as this:

public Rectangle() {
System.out.println("New rectangle dimensions \n" + "---------------
---------------------------");
userInputDimensions();
calculateArea();
}

Or is it better to just call the functions from the main class and keep the constructor empty? Everything works, I'm just trying to get some general advice on how to improve this code.

Main Class

public class Main {


public static void main(String[] args) {

Rectangle rectangle1 = new Rectangle();
Rectangle rectangle2 = new Rectangle();
Rectangle.CompareAreasOfRectangles test = rectangle1.new 
CompareAreasOfRectangles(rectangle1, rectangle2);




}
}

Rectangle Class

import java.util.*;

public class Rectangle {
private double rectangleWidth;
private double rectangleLength;
private double areaOfRectangle;

public Rectangle() {
    System.out.println("New rectangle dimensions \n" + "---------------
---------------------------");
    userInputDimensions();
    calculateArea();
}

//Get Dimensions of Rectangle

private void userInputDimensions() {
    this.rectangleWidth = this.getPositiveInput("Please enter the width 
of the rectangle");
    this.rectangleLength = this.getPositiveInput("Please enter the 
length of the rectangle");
}

private double getPositiveInput(String prompt) {
    Scanner reader = new Scanner(System.in);
    double result;
    do {
        System.out.println(prompt);
        result = reader.nextDouble();
        if (result <= 0) {
            System.out.println("Enter a number greater than zero.");
        }
    } while (result <= 0);
    return result;
}

private void calculateArea() {
    this.areaOfRectangle = rectangleWidth * rectangleLength;
}

private double getAreaOfRectangle() {
    return areaOfRectangle;
}

// Compare areas of Rectangles

public class CompareAreasOfRectangles {
private Rectangle rectangle1;
private Rectangle rectangle2;

public CompareAreasOfRectangles(Rectangle rectangle1, Rectangle 
rectangle2) {
    this.rectangle1 = rectangle1;
    this.rectangle2 = rectangle2;

    if (rectangle1.getAreaOfRectangle() > 
rectangle2.getAreaOfRectangle()) {
        System.out.println("The area of Rectangle 1 is greater than the 
area of Rectangle 2");
    } else if (rectangle1.getAreaOfRectangle() < 
rectangle2.getAreaOfRectangle()) {
        System.out.println("The area of Rectangle 2 is greater than the 
area of Rectangle 1");
    } else {
        System.out.println("The areas are equal");
    }
}
}
}
\$\endgroup\$
  • \$\begingroup\$ @Rao Improving the formatting of the code should be left for the answers, as it is a comment on readability of the code and people should learn to write readable code. \$\endgroup\$ – Graipher Oct 25 '17 at 8:18
1
\$\begingroup\$

A few things I'd consider:

Printing out the dimensions in the constructor is not wise. Consider if you decide to use this class in a different project, but don't need to print out the dimensions. One way around this is to override the toString method and supply a formatted string. This way printing it out is entirely optional and the destination it's printed to is also entirely optional.

Implementing a compareTo method in the class negates needing a separate class for that. This will also clean up other portions of your code. To that end making the area a Double allows us to leverage the compareTo method in that wrapper class.

It would make sense to keep the methods for getting the data outside of the class. The class should just have to deal with the data itself.

The class could look something like this:

public class MyRectangle
{

    private double length = 0;
    private double width = 0;
    private Double area = 0.0;

    public MyRectangle()
    {

    }

    public MyRectangle(double length, double width)
    {
        this.length = length;
        this.width = width;
        area = length * width;
    }

    public double getArea()
    {
        return area;
    }

    public int compareTo(MyRectangle other)
    {
        return this.area.compareTo(other.area);
    }

    @Override
    public String toString()
    {
        return String.format("The dimensions are: Length - %1$f, Width - %2$f, Area - %3$f", length, width, area);
    }
}

The helper methods and main could look like this:

public static MyRectangle makeRectangle(int designation)
{

    double width = getPositiveInput("Please enter the width of rectangle "+ designation);
    double length = getPositiveInput("Please enter the length of rectangle " + designation);
    return new MyRectangle(length, width);
}

public static double getPositiveInput(String prompt)
{
    Scanner reader = new Scanner(System.in);
    double result;
    do
    {
        System.out.println(prompt);
        result = reader.nextDouble();
        if (result <= 0)
        {
            System.out.println("Enter a number greater than zero.");
        }
    } while (result <= 0);
    return result;
}
public static String compareRectangles(MyRectangle a, MyRectangle b)
{
    int result = a.compareTo(b);
    if(result == 0)
    {
       return "The areas are equal";
    }
    if(result > 0)
    {
        return "The area of Rectangle 1 is greater than the area of Rectangle 2";
    }
    if(result < 0)
    {
        return "The area of Rectangle 2 is greater than the area of Rectangle 1";
    }
    return "";
}
public static void main(String[] args)
{
    MyRectangle rectangle1 = makeRectangle(1);
    System.out.println(rectangle1);        
    MyRectangle rectangle2 = makeRectangle(2);
    System.out.println(rectangle2);
    System.out.println(compareRectangles(rectangle1,rectangle2));
}
\$\endgroup\$
2
\$\begingroup\$

It is always a best practice to use constructor only for initializing default values. Whatever logic or the function which you want to use is good to call directly from main method or separate method.

\$\endgroup\$
1
\$\begingroup\$

Had nothing (important) to do, so wrote it out the way I would have done it.

public class Rectangle {

  private double area;

  public Rectangle(double length, double width) {
    this.area = length * width;
  }

  public double getArea() {
    return area;
  }

  public static class RectangleComparer {

    private Rectangle rectangle1;
    private Rectangle rectangle2;

    public RectangleComparer(Rectangle rectangle1, Rectangle rectangle2) {
      this.rectangle1 = rectangle1;
      this.rectangle2 = rectangle2;
    }

    public void printBiggestRectangle() {
      if (rectangle1.getArea() > rectangle2.getArea()) {
        System.out.println("The area of Rectangle 1 is greater than the area of Rectangle 2");
      } else if (rectangle1.getArea() < rectangle2.getArea()) {
        System.out.println("The area of Rectangle 2 is greater than the area of Rectangle 1");
      } else {
        System.out.println("The areas are equal");
      }
    }

  }

}


public class App {

  private static Scanner scanner = new Scanner(System.in);

  public static void main(String[] args) {
    double length1 = getUserValue("length");
    double width1 = getUserValue("width");

    double length2 = getUserValue("length");
    double width2 = getUserValue("width");

    Rectangle rectangle1 = new Rectangle(length1, width1);
    Rectangle rectangle2 = new Rectangle(length2, width2);
    RectangleComparer comparer = new RectangleComparer(rectangle1, rectangle2);

    comparer.printBiggestRectangle();
  }

  private static double getUserValue(String widthOrLength) {
    System.out.println("Please enter the " + widthOrLength + " of the rectangle");
    double result;
    do {
      result = scanner.nextDouble();
      if (result <= 0) {
        System.out.println("Enter a number greater than zero.");
      }
    }
    while (result <= 0);
    return result;
  }

}

Why did I remove the length and width?

Because they are not needed, a field area is enough. I made the name area instead of rectangleArea because itis logical that we're talking about the area of the Object it is in.

Why did I remove the Scanner out of Rectangle?

I was slightly amused when I saw that, because I wasn't expecting it there. So I brought it a layer higher so that it is obvious what is done.

Why not put logic in your constructor?

You should not do this, because you should see a class as an object irl. And normally when you create a object you give it an order to do something before it does it. eg You wouldn't create a car that turns on and drives 150km/u the moment that it is build. So don't do it in your code.

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