4
\$\begingroup\$

I've done a job interview assignment on shapes (surprising ah?!) which is a C# console application which creates a list of shapes basically. It's available here, but for my question the only relevant part is the following:

This is where shapes are created - AddNewShape method (I presented this in the interview):

switch (pressedKey)
{
    case ConsoleKey.D1:
        shape = new Square(GetNumFromConsoleAndVerifyIt());
        break;
    case ConsoleKey.D2:
        double height = GetNumFromConsoleAndVerifyIt();
        double width = GetNumFromConsoleAndVerifyIt();
        shape = new Rectangle(height, width);
        break;
    case ConsoleKey.D3:
        shape = new Circle(GetNumFromConsoleAndVerifyIt());
        break;
    case ConsoleKey.D4:
        double triangleHeight = GetNumFromConsoleAndVerifyIt();
        double triangleWidth = GetNumFromConsoleAndVerifyIt();
        shape = new RightTriangle(triangleHeight, triangleWidth);
        break;
    default:
        throw new ArgumentException();
}

The problem that was discussed about this section is that it's not "Open for Extension" (Personally I prefer the 'Predicted Variations' version of this principle). So later on I refactored this part:

// Use reflection to get the type and it's required params:
Type shapeType = shapeTypes.Where(t => t.FullName.ToLower().Contains(shapeNameEntered.ToLower())).First();
var ctor = shapeType.GetConstructors()[0];

List<object> sides = new List<object>();
foreach (ParameterInfo pi in ctor.GetParameters())
{
    sides.Add(GetNumFromConsole(pi.Name));
}
shape = (I2DShape)ctor.Invoke(sides.ToArray());

And now you can clone my project and add whatever shape you want, implement I2DShape (also - have only one ctor) and change nothing in the Program.cs etc...

So, does this follow the principle? Did I get it right using reflection or should I have used a different approach?

\$\endgroup\$
  • 1
    \$\begingroup\$ Out of curiosity, why the title change? \$\endgroup\$ – Moshisho Mar 15 '17 at 12:39
  • 2
    \$\begingroup\$ State what your code does in your title, not your main concerns about it. See: How do I ask a good question? \$\endgroup\$ – t3chb0t Mar 15 '17 at 12:51
  • \$\begingroup\$ I see my question is gaining popularity (which is really cool!) and personally, even though there's an accepted answer, I'd love to see other suggestions if you have one, on how to approach this sort of problem. \$\endgroup\$ – Moshisho Mar 16 '17 at 11:43
9
\$\begingroup\$

I find neither approach is good.

What's wrong with the switch?

  • It needs to be modified with each change and even only if you want to do something with only one shape.
  • It's bound to key-codes.
  • You cannot test it.

What's wrong with reflection?

  • Reflection is usually the last resort solution. I think you are not at a dead end yet.
  • The way you implemented it is very unsafe because you use the FullName and Contains. What if a Rectangle would be in a namespace like CirclesAndSqures and you are looking for a circle? You'll find something that is not a circle but a rectangle only because its namespace has the name circle in it.
  • You cannot test it.

Instead I suggest the following. Let's say you have an abstract Shape

abstract class Shape { }

and some concrete types

class Circle : Shape { }

For each type you can write a factory that implements a common interface:

interface IShapeFactory
{
    Shape CreateShape();
}

Each factory of course can depend on other services like reading from the console etc.

class CircleFactory : IShapeFactory 
{
    public Shape CreateShape() 
    {
        // read params etc
        return new Circle();
    }
}

Finally you create a dictionary with all the shape factories

var shapeFactories = new Dictionary<string, IShapeFactory>(StringComparer.OrdinalIgnoreCase)
{
    [nameof(Circle)] = new CircleFactory()
};

var shapeName = // read shape name...

if (shapeFactories.TryGetValue(shapeName, out IShapeFactory shapeFactory))
{
    var shape = shapeFactory.CreateShape();
}

You can now add as many shapes and factories as you want without touching any other shapes or common code.

\$\endgroup\$
  • \$\begingroup\$ What is the consideration for making Shape class abstract rather than an IShape interface? And when I create a new factory I'll need to add it to the shapeFactories is it still OK with the "closed for modification"? \$\endgroup\$ – Moshisho Mar 15 '17 at 12:43
  • 1
    \$\begingroup\$ @Moshisho none, it can be either one ;-) It's just an example. But if you're going to add some common functionality you might want to like abstract class better if you find you are repeating the same code in each implementation. \$\endgroup\$ – t3chb0t Mar 15 '17 at 12:47
  • \$\begingroup\$ Sorry to bug you, but last question, I edited the first comment before you saw it... When I create a new factory I'll need to add it to the shapeFactories is it still OK with the "closed for modification"? Perhaps filling the dictionary from a config file is the solution? \$\endgroup\$ – Moshisho Mar 15 '17 at 13:46
  • 2
    \$\begingroup\$ @Moshisho the open/closed principle is not a dogma to be followed at all costs. There are situations, like your dictionary, where it's not applicable. \$\endgroup\$ – alebianco Mar 15 '17 at 15:00
  • 1
    \$\begingroup\$ @Moshisho open/closed means you should not need to modify the core api/implementation. The only place that you can open and change is where you put all the modules together like e.g. where you build the dicitonary in the Program or where you build the IoC container etc. \$\endgroup\$ – t3chb0t Mar 15 '17 at 17:05

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.