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.

This code builds template .cpp, .c, .h and .java files based on a file name and #include, import lines provided at the command line. The file opens after creation.

I am looking for a review of this code and seeing if there is anything that can be more efficient.

/* .cpp Example:
*     template awesome.cpp iostream vector string
* Output:
*
*     #include <iostream>
*     #include <vector>
*     #include <string>
*
*     using namespace std;
*
*     int main(int argc, char * argv[])
*     {
*
*        return 0;
*     }
*
* .c Example:
*     template something.c stdio.h
* Output:
*
*     #include <stdio.h>
*
*     int main(int argc, char * argv[])
*     {
*
*        return 0;
*     }
*
*
* .h Example:
*     template awesome.h vector string
*
* Output:
*
*     #ifndef TEST_H
*     #def TEST_H
*
*
*     #include <iostream>
*     #include <string>
*
*
*     #endif // TEST_H
*
*
*
*
* .java Example:
*     template something.java util.* math.*
*
* Output:
*
*     import java.util.*;
*     import java.math.*;
*
*     public class something {
*
*        public static void main(String[] args) {
*
*
*
*
*        }
*     }
*
*********************************************************/

#include <iostream>
#include <algorithm>
#include <fstream>
#include <vector>
#include <string>


using namespace std;

// helper functions
string getFileType(string & filename);
void addImportAndIncludeLines(vector<string> & linesOfFile,
                              string & fileType, char * dependency);
void buildFile(vector<string> & linesOfFile, string & fileType);
void buildCOrCPlusPlusFile(vector<string> & linesOfFile, string & fileType);
void buildHeaderFile(vector<string> & linesOfFile, string & filename);
void buildJavaFile(vector<string> & linesOfFile, string & filename);
void writeToFile(vector<string> & linesOfFile, string & filename) throw(string);
void addHeaderFileDeclaration(vector<string> & linesOfFile, string & filename);
void capitalizeHeaderFileName(string & filename);


/*********************************************************
* main - handles the command line arguments and builds
* the template file, then opens it.
*********************************************************/
int main(int argc, char * argv[])
{
   // declare variables
   string filename;
   string fileType;
   vector<string> linesOfFile;


   // make sure something was actually passed in
   // if not, show proper usage
   if (argc > 1)
   {
      filename = argv[1];
   }
   else
   {
      cout << "Usage: template <filename> (optional: <files> <to> <include>)\n";
      return 0;
   }

   // get the file extension
   fileType = getFileType(filename);

   if (strcmp(fileType.c_str(), "h") == 0)
      addHeaderFileDeclaration(linesOfFile, filename);

   // add any #include / import files that were entered
   // at the command line
   for (int i = 2; i < argc; i++)
      addImportAndIncludeLines(linesOfFile, fileType, argv[i]);

   // build the template file
   buildFile(linesOfFile, filename);

   // try to write the template to the provided file name
   // then open it to begin editing
   try
   {
      writeToFile(linesOfFile, filename);

      // command to open the file after creation
      string command = "open ";
      command += filename;

      // open the file
      system(command.c_str());

   }
   catch (string error)
   {
      cout << error << endl;
   }



   return 0;
}

/*********************************************************
* returns the file extension
*********************************************************/
string getFileType(string & filename)
{
   return filename.substr(filename.find('.') + 1);
}

/*********************************************************
* addHeaderFileDeclaration adds the #ifndef statement
* for the header file
*********************************************************/
void addHeaderFileDeclaration(vector<string> & linesOfFile, string & filename)
{
   string headerDeclaration = filename.substr(0, filename.find('.'));
   capitalizeHeaderFileName(headerDeclaration);
   string ifStatement = "#ifndef ";
   ifStatement += headerDeclaration;
   linesOfFile.push_back(ifStatement);
   string defStatement = "#def ";
   defStatement += headerDeclaration;
   linesOfFile.push_back(defStatement);
   linesOfFile.push_back("\n");

}

/*********************************************************
* capitalizeHeaderFileName capitilatizes the provided
* filename and adds '_H' to it for the #ifndef statement
*********************************************************/
void capitalizeHeaderFileName(string & filename)
{
   transform(filename.begin(), filename.end(),filename.begin(), ::toupper);
   filename += "_H";

}

/*********************************************************
* addImportAndIncludeLines adds the dependency properly
* formatted based on the file type:
* #include <dependency>;
* import java.dependency;
*********************************************************/
void addImportAndIncludeLines(vector<string> & linesOfFile, string & fileType, char * dependency)
{

   // decide whether to #include or import.java.. based on
   // file extension
    switch (fileType.at(fileType.size() - 1)) {
        case 'c':
        case 'h':
        case 'p':
        {
            string includeString = "#include <";
            includeString += dependency;
            includeString += ">";
            linesOfFile.push_back(includeString);
            break;
        }
        case 'a':
        {
            string importString = "import java.";
            importString += dependency;
            importString += ";";
            linesOfFile.push_back(importString);
            break;
        }

    }
}

/*********************************************************
* buildFile builds the correct file based on file type
*********************************************************/
void buildFile(vector<string> & linesOfFile, string & filename)
{

    // calls the proper template builder based on file extension
    string fileType = getFileType(filename);

    switch (fileType.at(fileType.size() - 1)) {
        case 'p':
        case 'c':
            buildCOrCPlusPlusFile(linesOfFile, fileType);
            break;
        case 'h':
            buildHeaderFile(linesOfFile, filename);
            break;
        case 'a':
            buildJavaFile(linesOfFile, filename);

    }
}


/*********************************************************
* buildCPlusPlusFile - builds the template .cpp file plus
* the #include lines added at the command line
*********************************************************/
void buildCOrCPlusPlusFile(vector<string> & linesOfFile, string & fileType)
{
    if (strcmp(fileType.c_str(), "cpp") == 0) {
        linesOfFile.push_back("\nusing namespace std;");
    }
    linesOfFile.push_back("\nint main(int argc, char * argv[])\n{\n");
    linesOfFile.push_back("\treturn 0;\n}");

}


/*********************************************************
* buildHeaderFile - builds the template .h file plus
* the #include lines added at the command line
*********************************************************/
void buildHeaderFile(vector<string> & linesOfFile, string & filename)
{
   string headerDeclaration = filename.substr(0, filename.find('.'));
   capitalizeHeaderFileName(headerDeclaration);
   linesOfFile.push_back("\n");
   string endIf = "#endif // ";
   endIf += headerDeclaration;
   linesOfFile.push_back(endIf);
}


/*********************************************************
* buildJavaFile - builds the template .java file plus
* the import lines added at the command line
*********************************************************/
void buildJavaFile(vector<string> & linesOfFile, string & filename)
{
   // adds the class name based on the file name
   string className = "\npublic class ";
   className += filename.substr(0, filename.find('.'));
   className += " {";
   linesOfFile.push_back(className);

   // ...a lot of \'s, sorry!
   linesOfFile.push_back("\n\tpublic static void main(String[] args) {\n\n\n\n\n\t}\n}");

}


/*********************************************************
* writeToFile writes the template to a file with the
* provided file name.
*********************************************************/
void writeToFile(vector<string> & linesOfFile, string & filename) throw(string)
{
   ofstream output;
   output.open(filename.c_str());

   // throws error if the file can't be opened to write
   if (!output.is_open())
   {
      string error = "Error opening file \'";
      error += filename;
      error += "\'";
      throw error;
   }

   for (int i = 0; i < linesOfFile.size(); i++)
      output << linesOfFile.at(i) << endl;

   output.close();
}
share|improve this question
add comment

3 Answers 3

up vote 26 down vote accepted
  • Do not use using namespace std;, instead explicitly use std:: where required. [Why is “using namespace std;” considered bad practice?]

  • Consider not using prototypes (which requires you to maintain the same function header in two places) unless necessary. This means putting main() at the bottom of your code and functions that it uses above it.

  • Comments such as // declare variables add little value.

  • Reverse the sense of the if (argc > 1) test and check if (argc < 2), since you're going to return 0 in the error branch anyway. That way you can skip the else and put filename = argv[1] outside the if block.

  • Use fileType == "h" instead of strcmp. In general, avoid use of the str* library functions when using std::string.

  • Always always always put braces around blocks, even if the block is one statement. Otherwise, you'll get errors like Apple's goto fail bug.

  • In getFileType(), what happens if there is no . in the filename?

  • In addHeaderFileDeclaration(), also consider the possible missing ..

  • If filename contains more than one ., then the include guard will have a syntax error (since . is not permitted).

  • #def should be #define.

  • You should decide whether linesOfFile contains lines with or without a newline. In some places you do linesOfFile.push_back("\n") which will add two blank lines (perhaps you meant to do linesOfFile.push_back("")?)

  • capitalizeHeaderFileName modifies its parameter. It would be better to take a const string & parameter and modify a temporary value within the function, and return that.

  • Anywhere you take a string & parameter and don't modify it within the function, you should use const string &. For example the fileType parameter of addImportAndIncludeLines.

  • Switching on the last character of the filename works for your current set of files, but will fail as soon as you get a conflicting last character (eg. .scala files would confict with .java). Use the full file extension when you need to make a decision based on file type.

  • Only allowing import from the java.* namespace seems like an unnecessary limitation.

  • In buildFile(), you are missing a break; in the case 'a'. It's true that it's not required there, but it's a bug just waiting to happen.

  • In buildHeaderFile, you are computing the name of the include guard again, when you already did the same logic in addHeaderFileDeclaration. Consider unifying this somehow so you only have to compute it once.

  • Do not throw a string! Use a proper exception class if you decide to use exceptions.

  • Use an iterator to write out the lines of the file in writeToFile. In fact, you could use std::copy() and avoid the loop entirely.

  • Consider using the ofstream constructor that takes a file name, instead of using a separate call to open.

  • Don't use filename.c_str() because ofstream has an overload that takes a string.

  • The ofstream destructor will automatically close the output file so you don't have to explicitly call output.close().

share|improve this answer
2  
Don't use filename.c_str() because ofstream has an overload that takes a string. - that's only available in C++11, which the OP may not be using (though it's still good to mention that point). –  Jamal Jun 16 at 3:53
    
He tagged C++, not old-C++. –  DeadMG Jun 16 at 8:48
2  
@DeadMG C++ 3.x is far from old, in fact probably the vast majority of C++ projects out there still use it. Now if he were using 1.x that'd be old. –  jwenting Jun 16 at 9:34
    
using namespace std; is absolutely fine in the actual implementation, you just shouldn't use it in headers. –  API-Beast Jun 17 at 10:05
    
Same with prototypes, both help readability immensely. using std because it gets rid of unnecessary verbosity, and prototypes because the source code matches the application flow better. Prototypes also help understanding the source code quickly without going through the actual implementation. –  API-Beast Jun 17 at 10:11
add comment

I would add:

  • do not open the ofstream in writeToFile. Have writeToFile take an std::ostream& parameter instead of the file path. You can then try to open the file early (and fail fast if case be), instead of processing everything and finally failing because you can't create the file.
  • the fileType parameter in buildCOrCPlusPlusFile is a gratuitous change with regards to the filename parameter taken by other buildXXXFile functions. Pass filenameto uniformize signatures and deduce the fileType from within the function.
  • use polymorphism instead of two identical switches in addImportAndIncludeLines and buildFile. (By the way, the implementation of buildFile is clearly calling for virtual dispatch) It means you can declare an interface and implement one class per file type.

This would lead to:

struct GeneratedFile {
    virtual void buildFile(vector<string> & linesOfFile, string & filename) = 0;

    virtual void addImportAndIncludeLines(vector<string> & linesOfFile, char * dependency) = 0;
};

struct JavaFile : GeneratedFile {
   /* Implement specific versions of virtual functions */
};

struct COrCPlusPlusFile : GeneratedFile {
   /* Implement specific versions of virtual functions */
};

struct HeaderFile : GeneratedFile {
   /* Implement specific versions of virtual functions */
};

With this, all you need to do is determine the fileType only once in your program, (instead of twice) and select the appropriate derived class once and for all, letting it take care of the specificities all by itself all the way to the end of the program.

share|improve this answer
2  
Well formed answer. these are the kinds of answers we are looking for on CodeReview! –  Malachi Jun 16 at 15:41
add comment

The other answer covered almost all things, but it is lacking some things :

  • missing #include <cstring>, since you are using c string functions (for example strcmp), but since you shouldn't use them, feel free to ignore this point
  • always catch exceptions by const reference. That is, change the catch clause to catch (const string & error).
  • in addImportAndIncludeLines and buildFile functions you are missing the default case, where you should throw an error if other options are not possible
  • in writeToFile, consider enabling ofstream's exceptions, instead of throwing string. That way, you do not need to check whether writing failed (which is also missing).

With last point, the function writeToFile looks like :

void writeToFile(vector<string> & linesOfFile, string & filename) throw(string)
{
   ofstream output;
   output.exceptions( std::ifstream::failbit | std::ifstream::badbit )
   output.open(filename.c_str());

   for (int i = 0; i < linesOfFile.size(); i++)
      output << linesOfFile.at(i) << endl;

   output.close();
}

but you need to catch std::exception.

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.