12

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.

  • 1
    Yes, it's bad practice; and no it's not harmless. The line Name = Name.ToUpper(); makes the code harder to follow in your head as the value of Name changes. Your second example is not only more future-proof, it's easier to reason what it's doing. – David Arno Jun 22 '16 at 11:17
  • 2
    But what about if (param == NULL) param = default_value;? – aragaer Jun 22 '16 at 11:22
  • I think it's not as easy as yes / no. – MrSmith42 Jun 22 '16 at 11:25
  • 3
    If "a developer in the future decides to have all values passed in by ref", I would probably have an argument with that dev, parameter reuse involved or not ;-) Honestly, when one decides to pass a value by ref which was not passed that way beforehand, and so converting local access to a non-local access for some reason, he has always to check the consequences carefully. – Doc Brown Jun 22 '16 at 12:59
  • 2
    I primarily work in a paradigm where we never reassign any variables. Ever. It's funny to imagine someone struggling with whether they should reassign a small minority of variables and then not worry about going hog wild with the rest. – Karl Bielefeldt Jun 23 '16 at 2:33
10

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.

0

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).

  • You could also in most cases use some kind of annotation or special comment to indicate to the code analyzer that this is intended and not a unnoticed risk. – MrSmith42 Jun 29 '16 at 10:03
0

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!

-1

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.

  • 4
    -1 This does not answer the original question. The code provided is just a sample code for showing the question at hand. Please answer the question, instead of "attacking" / reviewing the provided sample code. – Niklas H Jun 23 '16 at 8:45
  • 1
    @NiklasH This answers the question perfectly: if you manipulate the parameter inside the function and as the TO mentioned you accidentally work with a reference instead of a copy, this causes side effects (as I said) and this is bad. In order to prevent that indicate that you are going to alter the variable by carefully choosing the method name (think of Ruby's '!') and/or choosing a return type. If you do not alter the variable, work only with copies. – Thomas Junk Jun 23 '16 at 18:14

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.