Class factory desing question

  • Thread starter Thread starter kpg
  • Start date Start date
K

kpg

Hi all,

I'm having difficulty coming up with a design that I like for this
particular problem. He's the deal:

I have an abstract class and a set of concrete subclasses based on that
class, typical abstract class factory pattern. The abstract class is a
shape class, and the subclasses are types of shapes, circle, square,
arc, and so on.

I read an xml file which defines the shapes, it looks as expected:

<shapes>
<circle>
<x>0</x>
<y>0</y>
<radius>0</radius>
</circle>

... mode shapes here

</shapes>

So as I process the xml, I find out what kind of shape I need, I create
it and set the shape's properties based on the xml. The shape is then
added to a shape collection for later use.

Here is the class definition:

public abstract class Shape
{
public int x;
public int y;
}

public class Circle : Shape
{
public int radius;
}

public class Arc : Circle
{
public int start;
public int sweep;
}

public class Square : Shape
{
public int width;
public int height;
}


Now, I could have a case statement and do this:

//case need to build a circle
Circle myCircle = new Circle();
myCircle.x = 0;
myCircle.y = 0;
myCircle.radius = 15;

//case need to build an arc
Arc myArc = new Arc();
myArc.x = 0; //duplicate code
myArc.y = 0; //duplicate code
myArc.radius = 15; //duplicate code
myArc.start = 90;
myArc.sweep = 180;

//case need to build a Square
Square mySquare = new Square();
mySquare.x = 0; //duplicate code
mySquare.y = 0; //duplicate code
mySquare.width = 20;
mySquare.height = 40;


But this seems inefficient, all shapes have an x and y (at least) and
it needs to be set each time for each subclass.

So I could use a class factory and do this:

Shape myShape;

myShape = ShapeFactory(); //assume this retunrs the appropriate class

myShape.x = 0;
myShape.y = 0;

if (myShape is Circle)
{
Circle myCircle = (Circle)myShape; //downcast
myCircle.radius = 15;

if (myShape is Arc)
{
Arc myArc = (Arc)myShape; //downcast
myArc.start = 90;
myArc.sweep = 180;
}
}

if (myShape is Square)
{
Square mySquare = (Square)myShape; //downcast
mySquare.width = 20;
mySquare.height = 40;
}

But somehow this does not seem more efficient, in a way it is more
complex and harder to maintain.

The problem comes from the need to downcast the shape object into its
concrete class. If there were not strong typing I could just assign the
value (with the danger of a runtime crash if its the wrong type of
object.)

The above example demonstrates my problem, but in the actual code its
even worse, because in the above example I'm assigning values in place,
in the real code I grab an xml values, say radius, and set it to the
shape object as part of a case statement like this:

//what I would like to do...
switch (attribulename)
{
case "radius':
myshape.radius = attributevalue;

...more case statements


}


//what i am forced to do...
switch (attribulename)
{
case "radius':
if (myShape is Circle)
{
Circle myCicrle = (Circle)myShape;
myCircle.raduis = attributevalue;
}
if (myShape is Arc)
{
Arc myArc = (Arc)myShape;
myArc.raduis = attributevalue;
}

...more case statements

}

//the resultant code, with dozens of shapes, is atrocious.

One solution is to create one super class, that has all possible
attributes in it for every kind of shape, and then have a shapetype
property that tells me what kind of shape it really is. This seems to
destroy any benefit from subclassing altogether.

At this point, I'm wondering if there is any benefit to subclassing,
aside the "It's the proper thing to do" argument.

Open to any suggestions of ideas.

Thanks,
kpg
 
A factory class is a creational pattern.

What you are describing is a strategy/composite (behavioural/structural
pattern) and an abstract factory pattern for creating these families if an
abstract factory is necessary a factory method may do depends on the exact
details.
 
I just read the rest. You seem confused over factory patterns.

IShape{}
Circle : IShape{}
Square : IShape{}

So a basic factory method would be, abstract factory methods are much more
complex and rarely needed IMHO.

public static IShape ShapeFactory(shapeEum shape)
{
switch (shapeEnum)
{
case : shapeEnum.Circle
return new Circle();
case : shapeEnum.Square
return new Square();
}

if X and Y are common to all and in the interface you could do this


public static IShape ShapeFactory(shapeEum shape, int x, int y)
{
switch (shapeEnum)
{
case : shapeEnum.Circle
return new Circle(x, y);
case : shapeEnum.Square
return new Square(x, y);
}

}
 
Oh nd I forgot to mention using abstract classes rather than interfaces
allows you to do much more ofthe removng repetitive common coding between
the classes.
 
I just read the rest. You seem confused over factory patterns.
Click to expand...

Confused? Me?

Yes, indeed, A quick reference in my patters book (gang of 4) shows
that the composite pattern is indeed what I am describing. I guess I
need to read that chapter.

Totally unrelated, I ran across a way to simplity the code a bit (well,
a good bit) at the expense of complicating the class - by using
Interfaces. In this way I could deinfe an interface for each 'group' of
attributes, like color, radius, font - and have my concreate classes use
these interfaces as a grab bag of properties, as such, so I can pick and
choose what each shape needs to have in the way of porperties.

This lets my code do relativly simple checks like this:

//case need to set the color property...
if (myShape is IColor) ((IColor)myShape).clr = Color.Green;

The Interface definitions:


public interface IShape
{
int x { get; set;}
int y { get; set;}

}

public interface IColor
{
Color clr { get; set;}
}
public interface IRadius
{
int radius { get; set;}
}

public class Circle : IShape,IColor,IRadius
{
int myX;
int myY;
int myRadius;
Color myClr;

public int x
{
get
{
return myX;
}
set
{
myX = value;
}
}

public int y
{
get
{
return myY;
}
set
{
myY = value;
}
}

public int radius
{
get
{
return myRadius;
}
set
{
myRadius = value;
}
}

public Color clr
{
get
{
return myClr;
}
set
{
myClr = value;
}
}


}

public class Arc : Circle
{
public int start;
public int sweep;

}

public class Square : IShape, IColor
{
public int width;
public int height;

int myX;
int myY;
Color myClr;

public int x
{
get
{
return myX;
}
set
{
myX = value;
}
}

public int y
{
get
{
return myY;
}
set
{
myY = value;
}
}


public Color clr
{
get
{
return myClr;
}
set
{
myClr = value;
}
}

}
 
abstract class Shape {
int x, y;
public Shape(XmlDocument doc) {
this.x = doc.whatever.x;
this.y = doc.whatever.y;
}
public static Shape Create(XmlDocument doc) {
switch (doc.type) {
case "Circle":
return new Circle(doc);
case "Cube":
return new Cube(doc);
}
}
}

class Circle : Shape {
int radius;
public Circle(XmlDocument doc)
: base(doc) {
radius = doc.whatever.radius;
}
}

you can
 
You could but ask yourself what does this give you?

If you have many classes tha inherit IShape,IColor,IRadius

Create a CircleBase class with circle/elipses sub classes and implement an
Abstract Factory.

BTW Patterns are black art and we ll get it wrong, at the end of the day if
you impleent something that helps you code, readability, maintainbility ect
etc do not worry too much what the pattern is called just be consitent when
you structure code that requires similar patterns. Using every pattern in
the GOF book would probably result in unreadable code so 'pick you battles'.
 
IMHO you just strongly coupled the shape framework to XMLDocument, and not
only that but dealing with XMLDocument is inherently slow.
 
Paul said:
IMHO you just strongly coupled the shape framework to XMLDocument, and not
only that but dealing with XMLDocument is inherently slow.
Click to expand...

Yes, he did: presumably, the different shapes are just shapes and aren't
supposed to know such details as how to create themselves from an XML
document in a particular format, so you're right. The person creating
the factory may not even be the person who created the shape classes.

On the other hand: *someone* has to read the data from the XMLDocument.
Where that happens doesn't alter the speed.
 
Back
Top