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.
SmallShaded. If the parameter is
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
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.
- Change the enums to remove the 3 values that currently produce
shading and add in:
- Add another parameter to indicate the colour of the shading. This
parameter is meaningless unless a shading option for
- Separate the functionality of shading out of this method and put it
in another method, which must be called in addition to
- Introduce a static method to set the static colour variable before each form is printed.
- 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
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.
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.
TextInCellin two: one for normal cells and a separate function for header cells. Currently,
TextInCellseems to have too many responsibilities.