Sign in to follow this  

Please rate my code, be as critical as you like...

This topic is 3487 days old which is more than the 365 day threshold we allow for new replies. Please post a new topic.

If you intended to correct an error in the post then please contact us.

Recommended Posts

HI, I need some c++ programmers to rate my code here, I wrote a logger class. It is not finished as I have a couple of questions, however here is what I have so far. It is capable of storing messages with a time and message, using an std::queue. Logger.h:
class Logger;

#ifndef class_Logger_h
#define class_Logger_h 1
#include <queue>
#include <string>
#include <ctime>

#include "Logging.h"

class LogEntry
{
    public:
        LogEntry(const std::string &m)
        {
            time = std::time(0);
            message = m;
        }
        
        std::string GetAllContents()
        {
            std::stringstream msg; msg<<std::asctime(std::gmtime(&time))<<": "<<message<<"\n\n";
            return std::string(msg.str());
        }
            
        void DumpToConsole()
        {
            std::cout<<GetAllContents();
        }

    private:
        std::string message;
        std::time_t time;
};

class Logger
{
    public:
        Logger():auto_console_echo(false){};
        ~Logger();
        
        void LogMessage(const std::string &m);
        void LogMessage(const std::stringstream &m);
        void LogException(const Exception &e);
        
        void FlushToCommandLine();
        void FlushToFile();
        
        void SetAutoConsoleEcho(bool t){auto_console_echo = t;}
        bool GetAutoConsoleEcho(){return auto_console_echo;}
        
        int GetLength(){return messages.size();}
    private:
        std::queue<LogEntry> messages;
        bool auto_console_echo;
};

#endif





Logger.cpp:

#include "Logging.h"

Logger::~Logger()
{    
    FlushToFile();
}

void Logger::LogMessage(const std::string &m)
{
    LogEntry entry(m);
    if(auto_console_echo)
    {
        entry.DumpToConsole();
    }
    
    messages.push(LogEntry(m));
}

void Logger::LogMessage(const std::stringstream &m)
{
    LogMessage(m.str());
}

void Logger::LogException(Exception e)
{
    std::string n,r;
    e.GetDetails(n, r);
    LogMessage(n+r);
}

void Logger::FlushToCommandLine()
{
    while(messages.size())
    {
        messages.front().DumpToConsole();
        messages.pop();
    }
}

void Logger::FlushToFile()
{

}





I have yet to write the functionality to flush to a file, as I am not sure exactly what to use for this or if its the way to to about it. Also, I want to give it the functionality to echo every message to stdout when it is received, is this a sensible thing to do? It is enabled and disabled by SetAutoConsoleEcho(bool). Also, if I set the parameter of LogException to const exception & e, i get the error message Logger.cpp:36: error: passing 'const Exception' as 'this' argument of 'void Exception::GetDetails(std::string&, std::string&)' discards qualifiers What could be the cause of this? Thanks for any input.

Share this post


Link to post
Share on other sites
getdetails is no const function, thus it cannot be used when your exception is const.
Every function which does not change the state of the class (or returns a this-pointer, and whatnot) should be declared const.


You should state in which way to rate your code - style-wise?

You might want to add a "class" for messages like warning, critical, success, ...
And maybe html output with colors for the file output would be nice to have.

Share this post


Link to post
Share on other sites
Using a const Exception& would be a reference to an Exception you can't change, meaning you can only call its constant functions. Check out the C++ FAQ Lite on const-correctness.

As for the code itself, I don't really have much to say. The only thing that barked at me was why a Logger would know how to/want to log an Exception. I think it's up to the caller to grab the exception, and then log the error if it wants to using the string logging functions.

Share this post


Link to post
Share on other sites
Quote:
Original post by agi_shi
Using a const Exception& would be a reference to an Exception you can't change, meaning you can only call its constant functions. Check out the C++ FAQ Lite on const-correctness.

As for the code itself, I don't really have much to say. The only thing that barked at me was why a Logger would know how to/want to log an Exception. I think it's up to the caller to grab the exception, and then log the error if it wants to using the string logging functions.


Thanks for the advice,

The exception logger was just an idea, when I think about it I realise that exceptions are supposed to be critical, eg.a runtime error. My idea was that if I had an exception, in the 'catch' block I could use logger->LogException(e) or something, to record it that way.


try
{
DoStuff()
}
catch(Exception e)
{
OhNoes();
logger.LogException(e);
}



since not all exceptions result in program termination.

Share this post


Link to post
Share on other sites
Quote:
Original post by speciesUnknown
I have yet to write the functionality to flush to a file, as I am not sure exactly what to use for this or if its the way to to about it. Also, I want to give it the functionality to echo every message to stdout when it is received, is this a sensible thing to do? It is enabled and disabled by SetAutoConsoleEcho(bool).
Why just std::cout? Why not allow it to auto-echo to any stream?
Quote:
Also, if I set the parameter of LogException to const exception & e, i get the error message
Logger.cpp:36: error: passing 'const Exception' as 'this' argument of 'void Exception::GetDetails(std::string&, std::string&)' discards qualifiers


What could be the cause of this?
GetDetails is not const-correct.

The following is just looking at your current implementation, not considering whether the general design could be improved:
// nothing in this header needs this forward declaration, which
// means it's in the wrong place
//class Logger;

#ifndef class_Logger_h
#define class_Logger_h 1
#include <queue>
#include <string>
#include <ctime>

#include "Logging.h"

class LogEntry
{
public:
// use initialiser lists
LogEntry(const std::string &m)
:
message(m),
time(std::time(0))
{
}

std::string GetAllContents()
{
// stringstream is unnecessary here
return std::string(std::asctime(std::gmtime(&time))) + ": " + message + "\n\n";
}

void Dump(std::ostream & stream = std::cout)
{
stream << GetAllContents();
}

private:
std::string message;
std::time_t time;
};

class Logger
{
public:
Logger():auto_console_echo(false){};
~Logger();

void LogMessage(const std::string &m);
// this function does not need to be a member,
// in fact it's not really needed at all
// it doesn't provide any significant utility
//void LogMessage(const std::stringstream &m);
void LogException(const Exception &e);

void FlushToCommandLine();
void FlushToFile();

void SetAutoConsoleEcho(bool t){auto_console_echo = t;}
bool GetAutoConsoleEcho(){return auto_console_echo;}

int GetLength(){return messages.size();}
private:
std::queue<LogEntry> messages;
bool auto_console_echo;
};

#endif

// Err, don't you mean Logger.h?
#include "Logging.h"

Logger::~Logger()
{
FlushToFile();
}

void Logger::LogMessage(const std::string &m)
{
LogEntry entry(m);
if(auto_console_echo)
{
entry.Dump();
}
// why construct a second LogEntry?
messages.push(entry);
}

/*void Logger::LogMessage(const std::stringstream &m)
{
LogMessage(m.str());
}*/


void Logger::LogException(Exception e)
{
// what the heck are n and r?
std::string n,r;
e.GetDetails(n, r);
LogMessage(n+r);
}

void Logger::FlushToCommandLine()
{
// use empty, not size - it may be more efficient
while(!messages.empty())
{
messages.front().Dump();
messages.pop();
}
}

void Logger::FlushToFile()
{

}

Σnigma

Share this post


Link to post
Share on other sites
Obvious question perhaps, but what's the point in a logger which buffers log messages? Don't you typically want them written out ASAP, so that if your program crashes, you've still got all your log information?

Share this post


Link to post
Share on other sites
Logger.h:

#include "Logging.h"

class LogEntry
{
public:

// Initializer lists
LogEntry(const std::string &m) : time(std::time(0)), message(m) {}

// GetAllContents is fundamentally a 'convert-to-string' function.
// Remove it and use 'operator <<' and 'boost::lexical_cast' instead.

// DumpToConsole is useless once you implement 'operator <<'

// Allow access to the printing operator
friend std::ostream & operator<< (std::ostream &, const LogEntry &);

private:

std::string message;
std::time_t time;
};

// Printing...
std::ostream & operator<< (std::ostream & o, const LogEntry & l)
{
return o << std::asctime(std::gmtime(&l.time)) << ": " << l.message << "\n";
}

class Logger
{
public:
Logger():auto_console_echo(false){}
~Logger();

// All these functions involve logging. Why not call them 'log' ?
// Also dropped the stringstream, since it causes useless clutter.
void Log(const std::string &m);
void Log(const Exception &e);

// The two flush functions are basically 'output to ostream'.
void Flush(std::ostream &);

void SetAutoConsoleEcho(bool t){auto_console_echo = t;}
bool GetAutoConsoleEcho(){return auto_console_echo;}

// Use an appropriate return type and make 'const'.
std::vector<LogEntry>::size_type GetLength() const
{
return messages.size();
}

private:
// Your intended functionality, while close to a queue, is actually
// 'traverse and print', then 'clear'. A vector is more adapted.
std::vector<LogEntry> messages;
bool auto_console_echo;
};

#endif


Logger.cpp:

#include "Logging.h"

Logger::~Logger()
{
// Specify the output directly.
Flush(std::cout);
}

void Logger::Log(const std::string &m)
{
LogEntry entry(m);
if(auto_console_echo)
std::cout << m << std::endl;

// Use the already constructed value
messages.push(entry);
}

void Logger::Log(Exception e)
{
std::string n,r;
e.GetDetails(n, r);
Log(n+r);
}

void Logger::Flush(std::ostream & o)
{
// Write in terms of traversal
std::copy(messages.begin(),
messages.end(),
std::ostream_iterator<LogEntry>(o,"\n"));

messages.clear();
}

Share this post


Link to post
Share on other sites
Quote:
Original post by Enigma
Quote:
Original post by speciesUnknown
I have yet to write the functionality to flush to a file, as I am not sure exactly what to use for this or if its the way to to about it. Also, I want to give it the functionality to echo every message to stdout when it is received, is this a sensible thing to do? It is enabled and disabled by SetAutoConsoleEcho(bool).
Why just std::cout? Why not allow it to auto-echo to any stream?
Quote:
Also, if I set the parameter of LogException to const exception & e, i get the error message
Logger.cpp:36: error: passing 'const Exception' as 'this' argument of 'void Exception::GetDetails(std::string&, std::string&)' discards qualifiers


What could be the cause of this?
GetDetails is not const-correct.

The following is just looking at your current implementation, not considering whether the general design could be improved:
*** Source Snippet Removed ***
*** Source Snippet Removed ***
Σnigma


Wow thanks Σnigma, lots to think about here.

Share this post


Link to post
Share on other sites
Quote:
Original post by ToohrVyk
Logger.h:

#include "Logging.h"

class LogEntry
{
public:

// Initializer lists
LogEntry(const std::string &m) : time(std::time(0)), message(m) {}

// GetAllContents is fundamentally a 'convert-to-string' function.
// Remove it and use 'operator <<' and 'boost::lexical_cast' instead.

// DumpToConsole is useless once you implement 'operator <<'

// Allow access to the printing operator
friend std::ostream & operator<< (std::ostream &, const LogEntry &);

private:

std::string message;
std::time_t time;
};

// Printing...
std::ostream & operator<< (std::ostream & o, const LogEntry & l)
{
return o << std::asctime(std::gmtime(&l.time)) << ": " << l.message << "\n";
}

class Logger
{
public:
Logger():auto_console_echo(false){}
~Logger();

// All these functions involve logging. Why not call them 'log' ?
// Also dropped the stringstream, since it causes useless clutter.
void Log(const std::string &m);
void Log(const Exception &e);

// The two flush functions are basically 'output to ostream'.
void Flush(std::ostream &);

void SetAutoConsoleEcho(bool t){auto_console_echo = t;}
bool GetAutoConsoleEcho(){return auto_console_echo;}

// Use an appropriate return type and make 'const'.
std::vector<LogEntry>::size_type GetLength() const
{
return messages.size();
}

private:
// Your intended functionality, while close to a queue, is actually
// 'traverse and print', then 'clear'. A vector is more adapted.
std::vector<LogEntry> messages;
bool auto_console_echo;
};

#endif


Logger.cpp:

#include "Logging.h"

Logger::~Logger()
{
// Specify the output directly.
Flush(std::cout);
}

void Logger::Log(const std::string &m)
{
LogEntry entry(m);
if(auto_console_echo)
std::cout << m << std::endl;

// Use the already constructed value
messages.push(entry);
}

void Logger::Log(Exception e)
{
std::string n,r;
e.GetDetails(n, r);
Log(n+r);
}

void Logger::Flush(std::ostream & o)
{
// Write in terms of traversal
std::copy(messages.begin(),
messages.end(),
std::ostream_iterator<LogEntry>(o,"\n"));

messages.clear();
}


Lots of useful stuff here too, thanks a lot. I'm currently implementing the ostream system.

Share this post


Link to post
Share on other sites

This topic is 3487 days old which is more than the 365 day threshold we allow for new replies. Please post a new topic.

If you intended to correct an error in the post then please contact us.

Create an account or sign in to comment

You need to be a member in order to leave a comment

Create an account

Sign up for a new account in our community. It's easy!

Register a new account

Sign in

Already have an account? Sign in here.

Sign In Now

Sign in to follow this