concatenate two char arrays into single char array using pointers

Question

I am trying to take a text file with names (ex: john doe) and fin the first name and last name. Then, I want to take these two char arrays and concatenate them together using pointers. The code that is commented out is working code that takes the two char arrays and puts them into a single char array ie concatenating them together. This project requires that I use pointers, and that I use char arrays I am not asking for you to do it for me, but please help me realize what I am doing wrong. Thanks

EDIT: the error I am getting is a seg fault..so Im thining my playerPtr is going out of bounds somewhere??

void readPlayer(char *finName2, player *playerPtr)
{
player *playerHome = playerPtr;
ifstream fin;
char *tempfName= new char[20];
char *templName= new char[20];
char *tempName= new char[20];
char *tempNameHome = tempName;
fin.open(finName2);

if(!fin.good())
{
cout << "Error with player file!" << endl;       
}
else
{
fin >> tempfName;
fin >> templName;  //prime file
cout << tempfName << templName;
while(fin.good())
{
  for(int i =0;i<5;i++)
  {
    //find the length
    //int index =0, length=0;
while(*tempfName != '\0')
    //while(tempfName[length] != '\0')
{

      tempfName++;
}
      strcopy(tempName,tempfName);

  //now add space after first name
     *tempName = ' ';
      tempName++;
      //tempfName[length] = ' ';
      //tempfName++;
      //length++;

      while(*templName != '\0')
    //while(templName[index] != '\0')
  {
        templName++;
    //tempfName[length] = templName[index];
    //length++;
    //index++;
  }
      strcopy(tempName,templName);
      //tempName++;
      //tempfName[length]='\0';          
      strcopy((*playerPtr).name,tempName);
      playerPtr++;
  fin >> tempfName;
  fin >> templName;
      }
   }
}
delete[] tempfName;
delete[] templName;
delete[]tempName;
}

as i recall `strcat` is one of the very first examples in "The C Programming Language" by Brian Kernighan & Dennis Ritchie — Cheers and hth. - Alf, Sep 29 '14 at 04:12
`while(*tempfName != '\0') { tempfName++; }` At the end of this passage, you have `tempfName` pointing to the terminating `NUL` - in other words, to an empty string. It no longer points to an actual string read from the file. Also, it no longer points to a piece of memory allocated with `new`, so `delete[] tempfName` at the end exhibits undefined behavior. This is probably the immediate cause of the crash. — Igor Tandetnik, Sep 29 '14 at 04:23
Did you allocate memory for `playerPtr.name` before doing `strcopy((*playerPtr).name,tempName)`? — cppcoder, Sep 29 '14 at 04:28
@IgorTandetnik ahh wow. so I should use *tempfName++; then? im trying to increment through the pointer to copy in the first name — matt, Sep 29 '14 at 04:28
@cppcoder yes i allocated memory for the function in another initialization function.. i only posted this code because the program is rather large.. — matt, Sep 29 '14 at 04:30
Why do you want to modify `tempfName` (or `templName`) at all? This makes no sense. Just leave them be. Also, in `strcopy((*playerPtr).name,tempName)` and `delete[] tempName` use `tempNameHome` in place of `tempName`. You had the presence of mind to save the original pointer, but not enough of it to actually use the same. — Igor Tandetnik, Sep 29 '14 at 04:37
@IgorTandetnik I want to take tempfName and templName which is "joe""bob" and merge it into (*playerPtr).name = "joe bob" to include the space..im confused by what you mean — matt, Sep 29 '14 at 04:41
And how precisely is this `while` loop supposed to advance your lofty goal? What's the purpose of it, in your opinion? — Igor Tandetnik, Sep 29 '14 at 04:49
@IgorTandetnik I want to put "joe" into the tempName, then add a space, then put "bob" into the tempName, then copy that to playerPtr...is that not what I am doing? isnt it clear what i want to do — matt, Sep 29 '14 at 04:58
It's clear what you want to do, but it's not what you are doing. I humbly submit now would be a good time to learn how to use the debugger - this way you can see firsthand what your code is actually doing. — Igor Tandetnik, Sep 29 '14 at 05:04

score 0 · Answer 1 · answered Sep 29 '14 at 04:26

0

Your tempfName & templName are incremented all the time and they move beyond their allocated memory. you need to reset their position.
Plus I can see that the

fin >> tempfName;
fin >> templName;

is inside the for loop, which means fin.Good is only checked once every 5 times.

answered Sep 29 '14 at 04:26

atlanteh

5,615
2
33
54

ahh..so I should use *tempfName++; insteaD? im trying to increment through the letters in the first name – matt Sep 29 '14 at 04:29
Don't call the arrays temp, and use another temp pointers to increment and when you're done, reset the temp pointer to the array location. – atlanteh Sep 29 '14 at 04:32
would you provide an example? im confused as to what you mean – matt Sep 29 '14 at 04:37
@matt he means you're abandoning your original pointer values assigned from the `new` allocations, as the only thing originally pointing to them were `tempfName` and `templName`, and *both* walk up their respective blocks never to return to their original value again (the only values valid to send to `delete[]`, btw). Throwing *even-more* code at the problem isn't the solution; Identifying what is wrong with the *current* code and making the appropriate surgical changes is (or nuke everything and use `std::string` like any sane person programming in C++ would do). – WhozCraig Sep 29 '14 at 04:52
@WhozCraig its an exercise for class and were required to use c-style strings. so i need to reset my temp pointers, i see that i never did that. i know its a weird way to go about programming this, but the requirement sucks. ive been struggling to implement this function correctly – matt Sep 29 '14 at 04:59
Another point - why do you even move the fName & lName pointers? It seems you're moving them for no reason at all. Just copy their content and move the tempName alone. (you should reset it at the end of each iteration) – atlanteh Sep 29 '14 at 05:03
@matt what is the size of the mystery `name` member of `player` ? I hope it is `40`, but just double checking. – WhozCraig Sep 29 '14 at 05:03
@atlanteh so i dont need to increment them? how would i add in the space then? just in tempName? like.. *tempName++; strcopy(*tempName, " "); – matt Sep 29 '14 at 05:10
@WhozCraig it is of size 40 – matt Sep 29 '14 at 05:11
You don't need strcopy for that. just *tempName = ' '; – atlanteh Sep 29 '14 at 07:34

score 0 · Answer 2 · answered Sep 29 '14 at 05:16

Problems that I see (mentioned in comments too):

Incrementing tempFName and tempLName in loops

        while(*tempfName != '\0')
        {
           tempfName++;
        }
        strcopy(tempName,tempfName);

At the end of the above loop, tempFName points to the end of the string - it points to the terminating null character. strcopy should copy nothing to tempName.

You have the same problem with the loop:

        while(*templName != '\0')
        {
           templName++;
        }
        strcopy(tempName,templName);

Setting the value of *tempName after the first loop

        //now add space after first name
        *tempName = ' ';
        tempName++;

This will be valid only if tempName points to the end of the copied string after the call to strcopy. If not, you are just setting the value of the first character in tempName to ' '. Incrementing tempName makes sense only of tempName points to the end of the copied string. Otherwise, it points to the second character.

As a result of the above errors, your code is subject to errors caused by out of bound memory access after the first iteration of the for loop. Nothing after that can be relied upon to behave in a reasonable manner.

I suggest the following changes to fix the above errors.

Don't increment the variables tempFName and tempLName at all

You don't need to at all.

Remove the lines:

        while(*tempfName != '\0')
        {
           tempfName++;
        }

just use:

        strcopy(tempName,tempfName);

Use a temporary pointer to go to the end of tempName

After the first call to strcopy, use:

        char* temp = tempName;
        while ( *temp != '\0' ) ++temp;
        *temp = ' ';
        ++temp;
        *temp = '\0';

Use the temporary pointer for the second strcopy

Remove the lines:

        while(*templName != '\0')
        {
           templName++;
        }

Replace the line:

        strcopy(tempName,templName);

with

        strcopy(temp,tempfName);

Alternate strategy

If you implement your version of strcat, you can simply use:

tempName[0] = '\0';
strcat(tempName, tempFName);
strcat(tempName, " ");
strcat(tempName, tempLName);

That will remove much of the clutter in the for loop.

@matt, I can only make sense of posted code. Without the complete program and the input you are feeding it, it's difficult to suggest something to fix the remaining errors. — R Sahu, Sep 29 '14 at 15:33

concatenate two char arrays into single char array using pointers

2 Answers2

Linked