File validator configured using XML

Question

I am making an application to check for any files that does not follow a specific pattern defined in a XML file. The application works well, but the source code is not the greatest, and memory management might slow things down a little.

So I'm asking for a review of my code and to especially give me some general tips about optimizing a application, I am not asking for anyone to optimize the application for me or anything but it would certainly help to get some steps to optimize my application and how to organize my code properly.

I believe it can be done in a more cleaner way than I have done it.

Again this question is not necessarily specific to my application only, I just want some tips on what can be done to optimize my code, and even other applications. So before releasing my first version of this application I need to optimize it, and I don't really know how.

I have a fully function application but it is in debug stage where I try to find memory leaks and ways of making things faster, but I need a little help.

The source can be downloaded here but the main source file is probably not too long to put here.

"ncv.cpp"

#define _CRT_SECURE_NO_WARNINGS     // This program is currently purely made functional, and is not yet secure.

#include <tinyxml2.h>
#include <argtable2.h>

#include "ncv.hpp"
#include "fsys.h"

#include <cstdlib>
#include <iostream>
#include <filesystem>

#if defined( _MSC_VER )
    #define WIN32_LEAN_AND_MEAN
    #include <windows.h>
#endif

using namespace ncv;

/* Define constant strings */
const std::string *NCV_VERSION = new std::string("1.0.06.8");
const std::string *HELP_HEADER = new std::string("Naming Convention Validator Tool, or NCV Tool Version " + *NCV_VERSION + "\n"
                              + "Purpose: Validates whether or not the file(s) follow the naming convention CADS4_VEHICLE_%Date%_< OPTIONAL >.\n\n");
int main(int argc, char* argv[])
{
    /* global arg_xxx structs */
    struct arg_lit  *help;
    struct arg_lit  *r;
    struct arg_lit  *vb;
    struct arg_file *c;
    struct arg_file *input;
    struct arg_end  *end;

    void* args[] = {
        help = arg_lit0("hH", "help", "Display this help screen and the version of NCV Tool."),
        r = arg_lit0("rR", "recursion", "If one or more folders are entered as the file args, subfolders will be read."),
        vb = arg_lit0("vV", "verbose", "Enable verbose output."),
        c = arg_file0("cC", "config", "<file>", "Specify a XML configuration file to use rather than the default one."),
        input = arg_filen(NULL, NULL, "<file>", 1, 512, "Input files and/or folders."),
        end = arg_end(10),
    };
    if (arg_nullcheck(args)){
        printf("error: insufficient memory\n");
        return IO_ERROR;
    }
    c->filename[0] = "config/config.xml";

    int nerrs = arg_parse(argc, argv, args);
    // Ignore any errors, display usage.
    if (help->count > 0){
        ncv::printUsage(args);
        return SUCCESS;
    }
    if (nerrs > 0){
        arg_print_errors(stdout, end, "NCV Tool");
        printf("\nUsage: \"NCV Tool\".exe");
        arg_print_syntax(stdout, args, "\n");
        return ARGV_PARSE_ERROR;
    }
    const char* config = c->filename[0];

    bool recursive = ((r->count) ? true : false);
    bool verbose = ((vb->count) ? true : false);

    if (verbose)
    {
        printf("Include sub-directories = %s\n", ((r->count) ? "YES" : "NO"));
        printf("Verbose output          = %s\n", ((vb->count) ? "YES" : "NO"));
        printf("Configuration file used = %s\n", config);
        printf("\n");
    }

    // If not the configuration XML file can be opened, exit!
    FILE* fp = fopen( config, "r" );
    if ( !fp ) {
        printf( "Error opening configuration file, does it still exist?\n");
        return IO_ERROR;
    }
    fclose( fp );

    ncv::str_vec_t* v = new ncv::str_vec_t();

    // Load the configuration XML file.
    if (verbose)
        printf("Loading configuration file: %s\n", config);
    tinyxml2::XMLDocument* doc = new tinyxml2::XMLDocument();
    doc->LoadFile( config );
    int errorID = doc->ErrorID();
    if(!errorID){
        tinyxml2::XMLElement *rootnode = doc->FirstChildElement("regexps");
        for (tinyxml2::XMLElement* child = rootnode->FirstChildElement(); child; child = child->NextSiblingElement())
        {
            const char* text = child->GetText();
            v->push_back(std::string(text));
            if (verbose)
                printf("Read XML entry: %s\n", text);
        }
    }else{
        printf( "Error loading the XML file, doc->ErrorID() == %d.\n", errorID);
        return IO_ERROR;
    }
    delete doc;

    ncv::str_vec_t* files = new ncv::str_vec_t();
    if (verbose)
        printf("Parsing files from input...");
    for (int i = 0; i < input->count;i++)
        fsys::listF(input->filename[i], files, recursive, verbose );
    for (str_vec_t::iterator it = files->begin(); it != files->end(); ++it) {
        std::cout << *it << std::endl;
        // Validate if the file follows any of the patterns defined in the configuration XML file, deallocation of vector will be handled by this method as well.
        if (ncv::certifyStr(*v, *it, verbose)){
            std::cout << *it << " does not follow a standard naming convention!" << std::endl;
        }
    }
    if (verbose)
        printf("Deallocating memory and quitting...\n");
    delete NCV_VERSION;
    delete HELP_HEADER;

    arg_freetable(args, sizeof(args) / sizeof(args[0])); // Deallocate the memory allocated to each of the arg_xxx structs, by using the arg_freetable function.

    v->clear();
    delete v;

    files->clear();
    delete files;

    delete[] * args;
    return SUCCESS;
}

void ncv::printUsage(void *args[]){
    printf("%s\n\n",HELP_HEADER->c_str());
    printf("Usage: \"NCV Tool\".exe");
    arg_print_syntaxv(stdout,args,"\n");
    arg_print_glossary(stdout,args,"%-25s %s\n");
}

This application uses two libraries: argtable2 tinyxml2

Any thoughts and ideas is much appreciated!

Answer 1

This is a reserved identifier.

#define _CRT_SECURE_NO_WARNINGS 

Unless you are using a software package that asks you to set this it is probably not a good idea.

Put header files in order. Most specific (ie your header file first). To most generic last (The C libraries are the most generic).

#include <tinyxml2.h>
#include <argtable2.h>

#include "ncv.hpp"
#include "fsys.h"

#include <cstdlib>
#include <iostream>
#include <filesystem>

Not recommended.

using namespace ncv;

But it is your source file so I am not going to complain too much. Just note that you are being lazy.

These do not need to be dynamically created.

const std::string *NCV_VERSION = new std::string("1.0.06.8");
const std::string *HELP_HEADER = new std::string("Naming Convention Validator Tool, or NCV Tool Version " + *NCV_VERSION + "\n"
                              + "Purpose: Validates whether or not the file(s) follow the naming convention CADS4_VEHICLE_%Date%_< OPTIONAL >.\n\n"
                              + "Written by Linus Styren.\n");

This would have worked just as well:

const std::string NCV_VERSION = "1.0.06.8";
const std::string HELP_HEADER = "Naming Convention Validator Tool, or NCV Tool Version "
                                + NCV_VERSION
                                + "\n"
                                + "Purpose: Validates whether or not the file(s) follow the naming convention CADS4_VEHICLE_%Date%_< OPTIONAL >.\n\n"
                                + "Written by Linus Styren.\n";

In C++ you don't need to proceed the struct type name with struct.

 struct arg_lit  *help;

 // This is equivalent to

 arg_lit  *help;

 // Also in C++ the type is much more important than in C.
 // So it is more traditional to put the * on the left hand type
 // (as it is part of the type information).

 arg_lit*  help;

Next it is not usual in modern C++ to see pointers. Pointers do not have any ownership symantics. So we don't know who is supposed to clean up the memory they point at (if at all). This is the largest failing of C and requires extensive use of documentation. In C++ we have a bunch of types for handling ownership semantics directly in the application code. So you should go an learn about reference and smart pointers.

Creating an array and assigning values in the array at the same time.

    void* args[] = {
        help = arg_lit0("hH", "help", "Display this help screen and the version of NCV Tool."),
        r = arg_lit0("rR", "recursion", "If one or more folders are entered as the file args, subfolders will be read."),
        vb = arg_lit0("vV", "verbose", "Enable verbose output."),
        c = arg_file0("cC", "config", "<file>", "Specify a XML configuration file to use rather than the default one."),
        input = arg_filen(NULL, NULL, "<file>", 1, 512, "Input files and/or folders."),
        end = arg_end(10),
    };

That seems like a recipe for disaster.

This seems like an overly verbose way of doing it.

    bool recursive = ((r->count) ? true : false);
    bool verbose = ((vb->count) ? true : false);

    // Why not:

    bool recursive = r->count;
    bool verbose   = vb->count;

If this is C++ you should be using the C++ stream operators in preference to printf. They are at least type safe.

        printf("Include sub-directories = %s\n", ((r->count) ? "YES" : "NO"));
        printf("Verbose output          = %s\n", ((vb->count) ? "YES" : "NO"));
        printf("Configuration file used = %s\n", config);
        printf("\n");

If this is C++ why are you using C FILE operators. Use the C++ std::ifstream type.

    FILE* fp = fopen( config, "r" );

Does not look like you need dynamic allocation here.

    ncv::str_vec_t* v = new ncv::str_vec_t();

Just create a local object.

    ncv::str_vec_t   v;

Also your variable names are a bit on the short side. This makes it hard to understand what you are trying to do. A bit of verbose ness will help in self documenting the code.

Again does not look like you need a dynamic object.

    tinyxml2::XMLDocument* doc = new tinyxml2::XMLDocument();
    doc->LoadFile( config );
    int errorID = doc->ErrorID();

Much easier to do this:

    tinyxml2::XMLDocument  doc;
    doc.LoadFile( config );
    int errorID = doc.ErrorID();

OK. Here you do need a pointer.

        tinyxml2::XMLElement *rootnode = doc.FirstChildElement("regexps");

Can you assume the rootnode is not NULL?

        for (tinyxml2::XMLElement* child = rootnode->FirstChildElement(); child; child = child->NextSiblingElement())

Because you dynamiically allocated the doc you are now deleting it.

    delete doc;

There is a slight problem with this. If an exception is thrown you will leak memory. That is why in C++ we use smart pointers to make sure delete is called automatically and correctly. But if you do as I have shown above you do not even need to call delete.

Same comment as above.

    delete NCV_VERSION;
    delete HELP_HEADER;

All this clean up you are doing here.

    v->clear();
    delete v;

    files->clear();
    delete files;

    delete[] * args;
    return SUCCESS;

Should be done automatically by the compiler. Just prefer to use normal automatic objects and the compiler will do all this for you.