-
Notifications
You must be signed in to change notification settings - Fork 11
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix code style #9
Comments
Would you detail a bit more about what is exactly wrong so we can fix it? Currently, this is what is intended:
Not arguing against it. We just need to define a style than. |
Changed labe from good-first-issue to question because we need to discuss this. |
I am sorry I did not illustrated my points here before Excessive commentsI 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 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 Wrong indentationI 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_caseHere 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 Bloated source fileMy 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 Input and output functionsAfter another look at the source code, I noticed the usage of the standard input and output functions like |
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'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).
c90 is the maximum of portability, but I'd be ok with c99. I think C11 is overkill at the expenses of portability.
Fair enough.
Sure, it's ugly.
I'm indifferent if the hole thing is less than half-screen. But we can decide on some standard.
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.
That would be a good idea.
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. |
As a suggestion, you could write an issue with a task list, like
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). |
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. |
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. |
Fix codestyle and basic modularization
Excessive comments, wrong indentation, inconsistent snake_case and bloated source file snaskii.c
The text was updated successfully, but these errors were encountered: