CODEWITHSUNDEEP
c++classvariables
FirePrism
FirePrism

Reputation: 45

Set public class variable in constructor

I want to write a public class variable with a value that is given when the constructor is called. I have a solution that I thought was correct until someone told me it might be wrong (the line with "Foo::alpha = alpha;").
So when I run 

Foo bar(g, 0);
bar.run();

then I want that the value 0 is written to the class variable so I can access it later in the run method.

Foo.h

class Foo: public Framework::Centrality {
public:
    double alpha;
    Foo(const Graph& G, double alpha=1);
    void run() override;
};

Foo.cpp

Foo::Foo(const Graph& G, double alpha) : Centrality(G) {
    if (alpha >= 0 && alpha <= 1) {
        Foo::alpha = alpha;
    } else {
        throw std::runtime_error("...");
    }
}

Upvotes: 2

Views: 436

Answers (4)

rubenvb
rubenvb

Reputation: 76785

What you're doing is technically correct, but you should really use the member initialization list for this:

#include <stdexcept>

class Base {};

class Ace :  Base
{
  public:
    Ace(double alpha = 0) : Base(), alpha(alpha)
    {
      if(alpha < 0 || alpha > 1)
        throw std::runtime_error("");
    }
    double alpha;
}

You can use a different name for the constructor parameter alpha, but it is not required by the language.

Upvotes: 3

orfdorf
orfdorf

Reputation: 1010

It's not wrong. It's just not pretty. When the constructor's parameter has the same name as a data member on the class, you have a few options:

1) Use the member initializer list, which according to the standard, must do what you're intending for it to do here:

Foo::Foo(const Graph& G, double alpha) :
    Centrality(G), alpha(alpha)
{
    if(alpha < 0 || alpha > 1)
        throw std::runtime_error("...");
}

2) Fully qualify the member alpha, as you are doing:

Foo::Foo(const Graph& G, double alpha) :
    Centrality(G)
{
    if(alpha >= 0 && alpha <=1 )
        Foo::alpha = alpha;
    else
        throw std::runtime_error("...");
}

3) Use the this pointer to specify the member:

Foo::Foo(const Graph& G, double alpha) :
    Centrality(G)
{
    if(alpha >= 0 && alpha <=1 )
        this->alpha = alpha;
    else
        throw std::runtime_error("...");
}

4) Name them something different. A common convention is to prefix the names of data members with m or m_:

class Foo : public Framework::Centrality
{
    /* other stuff */

 private:
    double m_Alpha;
};


Foo::Foo(const Graph& G, double alpha) :
    Centrality(G)
{
    if(alpha >= 0 && alpha <=1 )
        m_Alpha = alpha;
    else
        throw std::runtime_error("...");
}

Another common thing I see is prefixing the paramter in the cpp with an underscore:

class Foo : public Framework::Centraliy
{
    Foo(const Graph& G, double alpha=1);
};

Foo::Foo(const Graph& G, double _alpha) :
    Centrality(G)
{
    ...
}

5) Or if using the pImpl idiom, (I like to name my Impl member m):

class Foo : public Framework::Centrality
{
    /* other stuff */

 private:
    struct Impl;
    std::unique_ptr<Impl> m;
};

// In foo.cpp
struct Foo::Impl
{
    double alpha;
}

Foo::Foo(const Graph& G, double alpha) :
    Centrality(G),
    m(make_unique<Impl>())
{
    if(alpha >= 0 && alpha <=1 )
        m->alpha = alpha;
    else
        throw std::runtime_error("...");
}

Upvotes: 0

nbro
nbro

Reputation: 15878

You can simply assign the value to alpha using this-> without using Foo:::

Foo::Foo(const Graph& G, double alpha) : Centrality(G) {
    if (alpha >= 0 && alpha <= 1) {
        this->alpha = alpha;
    } else {
        throw std::runtime_error("...");
    }
}

Foo:: is usually used to access static variables or methods or to refer to a method of the class during the implementation. Your code is not incorrect, but usually it's not used to initialise a non-static member public variable.

Upvotes: 1

Lightness Races in Orbit
Lightness Races in Orbit

Reputation: 385395

What you're doing is fine.

Some people may claim that Foo::alpha looks like it names a static member and may therefore be confusing, but my response to that is that such people would only think so because they do not know C++.

Foo::alpha unambiguously names the member alpha in the class Foo; since you're in a member function, that member can be (and is) a non-static member.

It would be more conventional to write this->alpha or rename the constructor argument, but what you're doing is fine.

Upvotes: 3

Related Questions