Simple log writer class

Question

I've written a simple log writing class. My code is also hosted on my Github.

Specifically, I am looking for criticism on these items:

1. Resource management. What is a more correct/safe way to obtaining a hold to stream-like objects?
2. Clarity. Is my code as clear as it could be?
3. Error handling. I've only had a few classes in C++ and every single class has only ever required me to catch std::exception. I have never needed to handle specific exceptions.
4. Code organization
5. Naming conventions

//
// Logger.h
// Kyle Shores
// 11/19/2016
//
// Defines the interface for writing data to a log file.

#include <string>
#include <iostream>
#include <fstream>
#include <chrono>
#include <ctime>
#include <exception>
#include <boost/filesystem.hpp>
#include <vector>

namespace fs = boost::filesystem;

class Logger
{
public:
    Logger();
    ~Logger();

    // all open functions return true on a successful open or true if the logger is already open
    // false otherwise
    bool open();
    bool open(const char* pPath);
    bool open(const fs::path pPath);

    // write a single message
    void write_log(const std::string& pMessage);
    // write a group of messages
    void write_log(const std::vector<std::string>& pMessages);

    // return true if the stream was successfully closed or if the stream was already closed
    // false otherwise
    bool close();

private:
    bool create_and_open();

private:
    fs::path mDir;
    fs::path mFile;
    fs::path mFull_path;
    std::ofstream mStream;

    // default values
    std::string mDefault_dir = "./";
    std::string mDefault_file = "log.log";
};


//
// Logger.cpp
// Kyle Shores
// 11/19/2016
//
// Implements the Logger class which writes data to a log file.

#include "Logger.hpp"    

//=============================================================
//                      Public
//=============================================================

Logger::Logger()
{
}

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

//==================================
//        Open functions
//==================================
bool Logger::open()
{
    if (!mStream.is_open())
    {
        // use the default file and directory
        mDir = mDefault_dir;
        mFile = mDefault_file;
        return create_and_open();
    }
    return true;
}

bool Logger::open(const char* pPath)
{
    if (!mStream.is_open())
    {
        fs::path path{pPath};
        mDir = path.parent_path();
        mFile = path.filename();
        return create_and_open();
    }
    return true;
}

bool Logger::open(const fs::path pPath)
{
    if (!mStream.is_open())
    {
        mDir = pPath.parent_path();
        mFile = pPath.filename();
        return create_and_open();
    }
    return true;
}

//==================================
//        Write log functions
//==================================

void Logger::write_log(const std::string& pMessage)
{
    try
    {
        // get the current time
        std::chrono::system_clock::time_point now = std::chrono::system_clock::now();
        time_t tt = std::chrono::system_clock::to_time_t(now);

        // strip the newline passed back by ctime
        auto tt_stripped = std::strtok( ctime(&tt), "\n");
        mStream << "[" << tt_stripped << "] : " << pMessage << std::endl;
    }
    catch (std::exception& e)
    {
        std::cerr << "Error while trying to log a message:\n" << e.what() << std::endl;
    }
}

void Logger::write_log(const std::vector<std::string>& pMessages)
{
    try
    {
        // get the current time
        std::chrono::system_clock::time_point now = std::chrono::system_clock::now();
        time_t tt = std::chrono::system_clock::to_time_t(now);

        // strip the newline passed back by ctime
        auto tt_stripped = std::strtok( ctime(&tt), "\n");
        size_t whitespace = strlen(tt_stripped) + 5;
                                            //^ + 5 to align the text

        for (int i = 0; i < pMessages.size(); ++i)
        {
            if (i == 0)
                mStream << "[" << tt_stripped << "] : " << pMessages[i] << std::endl;
            else
                mStream << std::string(whitespace, ' ') << pMessages[i] << std::endl;
        }
    }
    catch (std::exception& e)
    {
        std::cerr << "Error while trying to log a message:\n" << e.what() << std::endl;
    }
}

//==================================
//        Close function
//==================================

bool Logger::close()
{
    try
    {
        if (mStream.is_open())
        {
            mStream.flush();
            mStream.close();
            return !mStream.is_open();
        }
        else
            return false;
    }
    catch (std::exception& e)
    {
            std::cerr << "Error: " << e.what() <<std::endl;
            return false;
    }
}

//=============================================================
//                      Private
//=============================================================

bool Logger::create_and_open()
{
    // open a log with the specified directory and file
    try
    { 
        // try to create any intermediate directories requested to host the file
        if (!fs::exists(mDir))
        {
            if (!fs::create_directories(mDir))
            {
                std::cerr << "Error creating directories to open the file." << std::endl;
                return false;
            }
        }

        // create the full path and open the file
        mFull_path = mDir / mFile;
        mStream.open(mFull_path.native(), std::ios_base::app | std::ios_base::in);

        if (!mStream.is_open())
        {
            std::cerr << "Error opening log file (" << mFull_path << "):\n" << std::endl;
            return false;
        }

        return true;
    }
    catch (std::exception& e)
    {
        std::cerr << "Error opening log file (" << mFile << "):\n" << e.what() << std::endl;
        return false;
    }
}

Answer 1

The code is pretty good, and quite readable. Here are some of the things I noticed:

Header guards

Your Logger.h needs header guards:

#ifndef LOGGER_H_
#define LOGGER_H_

// rest of header file

#endif

Alternatively, you could use #pragma once at the top of the file, although it is theoretically less portable.

Leaking namespaces

namespace fs = boost::filesystem;

This is in the header file. That means that anyone who includes Logger.h now has a namespace fs which is boost::filesystem. This is bad, as if I am a user of your library, I would not expect it to define a fs namespace.

To fix this, move that line of code to your Logger.cpp, and use boost::filesystem in place of fs within your header file.

Namespaces

Your Logger should be in a namespace of some sort. When defining small utilities such as this, I'm usually lazy and simply place it in a utils namespace.

Invert the try in your close function

bool Logger::close()
{
    try
    {
        if (mStream.is_open())
        {
            mStream.flush();
            mStream.close();
            return !mStream.is_open();
        }
        else
            return false;
    }
    catch (std::exception& e)
    {
            std::cerr << "Error: " << e.what() <<std::endl;
            return false;
    }
}

There's a little overhead to a try-catch block; I would have expected this to look like this:

bool Logger::close()
{
    if (mStream.is_open())
    {
        try
        {
            mStream.flush();
            mStream.close();
            return !mStream.is_open();
        }
        catch (std::exception& e)
        {
            std::cerr << "Error: " << e.what() <<std::endl;
            return false;
        }
    }
    return false;
}

Only include what you need

I strongly recommend that the header file only includes what it needs, and not what the cpp file needs as well. This can improve compilation time for anyone who uses your Logger.h. I commented out the header files you don't use in your Logger.h:

#include <string>
// #include <iostream>
#include <fstream>
// #include <chrono>
// #include <ctime>
// #include <exception>
#include <boost/filesystem.hpp>
#include <vector>

Then you'd just make your Logger.cpp to look like this:

#include "Logger.hpp"

#include <iostream>
#include <chrono>
#include <ctime>
#include <exception>

What is a logger?

The class you wrote isn't actually a logger in what people would expect. Yes, it allows you to log messages to a file with a timestamp, but most loggers out there also have something known as "logging levels", such as Debug, Warning, and Error. The idea is that you can tell the logger what level to log, and it would ignore anything which doesn't fit under that level. Your logger doesn't do this.

Good comments

Your comments are, for the most part, excellent. They mostly help with understanding the code and what the functions do, and there are not too many comments. Good job.