How to break up a large class

Question

I have a large Shape class, instances of which can (should) be able to do lots of things. I have many "domain" shape classes which inherit from this class, but do not provide any different functionality other than drawing themselves.

I have tried subclassing the Shape class, but then all of the "domain" objects will still inherit this subclass.

How do I break up the class? (it is 300 text lines, C#)

Answer 1

300 lines seems reasonable to me.

post the code if you really want better help

Answer 2

you could break up by delegating functions to other helper classes.

but I agree that 300 lines of code isn't terrible.

+1 for posting the code

Answer 3

A couple of ideas (more like heuristics):

1) Examine the fields of the class. If a group of fields is only used in a few methods, that might be a sign that that group of fields and the methods that use it might belong in another class.

2) Assuming a well-named class, compare the name of the class to what the class actually does. If you find methods that do things above and beyond what you'd expect from the class' name, that might be a sign that those methods belong in a different class. For example, if your class represents a Customer but also opens, closes, and writes to a log file, break out the log file code into a Logger class. See also: Single Responsibility Principle (PDF) for some interesting ideas .

3) If some of the methods primarily call methods on one other class, that could be a sign that those methods should be moved to the class they're frequently using (e.g. Feature Envy).

CAUTION: Like they say, breaking up is hard to do. If there is risk in breaking up the class, you may want to put some tests in place so that you know you're not breaking anything as you refactor. Consider reading "Working Effectively with Legacy Code" and the "Refactoring" book.

Answer 4

Thanks for the code.

Here are a few things you might try:

1) Refactor duplicate code. This kind of code was duplicated about seven times:

       Visio.Cell pinX = GetLayoutCell(Visio.VisCellIndices.visXFormPinX);
        if (pinX != null)
        {
            pinX.set_Result("cm", value);
        }

Note: PinY also calculates pinX but doesn't use its value.

Similar duplication exists in: Pos{X,Y}{Start,End}

What makes this class more challenging to break up is that it's a wrapper around an already complex class.

Not knowing the domain very well (although I'm an expert with the Shape, Circle, Square concept), I'd be tempted to break the class into several classes that each share the same core Shape object.

Here is a sketch:

class EnvironShape {
   private ShapeProperties _properties;   // contains property management code
   private ShapeCollection _children;     // contains code for acting on children
   private Decorators      _decorators;   // code for accessing decorators
   private Layers          _layers;       // layer management code
   private Position        _position;     // code for working with the shape's position
   // Other code omitted
}

I would not immediately and directly expose these objects (e.g. public ShapeCollection GetChildren()) but I would start off making the EnvironShape delegate to these objects.