3
\$\begingroup\$

I recently started out with C++ and I am working on an application that can watermark entire directories of images at once.

Currently you can call the program with an argument of the target directory where the images are stored and an argument for the path of which watermark image to use.

It works so far but I think my code is a bit messy. I don't really know how to make it more readable and more clean, so I am asking for your help.

I would really appreciate it if people could look at my code and review it. All feedback is welcome.

A link to my GitHub repository: github repo

Main.CPP:

#include <iostream>
#include "FileSystem.h"
#include "Watermarker.h"

int main(int arc, const char** argv)
{
    // Get the given argument for the target directory
    std::string targetDirectory = argv[1];

    // Get the given argument for the watermark file
    std::string watermarkPath = argv[2];

    FileSystem fileSystem;

    // Check if the target directory is valid
    if (!fileSystem.checkDirectory(targetDirectory)) { return 1; }

    // Create an output directory where the new watermarked images will be stored
    std::string path = targetDirectory + "\\watermarked_images";
    fileSystem.createDirectory(path);

    Watermarker marker;

    // Mark all the images in the target directory with the watermarkPath image
    marker.mark(targetDirectory, watermarkPath);
}

FileSystem.h

#pragma once

#include <string>
#include <vector>
#include <boost/filesystem.hpp>

class FileSystem
{
private:
    std::vector<std::string> validExtensions = {
        ".png",
        ".jpg",
        ".tiff",
        ".jpeg"
    };

public:
    void createDirectory(std::string& directory);
    bool checkDirectory(std::string& directory);
    bool checkFile(boost::filesystem::path& filePath);
};

FileSystem.CPP

#include "FileSystem.h"
#include <iostream>
#include <boost/algorithm/string.hpp>

void FileSystem::createDirectory(std::string& directory)
{
    boost::filesystem::create_directory(directory);
}

bool FileSystem::checkDirectory(std::string& directory)
{
    if (boost::filesystem::exists(directory))
    {
        if (!boost::filesystem::is_directory(directory))
        {
            std::cout << "The given path is not a directory.\n";
            return false;
        }
    }
    else
    {
        std::cout << directory << " does not exist\n";
        return false;
    }

    return true;
}

bool FileSystem::checkFile(boost::filesystem::path& filePath)
{
    // Check if the file is a regular file
    if (!boost::filesystem::is_regular_file(filePath)) { return false; }

    // Get the file extension in lower case
    std::string fileExtension = filePath.extension().string();
    boost::to_lower(fileExtension);

    // Check if the file extension matches one of the valid ones
    if (std::find(validExtensions.begin(), validExtensions.end(), fileExtension) != validExtensions.end())
    {
        return true;
    }
    else
    {
        return false;
    }
}

Watermarker.h

#pragma once

#include <iostream>

class Watermarker
{
public:
    void mark(std::string& targetDirectory, std::string& watermarkPath);
};

Watermarker.cpp

#include "Watermarker.h"
#include "FileSystem.h"
#include <boost/filesystem.hpp>
#include <opencv2/core.hpp>
#include <opencv2/highgui.hpp>
#include <opencv2/imgproc.hpp>

void Watermarker::mark(std::string& targetDirectory, std::string& watermarkPath)
{
    boost::filesystem::path path;

    // Loop through all the files in the target directory
    for (auto& entry : boost::filesystem::directory_iterator(targetDirectory))
    {
        // Set the boost filesystem path to the path of the file
        path = entry.path();

        FileSystem fileSystem;
        
        // If the file is not valid, skip it
        if (!fileSystem.checkFile(path)) { continue; }

        // Get the original and watermark image
        cv::Mat image = cv::imread(path.string());
        cv::Mat watermarkImage = cv::imread(watermarkPath, cv::IMREAD_UNCHANGED);

        // Make the watermark image the same size as the original image
        cv::resize(watermarkImage, watermarkImage, image.size(), 0, 0, cv::InterpolationFlags::INTER_LINEAR);

        // Give the images 4 channels
        cv::cvtColor(image, image, cv::COLOR_RGB2RGBA);
        cv::cvtColor(watermarkImage, watermarkImage, cv::COLOR_RGB2RGBA);

        cv::Mat newImage;

        // Add the watermark to the original image
        cv::addWeighted(watermarkImage, 0.3, image, 1, 0.0, newImage);

        // Save the new image to the new watermarked_images folder
        cv::imwrite(std::string(targetDirectory + "\\watermarked_images\\" + path.filename().string()).c_str(), newImage);
    }
}
\$\endgroup\$
1
  • \$\begingroup\$ Sorry for the extra edits - I misread who added the code to the question. It's ready for review! \$\endgroup\$ Commented Feb 4, 2021 at 7:35

2 Answers 2

2
\$\begingroup\$

#pragma once is not standard C++ - use conventional #ifndef/#define include guards until/unless that gets standardised (by that time, Modules will be a better approach, anyway).

This looks strange:

#include <iostream>

class Watermarker
{
public:
    void mark(std::string& targetDirectory, std::string& watermarkPath);
};

Why include <iostream>? We don't use anything from there. More importantly, we're missing an include of <string>.

Why is mark() a non-static member function? It doesn't require any object state, nor even any class state. It can be a free function.

Why do we pass mutable references to the paths? I don't think there's a good reason the caller would want either of those strings to be modified, so just take a copy (or use a const reference if the implementation won't modify the values).

class FileSystem

That's a confusing name, given that we use boost::filesystem in the same code.

int main(int arc, const char** argv)
{
    // Get the given argument for the target directory
    std::string targetDirectory = argv[1];

    // Get the given argument for the watermark file
    std::string watermarkPath = argv[2];

Conventionally, we call the first argument argc, not arc. More important is that we cannot access argv[1] and argv[2] unless we know argc is at least 3. And if it's not exactly 3, we should be informing the user (via std::cerr) that the usage is incorrect, and exiting with a non-zero status (e.g. EXIT_FAILURE, from <cstdlib>).

    if (!fileSystem.checkFile(path)) { continue; }

    // Get the original and watermark image
    cv::Mat image = cv::imread(path.string());

There's a gap between the check and the read where anything could happen in a multi-process environment. And given that we never checked whether we had permission to read the file, cv::imread() could fail even if nothing changed in that time. Read the fine manual:

If the image cannot be read (because of missing file, improper permissions, unsupported or invalid format), the function returns an empty matrix (Mat::data==NULL).

It's probably better to just remove FileSystem::checkFile() and rely on OpenCV to report whether each file is readable.

    cv::imwrite(std::string(targetDirectory + "\\watermarked_images\\" + path.filename().string()).c_str(), newImage);

Here's another place where we forgot to check whether the operation succeeded. (In fairness, the example in the manual also makes this mistake). It's poor practice to continue silently when we didn't achieve what the user asked for.

Also here, we have a string fragment "\\watermarked_images\\" that has to agree with that in the calling code. Firstly, it looks like \ is being assumed to be a path separator, which is needlessly non-portable (especially given that we're using boost::filesystem). Secondly, the destination path ought to be passed in (and would be better if it could be specified as a program option).

\$\endgroup\$
1
\$\begingroup\$

#pragma once:

If you're making a library that needs to work for any compiler on any platform, you need proper include guards. For something like this #pragma once is the way to go. (It's easier and less error prone, and supported by all the major modern compilers).

[Other than this I mainly agree with Toby Speight's answer ;).]


#include order:

#include <iostream>
#include "FileSystem.h"
#include "Watermarker.h"

Conventional include ordering is to put local includes at the top, third party library includes next, and standard library includes last:

#include "FileSystem.h"
#include "Watermarker.h"

#include <boost/filesystem.hpp> // added for illustration

#include <iostream>

Our local headers should #include everything they use themselves. With this ordering we'll get a compiler error if we've forgotten to #include something in the local header.

e.g. if FileSystem.h depends on <iostream>, but forgets to include it, we want to always get a compiler error when we try to use FileSystem.h in a .cpp file (so we can fix the issue). If we include <iostream> first in the .cpp file, we won't get a compiler error here, but might do at some arbitrary time in the future when we don't happen to include <iostream> too.


class Filesystem:

std::vector<std::string> validExtensions should be static, since it isn't specific to a class instance. In fact, this class has no instance state (member variables), so it doesn't need to be a class. We could perhaps make it a namespace instead.

I'd probably just put all the code for this application in one namespace though, rather than having a separate namespace for a few utilities.

Arguably, we could inline the contents of these functions into the functions that call them without losing much readability.


class Watermarker:

Again, this class has no members, so it doesn't need to be a class.

While it's true that there's no point checking if a file exists, or is readable before we actually try to open the file, we should still check that our iterator points to a file, not a directory, and that it has one of the extensions we care about.

[Another slight disagreement with Toby Speight's answer.]

So checkFile() is necessary. But I do think that it but should be split into two parts, e.g.: isFile(path) and hasExtension(path, validExtensions);.

I think we could split the mark function into two parts. One function (processDir()) to deal with iterating the directory (we could just inline the file and extension check here). A second function addWatermark() to do the actual watermarking with OpenCV.


possible performance improvement:

We read the same watermark image file over and over again.

We can read the image and convert to RGBA just once. Then we can copy and resize it each time, which is probably faster than reading from disk.


project structure:

While separating parts of a project into different files is generally a very good thing, it can actually make things less readable for a very small application like this!

validExtensions, for example, is application specific, and quite important, but hard to find in a separate file. Meanwhile Watermarker.cpp also depends on FileSystem.h (and those valid extensions), which makes it less readable (because we have to jump around the codebase to understand it) and less reusable.

Perhaps, in this case, it would be better to put everything in main.cpp, something like this:

#include <boost/algorithm/string.hpp>
#include <boost/filesystem.hpp>

#include <opencv2/core.hpp>
#include <opencv2/highgui.hpp>
#include <opencv2/imgproc.hpp>

#include <algorithm>
#include <cstdlib>
#include <initializer_list>
#include <iostream>
#include <string>

namespace fs = boost::filesystem;

namespace wtr
{

    void addWatermark(std::string const& targetFile, std::string const& outputFile, std::string const& watermarkFile)
    {
        cv::Mat original = cv::imread(targetFile); // todo: handle failure!
        cv::cvtColor(original, original, cv::COLOR_RGB2RGBA);

        cv::Mat watermark = cv::imread(watermarkFile, cv::IMREAD_UNCHANGED); // todo: handle failure!
        cv::resize(watermark, watermark, image.size(), 0, 0, cv::InterpolationFlags::INTER_LINEAR);
        cv::cvtColor(watermark, watermark, cv::COLOR_RGB2RGBA);

        cv::Mat composite;
        cv::addWeighted(watermark, 0.3, original, 1, 0.0, composite); // todo: make 0.3 a function argument!

        cv::imwrite(outputFile, composite); // todo: handle failure!
    }

    template<class It, class T>
    bool contains(It begin, It end, T const& value)
    {
        return std::find(begin, end, value) != end;
    }

    void processDir(std::string const& targetDir, std::string const& outputDir, std::string const& watermarkFile)
    {
        for (auto& entry : boost::filesystem::directory_iterator(targetDir))
        {
            fs::path path = entry.path();

            if (!fs::is_regular_file(path))
                continue;

            auto validExtensions = { ".png", ".jpg", ".jpeg", ".tiff" };
            if (!contains(validExtensions.begin(), validExtensions.end(), boost::to_lower_copy(path.extension().string())))
                continue;
            
            addWatermark(path.string(), outputDir + path.filename().string(), watermarkFile);
        }
    }

} // wtr

int main(int argc, const char** argv)
{
    // todo: check argc is correct!!!
    std::string targetDir = argv[1];
    std::string watermarkFile = argv[2];

    if (!fs::is_directory(targetDir)) // note: only one check needed - if the file doesn't exist, it won't be a directory!
    {
        std::cout << "The given path is not a directory.\n";
        return EXIT_FAILURE;
    }

    std::string outputDir = targetDir + "\\watermarked_images";
    fs::create_directory(outputDir);
    // todo: handle failure!!!

    wtr::processDir(targetDir, outputDir, watermarkFile); // note: pass in output directory path instead of recalculating it inside the function.
}

[note: not tested]

If our code-base grows, or we want to reuse the functions or move them to a library, it's easy to separate parts out. For example, now addWatermark only depends on the standard library and OpenCV (so it could be grouped with other OpenCV utility functions), and contains could similarly be moved to a separate file.

But... until then, YAGNI.


\$\endgroup\$
1
  • 1
    \$\begingroup\$ I do disagree about using the filename as indicator of whether something is an image - OpenCV is a lot more reliable at detecting that! But I guess it's really for the asker to decide what's wanted - all files matching that pattern, or all files containing raster images? (I went with the description, and you with the code). \$\endgroup\$ Commented Feb 4, 2021 at 13:10

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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