Is it bad practice to reuse method parameters?

Question

There are times when I will need to modify a value passed into a method from within the method itself. An example would be sanitizing a string such as this method here:

void SanitizeName(string Name)
{
    Name = Name.ToUpper();

    //now do something here with name
}

This is purely harmless since the Name argument is not being passed in by reference. However, if, for some reason, a developer in the future decides to have all values passed in by ref, any sanitizing of the string will affect the value outside the method which could have detrimental results.

Therefore, instead of reassigning to the argument itself, I always create a local copy like so:

void SanitizeName(string Name)
{
    var SanitizedName = Name.ToUpper();

    //now do something here with name
}

This ensures that changing thow the value is passed in will never affect the goings on outside the method, but I am wondering if I am being excessively paranoid about this.

Answer 1

I think it depends on your coding conventions in your project.

I personally let eclipse automatically add the final keyword to every variable and parameter. This way you see at the first glance if a parameter is reused.

In the project at my job we do not recommend to reuse parameters, but if you just want to call e.g. .trim() or set a default in a null case, we reuse the parameter most of the times, because introducing a new variable is in such cases less readable than the reuse of the parameter.

You should really not reuse a parameter to store a very different content because the name would no longer refer to its content. But this is true for every reassigment of a variable and not restricted to parameters.

So get together with your team and formulate coding conventions which cover this matter.

Answer 2

If you use a static code analyzer for security purposes, it may get confused, and think you haven't validated or sanitized the input parameter variable prior to use. If you use Name in a SQL query, for example, it might claim a SQL injection vulnerability, which would cost you time in explaining away. That is bad. On the other hand, using a distinctly named variable for the sanitized input, without actually sanitizing the input, is a quick way to quiet naïve code analyzers (false negative vulnerability finding).

Answer 3

The answer to this depends 100% on who is going to read your code. What styles do they find most helpful?

I have found the most general case is that one avoids reassigning values to function arguments because too many developers have mental models of how function calling works which assume that you never do this. This problem can be exacerbated by debuggers which print the values of the arguments you call each function with. This information is technically not correct if you edit the arguments, and that can lead to some strange frustrations.

That being said, mental models change. In your particular development environment, it may be desirable to have name be "the best representation of the name at this point in time." It may be desirable to more explicitly link the variables you are working on to their arguments, even though you've done some modifications to their values along the way. There may even be substantial runtime advantages to reusing some particular variables rather than creating more bulky objects. After all, when you're working with 4GB strings, it is nice to minimize the number of extra copies you have to make!

Answer 4

I have a completely diffrent problem with your code example:

Your method's name is SanitizeName. In this case, I expect it to sanitize a name; because that is, what you tell the reader of your function.

The only thing, which you function should do, is sanitizing a given name. Without reading your code, I would expect the following:

string SanitizeName(string Name)
{
    //somehow sanitize the name
    return Result;
}

But you imply, that your method does a bit more than only sanitizng. That's a code smell and should be avoided.

Your question is not about: Is it bad practice to reuse method parameters? It is more: are sideeffects and unexected behaviour bad practice?

The answer to that is clearly: yes!

This is purely harmless since the Name argument is not being passed in by reference. However, if, for some reason, a developer in the future decides to have all values passed in by ref, any sanitizing of the string will affect the value outside the method which could have detrimental results

You mislead your reader by not returning anything. Your method name clearly indicates, that you are doing something on Name. So, where should the result go? By your function signature void reads I use Name but neither do any harm to id, nor tell you about the result (explicitely). Perhaps there is an exception thrown, maybe not; but Name is not altered. This is the semantic opposite to your method name.

The problem is less the reuse than the sideeffects. To prevent sideeffects do not reuse a variable. If you have no sideeffects there is no problem.