How to avoid changing the signature on a well-used method?

Question

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.

Answer 1

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.

Answer 2

In this specific case, I guess you can get away without an additional overload or significantly changing the signature - at least not by changing it in a way the compiler will complain about it.

If the typical use of CellStyle is just by using values like CellStyle.SmallRed as a literal in your code (and not many places in your code assuming implicitly or explicitly this is mandatory an enum), you can also try if you can replace the CellStyle enum by a CellStyle class with three parameters FontSize (a new enum with values Small, Big, Normal), Color (an enum with all colors you need) and FontStyle (an enum with values like Bold, Italics, Standard). Then, provide some static predefined members like

  class CellStyle
  {
      public static readonly CellStyle SmallRed 
         = new CellStyle(){
            FontSize=FontSize.Small, 
            Color=Color.Red
            FontStyle=FontStyle.Standard};
      // ...
  }

as a replacement for all your 15 enum values (Beware, this is untested "air code", I am pretty sure it won't compile, but I hope you get the idea).

If you are lucky, 99% of your code will compile out of the box, since the syntax for using these static constants is exactly the same as for using the enum values. The few places where the compiler complains can probably be fixed with very little effort. Obviously, the one place where you have to change the code is the implementation of your TextInCell method, but that is clearly necessary for any other solution, too.

Answer 3

In my opinion, the method signature itself is a code smell. If I find myself having more than two or maybe three arguments in a signature, it's usually because those parameters should be in the form of class properties instead of a single method.

Once you're at the point you are (with 1k+ calls to the method), your good options are limited. The safest means (in terms of not breaking existing code) is to add optional parameters to the end of the signature such that the previous callers can continue to work without any changes. Note that this doesn't do anything for ensuring that the logic within that method stays correct, though. And of course, while this is the safest means to ensure you don't break the calling contract, it's not a good long-term solution in that it merely adds to the original problem.

If it were me, I'd look at refactoring whatever's in that thing into it's own class. Then I'd look at how I could do a find/replace within the existing callers to convert it. Then I'd have a much better result at the cost of a much more difficult conversion.

Another options may be to add optional parameters (without changing anything in the existing params), then use the method as a legacy wrapper. Meaning, within that method, you instantiate a new, properly coded class, and apply the parameters to properties, at which point the method itself really only exists for the purpose of legacy callers.

I'm not really sure how this plays in with your FormDesigner class, but looking at it from the scope of the method itself, there's a few ideas at least on how you might approach it.

1 is risky for breaking existing callers.

2 could work, as indicated in my text above but may not be clean over the long term.

3 I wouldn't prefer: any time you have a method that must be called after another method in order to work correctly, there's usually a better way to do it.

4 seems like it's not quite right either, as really you should have one unit of code to handle this stuff, no?

5 is a lot of work but seems to be the method that will steer you to more maintainable code in the future.

Answer 4

Switch to a backward-compatible extension method scheme that supports fluent syntax

This is way easier than it sounds, and requires no changes to your legacy code.

Given that your method looks like this:

public static Cell TextInCell(  Cell cell, 
    string text, 
    CellStyle style = CellStyle.Normal, 
    int mergeRight = 0, 
    bool underline = false, 
    ParagraphAlignment? horizontalAlignment = null, 
    VerticalAlignment? verticalAlignment = null)
{
    /*
        Main code goes here
    */
}

It is ripe for being turned into an extension method, like this:

public static Cell TextInCell(this Cell cell,  //All I did was add "this"
    string text, 
    CellStyle style = CellStyle.Normal, 
    int mergeRight = 0, 
    bool underline = false, 
    ParagraphAlignment? horizontalAlignment = null, 
    VerticalAlignment? verticalAlignment = null)
{
    /*
        Main code goes here
    */
}

And it is ripe for being used in a fluent syntax:

public static Cell TextInCell(this Cell cell, Etc etc)
{
    /*
        Main code goes here
    */
    return cell;  //<-- this is what enables the fluent syntax
}

That means you can write other extension methods on Cell and chain them together. For example, to accommodate your new requirement, you could write:

static Cell Background(this Cell cell, Color color)
{
    cell.BackgroundColor = color;
    return cell;
}

Then call it with:

HelperClass.TextInCell(cell, "My text").Background(Color.Green);

Or (this does the same thing):

cell.TextInCell("My text").Background(Color.Green);

You can keep going with any other methods you wish to add, potentially making your API much more flexible (and way cooler):

cell.TextInCell("My text")
    .Style(Styles.Plain)
    .Background(Color.Green)
    .Alignment(Alignment.Left)
    .PetAnimal(Animals.Goat);

Thus you can keep your old code and introduce new features by chaining the calls, just as you would with, say, jquery.

Bonus: Changing the existing static method to an extension method will not break your existing calls. You can call it with either

cell.TextInCell(....)

or

HelperClass.TextInCell(cell, ...);

Both will compile and work exactly the same. So this scheme is 100% backward compatible and you won't have to monkey with your old code at all. You might have to add a using statement at the top of a few code files and that is it.

Answer 5

Does option (4) constitute a professional solution ?

I'd never go for it. Introducing global state is bad; it's acceptable when the alternatives are also bad, but this is not the case here.

It is obviously the easiest option.

It's not. A simple overload is about equally easy and you'll never have to worry about the global state.

IIUYC, your enum is growing from 15 members to 18 and because of the structure, you don't want to simply let it grow. That's sane. Adding yet another parameter to such a full-blown method would be insane as there are already too many.

Conservative solution

The problem is that your old enumerated CellStyle isn't expressive enough, and this can be solved in a few simple trivial steps:

1. You need CellStyleAndShadingColor (this is a stupid name, but let's rename it later). This new class is composed of two parts as the name says. For ShadingColor use an enum if you want and use Color or whatever.

2. Now rewrite TextInCell so that it works with CellStyleAndShadingColor in place of CellStyle simply by extracting the CellStyle and leaving all the rest (congrats, you've got 1k errors).

3. Then add an overload taking the same arguments as the old method and delegating to the new one (now, 1k errors disappear).

4. Now, everything should work exactly as before. This could be a big advantage when some manual test was needed, but we did only a very trivial change. So it's time to implement the shading.

Long term solution

The problem with being conservative is that things hardly ever get better. I would not go for a single parameter object as this would (at least partly) shift the ugliness to the parameter object. An extension method called like

Cell.SetText("text")
  .Style(`BoldHeading`, `Blue`)
  .Align(`ParagraphAlignment.SPECIAL`);

grouping related parameter together might be the best (assuming there's a good logical grouping; otherwise this has been already proposed). Instead of the extension method, you could start with an instance holding the shading (your solution 5). In any case, a method forwarding the legacy calls saves you from having to touch existing call sites.

Answer 6

I decided that I need to move the TextInCell method (and a couple of other closely related methods) to an object which stores the option for the colour (blue, green) as its state. Then I added an instance of this new class as a private readonly variable of the FormDesigner class, which is responsible for 99.8% of all the usages. Since the colours are never mixed up within a form, it makes sense - the form designer has a helper object which 'knows' which colour its shading is. Most of the changes were accomplished by global search-and-replace, and the whole thing was ready to commit after 2 hours.