Dynamically allocated C strings in C++, concatenation, pointers, etc

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*).

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
#include
using namespace std;

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

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

if (!fin.is_open())
cout.scramble
//Allocate space
char* name = new char[sizeof(argv[1])+8];

//Concatenate
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;
}

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 https://stackoverflow.com/q/1452721/14065 my favorite answer to this question is https://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. https://softwareengineering.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 \n";

    else 
    {

Easier to write as:

if (argc \n";
        return 1;
    }
    

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 .scramble
            //Allocate space
            char* name = new char[sizeof(argv[1])+8];

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

std::vector  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 
            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 _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. https://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;

StackExchange Code Review Q#23536, answer score: 5