Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

If anyone could help me simplify this code, it would be greatly appreciated. There are plenty of if's and else's, and someone else told me that using switch/case statements could really clean up the code. However, he didn't implement it for me, so I don't know what I should switch/case.

Also take note that the "if (!stricmp(argv[arg], "-vmf"))" and "if (!stricmp(argv[arg], "-bsp"))" blocks are exactly the same, minus the usage of "vmf" and "bsp", and that the "wad" block is somewhat similar.

#include <cstdio>
#include <cstring>
#include <fstream>
#include <sys\stat.h>

#include "arg.h"
#include "files.h"
#include "stricmp.h"

using namespace std;

//"ifError" takes all the argument flags, as well as a string to output. If silent flag is
//enabled, then "ifError" will not output that text. I put this into the function to reduce
//lines and characters of code.
void ifError(argFlags& argFlags, char* errorMessage)
{
    if (!argFlags.sil)
    {
        if (!argFlags.err)
        {
            printf("Converts DOOM WAD into VMF and/or BSP file(s).\n\n");
            printf("DOOM2BSP [-silent] [-WAD [path][name]] [-VMF [path]] [-BSP [path]]\n\n");
            printf("  -WAD     Location of WAD.\n");
            printf("  -VMF     Create VMF file(s) in path.\n");
            printf("  -BSP     Create BSP file(s) in path.\n");
            printf("  -silent  Disable text output.\n\n");
        }
        printf(errorMessage);
    }
    argFlags.err = true;
}

//Checks if directory exists.
bool checkDir(char* path)
{
    struct stat st;
    if (!stat(path, &st))
        if (st.st_mode & S_IFDIR != 0)
            return true;
    return false;
}

//This function analyzes our arguments, deals with them accordingly, and decides if there
//are any errors or not. Also opens wad if it exists.
void handleArgs(argFlags& argFlags, files& files, int& argc, char**& argv)
{
    //Analyzes all arguments to find "-silent" arg. If silent arg is found, the silent flag
    //is turned on, signifying that this program should not output any text.
    for (int arg = 1; arg < argc; ++arg)
        if (!stricmp(argv[arg], "-silent"))
            argFlags.sil = true;

    //Analyzes the rest of the arguments, with the silent flag in mind.
    for (int arg = 1; arg < argc; ++arg)
    {
        //If the "-wad" arg is found, turn on wad flag. Look for wad name. If wad name is
        //found, check if for its validity and existance. If check fails or if wad name arg
        //is not found, output error.
        if (!stricmp(argv[arg], "-wad"))
        {
            argFlags.wad = true;
            if ((arg + 1) < argc)
            {
                files.wad.open(argv[arg + 1], fstream::out | fstream::binary);
                if (!files.wad.good())
                    ifError(argFlags, "Error: WAD name is invalid or does not exist.\n");
            }
            else
                ifError(argFlags, "Error: WAD name is not specified.\n");
        }
        //If the "-vmf" arg is found, turn on vmf flag. Look for vmf path name. If vmf path
        //name is found, check for its validity and existance. If check fails or if vmf
        //path arg is not found, output error.
        else if (!stricmp(argv[arg], "-vmf"))
        {
            argFlags.vmf = true;
            if ((arg + 1) < argc)
            {
                files.vmfPath = argv[arg + 1];
                if (!checkDir(files.vmfPath))
                    ifError(argFlags, "Error: VMF path is invalid or does not exist.\n");
            }
            else
                ifError(argFlags, "Error: VMF path is not specified.\n");
        }
        //If the "-bsp" arg is found, turn on bsp flag. Look for bsp path name. If bsp path
        //name is found, check for its validity and existance. If check fails or if bsp
        //path arg is not found, output error.
        else if (!stricmp(argv[arg], "-bsp"))
        {
            argFlags.bsp = true;
            if ((arg + 1) < argc)
            {
                files.bspPath = argv[arg + 1];
                if (!checkDir(files.bspPath))
                    ifError(argFlags, "Error: BSP path is invalid or does not exist.\n");
            }
            else
                ifError(argFlags, "Error: BSP path is not specified.\n");
        }
    }

    //Decently self-explanatory.
    if (argc == 1)
        ifError(argFlags, "Error: No parameters.\n");
    else if (!argFlags.wad)
        ifError(argFlags, "Error: WAD parameters missing.\n");
    else if ((!argFlags.vmf) && (!argFlags.bsp))
        ifError(argFlags, "Error: VMF/BSP parameters missing.\n");
}
share|improve this question
2  
I think you should read a book about refactoring code, as it will help you to understand the changes you will make, and how to set up testing, etc. –  Horus Feb 11 '13 at 3:53
1  
Use Boost.ProgramOptions. –  Nicol Bolas Feb 11 '13 at 5:18
    
I'll take this opportunity to strongly recommend reading Clean Code by Robert C. Martin. The examples may be in Java, but it really applies to any language. –  codesparkle Feb 15 '13 at 18:03
add comment

4 Answers 4

I think you should separate the logic parsing the arguments provided from the logic using the argument parsed. At the moment, you are trying to do everything in one place and I'm not quite sure this is correct (for instance, if a filename is also an option name, this leads to an unexpected behavior).

I'd suggest something like (I'm not even sure it would compile but that just to give you an idea) :

void handleArgs(argFlags& argFlags, files& files, int& argc, char**& argv)
{
    int waitingForArg = 0; // To know if we are expecting an other argument to be provided : 0:no, 1:wad, 2:vmf, 3:bsp
    for (int i = 1; i < argc; i++)
    {
        char* arg = argv[i];
        switch (waitingForArg)
        {
            case 0:
                if (!stricmp(arg, "-silent"))
                    argFlags.sil = true;
                else if (!stricmp(arg, "-wad")
                    waitingForArg = 1;
                else if (!stricmp(arg, "-vmf")
                    waitingForArg = 2;
                else if (!stricmp(arg, "-bsp")
                    waitingForArg = 3;
                break;
            case 1:
                argFlags.wadFile = arg; waitingForArg = 0;
                break;
            case 2:
                argFlags.vmfFile = arg; waitingForArg = 0;
                break;
            case 3:
                argFlags.bspFile = arg; waitingForArg = 0;
                break;
        }
    }

    if (waitingForArg)
    {
        ifError(argFlags, "Error: file name must be provided with option -wad, -vmf or -bsp");
        return;
    }

    // Logic involving wadFile, wmfFile, bspFile. Checking if they are set and/or if they correspond to existing files.
}

This is just a first step to make things easier to follow (on top of being more correct and more efficient).

Then to remove code duplication, you might want to do something like using an array to store the different possible options (["-wad","-vmf","-bsp"]). In order to do so, you'd replace the "else if (!stricmp(arg, "OPTIONS"))" with a loops on the different options. Then you might also be happy to store the flags as an array. I think this would make more sense if you had more flags to handle.

I also notice that you have tagged your question as C++ but your code seems to be using some C-style (printf and stuff). This might be something you should fix as well.

share|improve this answer
add comment

Josay is correct and you should accept his answer. I'd like to mention a few more things.

  1. Please use braces everywhere, the code you're using is known to lead to difficult reading and subtle bugs. Consider:

    if (condition)
        a();
        b();
    

    b() will be executed unconditionally.

  2. switch wouldn't have helped with your code since you can only switch on integers. It doesn't make the code much shorter unless you have a lot of cases to handle, and the main benefit isn't shorter code anyway: it's better readability.

  3. stricmp is Windows-only, the POSIX equivalent is strcasecmp, but I don't think Windows supports it. If you want to be cross-platform, go for Boost and iequals.
  4. You're using C++, so you may want to take advantage of it. With your current code it mostly means using string instead of char* and std::cout instead of printf.
share|improve this answer
add comment

Here's a version of ifError() with reduced nesting level by bailing out with an early return:

void ifError(argFlags& argFlags, char* errorMessage)
{
    argFlags.err = true;
    if (argFlags.sil)
        return;
    if (!argFlags.err)
    {
        printf("Converts DOOM WAD into VMF and/or BSP file(s).\n\n");
        printf("DOOM2BSP [-silent] [-WAD [path][name]] [-VMF [path]] [-BSP [path]]\n\n");
        printf("  -WAD     Location of WAD.\n");
        printf("  -VMF     Create VMF file(s) in path.\n");
        printf("  -BSP     Create BSP file(s) in path.\n");
        printf("  -silent  Disable text output.\n\n");
    }
    printf(errorMessage);
}

In checkDir() you can merge the two if statements. Make sure to keep the same order, because the second condition needs to operate on st which is initialized in the first condition:

bool checkDir(char* path)
{
    struct stat st;
    if (!stat(path, &st) && st.st_mode & S_IFDIR != 0)
            return true;
    return false;
}

There's not much to simplify in handleArgs(). Note that using switch is not possible because in C++ strings cannot be used with switch (neither C-strings, nor std::string.)

share|improve this answer
add comment

I suggest using a table of :

for each entry in table do:
    if entry.text == parameter
        execute entry.function_pointer
    end-if
end-for

Example table:

"WAD", Process_WAD_argument
"VMF", Process_VMF_argument

The nice feature about tables in this scenario is that you can add parameters without changing the parsing code: add another line to the table.

share|improve this answer
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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