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.

I'm writing a small Win32 Console app that manipulates the contents of binary files. It accepts the filename as a command line parameter and writes to a file named filename.scramble

This is the first time I'm dynamically allocating memory for C strings (which I'm using since the fstream functions use const char*).

The code is as follows. I've added the comments to help you understand what I'm trying to do. Please tell me whether I'm doing anything unsafe with pointers or whether I'm allocating memory wrong or any unsafe practice you see regarding dynamically allocating and concatenating C strings in C++. My question is specifically about the strings and the memory management.

#include<iostream>
#include<fstream>
using namespace std;

int main(int argc, char *argv[])
{
    //Program needs at least one extra argument
    //If less, print 'usage'
    if (argc < 2) 
        cout<<"usage: "<< argv[0] <<" <filename>\n";

    else 
    {
        //Assuming argv[1] is a file name 
        fstream fin (argv[1],ios::binary|ios::in);

        if (!fin.is_open())
            cout<<"Could not open file\n";
        else 
        {
            //Calculate size of file
            fin.seekg(0,ios::end);
            unsigned long long size = fin.tellg();
            //Find out how many left over characters
            //if using blocks of 16 characters
            int lastset=size%16;
            //Change size to number of blocks
            size /= 16;

            //File name must be <original filename>.scramble
            //Allocate space
            char* name = new char[sizeof(argv[1])+8];

            //Concatenate <original filename>
            strcat_s(name,sizeof(argv[1]),argv[1]);
            //Concatenate ".scramble"
            strcat_s(name,sizeof(argv[1])+8,".scramble");

            fstream fout (name,ios::binary|ios::out);
            //Buffer
            char memblock[16];
            fin.seekg(0,ios::beg);

            //Read from fin and write to fout in blocks of 16 characters
            for(unsigned long long i=0;i<size;++i)
            {
                fin.read(memblock,16);
                /*
                *
                * Manipulation on memblock to be done here
                *
                */
                fout.write(memblock,16);
            }

            //No manipulation for last n bytes, n<16
            fin.read(memblock,lastset);
            fout.write(memblock,lastset);

            fin.close();
            fout.close();

            delete[] name;
        }
    }
    return 0;
}
share|improve this question
    
I'd suggest creating the output file name using something like std::string name(argv[1] + std::string(".scramble")); –  William Morris Mar 9 '13 at 2:52
add comment

1 Answer

There is no need for manual dynamic memory allocation.

Don't use this:

using namespace std;

It causes lots of problems in anything but a small application. So get into the habbit of not using it. If you muse be lazy specifically import the things you need. Notice in the code below I always use the prefix std:: five letters is not a big overhead for being exact.

using std::cout;
using std::ifstream;

For a good discussion see http://stackoverflow.com/q/1452721/14065 my favorite answer to this question is http://stackoverflow.com/a/1453605/14065

Don't write comments where you explain what the code is doing. The code is already verbose enough to explain what. If you are going to comment explain "WHY" you are doing this. Over commenting is a real problem and you should use comments judiciously and explain why. http://programmers.stackexchange.com/a/13/12917

    //Program needs at least one extra argument
    //If less, print 'usage'

If your condition exits the program. Return an error code immediately. This makes the alignment and reading of the rest of your code easier:

    if (argc < 2) 
        cout<<"usage: "<< argv[0] <<" <filename>\n";

    else 
    {

Easier to write as:

    if (argc < 2) 
    {
        cout<<"usage: "<< argv[0] <<" <filename>\n";
        return 1;
    }
    <rest of code>

Same comment as above.

        if (!fin.is_open())
            cout<<"Could not open file\n";

The best way to comment this. Is to put this code in a function with an appropriate name. Then don't write the comments (the code is self explanatory). You may write a comment about "why" you are using 16 as this will help a maintainer to modify dependant code.

            //Calculate size of file
            fin.seekg(0,ios::end);
            unsigned long long size = fin.tellg();
            //Find out how many left over characters
            //if using blocks of 16 characters
            int lastset=size%16;
            //Change size to number of blocks
            size /= 16;

Easier to read as:

            int size = getNumberOf16ByteBlocksFromStream(fin);

It is very rare that you need to use new/delete. It is even rarer that you would use delete as any object created with new should be put in an object that manages its lifespan correctly. If you don't do this writing exception safe code is exceedingly hard.

In this case you can achieve the same affect by using a std::vector

            //File name must be <original filename>.scramble
            //Allocate space
            char* name = new char[sizeof(argv[1])+8];

Use std::vector for simple buffers. Its exception safe.

            std::vector<char>  name(strlen(argv[1]) + 8);

            // To get the address of the first byte use &name[0];

Note that sizeof() gives you the number of bytes in the variable. This does not give you the length of the string. the size of the variable in this case is the size of the pointer that is pointing at the string (so 4 or 8 depending on the system). Use strlen() to get the size of a string.

Also the +8 is wrong. As .scramble is 9 characters and you need to add a byte for the terminating \0 character. So in reality you should use std::string to build the filename. Again the comments are worse the useless (not because you wrote bad comments, but because the comments can go out of sync with the code. Then a maintainer will wonder why the code does not match the comments. Also this code is so trivial that you should be able to see what it is trying to do).

            //Concatenate <original filename>
            strcat_s(name,sizeof(argv[1]),argv[1]);
            //Concatenate ".scramble"
            strcat_s(name,sizeof(argv[1])+8,".scramble");

            // PS you got the usage all long.
            // I am very surprised if this actually runs correctly.
            // Normal operation.
            //   1) string-copy the first part
            //   2) string-cat all subsequent parts.
            //   3) The size (Second field) in <X>_s() functions is the
            //           TOTAL SIZE OF THE BUFFER
            //      not how much you want to copy. This should ALWAYS
            //      match the value you used in allocation.

Easier to write as:

            std::string filename = argv[1];
            filename += ".scramble"

That's is a very small buffer:

            char memblock[16];

You should probably have re-set the stream up where you were messing around with it finding the size.

            fin.seekg(0,ios::beg);

No need to manually close files. The destructor will do that. http://codereview.stackexchange.com/a/544/507

        fin.close();
        fout.close();

This delete is not exception safe.
Not a problem in this code. But in general you should try and make them exception safe so you don't leak. This means using RAII.

            delete[] name;
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.