5
\$\begingroup\$

I had an exercise as below for an interview. I used two design patterns: factory and decorator. I put my code at the end of it.

How could I improve the solution?


Basic OOD & Solution Design Requirements

Develop a simple Visual Studio CONSOLE Application which simulates a vector-based drawing package.
Your application should support the following 5 drawing primitives (we'll call them widgets):

1) rectangle 2) square 3) ellipse 4) circle 5) textbox

The application should allow a user to Add a new widget to the drawing, stating the location and size/shape of the widget.
The location can be a standard x/y coordinate on an imaginary page. The size/shape depends on the widget, as follows:

  • rectangle – width and height
  • square – width
  • ellipse –horizontal and vertical diameter
  • circle – diameter
  • textbox – bounding rectangle (i.e., the rectangle which surrounds the textbox; the text will be centred within this rectangle).

NOTE: - there is no need to worry about rotation. - integer units are fine for all dimensions. - for the textbox you only need to configure the text to display (which will be rendered horizontally within the bounding rectangle). Don’t worry about font face /size / alignment, etc. - as per our coding standards, please use a separate .cs file for each class.

Your console application should be able to 'print out' the current drawing by printing the key details of each widget (type, location, size/shape) to the console.
You don't need to actually render the widgets in any manner - we're just simulating a drawing package at this stage.

Your application can use the Console Application Main() method to test the drawing simulator by adding a hard-coded set of widgets to the drawing.

PLEASE ADD ONE OF EACH WIDGET TO YOUR DRAWING

** Important Note: You DO NOT need to concern yourself with handling any input from the user. Our widgets will be hard-coded within the Main() method.

Time allowed: 2 hours.

My solution follows:

public interface IDraw
{
    void Print();
}

class Shape
{
    private int X { get; set; }
    private int Y { get; set; }

    public Shape(int x, int y)
    {
        this.X = x;
        this.Y = y;
    }

    public string GetLocation()
    {
        return "(" + this.X + "," + this.Y + ")";
    }
}

class Square : Shape, IDraw
{
    public Square(int x, int y) : base(x, y)
    {
    }

    public int size { get; set; }

    public void Print()
    {
        Console.WriteLine("{0} {1}  size={2}", typeof(Square).Name, base.GetLocation(), this.size);
    }
}

class Rectangle : Shape, IDraw
{

    public int Width { get; set; }

    public int Height { get; set; }

    public Rectangle(int x, int y) : base(x, y)
    {
    }

class Textbox : Rectangle, IDraw
{
public Textbox(int x, int y)
: base(x, y)
{ }

public string Text { get; set; }

public new void Print()
{
Console.WriteLine("{0} {1} Width={2} Height={3} Text={4}", typeof(Textbox).Name, base.GetLocation(), this.Width, this.Height, "\"" + this.Text + "\"");
}
}
    public void Print()
    {
        Console.WriteLine("{0} {1} Width={2} Height={3} ", typeof(Rectangle).Name, base.GetLocation(), this.Width, this.Height);
    }
}

class Ellipse : Shape, IDraw
{

    public Ellipse(int x, int y) : base(x, y)
    {
    }

    public int HorizontalDiameter { get; set; }

    public int VerticalDiameter { get; set; }

    public void Print()
    {
        Console.WriteLine("{0} {1} diameterH={2} diameterY={3}", typeof(Ellipse).Name, base.GetLocation(), this.HorizontalDiameter, this.VerticalDiameter);
    }
}

class Circle : Shape, IDraw
{

    public Circle(int x, int y) : base(x, y)
    {
    }

    public int size { get; set; }

    public void Print()
    {
        Console.WriteLine("{0} {1} size={2}", typeof(Circle).Name, base.GetLocation(), this.size);
    }
}

class Program
{

    static void Main(string[] args)
    {

        //Hard-coded set of widgets 
        var widgets = new List<IDraw>() 
        {
            new Rectangle(10, 10) { Width = 30, Height = 40 },
            new Square(15, 30) { size = 35 },
            new Ellipse(100, 150) { HorizontalDiameter = 300, VerticalDiameter = 200 },
            new Circle(1, 1) { size = 300 },
            new Textbox(5, 5) { Width = 200, Height = 100, Text = "Sample text!" }
        };


        foreach (var widget in widgets)
        {
            try
            {
                widget.Print();
            }
            catch (Exception ex)
            {
                Console.WriteLine(ex.Message);
            }

        }

        Console.ReadKey();
    }
}
\$\endgroup\$
7
  • 1
    \$\begingroup\$ This isn't really on topic for SO. Programmers...maybe. (If you re-work the actual question you're asking.) \$\endgroup\$
    – Servy
    Commented Oct 4, 2012 at 14:57
  • 3
    \$\begingroup\$ We can't tell you why someone else decided against you. Could be any number of reasons. Call them and ask for feedback - you might learn something. \$\endgroup\$
    – Oded
    Commented Oct 4, 2012 at 14:58
  • \$\begingroup\$ @Servy - Not the way it is, not if the question is about why this is supposedly incorrect. \$\endgroup\$
    – Oded
    Commented Oct 4, 2012 at 14:58
  • \$\begingroup\$ @Oded Yeah, that's why I said "maybe". It's more that, this question can't really be fixed up enough to stay on SO; it might be able to be re-phrased into something that's appropriate for programmers. It's also why I voted to close as NARQ rather than off topic. \$\endgroup\$
    – Servy
    Commented Oct 4, 2012 at 15:00
  • \$\begingroup\$ Did you not implement "Textbox"? \$\endgroup\$ Commented Oct 4, 2012 at 15:12

1 Answer 1

4
\$\begingroup\$

Off the bat I see a couple of things with regards to MS published coding standards, although I'm not sure what their standards are.

I'm not sure why the X/Y locations are private. Seems to me that you'd want those properties exposed as public members with regular fields backing them and default values set. Exa:

private int _x = 0;
public int X { 
  get { return _x; }
  set { _x = value; }
}

As it stands now, the only way to get the location is through the GetLocation method which returns a string already formatted for display... not exactly what you want as that couples display to implementation which is a no-no.

Next public properties should always follow proper capitalization. You have:

public int size { get; set; }

which should be:

public int Size { get; set; }

Although, again, I'd set a default value or ensure some type of constraints on it.

Next, instead of having a generic try .. catch. I'd simply test if the object implemented the IDraw interface and, if so, call it's method. The try .. catch, especially with a generic catch, is lazy and for those whom performance is critical not a good thing.

Finally, I'm not sure that I would implement the print formatting within the classes themselves anyway. Again, that's coupling presentation with logic. Perhaps it would have been better to have another class whose job it is to emit the objects.

Finally/Finally, I would think 2 hours is probably 4 times longer than necessary to whip this out, even under pressure. Then again, I type fast.


All in all, if I was judging your entry against the requirements I'd estimate that you were a junior developer that needs some training. Yes, you met the requirements however the code itself would need additional work before going into a production type environment. If I was looking for a senior guy I wouldn't bother bringing you in for more questions.

Hope that helps. This answer is not meant to be bashing in any way but rather just a real world breakdown of what one person that routinely hires developers (not for them) sees wrong. So I hope it's taken in that vain.


Just my 0.02. Apart from the above points, especially since you presented it as a simple OOD exercise, I would have implemented a "Drawing" class with Print and Add methods instead of using a simple List.

Furthermore, your use of the new modifier in the Print method of Rectangle really annoys me, and would have made me look for someone else.

I think that I would have defined Shape as implementing IDraw, too, with a virtual version of Print printing just the position of the Shape (or even an abstract version of the method, if you prefer). Then you can override the method as you want in all your subclasses.

In the majority of cases having to use the new modifiers makes me look for bad design decisions.

\$\endgroup\$
8
  • 1
    \$\begingroup\$ For the record, I did vote to close AND answered the question (in a wiki format). I'm not sure this question belongs here but at the same time I think helping people understand how to grow is a good thing. \$\endgroup\$ Commented Oct 4, 2012 at 15:32
  • \$\begingroup\$ Thank you very much. I think it was a graet review of codes. You are right about creating a new class and having those methods there. It was a main thing that I missied. Not sure why this quetion has been blocked as it is a good practice for reviewing and coding. \$\endgroup\$
    – patricgh
    Commented Oct 5, 2012 at 12:04
  • \$\begingroup\$ @patricgh: I changed your question slightly to be more inline with a regular code review type instead of asking the community to guess what the hiring team thought. \$\endgroup\$ Commented Oct 5, 2012 at 14:01
  • \$\begingroup\$ Thank you very much. I think, now, it makes mush sense than before. Hope it can help other coders. \$\endgroup\$
    – patricgh
    Commented Oct 5, 2012 at 15:39
  • 1
    \$\begingroup\$ "Typically in c# private variables are prefixed with an underscore." Not in current Microsoft standards, no. Camel-cased with no additional decoration is preferred layout for the name. \$\endgroup\$ Commented Oct 15, 2012 at 16:11

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.