C++ thread safe log file writer

Question

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?

Answer 1

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.

Answer 2

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.