Shape area calculation

Question

I know questions with this text has been asked and already answered, but my question is mainly limited to only one method of this flow and where it should be.

I have an assembly for Shape's area calculation (ShapeAreaCalculator) and this functionality is exposed to client program:

public enum ShapeType
{
    SquareShape,
    CircleShape,
    TriangleShape,
}
public interface IShape
{
    void CalculateArea();
    void ShowArea();
}
public class Square : IShape
{
    public void ShowArea()
    {
        Console.WriteLine("Logic For ShowArea of Square");
    }
    public void CalculateArea()
    {
        Console.WriteLine("Logic For CalcualteArea of Square");
    }
}
public class Circle : IShape
{
    public void ShowArea()
    {
        Console.WriteLine("Logic For ShowArea of Circle");
    }
    public void CalculateArea()
    {
        Console.WriteLine("Logic For CalculateArea of Circle");
    }
}
public class Triangle : IShape
{
    public void ShowArea()
    {
        Console.WriteLine("Logic For ShowArea of Triangle");
    }
    public void CalculateArea()
    {
        Console.WriteLine("Logic For CalculateArea of Triangle");
    }
}

Factory class:

public interface IShapeFactory
{
    IShape GetShape(ShapeType shapeType);
}
public class ShapeFactory : IShapeFactory
{
    public IShape GetShape(ShapeType shapeType)
    {
        switch(shapeType)
        {
            case ShapeType.SquareShape:
                return new Square();
            case ShapeType.CircleShape:
                return new Circle();
            case ShapeType.TriangleShape:
                return new Triangle();
            default :
                throw new ArgumentException();

        }
    }
}

And I'm using this at client side like this:

static void Main(string[] args)
{
    ShapeType selectedShape;
    while (true)
    {
        Console.WriteLine("Enter your choise");
        foreach (ShapeType shapeType in Enum.GetValues(typeof(ShapeType)))
        {
            Console.WriteLine((int)shapeType + " For " + Enum.GetName(shapeType.GetType(), shapeType));
        }
        int selection;
        if (int.TryParse(Console.ReadLine(), out selection) && Enum.IsDefined(typeof(ShapeType), selection))
        {
            selectedShape = (ShapeType)selection;
            break;
        }
    }
    CalculateAndShowAreaOfShape(selectedShape);
}
static void CalculateAndShowAreaOfShape(ShapeType selectedShape)
{
    IShapeFactory shapeFactory = new ShapeFactory();
    IShape currentShape = shapeFactory.GetShape(selectedShape);
    currentShape.CalculateArea();
    currentShape.ShowArea();
}

But now I am concerned about this method CalculateAndShowAreaOfShape(). If in the future some functionality added in the procedure of area calculation, client code will be needed to be modified. It is unnecessary coupling.

Should it be implemented at client end or somewhere in ShapeAreaCalculator assembly?

Also, is it good practice to ask for user input from assembly? (I think it will limit this assembly to be used in console application only.)

Answer 1

I think your names could be simplified. For instance, the ShapeType enum could be:

public enum ShapeType { Square, Circle, Triangle }

Because ShapeType.SquareShape sounds redundant, whereas ShapeType.Square still clearly indicates that the square is a shape type.

The ShowArea method is not needed. It would most likely be used to display console writes, but your shape classes should not concern themselves with writing to the console. You could modify ShowArea to return a string, but that string would be nothing more than the numeric area converted to a string.

CalculateAndShowArea should be in the client, as it issues several calls and it - not the respective IShape instance - should take care of console writes.

But most importantly, the area calculation should be done within the shape classes. The problem with your code as posted is it lacks many critical things that help distinguish among different shape types. For instance, a circle would be defined at the very least by a radius, and you may consider a center XY point along with a radius. This allows you to calculate the circle's area. Likewise, a square should be defined by a length of one of its sides. And again, a square could then calculate its area.

A triangle is a different creature. There are different ways to construct a triangle. One way is passing in 3 non-colinear points. Another way might be passing in 3 line segment lengths. But a triangle constructor would also have to contain validation that whatever was passed in creates a truly valid triangle.

Which makes a singular factory method to generate 3 very different shapes fairly useless, unless you imposed a restriction that it would be a circle with radius 1, a square with side length of 1, and a triangle with 2 sides of length 1. But if you do that, you've reduced the flexibility of the specific shape classes.

Also, the method name CalculateArea is bit long. A more standard name would be GetArea, but many people would really choose to have a readonly property named Area.

Example

public interface IShape
{
    double Area { get; }
    string Name { get; }
}
public class Square : IShape
{
    // May consider Length be a double
    public int Length { get; }
    public Square(int side)
    {
        Length = side;
    }
    public double Area => Length * Length;
    public string Name => "Square";
    public override string ToString() => $"{Name}: Length={Length}";
}
public class Circle : IShape
{
    // Might consider making this a double
    public int Radius { get; }
    public Circle(int radius)
    {
        Radius = radius;
    }
    public double Area => Math.PI * Radius * Radius;
    public string Name => "Circle";
    public override string ToString() => $"{Name}: Radius={Radius}";
}
// Triangle class is left to you.

This makes your shape constructor more tailor made for each specific shape, but it also means you may not be able to construct it via a singular factory method without some significant changes.

UPDATE

As noted by Jesse Slicer in the comments, each class could identify itself better. Jesse suggests having each class could have a Shape property that would echo its ShapeType. It a decent idea but one that I would modify slightly. I see the ShapeType enum as belong to the factory method. I think a design should allow for the possibility of a shape that isn't specified in the enum but that implements IShape. Thus I would have each class implement its type, and we can ditch the Name property.

public interface IShape
{
    double Area { get; }
    Type Type { get; }
}
public class Square : IShape
{
    // May consider Length be a double
    public int Length { get; }
    public Square(int side)
    {
        Length = side;
    }
    public double Area => Length * Length;
    public Type Type => GetType();
    public override string ToString() => $"{Type.Name}: Length={Length}";
}
public class Circle : IShape
{
    // Might consider making this a double
    public int Radius { get; }
    public Circle(int radius)
    {
        Radius = radius;
    }
    public double Area => Radius * 2 * Math.PI;
    public Type Type => GetType();
    public override string ToString() => $"{Type.Name}: Radius={Radius}";
}

Answer 2

If you're developing for a reusable shape library, you should definitely avoid both string literals (name) and any kind of output and focus on the core shape properties and behavior.

String literals is useless because they have to be localized in the UI anyway, and then a Type enum is more convenient for the developer/client.

Providing output functionality is maybe useful in debug situations - and here overriding ToString() could be helpful, but in general leave it to the client to display any information about the shapes.

Instead of implementing an interface I would derive my shape classes from an abstract Shape base class. This is good OOP practice and gives you the possibility to "lift" common properties to that base class.

In the example below (which is only a skeleton) I've used a base class Shape. The constructor takes a Point for Position and a ShapeTypes for Type. It shows how I use the base class to handle common properties for the sub classes. Whether the properties should be abstract or virtual is up to you to decide according to you needs (Centroid == Position is maybe not a good idea). The Type property is somewhat redundant, but convenient - with the potential to be set wrong, so think it over if it is needed?

public enum ShapeTypes
{
  Square,
  Circle,
  Triangle
}

public struct Point
{
  public readonly double X;
  public readonly double Y;

  public double DistanceTo(Point other)
  {
    return Math.Sqrt(Math.Pow(other.X - X, 2) + Math.Pow(other.Y - Y, 2));
  }
}

public abstract class Shape
{
  public Shape(Point position, ShapeTypes type)
  {
    Type = type;
    Position = position;
  }

  public ShapeTypes Type { get; private set; }
  public virtual Point Position { get; set; }
  public abstract double Area { get; }
  public virtual Point Centroid => Position; // TODO override apropriately in each sub class
}

public class Square : Shape
{
  public Square(Point topLeft, double size) : base(topLeft, ShapeTypes.Square)
  {
    // TODO: Verify input
    Size = size;
  }

  public double Size { get; set; }
  public override double Area => Size * Size;
}

public class Circle : Shape
{
  public Circle(Point center, double radius) : base(center, ShapeTypes.Circle)
  {
    // TODO: Verify input (radius >= 0)
    Radius = radius;
  }

  public double Radius { get; set; }
  public override double Area => Math.PI * Radius * Radius;
}

public class Triangle : Shape
{
  public Triangle(Point a, Point b, Point c) : base(a, ShapeTypes.Triangle)
  {
    // TODO: Verify input
    A = a;
    B = b;
    C = c;
  }

  public Point A { get => Position; set => Position = value; }
  public Point B { get; set; }
  public Point C { get; set; }

  public double LengthA => B.DistanceTo(C);
  public double LengthB => A.DistanceTo(C);
  public double LengthC => A.DistanceTo(B);


  public double Circumference => LengthA + LengthB + LengthC;

  public override double Area
  {
    get
    {
      double s = 0.5 * Circumference;
      return Math.Sqrt(s * (s - LengthA) * (s - LengthB) * (s - LengthC));
    }
  }
}

If you need a factory class/method, you'll need a parameterless constructor in the shape classes.

(Disclaimer: I've not tested the calculations, so you'll have to review them properly, if you find them useful).