33
\$\begingroup\$

To learn more about OOP, @nhgrif challenged me to implement a Shape abstract class (more details in code remarks below. Here is how I did it. Any and all advice appreciated!

Shape.java

/* nhgrif says:
 * Phrancis ready for inheritance/polymorphism?
 * Given the following abstract class:
 *
 *  public abstract class Shape {
 *      public abstract double area();
 *      public abstract double perimeter();
 *  }
 *
 * Implement a Circle, Triangle, and Rectangle class which extend the class Shape.
 * Ex: public class Circle extends Shape ... etc
 */

public abstract class Shape {
    public abstract double area();
    public abstract double perimeter();
}

Rectangle.java

public class Rectangle extends Shape {
    private final double width, length; //sides

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

    @Override
    public double area() {
        // A = w * l
        return width * length;
    }

    @Override
    public double perimeter() {
        // P = 2(w + l)
        return 2 * (width + length);
    }

}

Circle.java

public class Circle extends Shape {
    private final double radius;
    final double pi = Math.PI;

    public Circle() {
        this(1);
    }   
    public Circle(double radius) {
        this.radius = radius;
    }

    @Override
    public double area() {
        // A = π r^2
        return pi * Math.pow(radius, 2);
    }

    public double perimeter() {
        // P = 2πr
        return 2 * pi * radius;
    }
}

Triangle.java

public class Triangle extends Shape {
    private final double a, b, c; // sides

    public Triangle() {
        this(1,1,1);
    }
    public Triangle(double a, double b, double c) {
        this.a = a;
        this.b = b;
        this.c = c;
    }

    @Override
    public double area() {
        // Heron's formula:
        // A = SquareRoot(s * (s - a) * (s - b) * (s - c)) 
        // where s = (a + b + c) / 2, or 1/2 of the perimeter of the triangle 
        double s = (a + b + c) / 2;
        return Math.sqrt(s * (s - a) * (s - b) * (s - c));
    }

    @Override
    public double perimeter() {
        // P = a + b + c
        return a + b + c;
    }
}

And this is what I used to test it all, which it all works as intended:

TestShape.java

public class TestShape {
    public static void main(String[] args) {

        // Rectangle test
        double width = 5, length = 7;
        Shape rectangle = new Rectangle(width, length);
        System.out.println("Rectangle width: " + width + " and length: " + length
                + "\nResulting area: " + rectangle.area()
                + "\nResulting perimeter: " + rectangle.perimeter() + "\n");

        // Circle test
        double radius = 5;
        Shape circle = new Circle(radius);
        System.out.println("Circle radius: " + radius
            + "\nResulting Area: " + circle.area()
            + "\nResulting Perimeter: " + circle.perimeter() + "\n");

        // Triangle test
        double a = 5, b = 3, c = 4;
        Shape triangle = new Triangle(a,b,c);
        System.out.println("Triangle sides lengths: " + a + ", " + b + ", " + c
                + "\nResulting Area: " + triangle.area()
                + "\nResulting Perimeter: " + triangle.perimeter() + "\n");
    }
}
\$\endgroup\$
  • 6
    \$\begingroup\$ Now do the tricky ones: ellipse and square. <g> \$\endgroup\$ – Pete Becker Mar 10 '15 at 18:43
  • 1
    \$\begingroup\$ Well square could just extend Rectangle. Ellipse would be interesting for sure though. \$\endgroup\$ – Legato Mar 10 '15 at 21:49
  • \$\begingroup\$ @Legato - or rectangle could extend square. \$\endgroup\$ – Pete Becker Mar 10 '15 at 23:24
  • 4
    \$\begingroup\$ Yes, and rectangle extending square would imply always. \$\endgroup\$ – Legato Mar 10 '15 at 23:39
  • 1
    \$\begingroup\$ @PeteBecker Square extending Rectangle is only allowed if they are immutable. The other way around is not allowed. \$\endgroup\$ – RobAu Nov 27 '15 at 8:44
21
\$\begingroup\$

I am generally impressed with the consistency of the implementations, neatness, etc.

I have a comment about the basic premise. The Shape should not be an abstract class, but an interface. It has no concrete implementations for any methods, and making it an abstract class makes it hard to inherit from other places too. Consider this instead:

public interface Shape {
    public double area();
    public double perimeter();
}

In addition to that concern, the following are also things you should consider:

  • The pi field on your Circle class should be private. There's no need to re-expose an already public constant in another way.

  • I would avoid the 'unit' default constructors. They don't help with anything. When would someone want to call new Triangle() and not want to have the dimensions?

  • Your classes are missing a toString() method. These are useful for many reasons, especially debugging.

  • You don't have a plan for out-of-bound dimensions. What will you do with negative, NaN, or infinite input?

\$\endgroup\$
  • 2
    \$\begingroup\$ I would even suggest to not have a pi field and use Math.PI directly. Or do a import static java.lang.Math.PI; and use PI directly \$\endgroup\$ – Ivo Beckers Mar 11 '15 at 11:25
  • \$\begingroup\$ interface methods are public by default. (one of my pet peeves are redundant modifiers - Java is already verbose enough). \$\endgroup\$ – Boris the Spider Mar 11 '15 at 12:19
  • 2
    \$\begingroup\$ @BoristheSpider - yeah, they are public by default, I understand your concern, and you're right that it should be pointed out. As it happens, I learned Java while at a job where the code style guidelines required it, and it's a habit I have kept, and I now prefer making it explicit. I'm not claiming I'm right, but I am claiming that it would not be 'my' answer if I edited it out ;-) \$\endgroup\$ – rolfl Mar 11 '15 at 12:24
12
\$\begingroup\$

A nitpick on names: The dimensions of a rectangle should be width and height not width and length. The constructor of Triangle should take descriptive parameter names, such as side1Length. You have more excuse to use a and b for private fields (especially in this short, pure math code), but single-character names are generally frowned upon.

Add javadoc comments, at least to the methods in your abstract class/interface. The comment to your Shape class is almost a javadoc, but it is missing one *.

For extra credit, replace your main() method with 3 unit tests (1 test class with 3 methods) that can be easily run from the IDE.

\$\endgroup\$
  • \$\begingroup\$ I would suggest a unit test class for each implementation - this is the accepted standard approach. Other than that, all very good points. \$\endgroup\$ – Boris the Spider Mar 11 '15 at 12:21
  • \$\begingroup\$ @BoristheSpider You're right, but personally I would not stick firmly to this rule in such a trivial case, preferring to reduce the number of test classes and make test code reuse a simple matter of a private method testShape(Shape shape, double expectedArea, double expectedPerimiter) in class ShapeTest rather than build an inheritance heirarchy for it. \$\endgroup\$ – Aleksandr Dubinsky Mar 11 '15 at 15:31
7
\$\begingroup\$

I'm glad you did this, gives me a chance to further enforce/correct some misinformation.

final double pi = Math.PI; this is a circumstance where the value pi should be static and would appropriately be written as UPPERCASE PI.

  • Static since the value of pi is not dependent on any instance of Circle
  • Final, since of course you wouldn't want anything changing it
  • Uppercase since it is both these things, it is considered constant thus the naming convention.

Though, there's no need for you to take up namespace or memory when you can directly use Math.PI. For instance, your double area() method's implementation could be:

return Math.PI * Math.pow(radius, 2);

For the same reason instead of having placeholder variables that aren't used elsewhere:

double a = 5, b = 3, c = 4;
Shape triangle = new Triangle(a,b,c);

Simply use them

Shape triangle = new Triangle(5, 3, 4);

a, b, and c aren't really fitting names to begin with, if you do elect to use them try using more descriptive names such as base, side1, and side2 in this case.

I like that you use the abstract class to declare in your tests, doing that will afford you some flexibility in the future, just make sure you understand that when you rely on specific methods that only exist in a subclass you'd have to declare with that subclass.

\$\endgroup\$
  • \$\begingroup\$ The placeholder variables increase readability. \$\endgroup\$ – Aleksandr Dubinsky Mar 10 '15 at 23:21
  • 1
    \$\begingroup\$ I disagree that a, b and c are not good names. While using several consecutive single-letter names for fields would usually be a code smell, in this case they are appropriate. They are the standard names for the three sides of a triangle and are ubiquitous in triangle geometry. Calling them base, side1 and side2 falsely suggests that the three have distinct meanings. \$\endgroup\$ – jwg Mar 11 '15 at 11:41
6
\$\begingroup\$

One minor nitpick, really:

// where s = (a + b + c) / 2, or 1/2 of the perimeter of the triangle 
double s = (a + b + c) / 2;

In the comment you refer to the perimeter of the triangle, but in your code you don't use the actual perimeter. This is very small code duplication. Your code would be a bit more self-documenting if you would do:

double s = perimeter() / 2;

I personally have nothing against the a, b, c names in this case. The sides are often named that in Mathematics, I don't think it's horribly wrong to name them that here as well.

\$\endgroup\$
  • 1
    \$\begingroup\$ This is a minor point but a very good one. \$\endgroup\$ – jwg Mar 11 '15 at 11:42
4
\$\begingroup\$

Personally I think the default no-arg constructor is very unnecessary if these are immutable classes. Does a Rectangle by default having width 1 and height 1 make much sense?

And also, if the classes are immutable, you might as well calculate the areas and perimeters right away, so they can be just returned in the appropriate methods.

I also think the naming of methods could be better. Instead of area() I would use getArea().

The modified Rectangle would look like the following (note that the Shape class would also need to be edited):

public class Rectangle extends Shape {
  private final double width, height, area, perimeter;

  public Rectangle(double width, double height) {
    this.width = width;
    this.height= height;
    this.area = width * height;
    this.perimeter = 2 * (width + height);
  }

  @Override
  public double getArea() {
    return this.area;
  }

  @Override
  public double getPerimeter() {
    return this.perimeter;
  }

}
\$\endgroup\$
  • \$\begingroup\$ getX is usually a sign that the method returns a predefined value. Which is the case here but makes it inappropriate for his implementation. I don't really see an issue with the name, but if he had to change it I'd probably choose computeX \$\endgroup\$ – Legato Mar 11 '15 at 12:43
  • \$\begingroup\$ Right.. Or maybe calculateArea()? \$\endgroup\$ – juunas Mar 11 '15 at 13:20
  • \$\begingroup\$ Works just as well. \$\endgroup\$ – Legato Mar 11 '15 at 13:21
  • 2
    \$\begingroup\$ From Effective Java: Methods that return a non-boolean function or attribute of the object on which they’re invoked are usually named with a noun, a noun phrase, or a verb phrase beginning with the verb get, for example, size, hashCode, or getTime. There is a vocal contingent that claims that only the third form (beginning with get) is acceptable, but there is little basis for this claim. The first two forms usually lead to more readable code. A value class with few non-getter methods may use a noun like area(). calculateArea suggest the function has side-effects or is noticeably slow. \$\endgroup\$ – Aleksandr Dubinsky Mar 11 '15 at 15:47

protected by Vogel612 Nov 25 '18 at 20:01

Thank you for your interest in this question. Because it has attracted low-quality or spam answers that had to be removed, posting an answer now requires 10 reputation on this site (the association bonus does not count).

Would you like to answer one of these unanswered questions instead?

Not the answer you're looking for? Browse other questions tagged or ask your own question.