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;
 }
}

• sandun dhammika: This comment does not belongs here, because this is not a code review. But logger is more than a file writer. It's always a good idea to reuse system specific ( in Linux it's syslog) instead of handcoded logger. Because it provide many features, example non-blocking, idle time file writes, thread safty, memory management etc. Industry loggers are more complicated.

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.