current community

your communities

more stack exchange communities

company blog
Stack Exchange Inbox Reputation and Badges
tour help
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. Join them; it only takes a minute:

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top
up vote 2 down vote favorite

It seems there are a lot of "empty containers" (I do not know the correct, technical term) with regards to multiple levels of abstract classes and functional interfaces with little or no code. Is this the case in real working situations?

Here's the class diagram:

Class Diagram

And here's the code:

package shapeHierarchy;

public interface GetArea 
{
double getArea();
}

package shapeHierarchy;

public interface GetVolume 
{
double getVolume();
}

package shapeHierarchy;

public abstract class Shape implements GetArea
{
}
package shapeHierarchy;

public abstract class TwoDimensionalShape extends Shape
{
}

package shapeHierarchy;

public abstract class ThreeDimensionalShape extends Shape implements GetVolume 
{
}

package shapeHierarchy;

public class Circle extends TwoDimensionalShape
{
private final double radius;

//constructor
public Circle(double radius)
{
    //first subconstructor statement must call superclass constructor
    //"chain up" here
    this.radius = radius;
}

//accesor
public double getRadius()
{
    return radius;
}

@Override
public String toString()
{
    return String.format("%-10s%-10s%n%-10s%-10.1f%n",
            "Shape:", "Circle", "Radius:", getRadius());
}

//fulfill compiler contract with interface
@Override
public double getArea() 
{
    return Math.PI * (getRadius() * getRadius());
}

}

package shapeHierarchy;

public class Triangle extends TwoDimensionalShape
{
private final double height;
private final double base;

//constructor
public Triangle(double height, double base)
{
    //first subconstructor statement must call superclass constructor
    //"chain up" here
    this.height = height;
    this.base = base;
}

//accesors
public double getHeight()
{
    return height;
}

public double getBase()
{
    return base;
}

@Override
public String toString()
{
    return String.format("%-10s%-10s%n%-10s%-10.1f%n%-10s%-10.1f%n",
            "Shape:", "Triangle", "Base:", getBase(), "Height:", getHeight());

}

//fulfill compiler contract with interface
@Override
public double getArea() 
{
    return getHeight() * (getBase() / 2);
}

}


package shapeHierarchy;

public class Square extends TwoDimensionalShape
{
private final double side;

//constructor
public Square(double side)
{
    //first subconstructor statement must call superclass constructor
    //"chain up" here
    this.side = side;
}

//accesor
public double getSide()
{
    return side;
}

@Override
public String toString()
{
    return String.format("%-10s%-10s%n%-10s%-10.1f%n",
            "Shape:", "Square", "Side:", getSide());
}

//fulfill compiler contract with interface
@Override
public double getArea() 
{
    return getSide() * getSide();
}

}

package shapeHierarchy;

public class Sphere extends ThreeDimensionalShape
{
private final double radius;

//constructor
public Sphere(double radius)
{
    //first subconstructor statement must call superclass constructor
    //"chain up" here
    this.radius = radius;
}

//accesors
public double getRadius()
{
    return radius;
}

@Override
public String toString()
{
    return String.format("%-10s%-10s%n%-10s%-10.1f%n",
            "Shape:", "Sphere", "Radius:", getRadius());
}

//fulfill compiler contract with interfaces
@Override
public double getArea() 
{
    return 4.0 * Math.PI * (getRadius() * getRadius());
}

@Override
public double getVolume()
{
    return 4.0 * Math.PI * ((getRadius() * getRadius() * getRadius()) / 3.0);
}
}

package shapeHierarchy;

public class Cube extends ThreeDimensionalShape 
{
private final double side;

//constructor
public Cube(double side)
{
    //first subconstructor statement must call superclass constructor
    //"chain up" here
    this.side = side;
}

//accesors
public double getSide()
{
    return side;
}

@Override
public String toString()
{
    return String.format("%-10s%-10s%n%-10s%-10.1f%n",
            "Shape:", "Cube", "Side:", getSide());
}

//fulfill compiler contract with interfaces
@Override
public double getArea() 
{
    return 6.0 * (getSide() * getSide());
}

@Override
public double getVolume()
{
    return getSide() * getSide() * getSide();
}
}

package shapeHierarchy;

public class Tetrahedron extends ThreeDimensionalShape 
{
private final double edge;

//constructor
public Tetrahedron(double edge)
{
    //first subconstructor statement must call superclass constructor
    //"chain up" here
    this.edge = edge;
}

//accesors
public double getEdge()
{
    return edge;
}

@Override
public String toString()
{
    return String.format("%-10s%-10s%n%-10s%-10.1f%n",
            "Shape:", "Tetrahedron", "Edge:", getEdge());
}

//fulfill compiler contract with interfaces
@Override
public double getArea() 
{
    return Math.sqrt(3.0) * (getEdge() * getEdge());
}

@Override
public double getVolume()
{
    return (getEdge()*getEdge()*getEdge()) / (6.0 * Math.sqrt(2.0));
}
}


package shapeHierarchy;

public class ShapeTest 
{

public static void main(String[] args) 
{
    //create an array of generic shape objects
    Shape[] shapeObject = new Shape[6];

    //populate array with specific, concrete shapes
    shapeObject[0] = new Circle(9.0);
    shapeObject[1] = new Triangle(5.0, 7.0);
    shapeObject[2] = new Square(4.0);
    shapeObject[3] = new Sphere(8.0);
    shapeObject[4] = new Cube(6.0);
    shapeObject[5] = new Tetrahedron(3.0);

    //Ouput Header
    System.out.println("Two and three dimensional objects processed polymorphically\n");

    //generically process each shape object
    for(Shape currentShape : shapeObject)
    {
        System.out.print(currentShape);
        if (currentShape instanceof ThreeDimensionalShape)
        {   
            ThreeDimensionalShape shape3d = (ThreeDimensionalShape) currentShape;
            System.out.printf("%-10s%-10.1f%n",
                    "Area:", shape3d.getArea());
            System.out.printf("%-10s%-10.1f%n",
                    "Volume:", shape3d.getVolume());
        }
        if (currentShape instanceof TwoDimensionalShape)
        {
            TwoDimensionalShape shape2d = (TwoDimensionalShape) currentShape;
            System.out.printf("%-10s%-10.1f%n",
                    "Area:", shape2d.getArea());
        }
        System.out.println();
    }
}
}
share|improve this question
up vote 4 down vote

Some of the emptiness is coming from the two interfaces that I think are unnecessary. Rather than having GetArea and GetVolume as interfaces, simply have Shape specify getArea() and have ThreeDimensionalShape specify getVolume():

public interface Shape {
  public double getArea();
}
public interface ThreeDimensionalShape extends Shape {
  public double getVolume();
}

If you wanted a TwoDimensionalShape interface, it would indeed be empty, at least right now. That's okay though. Maybe you're planning on adding more 2D-specific methods later and know you'll need that layer in the hierarchy. Maybe you just want to tag things as being 2D--that's called a marker interface and the built-in library uses a few, such as RandomAccess and Serializable.

share|improve this answer
up vote 3 down vote

Some more observations (excluding the nice remarks about the interfaces).

Naming: TwoDimensionalShape and ThreeDimensionalShape are excessively long names, why not just Shape2D and Shape3D respectively?

toString(): patterns such as %-10s%-10s%n%-10s%-10.1f%n%-10s%-10.1f%n look like if they were written in Brainfuck. More readable alternatives are:

  1. Put constant parts inside the pattern, leave formatters only for variables, e.g. String.format("Shape: %s%nBase: %.1f%nHeight: %.1f%n", getBase(), getHeight());. Args in String.format are usually used for variables.

  2. Use a StringBuilder to build.

The "Shape: %s" part of this method may be extracted to the super-class. This will avoid repetitions in the concrete classes.

Expressions like 6.0 * Math.sqrt(2.0) should be extracted to static constants. They do not need to be evaluated each time the method is called.

Remove useless comments like //constructor, //accessors and others.

Validation: constructor arguments are not validated. If height, base or radius args are negative? Without entering into theoretical discussion, I suppose that negative values will be invalid in the context of your API.

In main(), the shapeObject array would be initialized better with

Shape[] shapeObject = new Shape[] {
  new Circle(9.0), 
  new Triangle(5.0, 7.0), 
  new Square(4.0), 
  new Sphere(8.0),
  new Cube(6.0),
  new Tetrahedron(3.0)
};

And you need to invoke instanceof only for the case of ThreeDimensionalShape, in order to check for getVolume() method. getArea() is already available for all Shape objects.

Formatting: opening braces should be moved to the end of their preceeding lines.

share|improve this answer
    
Thank you for all these observations!!!! I'll learn stringbuilder in the next chapter I see.. All the others I should have known and implemented. – Bianca Ruiz Nov 3 '15 at 22:39
up vote 2 down vote

Interfaces, as well es classes, are types – see Java Language Specification (JLS), 4.3. Reference Types and Values:

There are four kinds of reference types: class types (§8.1), interface types (§9.1), [...]

So, their names should be similar to class names – see Code Conventions for the Java TM Programming Language, 9 - Naming Conventions:

Class names should be nouns. [...]

Hence, as mentioned by MattPutnam in his answer, getArea and getVolume fit better to method names.

  • I'd use the variables radius, height, base, side instead of the associated getter methods in your area and volume calculations.
  • I'd omit mathematically unnecessary parentheses in your calculations for easier reading.

E.g.:

return 4.0 * Math.PI * radius * radius * radius / 3.0;

instead of:

return 4.0 * Math.PI * ((getRadius() * getRadius() * getRadius()) / 3.0);
share|improve this answer
    
I was under the assumption that using getter methods preserved data encapsulation, data hiding, etc. – Bianca Ruiz Nov 3 '15 at 22:40
    
@BiancaRuiz In principle, yes, but that doesn't include encapsulating or hiding the data from/within your own (internal) class implementation. It's meant for (external) users of your class. – Gerold Broser Nov 4 '15 at 17:06

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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