how would you have written this code?

[pietrolunz]

Hello, i'm new to C programming, and i wanted an opinion on this code. I was wondering if it could have been written in a more efficient way. By now the only bug i report is a segfault if i enter a alphanumerical value as the variable "o". Thank you in advance

#include <stdio.h>

main()
{
typedef struct plr {char player_name[30];} player;
int o;
printf("Insert players number:");
scanf("%d", &o);
player no_player[o];

if (o == 1)
	printf("You chose singleplayer\n");
else if ( o >= 1)
	printf("You chose %d players mode\n", o);
else
	printf("insert a valid value\n");

int i, e = 0;

for(i = 1; i <= o; i++, e++){
  printf("insert player %d name:", i);
  scanf("%s", &no_player[e].player_name);
  };
for(i = 0, e = 1; i < o; i++, e++){
  printf("Player %d: %s\n",e , no_player[i].player_name);
  };
}

[ssharish2005]

>player no_player[o];
You cannot have variable length of array. You can overcome this using dynamically allocating memory at the runtime.

And also you main seg fault is from here

scanf("%s", &no_player[e].player_name);

You dont need to provide the address of a string. The name of the string it self holds the base address of chat array. So what you need is this

scanf("%s", no_player[e].player_name);

And also you code indentation is very poor. If you had to get any help from this board. You need some well indented and a readable code.

ssharish

[quzah]

Arrays start at zero. Your first loop is going to skip no_player[ 0 ], and will go past the end when you try to reach no_player[ o ]. If you have an array with 3 elements, you get no_player[ 0 ], no_player[ 1 ] and no_player[ 2 ]. no_player[ 3 ] is an error to access.

>player no_player[o];
You cannot have variable length of array. You can overcome this using dynamically allocating memory at the runtime.

You can in C99.


Quzah.

[Salem]

Some points.

1. Be explicit about the declaration of main, and the return result.

int main ( ) {
    // code here
    return 0;
}

2. Avoid single letter variable names for anything other than loops (where i and j are preferred). For example, your 'o' variable should be something like
int numStudents;

3. General level of indentation could be better.

4. printf("insert a valid value\n");
In order to do this, you would need.

if ( scanf("%d", &var ) == 1 ) {
  if ( var == 1 ) // single player
  else if ( var > 1 ) // multiplayer
  else // <= 0 - not good
} else {
  int c;
  while ( (c=getchar()) != EOF && c != '\n' );  // stdin cleanup for next scanf call
  printf("insert a valid value\n");
}

5. player no_player[o];
This kind of variable length array is a feature of C99 (some newer compilers) or an extension to an older compiler. It might not work everywhere in other words.

6. for(i = 1; i <= o; i++, e++)
It would probably be clearer to have just one loop variable starting at 0, then have
printf("insert player %d name:", i+1);

7. The closing brace of code blocks do not need a ;

8. printf("Insert players number:");
If you want to be sure of seeing this before input, you should do
printf("Insert players number:");
fflush(stdout);

[Salem]

@Quzah
> Arrays start at zero. Your first loop is going to skip no_player[ 0 ], and will go past the end when you try to reach no_player[ o ]
The array is subscripted by e, which starts at 0
But it is very confusing to say the least - I thought it was an overrun first glance.

Using e as the subscript in the first loop and i as the subscript in the second loop is very BAD form.


@ssharish2005
> And also you main seg fault is from here
& on an array name is benign. You get the same pointer value as a result (it's just the wrong type).

Their "segfault" was down to typing in garbage input to begin with, leaving o uninitialised, and then creating a variable length array with some garbage value.

[quzah]

The array is subscripted by e, which starts at 0
But it is very confusing to say the least - I thought it was an overrun first glance.

Using e as the subscript in the first loop and i as the subscript in the second loop is very BAD form.

Ah, I see it now. That's one thing I hate about C99, copying C++'s "scatter variable declarations all around". Plus, I don't know why he didn't initialize e in the first chunk of his for loop so everyone would clearly see that.


Quzah.

[pietrolunz]

Plus, I don't know why he didn't initialize e in the first chunk of his for loop so everyone would clearly see that.


Quzah.

Your right, but i didn't because i thought that writing it in that form

 for(e = 0; e < o; e++){
  printf("insert player %d name:", e + 1);
  scanf("%s", &no_player[e].player_name);}

would have lead to increase the value of e by 2 units instead of one each loop, because of the e + 1

[quzah]

+ 1 to something doesn't increment it. It just gives you its value plus one.

prinf( "%d", 1 + 1 );

That doesn't give you an lvalue, neither does this:

printf("%d", e + 1 );

e stays the same, you are just getting that value and adding one to that value. That's why you have to do:

e = e + 1;

...to increment by adding one.

printf("%d", 1 + e );

You could do it that way and have the same effect, which clearly illustrates that it isn't producing an lvalue.


Quzah.

[TheBigH]

This is a very small nitpick, but it's good to avoid naming a variable 'o' or 'O', because at a quick glance it looks like a zero. There's a similar problem with 'l' (ell) and '1' (one).

In fact, it's good to give your variables descriptive names. You could have called 'o' something like 'number_of_players', which makes it clear exactly what the variable is for. It's longer to type but it makes your code easier to understand for other people (or for yourself in six months time, which is exactly the same).

[CommonTater]

Hello, i'm new to C programming, and i wanted an opinion on this code. I was wondering if it could have been written in a more efficient way. By now the only bug i report is a segfault if i enter a alphanumerical value as the variable "o". Thank you in advance

#include <stdio.h>

main()
{
typedef struct plr {char player_name[30];} player;
int o;
printf("Insert players number:");
scanf("%d", &o);
player no_player[o];

if (o == 1)
	printf("You chose singleplayer\n");
else if ( o >= 1)
	printf("You chose %d players mode\n", o);
else
	printf("insert a valid value\n");
int i, e = 0;

for(i = 1; i <= o; i++, e++){
  printf("insert player %d name:", i);
  scanf("%s", &no_player[e].player_name);
  };
for(i = 0, e = 1; i < o; i++, e++){
  printf("Player %d: %s\n",e , no_player[i].player_name);
  };
}

FWIW... the code I boldfaced is unnecessary... the user knows what they entered and can still see it on the screen. Plus the way you ask for the number of players is unclear, try... printf("How many players? ");

Also... putting an array inside a struct, as the struct's only element is an unnecessary complication in the naming of variables, just make an array of strings.

Also the way you construct your array of structs is C-99 specific --therefore not wrong-- unless you don't have a C-99 compiler... Then it's an error.

And, one last thing... it not main() ... it's int main (void) and it returns an errorlevel on exit...

[quzah]

Also the way you construct your array of structs is C-99 specific --therefore not wrong-- unless you don't have a C-99 compiler... Then it's an error.

Are you saying that having Turbo C makes his program an error?


Quzah.

[CommonTater]

Are you saying that having Turbo C makes his program an error?
Quzah.

LOL... well yes... I suppose I am. Certainly using Turbo C in this day and age is a serious error in judgement.