Refactoring – How to Avoid Changing the Signature on a Well-Used Method

changeparametersrefactoring

The Existing Code

I have a C# project with about 45000 lines of code. It has a utility/helper class which contains static methods which make it easier for my code to work with PDFSharp/Migradoc to produce PDF forms, which are predominantly structured using tables. One such method adds text to a cell and has the following header:

public static Cell TextInCell(  Cell cell, string text, 
   CellStyle style = CellStyle.Normal, 
   int mergeRight = 0, 
   bool underline = false, 
   ParagraphAlignment? horizontalAlignment = null, 
   VerticalAlignment? verticalAlignment = null)

CellStyle is my own enumeration with 15 members, its a mix of sizing, font and colour information e.g. Tiny, SmallRed, SmallShaded. If the parameter is BoldHeading, SmallHeading, or Heading then it applies a shading to a cell, the colour of which is kept in a private static readonly variable of the utility class.

The options are not totally independent. For example, when a heading option is picked the font colour is automatically set to white.

There are approximately 1000 places in my code in which this method is called, with around 200 involving one of the heading options of CellStyle. These usages are spread out amongst 24 classes, all of which have a common ancestor in the inheritance hierarchy: a class called FormDesigner.

New Requirement

Now the colour of the shading may vary, it can either be blue or green depending on the type of object which supplies the data for the forms.

Possible solutions

  1. Change the enums to remove the 3 values that currently produce
    shading and add in: BlueBoldHeading,
    BlueSmallHeading,BlueHeading, GreenBoldHeading,
    GreenSmallHeading, orGreenHeading.
  2. Add another parameter to indicate the colour of the shading. This
    parameter is meaningless unless a shading option for CellStyle is
    used.
  3. Separate the functionality of shading out of this method and put it
    in another method, which must be called in addition to TextInCell.
  4. Introduce a static method to set the static colour variable before
    each form is printed.
  5. Move this functionality to a new non-static class, which keeps hold
    of the option for shading. An instance can then be created in the constructor for FormDesigner.

Options (1) and (2) are bad because they would entail a lot of work and just increase the complexity of the method header, which I think is making the problem worse.

I think I'm going to go for option 5. I will replace all references to the static class with references to an instance, this can mostly be done automatically.

Questions

Does the way this method work constitute a code smell ? My understanding is that is something is prohibitively difficult to change, there is a code smell.

Does option (4) constitute a professional solution ? It is obviously the easiest option, but intuition tells me it is wrong. It is keeping state in a static variable, which I don't normally do; I keep state in objects and either pass around the objects or use parameters.

Best Answer

That a design is hard to change in a particular way is not always a problem, but in this case I think there is a superior design that makes this change easier.

If the method instead of taking half a dozen optional parameters took a single ParameterObject, one could easily add a color field to that parameter object without impacting existing callers. In this case you can introduce the superior design while maintaining backwards compatibility by introducing a new method taking a parameter object, rewriting the old method to forward to the new and then deprecating the old method.

Related Topic