Is this the example of singleton pattern?

Question

Is this the example of singleton pattern ?? If it is not then what can be the problems if we use this class as a logger. (Of-course it is not a full flexed logger )

#include <iostream>
#include <fstream>
using namespace std;


class logger
{
    private:
        static ofstream myfile;

        void openfile()
        {
            myfile.open ("example.txt");
        }
        void closefile()
        {
            myfile.close();
        }
    public:     
        void logit(string str)
        {
            if (myfile.is_open() == 1)
            {
                myfile << str << endl;
            }
            else
            {   
                openfile();
                myfile << str << endl;
            }   
        }
};

ofstream logger::myfile;
int main () 
{
    logger l;
    l.logit ("log from vod application");
    logger l2;
            l.logit ("log from guide application");

    logger l3;
    l1.logit ("log from application 3-1");
    l1.logit ("log from application 3-2");

            return 0;
}

Any discussion will be helpful.

Devesh

It's not a singleton, the problem you may experience is that if the file is removed/moved/otherwise tampered with while the problem is running it will not be recreated. But this problem doesn't have much to do with its singletonness or the lack thereof. — Michael Krelin - hacker, Oct 29 '13 at 12:02
It's more or less the Meyer's version of singleton, where the unique instance is myfile, and you don't have a getInstance method, but a direct access to logit(). It would be better if logit was static — Stephane Rolland, Oct 29 '13 at 12:03
@MichaelKrelin so even if it is not a singleton class, we can use it for logging without major issue. There are lots of discussion suggested that you should use singleton for logger? what exactly we gain if use singleton for logger, if we can achieve the same without singleton? — Devesh Agrawal, Oct 29 '13 at 12:07
@DeveshAgrawal, who cares if it fits the pattern? It solves the problem singleton is supposed to solve. Though why would one want to make logit non-static *in this case* is beyond me. — Michael Krelin - hacker, Oct 29 '13 at 12:37
@MichaelKrelin-hacker: Moving/deleting the file is unlikely to have any affect on the program (assuming a modern OS). The program will have a file desriptor open and thus the underlying inode will not be removed until the application closes its file descriptor, and moving a file has no affect on the iNode. — Martin York, Oct 29 '13 at 14:36
@LokiAstari, it will not affect the program, it will lead to the lack of log records ;) — Michael Krelin - hacker, Oct 29 '13 at 14:45
@MichaelKrelin-hacker: Only after the application exits. You can always get them from `/proc//fd/` while the program is running. — Martin York, Oct 29 '13 at 15:00
@MichaelKrelin-hacker: I don't consider that a programing problem. That's a user education problem. PEBKAC. Not matter what you do pragmatically you can not stop that kind of error. If somebody really wants to delete log files or even system log files you can't really stop them. You have to education them not too. Or don't give them access to the box. — Martin York, Oct 29 '13 at 17:30
@LokiAstari, depending on the nature of a program, it may be desirable that it recreates the log. And many programs do. I consider it a problem, though it's not really dramatic. When I mentioned that I didn't expect such a long discussion over it ;) — Michael Krelin - hacker, Oct 29 '13 at 18:11

wl2776 · Answer 1 · 2013-10-29T12:32:19.803

3

No, this is not a singleton.

To make a singleton, you have to make the constructor private. Your class doesn't declare any constructor, therefore, the compiler will generate default ones. This is bad, since there will be a copy constructor and assignment operator, which simply copy all members bit by bit. Bad things usually happen, if you copy the handle of opened file or pointer to allocated memory and try to operate with copies.

class logger {
    static logger* the_logger;
    // other private members

    logger() : the_logger(NULL)
        { /*logger construction*/} 
public:
    static logger* logger::instance(const string& filename) {
        if (the_logger == NULL) {
            the_logger = new logger(/*arguments*/);
        }
        return the_logger;
    }

    static void cleanup(void) {
        delete the_logger;
    }
    /* other public members*/
    void debug(...)
}


int main(void) 
{
    logger::instance()->debug(blah-blah-blah);
    logger::cleanup();
}

For simplicity, I've skipped the code, related to the simultaneous access to shared resources (file descriptor or output stream).

Also, if I remember correctly, your code will not compile, since static members can only be accessed with static member functions.

edited Oct 29 '13 at 12:32

answered Oct 29 '13 at 12:08

wl2776

4,099
4
35
77

This isn't the best way to implement the singleton anti-pattern in C++. Use `static logger& logger::instance() { static logger the_logger(/*arguments*/); return logger; }`. – Simple Oct 29 '13 at 13:07
What if I would want to tune parameters of the instance? In proposed implementation it is declared as static variable inside the function. – wl2776 Oct 29 '13 at 13:34
Then pass them as parameters. The point was you shouldn't be using `new` or pointers. `static logger& logger::instance(std::string const& filename) { static logger the_logger(filename); return logger; }`. – Simple Oct 29 '13 at 13:40
That is a really bad implementation of a singleton in C++ (Especially since you have to manually delete it (PS the cleanup should set the_logger to NULL so next time it can create a new one)). For a better design see http://stackoverflow.com/a/1008289/14065 – Martin York Oct 29 '13 at 14:52

score 1 · Answer 2 · answered Oct 29 '13 at 12:03

1

It can't possibly be a singleton:

there're 3 instances of it in your code1
the class has a public constructor that simply creates a new instance

answered Oct 29 '13 at 12:03

Paul Evans

27,315
3
37
54

1

It can be singleton just fine. It's just not adhering to the singleton *pattern*, which ensures that it *must* be a singleton :-) – Nikos C. Oct 29 '13 at 12:05
Then it is a wrapper around the singleton (`logger::myfile`) – wl2776 Oct 29 '13 at 12:39
logger:myfile is not a singleton either: the singleton pattern restricts a class to a single instance, and provides a global access to that instance. – Pete Oct 29 '13 at 12:45
@Pete, yes, you're right. I meant a wrapper around something that must be a singleton. `logger::myfile` is `ofstream`. – wl2776 Oct 29 '13 at 13:46

score 1 · Answer 3 · answered Oct 29 '13 at 12:42

1

As others have said, it is not a Singleton. It is a Monostate: http://c2.com/cgi/wiki?MonostatePattern, which is related but different.

answered Oct 29 '13 at 12:42

Pete

1,241
2
9
21

Is this the example of singleton pattern?

3 Answers3