Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Normalize constructor parameter names #1427

Merged
merged 2 commits into from
Mar 12, 2024
Merged

Normalize constructor parameter names #1427

merged 2 commits into from
Mar 12, 2024

Conversation

hollasch
Copy link
Collaborator

In order to avoid confusion between constructor parameter names and member variables, we either used alternate names for the parameters or prefixed the parameter names with an underscore. For example:

class foo {
  public:
    foo(int _i, bool semaphore, short bar) : i(_i), flag(semaphore)
    {
        baz = bar;
    }
   private:
     int i;
     bool flag;
     short baz;
};

However, this is unnecessary. Constructor initializer lists are unambiguous, as the assigned item always refers to the class member, and the assigned value is always from the parameter (unless prefixed with "this->").

Inside the constructor body, parameter names override member variables, and "this->" can be used to indicate the class member in the case of name collision.

Thus, the above code can be rewritten as this:

class foo {
  public:
    foo(int i, bool flag, short baz) : i(i), flag(flag)
    {
        this->baz = baz;
    }
   private:
     int i;
     bool flag;
     short baz;
};

I've taken advantage of this to clarify names used in constructors throughout the codebase. Mostly for constructor parameter names, sometimes for member variable names.

In order to avoid confusion between constructor parameter names and
member variables, we either used alternate names for the parameters or
prefixed the parameter names with an underscore. For example:

    class foo {
      public:
        foo(int _i, bool semaphore, short bar) : i(_i), flag(semaphore)
        {
            baz = bar;
        }
       private:
         int i;
         bool flag;
         short baz;
    };

However, this is unnecessary. Constructor initializer lists are
unambiguous, as the assigned item always refers to the class member, and
the assigned value is always from the parameter (unless prefixed with
"this->").

Inside the constructor body, parameter names override member variables,
and "this->" can be used to indicate the class member in the case of
name collision.

Thus, the above code can be rewritten as this:

    class foo {
      public:
        foo(int i, bool flag, short baz) : i(i), flag(flag)
        {
            this->baz = baz;
        }
       private:
         int i;
         bool flag;
         short baz;
    };

I've taken advantage of this to clarify names used in constructors
throughout the codebase. Mostly for constructor parameter names,
sometimes for member variable names.
@hollasch hollasch added this to the v4.0.0-alpha.2 milestone Mar 11, 2024
@hollasch hollasch requested a review from a team March 11, 2024 06:59
@hollasch hollasch self-assigned this Mar 11, 2024
Copy link
Contributor

@rupsis rupsis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1 Subjective opinion. Otherwise the changes make sense, and go a long way to unify the code.

@hollasch
Copy link
Collaborator Author

The problem is that col more typically refers to a column. And yes, c often refers to a char. color is out because it's a type name. (Personally, I prefer leading capitals on my type names, but that's not the convention here.

"color" actually isn't a great name, because it makes the common mistake of naming something after its kind/type, instead of what it is specifically. I'll revisit.

@hollasch hollasch merged commit ace8e94 into dev Mar 12, 2024
@hollasch hollasch deleted the ctor-names branch March 12, 2024 02:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants