1
\$\begingroup\$

Description:

Although shapes can be very different by nature, they can be sorted by the size of their area.

Create different shapes that can be part of a sortable list. The sort order is based on the size of their respective areas:

The area of a Square is the square of its side
The area of a Rectangle is width multiplied by height
The area of a Triangle is base multiplied by height divided by 2
The area of a Circle is the square of its radius multiplied by π
The area of a CustomShape is given

The default sort order of a list of shapes is ascending on area size:

var side = 1.1234D;
var radius = 1.1234D;
var base = 5D;
var height = 2D;

var shapes = new List<Shape>{ new Square(side),
                        new Circle(radius),
                        new Triangle(base, height) };
shapes.Sort();
  • Use the correct π constant for your circle area calculations:

    System.Math.PI

And you only have tests cases to base your code on.

using NUnit.Framework;
using System;
using System.Linq;
using System.Collections.Generic;

    [TestFixture]
    public class ShapesTests
    {
        [Test]
        public void ShapesAreSortableOnArea()
        {
            // Arrange
            double width, height, triangleBase, side, radius, area;
            Random random = new Random((int)DateTime.UtcNow.Ticks);

            var expected = new List<Shape>();

            area = 1.1234;
            expected.Add(new CustomShape(area));

            side = 1.1234;
            expected.Add(new Square(side));

            radius = 1.1234;
            expected.Add(new Circle(radius));

            triangleBase = 5;
            height = 2;
            expected.Add(new Triangle(triangleBase, height));

            height = 3;
            triangleBase = 4;
            expected.Add(new Triangle(triangleBase, height));

            width = 4;
            expected.Add(new Rectangle(width, height));

            area = 16.1;
            expected.Add(new CustomShape(area));

            var actual = expected.OrderBy(x => random.Next()).ToList(); 

            // Act
            actual.Sort();

            // Assert
            for (var i = 0; i < 5; i++)
                Assert.AreEqual(expected[i], actual[i]);
        }
    }

My answer below was correct I would like it reviewed and if possible refactored or Improved this was a question I got on Codewars.com and my solution passed all tests on the site.

    using System;
    using System.Collections.Generic;
    using System.Linq;

    public class Shape : IComparable<Shape>
        {
            public double Area { get; set; }

            public int CompareTo(Shape other)
            {
                return this.Area.CompareTo(other.Area);
            }

        }

        public class Square : Shape 
        {
            public Square(double side)
            {
                Area = side * side;
            }
        }

        public class CustomShape : Shape
        {
            public CustomShape(double area)
            {
                Area = area;
            }
        }

        public class Circle : Shape 
        {
            public Circle(double radius)
            {
                Area = (radius * radius) * Math.PI;
            }
        }

        public class Rectangle : Shape
        {
            public Rectangle(double width , double height)
            {
                Area = width * height;
            }
        }

        public class Triangle : Shape
        {
            public Triangle(double Base, double height)
            {
                Area = (Base * height) / 2;
            }
        }
\$\endgroup\$
  • \$\begingroup\$ This looks like a kata from Codewars. Please make sure to show what code is yours and what code is from the site. \$\endgroup\$ – Zeta Feb 8 '17 at 6:17
  • \$\begingroup\$ This question is from Codewars but @Zeta but this is ALL MY CODE... \$\endgroup\$ – Gringo Jaimes Feb 8 '17 at 17:27
  • \$\begingroup\$ No. The test case isn't. Neither is the description. Keep in mind that by posting your question you licence it under CC-BY-SA. \$\endgroup\$ – Zeta Feb 8 '17 at 18:42
5
\$\begingroup\$

The main thing I see in your solution, is that your derived classes, would need quite a bit of re-factoring if you needed them in a more general sense. I think having each shape with its own parameters and its own getter for the Area property, is much more adaptable:

public class Square : Shape
{
    public double side { get; set; }
    public double Area
    {
        get { return side * side; }
    }
    public Square(double side)
    {
        this.side = side;
    }
}
\$\endgroup\$
  • 2
    \$\begingroup\$ The principle is good, but to nitpick coding conventions: in C# the property should have an initial capital: Side. \$\endgroup\$ – Peter Taylor Feb 8 '17 at 11:11
3
\$\begingroup\$

Your hierarchy presumes that every descendant class will call Area setter. That's wishful thinking. :) You will get away with it in this small exercise, but you wont in more complex projects.

You should make Area abstract.

public abstract class ShapeBase : IComparable<Shape>
{
    public abstract double Area { get; }

    public int CompareTo(Shape other)
    {
        return this.Area.CompareTo(other.Area);
    }
}

This will enforce every descendant to override it. Note, that there is no public setter. It should not be possible to execute this code:

new Circle(1) { Area = 200000 };

Also, you should normally override equality methods if you implement IComparable<T>. If CompareTo returns 0, Equals should not return false.

\$\endgroup\$
  • \$\begingroup\$ does that mean I have to override all of equality then \$\endgroup\$ – Gringo Jaimes Feb 8 '17 at 17:26
  • \$\begingroup\$ @GringoJaimes, you have to override Object.Equals(), IEquatable<T>.Equals() and Object.GetHashCode(). Alternatively you can implement stand-alone IComparer<T>. Then you don't have to do all that. \$\endgroup\$ – Nikita B Feb 9 '17 at 7:39

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.