Printing the contents of a string array using pointers

Question

I'm currently studying C and I'm trying to just print the contents of a string array. I'm using pNames to point to the first char pointer and iterating from there.

A more proper approach would use this pointer, get a char* each time and use printf("%s", pNames[i]) to print a whole string. However, I thought I would try to print it character-by-character inside each string, as follows:

#include <stdio.h>
int main(int argc, char *argv[])
{
char *names[] = {
 "John", "Mona",
 "Lisa", "Frank"
};
char **pNames = names;
char *pArr;
int i = 0;
while(i < 4) {
 pArr = pNames[i];
 while(*pArr != '0円') {
 printf("%c\n", *(pArr++));
 }
 printf("\n");
 i++;
}
return 0;
} 

This code kind of works (prints each letter and then new line). How would you make it better?

• janos: @Michael it's your lucky day, fixed the bug for you, and adjusted the text accordingly. Don't get used to it, we normally review working code here.

Answer 1

Given that the code is really simple, I see mostly coding style issues with it.

Instead of this:

char *names[] = {
 "John", "Mona",
 "Lisa", "Frank"
};

I would prefer either of these writing styles:

char *names[] = { "John", "Mona", "Lisa", "Frank" };
// or
char *names[] = {
 "John",
 "Mona",
 "Lisa",
 "Frank"
};

The pNames variable is pointless. You could just use names.

Instead of the while loop, a for loop would be more natural.

This maybe a matter of taste, but I don't think the Hungarian notation like *pArr is great. And in any case you are using this pointer to step over character by character, so "Arr" is hardly a good name. I'd for go for pos instead. Or even just p.

You should declare variables in the smallest scope where they are used. For example *pos would be best declared inside the for loop. In C99 and above, the loop variable can be declared directly in the for statement.

The last return statement is unnecessary. The compiler will insert it automatically and make the main method return with 0 (= success).

Putting it together:

int main(int argc, char *argv[])
{
 char *names[] = { "John", "Mona", "Lisa", "Frank" };
 for (int i = 0; i < 4; ++i) {
 char *pos = names[i];
 while (*pos != '0円') {
 printf("%c\n", *(pos++));
 }
 printf("\n");
 }
} 

Actually it would be more interesting to use argc and argv for something:

int main(int argc, char *argv[])
{
 for (int i = 1; i < argc; ++i) {
 char *pos = argv[i];
 while (*pos != '0円') {
 printf("%c\n", *(pos++));
 }
 printf("\n");
 }
} 

• asn: Removing the loop and writing printf("%s\n",argv[i]). Does that work for printing all the names?

Answer 2

Other points not already mentioned:

Use const where possible

It's better to use const where possible so that it's clear when you are intending to modify things and clear when you're not. This helps the compiler help you find bugs early.

Avoid magic numbers

It's better to avoid having numbers like 4 in the program. If you want to change things later, you may well be left wondering "why 4? what does that mean?" Better would be to assign a constant with a meaningful name.

Use putchar rather than printf for single characters

The printf function is very useful, but putchar is often better suited for single-character output. The reason is that printf has to, at runtime, parse the format string, apply the arguments and then emit the results, while putchar only has to pass a character directly to the output. This particular code isn't exactly performance-critical but it's useful to acquire good coding habits from the beginning.

Use C idioms

It's more idiomatic C to have a loop like this for the characters:

while(*pArr) { /* ... */ }

rather than this:

while(*pArr != '0円') { /* ... */ }

Consider using a "sentinel" value for the end of a list

There are two common ways to iterate through a list in C. One is as you already have it, when you know how many values are in the list. The other way is to use a "sentinel" value -- some unique value that tells the program "this is the end of the list." For a list of names such as this program has, a rational choice for a sentinel value would be NULL.

Omit the return 0 from the end of main

Uniquely for main, if the program gets to the end of the function, it automatically does the equivalent of return 0 at the end, so you can (and should) omit it.

Result

Using all of these suggestions, one possible rewrite might look like this:

#include <stdio.h>
int main() 
{
 const char *names[] = { "John", "Mona", "Lisa", "Frank", NULL };
 for (int i=0; names[i]; ++i) {
 const char *ch = names[i]; 
 while(*ch) {
 putchar(*ch++);
 putchar('\n');
 }
 putchar('\n');
 }
} 

Answer 3

With a null sentinal the follwing is pretty simple:

for(const char** pNames = names; *pNames; pNames++) {
 /* *pNames points to one of the strings, do something with it */
 const char *pName = *pNames;
 while (*pName) {
 putchar(*pName++);
 }
 putchar('\n');
 }

Sure you don't need the {} in the while loop but I prefer to put all block bodies in {}'s since there have been some well known problems when code like:

 if (condition) 
 dosomething();

was modified to read:

 if (condition)
 dosomething();
 dommorestuff();

Answer 4

I find this a bit easier to read, and it avoids both magic numbers and a NULL at the end of the array:

#include <stdio.h>
int main()
{
 char *names[] = { "John", "Mona", "Lisa", "Frank" };
 int elements = sizeof (names) / sizeof (names[0]);
 for (int i = 0; i < elements; i++) {
 char *p = names[i];
 while (*p)
 putchar(*p++);
 putchar('\n');
 }
return 0;
}

Use printf("%c\n", *p++) if you want to print one character on a line, like your example did.