Programming Forums > Application Development > C++

Sane · Jul 15th, 2006, 5:21 PM

How can I return an array of characters from a function? I know there's always the option of returning a pointer, but when I try that, I'm probably doing it wrong because I have to use a for loop to reference the address of each individual pointer.

It'd be easiest to do it this way as well, so that I can just cout the function call, without needing to do anything to it.

(Toggle Plain Text)

#include <iostream>
using namespace std;


// CONSTANTS
const int MAX_SIZE = 256;


int get_size(char text[MAX_SIZE])
{
  for (int i=0;i<MAX_SIZE;i++)
  {
    if (!(text[i]))
      return i;
  }
  return MAX_SIZE;
}

int locate(char text[MAX_SIZE], char from[MAX_SIZE], 
           int size_f)
{
  int i2 = 0;
  
  for (int i1 = 0;i1 < MAX_SIZE;i1++)
  {
    if (text[i1] == from[i2])
    {
      i2 ++;
      if (i2 == size_f)
        return i1 - size_f + 1;
    }
    else if (!(text[i1]))
      return -1;
    else
      i2 = 0;
  } 
  
  return -1;
}

char* replace(char text[MAX_SIZE], char to[MAX_SIZE], 
              int where, int size_f, int size_t)
{
  char new_text[MAX_SIZE];
  
  int offs1 = size_f+where;
  int offs2 = size_t+where;
  
  // append text before change
  for (int i1=0; i1 <= where; i1++)
    new_text[i1] = text[i1];      

  // append change
  for (int i2=0; i2 <= size_t; i2++)
    new_text[i2+where] = to[i2];
  
  // append text after change
  for (int i3=0; i3 <= get_size(text)-size_f-where; i3++)
    new_text[i3+offs2] = text[i3+offs1];
  
  return new_text;
}

int main()
{
  char text[MAX_SIZE] = "This is some text, yaddi yaddi.";
  char from[MAX_SIZE] = "some";
  char to[MAX_SIZE]   = "no";
  
  int size_f = get_size(from);
  int size_t = get_size(to);
  
  int where = locate(text, from, size_f);
  
  if (where == -1)
    cout << "Value was not found anywhere inside the text";
  else  
    cout << replace(text, to, where, size_f, size_t);
    
  cin.sync();
  cin.get();
  return 0;   
}

Narue · Jul 15th, 2006, 5:38 PM

>How can I return an array of characters from a function?
You can't. Either return a pointer, or wrap the array in an object so that it can be copied as a single entity.

>I'm probably doing it wrong
Yes. Yes you are.

(Toggle Plain Text)

int get_size(char text[MAX_SIZE])
{
  for (int i=0;i<MAX_SIZE;i++)
  {
    if (!(text[i]))
      return i;
  }
  return MAX_SIZE;
}

That's just funky. After the loop, i will be MAX_SIZE anyway, so why confuse readers more than you have to?

(Toggle Plain Text)

int get_size ( char text[] )
{
  int i = 0;

  while ( i < MAX_SIZE && text[i] != '\0' )
    ++i;

  return i;
}

Of course, even better would be taking the max size as an argument rather than hard coding some arbitrary macro that destroys reusability.

Tell me, what's wrong with the following code:

(Toggle Plain Text)

char* replace(...)
{
  char new_text[MAX_SIZE];
  
  ...
  
  return new_text;
}

Too subtle? You're returning a pointer to local data. new_text is destroyed when replace returns, so your pointer to it refers to indeterminate data. This is undefined behavior. You must either dynamically allocate your memory, use an object that has an appropriate copy constructor, pass the array as an argument, or declare the array as static and suffer the pitfalls of that particular option.

>cin.sync();
That's not guaranteed to do what you think.

By the way, you're commenting the wrong parts of your code. Anything that isn't immediately obvious should have a comment, or be changed so that it's immediately obvious. The one thing I wondered while reading your code is precisely the comments you should have added: why are you reinventing the wheel when the standard library supports a huge portion of what you're trying to do?

Cache · Jul 15th, 2006, 5:52 PM

Quote:

Originally Posted by Sane

How can I return an array of characters from a function?

This question has been asked a few times. An un-surprisingly quick search yielded the following answer:
http://www.programmingforums.org/for...36&postcount=5

grumpy · Jul 15th, 2006, 6:24 PM

Narue and Cache have answered your question quite well. I'll just add a few little specific notes;

1) Your functions are essentially recreating the wheel of the C standard library. For example, your get_size() function achieves more or less the same thing as the strlen() function that is specified in <string.h>. Except that you are adding a few other quirks that aren't particularly useful.

2) Since this is a C++ group you might want to use the standard string class (std::string which is specified in the header <string>). This is (rather loosely) an object wrapper for an array of char. And can be passed to an output stream directly.

Sane · Jul 15th, 2006, 6:32 PM

Quote:

Originally Posted by grumpy

Your functions are essentially recreating the wheel of the C standard library.

That was the intention of this exercise. Using an array of characters was also another part of the exercise.

Thank you for the help.

By the way, Narue, I'm not sure if you were calling my approach to it incorrect. Because all I had to do was add the word static before the declaration of "new_text". Which is still returning a pointer, but it's not as if I had to do much.

grumpy · Jul 15th, 2006, 7:28 PM

Quote:

Originally Posted by Sane

By the way, Narue, I'm not sure if you were calling my approach to it incorrect. Because all I had to do was add the word static before the declaration of "new_text". Which is still returning a pointer, but it's not as if I had to do much.

Narue was describing your approach as incorrect, and she's right. Your implication that you're actually right because fixing the problem isn't necessarily a lot of work (eg adding a static keyword) is crap. You don't have to do much to put wheel nuts on correctly when changing a flat tyre either. But the not putting those nuts in place or not tightening them correctly has major safety implications, and neglecting to do so is therefore plainly wrong. Imagine the response in a court of law, after someone has had a major accident, if your defence is "all I had to do was tighten the nuts".

Narue · Jul 15th, 2006, 7:35 PM

>By the way, Narue, I'm not sure if you were calling my approach to it incorrect.
Your approach was broken, and therefore incorrect. I thought I made that abundantly clear.

>Because all I had to do was add the word static before the declaration of "new_text".
You'll notice that I went out of my way to mention that a static local variable has pitfalls. Unsurprisingly, you've chosen to pick the worst of your options (next to the undefined behavior you already had) and learn for yourself why it's a bad idea. That's really the best way to learn, in my opinion.

>Which is still returning a pointer, but it's not as if I had to do much.
In all honesty, I feel that your design sucks and needs to be scuttled. But rather than focus on subjective topics, I gave you concrete advice on the assumption that you would find my questioning of your design as an insult. People tend not to take "delete it all and start over" very well, so I reserve that for particularly awful code and real life subordinates.

grumpy · Jul 15th, 2006, 7:49 PM

Quote:

Originally Posted by Narue

People tend not to take "delete it all and start over" very well, so I reserve that for particularly awful code and real life subordinates.

Which means you would have been quite justified doing it in this case

Sane · Jul 15th, 2006, 7:54 PM

Quote:

Originally Posted by grumpy

Which means you would have been quite justified doing it in this case

The code is that bad?

Besides the lack of comments, would you please tell me what needs to be done to derail it of its "awfully designed" state? ... Without using the C standard functions that this is intended to replicate?

(Toggle Plain Text)

#include <iostream>
using namespace std;


// CONSTANTS
const int MAX_SIZE = 256;


int get_size ( char text[MAX_SIZE] )
{
  int i = 0;

  while ( i < MAX_SIZE && text[i] != '\0' )
    ++i;

  return i;
}

int locate(char text[MAX_SIZE], char from[MAX_SIZE], 
           int size_f)
{
  int i2 = 0;
  
  for (int i1 = 0;i1 < MAX_SIZE;i1++)
  {
    if (text[i1] == from[i2])
    {
      i2 ++;
      if (i2 == size_f)
        return i1 - size_f + 1;
    }
    else if (text[i1] == '\0')
      return -1;
    else
      i2 = 0;
  } 
  
  return -1;
}

char *replace(char text[MAX_SIZE], char to[MAX_SIZE], 
              int where, int size_f, int size_t)
{
  char *new_text = new char[MAX_SIZE];

  int offs1 = size_f+where;
  int offs2 = size_t+where;
  
  // append text before change
  for (int i1=0; i1 <= where; i1++)
    new_text[i1] = text[i1];      

  // append change
  for (int i2=0; i2 <= size_t; i2++)
    new_text[i2+where] = to[i2];
  
  // append text after change
  for (int i3=0; i3 <= get_size(text)-size_f-where; i3++)
    new_text[i3+offs2] = text[i3+offs1];
  
  return new_text;
}

int main()
{
  char *text = new char[MAX_SIZE];
  char from[MAX_SIZE] = "some";
  char to[MAX_SIZE]   = "no";
  
  int size_f = get_size(from);
  int size_t = get_size(to);
  int where = 0;
  
  text = "This is some text, yaddi yaddi oh. some more text.";

  while (!(where == -1))
  {
    where = locate(text, from, size_f);
    if (!(where == -1))
      text = replace(text, to, where, size_f, size_t);
  }
  
  cout << text;
    
  cin.sync();
  cin.get();
  return 0;   
}

grumpy · Jul 15th, 2006, 8:37 PM

OK; just remember you asked.

I am ignoring the fact that we could probably do all this with a few lines using either C or C++ standard library features, and that your comments don't help much for explaining what your code is doing. I also ignore the fact that you are using the C++ standard library by using <iostream>.

1) Narue's already mentioned the issue of hard-coding array sizes in function declarations.

2) It's not really all that obvious, without a close look, what i1, i2, and size_f are being used for in your locate() function. That gives you a maintenance problem if you pick this code up in a few months and need to improve it.

3) You have multiple return points from your locate() function. Again, that gives you a maintenance problem if you pick this code up in a few months and need to improve it.

4) Minor nit (as compilers will cope with this): you're using postincrement when preincrement will do the job.

5) It is not obvious what the variables offs1, offs2, i1, i2, i3 are being used for in replace(). Yes, I know offs1 and offs2 are offsets, but that only is obvious after looking at the rest of the code.

6) The size of your new_text array (or memory allocated) needs to be larger than MAX_SIZE

7) size_t is a type that is used frequently in both the C and C++ standard libraries. You are using it as a variable name, which means obscure incompatibilities with either the C or C++ standard libraries .... and you are using <iostream>.

8) Your replace() function dynamically allocates memory, and requires the caller to release it. This is an opportunity for the caller to experience a memory leak. Your main function fully exploits that opportunity by calling replace() repeatedly in a loop, and never releasing the dynamically allocated memory.

9) Minor: "if (text[i1] == '\0')" can be simplified to "if (text[i])"

10) Why, in main(), declare from and to as being of size MAX_SIZE when you are assigning their contents using string literals?

11) You main() function declares text as a pointer, and assigns it to new char [MAX_SIZE]. Then you reassign text to a string literal, and therefore another memory leak.

12) "if (!(where == -1))" is more readably expressed as "if (where != -1)"

13) As Narue mentioned, cin.sync() and cin.get() probably aren't doing what you intend, even if it appears they are.

I haven't looked all that closely, so there are probably some other issues I've missed.

Jul 15th, 2006, 5:21 PM	#1
Sane Programming Guru Join Date: Apr 2005 Posts: 1,799 Rep Power: 5	Returning An Array Of Characters How can I return an array of characters from a function? I know there's always the option of returning a pointer, but when I try that, I'm probably doing it wrong because I have to use a for loop to reference the address of each individual pointer. It'd be easiest to do it this way as well, so that I can just cout the function call, without needing to do anything to it. (Toggle Plain Text) #include <iostream> using namespace std; // CONSTANTS const int MAX_SIZE = 256; int get_size(char text[MAX_SIZE]) { for (int i=0;i<MAX_SIZE;i++) { if (!(text[i])) return i; } return MAX_SIZE; } int locate(char text[MAX_SIZE], char from[MAX_SIZE], int size_f) { int i2 = 0; for (int i1 = 0;i1 < MAX_SIZE;i1++) { if (text[i1] == from[i2]) { i2 ++; if (i2 == size_f) return i1 - size_f + 1; } else if (!(text[i1])) return -1; else i2 = 0; } return -1; } char* replace(char text[MAX_SIZE], char to[MAX_SIZE], int where, int size_f, int size_t) { char new_text[MAX_SIZE]; int offs1 = size_f+where; int offs2 = size_t+where; // append text before change for (int i1=0; i1 <= where; i1++) new_text[i1] = text[i1]; // append change for (int i2=0; i2 <= size_t; i2++) new_text[i2+where] = to[i2]; // append text after change for (int i3=0; i3 <= get_size(text)-size_f-where; i3++) new_text[i3+offs2] = text[i3+offs1]; return new_text; } int main() { char text[MAX_SIZE] = "This is some text, yaddi yaddi."; char from[MAX_SIZE] = "some"; char to[MAX_SIZE] = "no"; int size_f = get_size(from); int size_t = get_size(to); int where = locate(text, from, size_f); if (where == -1) cout << "Value was not found anywhere inside the text"; else cout << replace(text, to, where, size_f, size_t); cin.sync(); cin.get(); return 0; } #include <iostream> using namespace std; // CONSTANTS const int MAX_SIZE = 256; int get_size(char text[MAX_SIZE]) { for (int i=0;i<MAX_SIZE;i++) { if (!(text[i])) return i; } return MAX_SIZE; } int locate(char text[MAX_SIZE], char from[MAX_SIZE], int size_f) { int i2 = 0; for (int i1 = 0;i1 < MAX_SIZE;i1++) { if (text[i1] == from[i2]) { i2 ++; if (i2 == size_f) return i1 - size_f + 1; } else if (!(text[i1])) return -1; else i2 = 0; } return -1; } char* replace(char text[MAX_SIZE], char to[MAX_SIZE], int where, int size_f, int size_t) { char new_text[MAX_SIZE]; int offs1 = size_f+where; int offs2 = size_t+where; // append text before change for (int i1=0; i1 <= where; i1++) new_text[i1] = text[i1]; // append change for (int i2=0; i2 <= size_t; i2++) new_text[i2+where] = to[i2]; // append text after change for (int i3=0; i3 <= get_size(text)-size_f-where; i3++) new_text[i3+offs2] = text[i3+offs1]; return new_text; } int main() { char text[MAX_SIZE] = "This is some text, yaddi yaddi."; char from[MAX_SIZE] = "some"; char to[MAX_SIZE] = "no"; int size_f = get_size(from); int size_t = get_size(to); int where = locate(text, from, size_f); if (where == -1) cout << "Value was not found anywhere inside the text"; else cout << replace(text, to, where, size_f, size_t); cin.sync(); cin.get(); return 0; }

Jul 15th, 2006, 5:38 PM	#2
Narue Professional Programmer Join Date: Sep 2005 Posts: 419 Rep Power: 3	>How can I return an array of characters from a function? You can't. Either return a pointer, or wrap the array in an object so that it can be copied as a single entity. >I'm probably doing it wrong Yes. Yes you are. (Toggle Plain Text) int get_size(char text[MAX_SIZE]) { for (int i=0;i<MAX_SIZE;i++) { if (!(text[i])) return i; } return MAX_SIZE; } int get_size(char text[MAX_SIZE]) { for (int i=0;i<MAX_SIZE;i++) { if (!(text[i])) return i; } return MAX_SIZE; } That's just funky. After the loop, i will be MAX_SIZE anyway, so why confuse readers more than you have to? (Toggle Plain Text) int get_size ( char text[] ) { int i = 0; while ( i < MAX_SIZE && text[i] != '\0' ) ++i; return i; } int get_size ( char text[] ) { int i = 0; while ( i < MAX_SIZE && text[i] != '\0' ) ++i; return i; } Of course, even better would be taking the max size as an argument rather than hard coding some arbitrary macro that destroys reusability. Tell me, what's wrong with the following code: (Toggle Plain Text) char* replace(...) { char new_text[MAX_SIZE]; ... return new_text; } char* replace(...) { char new_text[MAX_SIZE]; ... return new_text; } Too subtle? You're returning a pointer to local data. new_text is destroyed when replace returns, so your pointer to it refers to indeterminate data. This is undefined behavior. You must either dynamically allocate your memory, use an object that has an appropriate copy constructor, pass the array as an argument, or declare the array as static and suffer the pitfalls of that particular option. >cin.sync(); That's not guaranteed to do what you think. By the way, you're commenting the wrong parts of your code. Anything that isn't immediately obvious should have a comment, or be changed so that it's immediately obvious. The one thing I wondered while reading your code is precisely the comments you should have added: why are you reinventing the wheel when the standard library supports a huge portion of what you're trying to do? __________________ Even if the voices aren't real, they have some pretty good ideas.

Jul 15th, 2006, 7:35 PM	#7
Narue Professional Programmer Join Date: Sep 2005 Posts: 419 Rep Power: 3	>By the way, Narue, I'm not sure if you were calling my approach to it incorrect. Your approach was broken, and therefore incorrect. I thought I made that abundantly clear. >Because all I had to do was add the word static before the declaration of "new_text". You'll notice that I went out of my way to mention that a static local variable has pitfalls. Unsurprisingly, you've chosen to pick the worst of your options (next to the undefined behavior you already had) and learn for yourself why it's a bad idea. That's really the best way to learn, in my opinion. >Which is still returning a pointer, but it's not as if I had to do much. In all honesty, I feel that your design sucks and needs to be scuttled. But rather than focus on subjective topics, I gave you concrete advice on the assumption that you would find my questioning of your design as an insult. People tend not to take "delete it all and start over" very well, so I reserve that for particularly awful code and real life subordinates. __________________ Even if the voices aren't real, they have some pretty good ideas.

Thread Tools
Show Printable Version Email this Page
Display Modes
Linear Mode Switch to Hybrid Mode Switch to Threaded Mode

Similar Threads
Thread	Thread Starter	Forum	Replies	Last Post
changing size of an array	Eric the Red	Java	3	Apr 3rd, 2006 8:19 PM
2 dimension array of characters	brad sue	C	8	Mar 15th, 2006 9:40 AM
Returning an Array	Kilo	C#	3	Dec 4th, 2005 10:31 PM
Installing IPB 2.03	bh4575	Other Web Development Languages	0	Apr 23rd, 2005 2:36 AM
Returning An Array From a Function	ViZioN	C++	5	Feb 21st, 2005 6:45 PM

Jul 15th, 2006, 6:24 PM	#4
grumpy Programming Guru Join Date: Jun 2005 Location: Adelaide, South Australia Posts: 1,198 Rep Power: 5	Narue and Cache have answered your question quite well. I'll just add a few little specific notes; 1) Your functions are essentially recreating the wheel of the C standard library. For example, your get_size() function achieves more or less the same thing as the strlen() function that is specified in <string.h>. Except that you are adding a few other quirks that aren't particularly useful. 2) Since this is a C++ group you might want to use the standard string class (std::string which is specified in the header <string>). This is (rather loosely) an object wrapper for an array of char. And can be passed to an output stream directly.

Jul 15th, 2006, 8:37 PM	#10
grumpy Programming Guru Join Date: Jun 2005 Location: Adelaide, South Australia Posts: 1,198 Rep Power: 5	OK; just remember you asked. I am ignoring the fact that we could probably do all this with a few lines using either C or C++ standard library features, and that your comments don't help much for explaining what your code is doing. I also ignore the fact that you are using the C++ standard library by using <iostream>. 1) Narue's already mentioned the issue of hard-coding array sizes in function declarations. 2) It's not really all that obvious, without a close look, what i1, i2, and size_f are being used for in your locate() function. That gives you a maintenance problem if you pick this code up in a few months and need to improve it. 3) You have multiple return points from your locate() function. Again, that gives you a maintenance problem if you pick this code up in a few months and need to improve it. 4) Minor nit (as compilers will cope with this): you're using postincrement when preincrement will do the job. 5) It is not obvious what the variables offs1, offs2, i1, i2, i3 are being used for in replace(). Yes, I know offs1 and offs2 are offsets, but that only is obvious after looking at the rest of the code. 6) The size of your new_text array (or memory allocated) needs to be larger than MAX_SIZE 7) size_t is a type that is used frequently in both the C and C++ standard libraries. You are using it as a variable name, which means obscure incompatibilities with either the C or C++ standard libraries .... and you are using <iostream>. 8) Your replace() function dynamically allocates memory, and requires the caller to release it. This is an opportunity for the caller to experience a memory leak. Your main function fully exploits that opportunity by calling replace() repeatedly in a loop, and never releasing the dynamically allocated memory. 9) Minor: "if (text[i1] == '\0')" can be simplified to "if (text[i])" 10) Why, in main(), declare from and to as being of size MAX_SIZE when you are assigning their contents using string literals? 11) You main() function declares text as a pointer, and assigns it to new char [MAX_SIZE]. Then you reassign text to a string literal, and therefore another memory leak. 12) "if (!(where == -1))" is more readably expressed as "if (where != -1)" 13) As Narue mentioned, cin.sync() and cin.get() probably aren't doing what you intend, even if it appears they are. I haven't looked all that closely, so there are probably some other issues I've missed.

Currently Active Users Viewing This Thread: 1 (0 members and 1 guests)

Programming Forums > Application Development > C++

Returning An Array Of Characters