49

This hit me recently on a project I was working on. Most people are familiar with property recursion:

public int Test 
{
   get { return this.test; }
   set { this.Test = value; }
}
private int test;

You accidentally put an upper-case T in this setter, and you've opened yourself up to a StackoverflowException. What's worse is if you've not defined it, often visual studio will auto-correct the casing for you to the invalid state.

I did something similar however in a constructor recently:

public TestClass(int test)
{
    this.Test = Test;
}

Unfortunately here you don't get a StackOverflowException, now you've got a programming error. In my case this value was passed to a WebService that instead used a default value (which wasn't 0) which caused me to miss the fact I had incorrectly assigned it. Integration tests all passed because this service didn't say

"Hey you forgot this really important field!"

What steps can I take to avoid this sort of behaviour? I've always been advised against defining variables like the following, and I don't like them personally, but I can't think of any other options:

private int _test;
private int mTest;

EDIT

Reasons that the underscore or m prefix are undesirable normally that I can think of are:

  • Readability
  • Slightly more difficult to scroll through members if you're inheriting from 3rd party classes as you get a mix of styles.
11
  • 2
    Excellent question. I guess you have to then question why not prefixing your private variables grants you any benefits other than readability. Because I would put this as a +1 in the prefixing pros column. Oct 30, 2013 at 13:07
  • 36
    I'm a big fan of private member variables being prefixed with '_'. ReSharper seems to agree as well. Oct 30, 2013 at 13:10
  • 2
    @golergka the problem with your scenario is that you have a giant unknown method, for smaller methods the extra cruft will damage readability Oct 30, 2013 at 16:25
  • 2
    @ratchetfreak giant unknown method is just an exaggerated example. But it can happen nevertheless; in real life, code is not always perfect, and while we can have insufficient resources to fix it, we can still make imperfect code more manageable. Also; how exactly these prefixes can damage readability? I really don't quite understand.
    – Max Yankov
    Oct 30, 2013 at 16:29
  • 4
    I also agree with others on the fact that if you are not using _ because it makes it less readable but that if as a result of that you are having coding errors instead then you need to reorder your priorities. ;-)
    – Chris
    Oct 30, 2013 at 17:27

5 Answers 5

64

Best way is to use "Auto implemented properties" here.

public int Test { get; set; }

If not possible to use "Auto implemented properties" for some reason use _ prefix(I don't prefer though).

If you also don't prefer to use some prefixes, then you have other option. You don't have to write the property code by hand. Let the IDE do it for you; that way you can avoid careless mistakes. (I don't know how I missed this in original answer)

Just type

private int test;

Select the field, Right click Refactor->Encapsulate Field. IDE will generate property snippet for you as below.

public int Test
{
    get { return test; }
    set { test = value; }
}

You don't need to bother clicking the context menu. If you prefer keyboard, shortcut is Ctrl + R + E.

Or get a Resharper, It will point your silly mistake immediately.

3
  • 2
    I didn't downvote, but I'm pretty sure there may be a number of scenarios where it's necessary to have direct access to the backing variable - or, someone wants to perform some small action in the setting function. That said, I like to make use of these when I can.
    – Katana314
    Oct 30, 2013 at 13:29
  • 12
    @Katana314 Of course, I've mentioned that also in my answer isn't it? If not possible to use "Auto implemented properties" for some reason use _ prefix Oct 30, 2013 at 13:31
  • 4
    Yup...so I'm out of reasons why someone would downvote you. It's not like auto-properties are a bad practice in general.
    – Katana314
    Oct 30, 2013 at 13:36
34

Integration tests all passed

Then they weren't exhaustive enough tests. If there's an error that wasn't discovered by the tests, then you've got another test to write.

That's really the only automated solution here. The compiler isn't going to complain, because the code is structurally and syntactically correct. It's just not logically correct at runtime.

You can define naming standards, even use tools like StyleCop to attempt to enforce those standards. That would probably allow you to cover a lot, though it's not an ironclad solution and errors can still get through. Personally I agree with you that decorating variable names is unsightly in the code. Perhaps in some cases it's a valid tradeoff?

Ultimately, automated tests are your defense against these kinds of errors. At its simplest, if an error gets through your tests and into production then the response should be:

  1. Write a test to reproduce the error.
  2. Fix the error.
  3. Use the test to validate the fix.

Granted, that only covers that one case, not every property definition in your code. But if this is happening a lot then you may have a personnel problem and not a technical problem. Somebody on the team is, well, sloppy. The solution to that problem may not be a technical one.

3
  • In this case it was code that I wrote, I believe I'd covered enough unit test wise and covered the main integration approach. What I think I was missing was writing some more Mocking based tests so I could verify other behaviours. I Mocked up an end-point but there was a bit of in-between going on which maybe I needed to Mock and verify set-up was occurring correctly. While I agree actually most of our solutions currently have no automated testing :( and it'd be nice to have a standard while we figure out the best way to roll more testing out.
    – Ian
    Oct 30, 2013 at 13:17
  • 1
    +1 for PEBKAC possibility! Not every problem in our technological world is easily solved by technology. Some of those problems, however, are easily solved by people.
    – Brian S
    Oct 30, 2013 at 14:30
  • 1
    David, while really your answer I think is the best solution for my case - it probably doesn't quite fit my question as a whole and help the rest of the community as much - just wanted to acknowledge that even though I'll accept the other answer you did at least read/address the whole question.
    – Ian
    Oct 31, 2013 at 8:46
6

Use code snippets.

For every property backed by a private field, use a custom code snippet you have created, instead of writing it up from scratch or letting IntelliSense do the job (poorly).

After all, this problem is about conventions and discipline, rather than language design. The case sensitive nature of C# and the subperfect code completion in Visual Studio are the reason we make these mistakes, not our lack of knowledge and design.

You best bet here is to eliminate the chance of accidents and having a predefined way of writing these repetitive things correctly is the best way to go. It also is much more automated compared to remembering conventions and enforcing them by hand.

There is a default code snippet in Visual Studio for this. Type propfull and hit Tab, then specify the instance variable name and the property name and you're good to go.

1
  • Indeed - although this doesn't catch all problems. In my case a constructor also called an issue.
    – Ian
    Oct 30, 2013 at 14:56
3

In some cases you cannot get around setters and getters. But maybe you don't need the setters and getters if you follow the Tell, Don't Ask principle? It basically says to prefer having the object that has the data do the work, not some other object query a lot from the data object, make decisions, and then write data back to the data object. See http://martinfowler.com/bliki/TellDontAsk.html

1
  • 1
    This is a very useful concept but it doesn't address the problem posted in the question.
    – Carbine
    Nov 6, 2013 at 4:29
2

Could you not just write a test to cover this?

int constructorValue = 4;
TestClass test = new TestClass(constructorValue);
Assert.Equals(test.Test, constructorValue);

You may not want to write tests immediately to cover yourself from future wobbles, but you've found a bug, why not protect yourself from it again?

For the record, if I need a private field to store the value for a pulic getter/setter, I always underscore it. There's just something an underscore that screams privacy!

public string Test
{
    get { return _test; }
    set { _test = value; }
}

private string _test;
1
  • Indeed - actually I could and probably should. I guess what would be more interesting is to try and use a tool to automatically generate these sort of tests to verify behaviour.
    – Ian
    Nov 6, 2013 at 10:10

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.