tags:

views:

68

answers:

2

The example below may not be problematic as is, but it should be enough to illustrate a point. Imagine that there is a lot more work than trimming going on.

public string Thingy 
{
    set 
    {
        // I guess we can throw a null reference exception here on null.
        value = value.Trim(); // Well, imagine that there is so much processing to do
        this.thingy = value;  // That this.thingy = value.Trim() would not fit on one line
        ...

So, if the assignment has to take two lines, then I either have to abusereuse the parameter, or create a temporary variable. I am not a big fan of temporary variables. On the other hand, I am not a fan of convoluted code. I did not include an example where a function is involved, but I am sure you can imagine it. One concern I have is if a function accepted a string and the parameter was "abused", and then someone changed the signature to ref in both places - this ought to mess things up, but ... who would knowingly make such a change if it already worked without a ref? Seems like it is their responsibility in this case. If I mess with the value of value, am I doing something non-trivial under the hood? If you think that both approaches are acceptable, then which do you prefer and why?

Thanks.

Edit: Here is what I mean when I say I am not a fan of temp variables. I do not like code like this:

string userName = userBox.Text;
if (userName.Length < 5) {
    MessageBox.Show("The user name " + userName + " that you entered is too short.");
    ....

Again, this may not be the best way to communicate a problem to the user, but it is just an illustration. The variable userName is unnecessary in my strong opinion in this case. I am not always against temporary variables, but when their use is very limited and they do not save that much typing, I strongly prefer not to use them.

+3  A: 

First off, it's not a big deal.

But I would introduce a temp variable here. It costs nothing and is less prone to errors. Imagine someone has to maintain the code later. Better if value only has 1 meaning and purpose.

And don't call it temp, call it cleanedValue or something.

Henk Holterman
When you write "it costs nothing", do you mean "it costs very little"?
Hamish Grubijan
+3  A: 

It is a good practice not to change the values of incoming parameters, even if you technically can. Don't touch the value.

I am not a big fan of temporary variables.

Well, programming is largely about creating temporary variables all over the place, reading and assigning values. You'd better start to love them. :)

One more remark regarding properties. Although you could technically put a lot of logic there, it is recommended to keep properties simple and try not to use any code that could throw exceptions. A need to call other functions may indicate that this property is better be made a method or that there is some initialization code needed somewhere. Just rethink what you're doing and whether it does really look like a property.

Developer Art
Agreed, but this kind of question deserves a 'why isn't ít a good practice'.
Henk Holterman
@Hamish Grubijan: It is not a "claim" but a generally known best practice. I'm not aware of any references to that matter but you are welcome to google for them.
Developer Art
Ok, will look. I did elaborate on temp variables though.
Hamish Grubijan
@Hamish Grubijan, This is elaborated upon in the Framework Design Guidelines book from Micrsoft (http://www.amazon.com/Framework-Design-Guidelines-Conventions-Libraries/dp/0321246756). However, I personally know at MS they treat them as guidelines and not rules.
tster