5

Is this code ambiguous or is it perfectly fine (approved by standards/has consistent behavior for any compilers in existence)?

struct SCustomData {
    int nCode;
    int nSum;
    int nIndex;
    SCustomData(int nCode, int nSum, int nIndex)
        : nCode(nCode)
        , nSum(nSum)
        , nIndex(nIndex)
    {}
};

edit:
yes, I am referring to the fact that the member variables have the same name with formal parameters of the constructor.

3
  • 3
    I'll be honest, I had to read it twice... I'd probably be an unhappy developer if I had to keep reading the same code twice. Jan 29, 2010 at 10:59
  • 3
    Sorry, but what's supposed to be ambiguous about that? Looks straightforward to me. Jan 29, 2010 at 11:16
  • What is ambiguous (for the human) is to have the same identifier for both an attribute and a parameter... Jan 29, 2010 at 16:14

6 Answers 6

6

No, in this case there are no ambiguity, but consider following:

struct SCustomData {
//...
    void SetCode(int nCode)
    {
            //OOPS!!! Here we do nothing!
            //nCode = nCode;

            //This works but still error prone
            this->nCode = nCode;
    }
};

You should draw attention to one of existing coding styles. For instance General Naming Rule in Google C++ Coding Styles or read excellent book "C++ Coding Standards: 101 Rules, Guidelines, and Best Practices" by Herb Sutter and Andrei Alexandrescu.

2
  • Simple rule I use all the time: first letter uppercase for parameters, lowercase for private fields. Jan 29, 2010 at 13:23
  • I prefer Sutter's style: camel Case - for parameters (code - in this example), camel Case and underscore ( _ ) - for private fields (code_ - in this example). Jan 29, 2010 at 13:30
4

Your example is unambiguous (to me), but it's not good practise, because it can quickly become as ambiguous as hell.

It's a long while since I've written any C++ in anger, so I'm guessing what the following will do.
Do you KNOW what it will do? Are you sure?

class Noddy
{
    int* i;
    Noddy(int *i)
    : i(i)
    {
        if(i == NULL)
            i = new int;
    }
};
7
  • 1
    Yeah, I know what it will do, but I wouldn't be betting money that my colleagues do :) Jan 29, 2010 at 11:18
  • Tell me Tell me Tell me! Is the inner i referring to the 'closest declared': the argument? (The initializer list can obviously only refer to the member)
    – xtofl
    Jan 29, 2010 at 11:34
  • 2
    @xtofl: Correct, to do what it "looks" like it should do, it would need to be "this->i" inside the curly braces {} Jan 29, 2010 at 12:14
  • 4
    @Steve: Yes, but it violates the rule that one should always write code with the assumption that the guy who ends up maintaining it will be a psychopath who knows where you live. Jan 29, 2010 at 16:23
  • 1
    I just mean that if you want to argue that the questioner's code shouldn't be given to a psychopath, argue that. I agree that this other, much worse code shouldn't be given to a psychopath :-) Jan 30, 2010 at 14:55
1

If you're referring to using the same name for members and constructor arguments, then that's absolutely fine. However, you might find some people who insist that it's bad practice for some reason.

If you need to access the members in the constructor body, then you need to be careful - either give the arguments different names, or use this-> to access members.

If you're referring to using pseudoHungarian warts to remind people that integers are integers, then that is technically allowed, but has absolutely no benefits and makes the code much harder to read. Please don't do it.

2
  • 1
    As one of "those people", my reasons are a) the OP had to ask about this - so probably will others reading such code and b) see your second para.
    – anon
    Jan 29, 2010 at 11:34
  • 1
    @Neil: (a) it is not possible to write C++ in such a way that people reading it don't need to know C++, (b) I entirely agree, in a function with a non-empty body, hiding member variables with parameters is asking for trouble. The difference is that in (b), it's easy to accidentally confuse the two even if you know the language well, just because you forgot about the hiding from one minute to the next. This results in bugs. In (a), there's only one plausible intention of the code and the question is whether you happen to be aware that it's legal. This results in an educational moment. Jan 29, 2010 at 15:42
1

In general, I've prefixed instance variables with underscores and named parameters in the constructor without any prefixes. At the very least, this will disambiguate your parameters from your instance variables. It also makes life less hectic if initializing within the body of the constructor.

struct SCustomData {
    int _nCode;
    int _nSum;
    int _nIndex;
    SCustomData(int nCode, int nSum, int nIndex)
        : _nCode(nCode), _nSum(nSum), _nIndex(nIndex)
    {}
};

// Alternatively
struct SCustomData {
    int _nCode;
    SCustomData(int nCode)
    {
        this->_nCode = nCode;
    }
};

I don't like stacking the variables the way it was written in the question. I like to save vertical space, and it's also easier for me to read left-to-right. (This is a personal preference of mine, not a mandatory rule or anything like that.)

1
  • 4
    Unfortunately, identifier names beginning with the underscore are reserved in both C and C++.
    – aib
    Jan 29, 2010 at 13:44
1

I would say that this is perfectly fine.

It is my preferred style for constructors that use the initialization list and don't have any code. I think that it reduces confusion because it is obvious which constructor parameter goes to which member.

0

It is perfectly standard compliant, but there are compilers out there that would not accept member variables having the same name as constructor parameters. In fact, I had to change my open source library for that reason. See this patch

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.