2

I am writing in C#, but this question may apply to other languages as well.

public class Test { int a = 10; // I created 'a' here public void M() { int a = 20; // I forgot that I already have 'a' in the class and I initial it again ; // do other stuff with "a"... } }

This way isn't against the declaring space rule, so the compiler will have no problem with it. I know I could use this.a if I want to access the a outside M() and they are not a variable actually.

My question is:

Is this way could make some people confused by allowing declare the same name in a sub scope? Will it be harder to debug or doing code review?

Improve this question
7
  • Why I immediately got downvote after like 3 sec? Am not supposed to post it here? Commented Jan 9, 2019 at 13:41
  • 6
    Use a linter that warns against this. Commented Jan 9, 2019 at 13:50
  • 4
    @cj9435042 If the class or method is so long that it becomes hard to avoid the same names, either your class is too big, or your names are too generic. Commented Jan 9, 2019 at 14:58
  • 2
    you probably got downvoted because there's a lot of people on StackExchange who see bad code in a question and inexplicably downvote the question even if the question is literally 'is my code bad, and if so, why' Commented Jan 9, 2019 at 16:17
  • 1
    to answer your question - yes it will make bugs more likely and debugging more difficult, but if you use decent variable names rather than just 'a' then conflicts are much less likely. Also, use a linting tool, as suggested below. Commented Jan 9, 2019 at 16:20

3 Answers 3

Reset to default
4

One can avoid these types of scenarios by using a tool like Resharper or other type of "clean code" / "linter" tool.

Most of these tools spot errors as soon as you type them in and will underline the offending lines of code and offer suggestions on to how to remedy.

Most projects I have been on, automatically run the linter/clean code tool prior to check in so developers are not allowed to check in code like the example cited above.

If you don't use a tool, then it's the responsibility of the developer to understand variable scope and not code in this fashion.

Personally, a good "clean code" / "linter" tool is a must for any non-trivial project as it will promote correct and consistent coding practices and allows one to spend more brain power solving the problem at hand. Also, it helps when refactoring and or modifying because one might be in a large class when someone else had already declared that variable and you are unaware of it.

Improve this answer
2

Such name shadowing can also be useful at times. For example you cite C# and for that language the design team are considering relaxing the name shadowing rules for local functions (scroll down to the "Static local functions" section and read the third question and answer for reference).

However, it remains the case that for many situations variable/field shadowing can indeed cause confusion and can make the code harder to read and debug. To that end, there are a number of techniques you can employ to minimise it:

  1. Use longer, meaningful names. Sure a is just an example in your code, but folk that regularly use short, often abbreviated names run into this problem more often. If your field and variable were something like someSortOfIntField and aCompletelyDifferentInt, then collisions are less likely.
  2. Use a naming convention to minimise name collisions. For example in C#, prefixing fields with _ is a common convention. _a and a are clearly visibly different and so reduce the risk of confusion.
  3. Use this to reference fields. I personally hate this approach, but others really like it. And code checking/linter tools are available that can enforce such conventions.
  4. Use tooling that detects such shadowing and reports it as a warning or error. For example, the popular C# tool, ReSharper, will warn on such name shadowing by default.
Improve this answer
0

As a general rule, we should declare variables in the innermost scope that is applicable, i.e. do not use a wider scope than necessary.

If a is being declared as an instance variable yet used as a local variable by the methods — i.e. always re-initializing it within the methods (not looking at the previous method's value) — then the instance variable is not maintaining (object) state between method invocations, and such a variable is being declared in a scope wider than necessary.  Under these circumstances, I would find the member variable at issue rather than the local variable that shadows it.

Another general rule that instance members should have the same lifetime as the object itself — if we find instance members that only come into play after certain state or conditions, that suggests we have another object (e.g. two or more objects are being conflated).

On the other hand, if the a instance variable is indeed carrying object state (i.e. is is used across/in-between methods invocations), then when a method (that because it is using a shadowing local variable of the same name) fails to use and/or update the instance variable, your tests ought to notice that something is amiss.  If they don't either insufficient tests or the variable wasn't actually holding object state.

Improve this answer

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.