C++ privately constructed class

Question

How can I call a function and keep my constructor private? If I make the class static, I need to declare an object name which the compiler uses to call the constructor, which it cannot if the constructor is private (also the object would be extraneous). Here is the code I am attempting to use (it is not compilable):

I want to keep the constructor private because I will later be doing a lot of checks before adding an object, modifying previous objects when all submitted variables are not unique rather than creating new objects.

#include <iostream>
#include <fstream>
#include <regex>
#include <string>
#include <list>
#include <map>

using namespace std;
using namespace tr1;

class Referral
{
public:
    string url;
    map<string, int> keywords;

    static bool submit(string url, string keyword, int occurrences)
    {
        //if(Referrals.all.size == 0){
        //  Referral(url, keyword, occurrences);
        //}
    }

private:
    list<string> urls;

    Referral(string url, string keyword, int occurrences)
    {
        url = url;
        keywords[keyword] = occurrences;
        Referrals.all.push_back(this);
    }
};

struct All
{
    list<Referral> all;
}Referrals;

int main()
{
    Referral.submit("url", "keyword", 1);
}

Answer 1

While the whole code has some smell around, you can make it work just by making slight changes that are unrelated to your question.

To make it compile, I have removed the regex include (I am not using a compiler with C++0x support) and the 'using namespace tr1'. Move the constructor implementation after the definition of the Referral global object. Change the . for a :: in the main function when you refer to a static method.

// changes...
//#include <regex>
...
//using namespace tr1;
...
class Referral { 
...
    Referral(string url, string keyword, int occurrences); // added ; moved the implementation bellow the Referrals variable definition
...
struct All  {
...
} Referrals;

// added constructor implementation here (Referrals global must be defined before use):
Referral::Referral(string url, string keyword, int occurrences)
{
   url = url;
   keywords[keyword] = occurrences;
   Referrals.all.push_back(*this); // added dereference, this is of type Referral*, not Referral
}

int main()
{
   Referral::submit("url","keyword",1);
}

Now, from a design point of view the code has a stench to it. If really want to have a global list where you add your Referral objects, consider making it a private static attribute of the Referral class so that you can have a little more control over it (only methods in the Referral class could break the contents). Make all your attributes private and provide only accessors to the functionality that user code will need (read-only access can suffice in most cases). Use initialization lists in your constructors, and initialize all members there in the same order they appear in the class definition.

With all that fixed, it still has some smell to it. The static function creates an instance of the class but the constructor is the one that includes itself in the map (??) It would make a little more sense if the constructor did not interact with the map, and the submit() method would create the object and then include it in the list...

I think you might benefit from expressing what you intend to do, many people here will help you both with design choices and reasons for them.

Answer 2

First, you need to make Submit a static function. Then you can just say

Referral::Submit( url, keyword, occurrences );

without creating a Referral instance.

Then, in your Submit function, you're only creating a temporary Referral object that disappears almost immediately. Probably what you want to do is create an instance dynamically with new. Depending on how you want to manage this, you may want to move the code pushing onto the list into Submit.

Lastly, I would make your list of Referral instances a static member variable rather than how you have it now.

(Also, passing those string arguments by reference would probably be a good idea.)

Answer 3

Based on your main code I think what you're shooting for is a singleton, which would look something like:

class Referral
{
private:
    Referral()
    {
        //...
    }

public:
    static Referral& instance()
    {
        static Referral instance_s;
        return instance_s;
    }

    bool submit(string url, string keyword, int occurrences)
    {
        //...
    }
};

Then your call in main would look like:

int main()
{
    Referral::instance().submit("url", "keyword", 1);
}

Another possibility is that you're looking to keep a list of Referrals around, in which case you can use a struct and a list of them to accomplish what you're looking for:

struct Referral
{
    Referral(string url, string keyword, int occurrences) :
        url_m(url), keyword_m(keyword), occurrences_m(occurrences)
    { }

    string url_m;
    string keyword_m;
    int    occurrences_m;
};

typedef std::vector<Referral> ReferralSet;

Then your call in main would look like:

int main()
{
    ReferralSet set;

    set.push_back(Referral("url", "keyword", 1));
}

Answer 4

What's wrong with having a private constructor and a static factory method?

class Example {
  Example() { ... }
public:
  static Example CreateExample() {
    return Example();
  }
};