5

I'm currently working with a very large system and have been asked to add an additional parameter to a method that's called from over 200 different places directly.

The method signature looks something like this:

public static bool SendMessageAndLog(long id, long userId, string message, string cc="", params Attachment[] attachments) 
{ ... }

I need to be able to log the id of the event this message is associated with. I'm kinda stuck between 2 solutions:

  1. Creating a new method that does exactly the same thing but takes the event ID as well, stripping the old method and making it call the new method
  2. Adding an optional parameter for the event id and going through and using named parameters for the 200 calls which seems like a massive pain

Are there any more other potential solutions to this? What would be the best practice in this case keeping in mind that I can't refactor this too much.

  • why can’t you refactor this too much? It seems to be a prime candidate for it. – Telastyn Mar 6 at 6:41
  • 2
    From where will this new event ID parameter be passed? From all those 200 different places? Or just from a few, new places? – Doc Brown Mar 6 at 6:55
  • @DocBrown Just a few places that require it – GoodPie Mar 6 at 7:18
  • @Telastyn I would love to and proposed different options for refactoring it but unfortunately I have other tasks that have a greater priority. – GoodPie Mar 6 at 7:19
  • 4
    Your two solutions don't seem to match. The first solution makes sense when this new parameter is not going to be used by (at least) a subset of the current usages (so they can continue using the old signature); but then the second solution implicitly asserts that the current usages need to be updated to the new signature anyway. Which is it? – Flater Mar 6 at 14:59
24

Create a new method with the additional parameter and move all the code from the original method in it, making it use the additional parameter the way you are supposed to. Leave the original method (without the additional parameter) which would just call the new method with some default value of the additional parameter, basically making it a wrapper method. This, of course, works only if there is a default value of the additional parameter. If there is not, but the value differs from call to call, then you have to bite the bullet and just refactor the whole thing, which should not be too hard, because once you break the signature by adding the additional parameter, the compiler will tell you where else you need to fix it.

In your particular case, if you have the message-event id mapping which does not change for the duration of the program execution, you can just create the map as an internal attribute of the class containing the method, and the first line of your method would be to read the map and get the ID of the event based on the message. You then use it the way you want to. That way, you do not need to change the method signature. It should not even be such a performance hit, because I do not expect that the map will have too many elements.

| improve this answer | |
  • 2
    If you do go the Wrapper Method route, keeping the old method to maintain compatibility with a previously-shipped codebase, I suggest marking the old method as "deprecated" so that people don't build new code on top of the old method that you, ideally, want to get rid of. – Phill W. Mar 9 at 14:59
3

While I agree with the accepted answer, there's also another two approaches you could adopt depending on the requirements.

1. Method Overloading

You can have multiple methods in a c# class that have the same name and perform the same action but take different parameters. Although admittedly not the best approach for your own scenario due to duplication of code. The method that fires will depend on the parameters you provide.

public static bool SendMessageAndLog(long id, long userId, string message, string cc="", 
                                     params Attachment[] attachments) 
{ ... }

// overloaded method
public static bool SendMessageAndLog(long id, long userId, string message,  string cc="", 
                                     int eventId, params Attachment[] attachments) 
{ ... }

2. Nullable parameters / Assign a default parameter value

This approach allows you to keep the one method, however all the parameters that you assign a default value to must be the right-most parameters in the method. This means if you need to add an additional parameter later on and can't re-write the code calling the method, then the new parameter must also have a default value assigned (Which can be null if you wish.).

You'd also need to remove params from the parameters in your case so again not ideal! Although I'm not a fan of params when you can just pass in a named list or array in its place anyway.

public static bool SendMessageAndLog(long id, long userId, string message, string cc="", 
                                     Attachment[] attachments, int? eventId = null) 
{ ... }

//You can now call this method in two ways...

//1)eventId param not provided so it will default to null inside method block
SendMessageAndLog(id, userId, message, attachments);

//2)eventId value provided so it will be whatever value you provide.
SendMessageAndLog(id, userid, message, attachments, eventId);   
| improve this answer | |
  • 1
    I like more #2, just add a simple default value check inside your method and you're good to go – Ivan García Topete Mar 6 at 15:34
  • 5
    Please keep in mind, adding an additional default parameter to a public method can have unexpected consequences. (1) Client modules compiled against the older definition will not be able to call the new method. (2) Changes to the default value will not apply automatically to previously compiled client modules. – psaxton Mar 6 at 22:04
  • Note that going from params Attachment[] attachments to Attachment[] attachments is a breaking change (in usage). It might not effect you much if you can change the calling code, but it's something to keep in mind. – Caramiriel Mar 11 at 12:12

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.