Fix code style

[pedrolmcastro]

Excessive comments, wrong indentation, inconsistent snake_case and bloated source file snaskii.c

[monacofj]

Would you detail a bit more about what is exactly wrong so we can fix it?

Currently, this is what is intended:

• Comments are deliberately plentiful so as to make easy for newcomer programmers
• Indentation follows the GNU Coding standards. Not everybody likes GCS, and we may change it if that fits the taste of the community. To that end, we should have some style-guide document.
• Case is in principle standard-C traditional, with snake_case for vars and functs, upper-case macros.
• What is bloating the code?

Not arguing against it. We just need to define a style than.

[monacofj]

Changed labe from good-first-issue to question because we need to discuss this.

[pedrolmcastro]

I am sorry I did not illustrated my points here before

Excessive comments

I get that you would want the code to be readable for everyone, but I really don't think that "The main function" at the top of a function called main() helps, or even commenting "Coordinate x of the snake's head" in front of a variable x in a struct that composes a head in the struct typedef to snake_t

I would argue you could also remove some comments like "Number of columns of the scene" just by changing the define from NCOLS to SCENE_COLUMNS. Also on this topic of comments, I think moving away from the c90 standard can be beneficial since, by using something more modern like c11 or even c99, would allow you to use single line comments with // instead of /**/

Wrong indentation

I think the problem I am trying to point here is caused by mixed usage of tabs and spaces, in my editor at least I see things like:

// Open and close braces in a different indentation than the statement

  for (j=0; j<NCOLS;j++)
{
 ...
}

// Missaligned comments

struct timeval beginning,	/* Time when game started. */
  now,                       /* Time now. */
  before,                   /* Time in the last frame. */
  elapsed_last,	                /* Elapsed time since last frame. */
  elapsed_total;               /* Elapsed time since game baginning. */

Now a particular oppinion is that putting braces in every for and if makes the code little more beginner friendly, for example:

// The following snippet

for (k=0; k<nscenes; k++)
  for (i=0; i<NROWS; i++)
    for (j=0; j<NCOLS; j++)
      scene[k][i][j] = BLANK;

// Would become (using c99 or higher to use initial declaration)

for (int k = 0; k < nscenes; k++)
{
  for (int i = 0; i < NROWS; i++)
  {
    for (int j = 0; j < NCOLS; j++)
    {
      scene[k][i][j] = BLANK;
    }
  }
}

Inconsistent snake_case

Here I am not complaining about the usage of snake_case, I am just saying that its application in the code seems inconsistent and arbitrary for me. For example, the function init_game() uses it, but clearscene() or readscenes() don't, so I recommend changing them to clear_scenes() and read_scenes() (or load_scenes() perhaps to fit its purpose better). The UPPER_CASE constants are ok

Bloated source file

My concern here is that the snaskii.c file already has 400+ lines without any game logic, I think creating a scene.c/scene.h for the common functions of clearing, loading and drawing scenes would help, also a movie.c/movie.h to handle the intro movie and a times.c/times.h to struct together the time variables used for the menu (beginning, now, last ...) and provide functions like update_times() to deal with them

Input and output functions

After another look at the source code, I noticed the usage of the standard input and output functions like getchar() and printf() in conjunction with the ncuses.h library, this is not recommended, though, since it can lead to some problems, the fix would be to use the equivalents given by the library like wgetch() and printw()

[monacofj]

"Coordinate x of the snake's head"_ in front of a variable x in a struct that composes a head in the struct typedef to snake_t

It should make it easier to user text search to locate code, but this is a matter of opinion. I'm ok with the removal of non-essential comments.

I would argue you could also remove some comments like "Number of columns of the scene" just by changing the define from NCOLS to SCENE_COLUMNS.

I'm also ok with the suggestion (I myself don't like super-long-verbose-java-like symbol names, but what the community prefers is fine for me).

Also on this topic of comments, I think moving away from the c90 standard can be beneficial since, by using something more modern like c11 or even c99, would allow you to use single line comments with // instead of /**/

c90 is the maximum of portability, but I'd be ok with c99. I think C11 is overkill at the expenses of portability.

Wrong indentation

Fair enough.

// Missaligned comments

struct timeval beginning, /* Time when game started. /
now, / Time now. /
before, / Time in the last frame. /
elapsed_last, / Elapsed time since last frame. /
elapsed_total; / Elapsed time since game baginning. */

Sure, it's ugly.

Now a particular oppinion is that putting braces in every for and if makes the code little more beginner friendly, for example:

I'm indifferent if the hole thing is less than half-screen. But we can decide on some standard.

Here I am not complaining about the usage of snake_case, I am just saying that its application in the code seems inconsistent and arbitrary for me. For example, the function init_game() uses it, but clearscene() or readscenes() don't, so I recommend changing them to clear_scenes() and read_scenes() (or load_scenes() perhaps to fit its purpose better). The UPPER_CASE constants are ok

I agree. The symbols defined in the program are snake_case, but the symbols imported from libncurses may be different. Where possible, consistence would help.

Bloated source file

My concern here is that the snaskii.c file already has 400+ lines without any game logic, I think creating a scene.c/scene.h for the common functions of clearing, loading and drawing scenes would help, also a movie.c/movie.h to handle the intro movie and a times.c/times.h to struct together the time variables used for the menu (beginning, now, last ...) and provide functions like update_times() to deal with them

That would be a good idea. snaskii.c is just a skeleton.

Input and output functions

After another look at the source code, I noticed the usage of the standard input and output functions like getchar() and printf() in conjunction with the ncuses.h library, this is not recommended, though, since it can lead to some problems, the fix would be to use the equivalents given by the library like wgetch() and printw()

Yes, this is a problem.

In short, I think an independent issue might be created to fix each of the pointed problems. Then this issue may be closed.

[CerqueiraMatheus]

As a suggestion, you could write an issue with a task list, like

• First improvement
• Second
• ...

And, then, allow multiple PR's to solve it.

If you do this, do not forget to ask collaborators to mention the new issue, but not using the keyword closes (e.g. this Pandas issue).

[monacofj]

Alternatively, since the fixes are not mutually dependent, perhaps independent issues, with each PR closing one of them, may also be useful to keep track of progress and allow automatic issue closing. Just my cents.

[pedrolmcastro]

I think creating multiple PRs for things like correcting the mixed tabs and spaces indentation in parallel with the other changes would just create unnecessary merge conflicts. Another option I thought was to create each PR after the previous was merged, but I think this would slow everything due to the necessary approvals. So ultimately I think the best option is to create a single PR and detail well every change I made on it.

On the same day I opened this issue, I created a branch to work on it and made the changes I specified latter (in my previous comment), since then I have a commit/push ready in my machine but I did not have time to work in any of it in the last few days, now I can push it to the branch and create a PR fixing everything I pointed out.