4
\$\begingroup\$

I made this class (MFC lib) to write a log files. Eventually I want to implement this code in a thread safe way.

Log.h

#pragma once
class CLog
{
public:
    CLog();
    ~CLog();
    int CLog::Log(CString log);
    void CLog::TWriteAllToLog();
};

Log.cpp

#include "stdafx.h"
#include "Log.h"
#include <queue>
#include <mutex>

using namespace std;

const unsigned long long MAX_LOG_FILE_SIZE{ 1024 * 1024 * 4 };
CStdioFile logFile;
CString _fileName;
CFileStatus fileStatus;
CFileException exceptionEx;
UINT_PTR shownTimer;
BOOL fileOpened;
mutex m;

queue<CString> dataLog;
void DoSafeWrite(CString log);
void PrintQueue();


CLog::CLog()
{
    /* want to implement auto flush method to write all 
       queue items into file in here with timer like method.

       but at the moment manually call the function "TWriteAllToLog()" to write log file.
    */
}

CLog::~CLog()
{
}


/*
    usage:

    CString str;
    str.Format(L"This is log test");

    CLog _log; //create object
    _log.Log(str); //call to Log function
*/
int CLog::Log(CString log)
{
    try{
        CTime t = CTime::GetCurrentTime();
        CString TStamp = t.Format("%a, %b %d, %Y  %H:%M:%S -> ");
        CString newLine("\n");
        log = TStamp + log + newLine;
        dataLog.push(log);      
        return 1;
    }
    catch (...){
        return -1;
    }

}
void CLog::TWriteAllToLog(){
    PrintQueue();
}
void PrintQueue()
{
    while (!dataLog.empty())
    {
        m.lock();
        CString str;
        CString &data = dataLog.front();        
        DoSafeWrite(data);
        dataLog.pop();
        m.unlock();
    }
}
CString GetFileName() //crate a file name
{
    COleDateTime dtDate = COleDateTime::GetCurrentTime();
    CString file = dtDate.Format(L"Log_%d_%m_%Y__%H_%M_%S.log");
    return file;
}

CString GetLastFileName(CString filename, bool write)
{
    const int FILE_CONTENT_LEN = 255;
    CString logfiledb{ "_FILE_.db" };
    char fileContent[FILE_CONTENT_LEN];
    CStdioFile lastLog;
    CFileStatus status;
    CString line;
    CFileException exception;
    try
    {
        if (CFile::GetStatus(logfiledb, status)) // craete log file and write the current access file into db
        {
            lastLog.Open(logfiledb, CFile::modeCreate | CFile::modeWrite | CFile::modeNoTruncate);
            lastLog.WriteString(filename);
            lastLog.Flush();
            lastLog.Close();
            return filename;
        }

    }
    catch (CFileException *e)
    {
        lastLog.Close();
    }
}
CString GetLastFileName(CString filename) //Read the '_FILE_.db' and get the last file name
{

    const int FILE_CONTENT_LEN = 255;
    CString logfiledb{ "_FILE_.db" };
    char fileContent[FILE_CONTENT_LEN];
    CStdioFile lastLog;
    CFileStatus status;
    CString line;
    CFileException exception;
    try
    {
        if (CFile::GetStatus(logfiledb, status)) //check file exsist
        {
            if (status.m_size > 5) //check file have alrady contain file name : too lazy method
            {
                if (lastLog.Open(logfiledb, CFile::modeRead, &exception))
                {
                    lastLog.ReadString(line);
                    lastLog.Close();
                    return line;
                }
            }
            else // if file not write the filename
            {
                lastLog.Open(logfiledb, CFile::modeCreate | CFile::modeWrite | CFile::modeNoTruncate);
                lastLog.WriteString(filename);
                lastLog.Flush();
                lastLog.Close();
                return filename;
            }
        }
        else //if file not exsist crate and write the file name
        {
            lastLog.Open(logfiledb, CFile::modeCreate | CFile::modeWrite | CFile::modeNoTruncate);

            lastLog.WriteString(filename);
            lastLog.Flush();
            lastLog.Close();
            return filename;
        }

    }
    catch (CFileException *e)
    {
        lastLog.Close();
    }
}
void DoSafeWrite(CString log)
{
    _fileName = GetLastFileName(GetFileName()); //get the file name
    try
    {
        if (CFile::GetStatus(_fileName, fileStatus)) //check file exist
        {
            unsigned long long fileSize = fileStatus.m_size;
            if (fileSize >= MAX_LOG_FILE_SIZE) //check size bound
            {
                _fileName = GetLastFileName(GetFileName(), true); //if file size not exceed get the last writed file name
                fileOpened = logFile.Open(_fileName, CFile::modeCreate | CFile::modeWrite); 
                if (fileOpened)
                {
                    logFile.SeekToEnd();
                    logFile.WriteString(log);
                    logFile.Flush();
                    logFile.Close();
                }
                else
                {
                    TRACE("Can't open file %s, error = %u\n", _fileName, exceptionEx.m_cause);
                }
            }
            else
            { //write file if size exceed
                fileOpened = logFile.Open(_fileName, CFile::modeCreate | CFile::modeWrite | CFile::modeNoTruncate);
                if (fileOpened)
                {
                    logFile.SeekToEnd();
                    logFile.WriteString(log);
                    logFile.Flush();
                    logFile.Close();
                }
                else
                {
                    TRACE("Can't open file %s, error = %u\n", _fileName, exceptionEx.m_cause);
                }
            }
        }
        else
        { //if file not found create new file and write
            fileOpened = logFile.Open(_fileName, CFile::modeCreate | CFile::modeWrite | CFile::modeNoTruncate);
            if (fileOpened)
            {
                logFile.SeekToEnd();
                logFile.WriteString(log);
                logFile.Flush();
                logFile.Close();
            }
            else
            {
                TRACE("Can't open file %s, error = %u\n", _fileName, exceptionEx.m_cause);
            }
        }
    }
    catch (CFileException *e)
    {
        logFile.Close();
    }
}

I want to implement above code in a thread safe way and also write the log file in PrintQueue() function with using a timer-like function in the CLog constructor. Can the existing code be improved?

\$\endgroup\$
6
  • 1
    \$\begingroup\$ You're asking for feature requests in your comments. That's not what we do. We can review the given code, but expect the code as-is to be working as intended. Please take a look at the help center. \$\endgroup\$
    – Mast
    Jul 13, 2016 at 11:13
  • \$\begingroup\$ I've tried to remove the off-topicness as much as possible. For you next question, please take a look at the How to ask - FAQ. \$\endgroup\$
    – Mast
    Jul 13, 2016 at 11:24
  • \$\begingroup\$ @Mast Understand & I will.. \$\endgroup\$
    – Elshan
    Jul 13, 2016 at 11:29
  • \$\begingroup\$ Take a look at log4cpp (log4cpp.sourceforge.net). It's a well-written library that allows flexible logging and it is thread-safe. It will save you some time. \$\endgroup\$
    – Natalia
    Jul 13, 2016 at 13:41
  • \$\begingroup\$ The queue is accessed in an unsynchronized way in two places. \$\endgroup\$
    – usr
    Jul 13, 2016 at 16:58

2 Answers 2

5
\$\begingroup\$
const unsigned long long MAX_LOG_FILE_SIZE{ 1024 * 1024 * 4 };

That's 3 magic numbers right there. The result is 4194304, you don't need a long long for that. A long usually holds till 2147483647.

Don't write custom destructors if they're empty anyway. Your compiler will take care of that.

You've included "stdafx.h". You don't need it.

void DoSafeWrite(CString log);

Make it an int instead and let it return whether the write was successful.

Namespaces

using namespace std; is considered bad practice. Short code is not a requirement in C++, clear code is preferred.

Trace

TRACE is a debugging macro and should NOT be used for released software. User proper exceptions and/or use status codes to indicate what happened. TRACE is not what you want.

\$\endgroup\$
7
  • \$\begingroup\$ Good suggestions.Any hint for using this code in simultaneously logging environment ? (using threads, and write logs) \$\endgroup\$
    – Elshan
    Jul 13, 2016 at 11:14
  • 2
    \$\begingroup\$ "stdafx.h" is often setup as the precompiled header pipeline in VC projects, if you don't include it the compiler barfs. Obviously you can setup your projects around this but it's not usually as simple as removing the include. \$\endgroup\$
    – forsvarir
    Jul 13, 2016 at 11:19
  • \$\begingroup\$ @forsvarir If you start with a 'blank' project, the compiler should be fine. Perhaps this happens with MFC projects specifically? \$\endgroup\$
    – Mast
    Jul 13, 2016 at 11:21
  • 1
    \$\begingroup\$ @devopsEMK Clean-up your code first and make sure it works fine single-threaded. Worry about multi-threading after that. It would be unfortunate if bad-habits made your multi-threaded version a pain to maintain et al. Feature requests are out of scope on this site, we do not change what your code does. If you have specific concerns about working code, you can add those to your next question. We call those follow-ups. \$\endgroup\$
    – Mast
    Jul 13, 2016 at 11:28
  • \$\begingroup\$ @Mast A completely blank project might get away with it, but any other project (console, windows etc) unless you untick the precompiled headers tick box VC helps you out by adding 'stdafx.cpp' setup to create the compiled header and configures all other files to use the precompiled header. It's a lot faster when you're doing MFC development, but it's not unique to it. You get this error: msdn.microsoft.com/query/… \$\endgroup\$
    – forsvarir
    Jul 13, 2016 at 11:29
4
\$\begingroup\$

Cleanup old code

Two of your methods have:

const int FILE_CONTENT_LEN = 255;
char fileContent[FILE_CONTENT_LEN];

You don't use either of them. There's at least one other variable that's not used (str in printQueue). When you refactor your code, don't forget to get rid of the bits that aren't needed anymore. Leaving them just adds clutter.

File management

You're opening and closing your log file for every log message you're writing. This is quite inefficient and can significantly limit throughput.

Queue management

Adding to a queue is a lot faster than writing to a file. You aren't limiting your log queue however. When I've implemented concurrent loggers, I've usually put some kind of size limit on the write queue. If you start hitting it then you need to make a decision if you want to discard log messages, or force the caller to slow down until the log catches up. If you don't have some kind of strategy, then you run the risk of your logger eating up all the memory available to your application.

Globals

You have a lot of globals declared in Log.cpp. If you're doing it to hide them from the rest of your code, then you should be declaring them as static, otherwise they can be imported into other code through the extern keyword. It would however be better to put them in a class. If you only want one instance of the variables then make the class a singleton. I'd suggest something like LogWriter. This singleton could have a thread function associated with it which is responsible for taking log entries from the queue and writing them out to the log file. This would also make it obvious who is supposed to call TWriteAllToLog which isn't clear at the moment.

m

m is a really bad name for a global variable. One letter variables are ok when you can see the context, but with globals you can't. 'm' is also often used in someway or another to indicate a class member variable.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.