review - Githubissues

This is a pedantic and subjective and opinionated review.
If it would be a program for one time unrelated job and I wouldn't have time, I would merge it.
If it would be a feature to a bigger database, I would require to sprinkle the code with static const and assert().
Do not use global variables https://wiki.c2.com/?GlobalVariablesAreBad
https://wiki.c2.com/?ThreeStarProgrammer Use structures. Don't init_func(data **p) { *p = something(); } prefer: struct stuff { data *data }; init_func(struct stuff *p) { p->data = something }.
You can research here: https://wiki.sei.cmu.edu/confluence/display/c/2+Rules

Medium: https://github.com/JeremiahCheatham/Super-Clock/blob/main/superclock-sdl.c#L15 Globals should be static, this is one file.

Medium: https://github.com/JeremiahCheatham/Super-Clock/blob/main/superclock-sdl.c#L17 Pointers should be constant const char *const TITLE= ...

Low: ^^ Subjective: I would go with an array with initialization static const char TITLE[] = "stuff";.

Low: subjective: https://github.com/JeremiahCheatham/Super-Clock/blob/main/superclock-sdl.c#L15 Subjective: I would go with unsigned or size_t for loop iterators over an array. This applies to 90% of the loops.

Medium: https://github.com/JeremiahCheatham/Super-Clock/blob/main/superclock-sdl.c#L23 Do not use global variables!

Low: Required: https://github.com/JeremiahCheatham/Super-Clock/blob/main/superclock-sdl.c#L23 They should be static.

Medium: ^^ Subjective: Globals should be within one structure or within one prefix namespace. Overall use structures to pack related data together. This could be a structure.

Medium: https://github.com/JeremiahCheatham/Super-Clock/blob/main/superclock-sdl.c#L29 All functions should be static.

Medium: https://github.com/JeremiahCheatham/Super-Clock/blob/main/superclock-sdl.c#L52 Use initialization SDL_Texture *texts[..] = { 0 };.

Medium: https://github.com/JeremiahCheatham/Super-Clock/blob/main/superclock-sdl.c#L100 show_fps ^= 1;.

High: https://github.com/JeremiahCheatham/Super-Clock/blob/main/superclock-sdl.c#L113 and https://github.com/JeremiahCheatham/Super-Clock/blob/main/superclock-sdl.c#L291 - remove argument to get_time, it is unused....

High: ^^ Enable compiler warnings to detect unused arguments. With gcc -Wall -Wextra, I recommend also -Wwrite-strings.

Low: Subjective: https://github.com/JeremiahCheatham/Super-Clock/blob/main/superclock-sdl.c#L127 I fear digits[i] + offset is out of bounds, I would add assert(digits[i] + offset < sizeof(texts)/sizoef(*tesxts));

Required: https://github.com/JeremiahCheatham/Super-Clock/blob/main/superclock-sdl.c#L147 Errors go to stderr, not stdout.

Low: subjective: https://github.com/JeremiahCheatham/Super-Clock/blob/main/superclock-sdl.c#L160 Hm... I see how in this case, because SDL_DestroyWindow(NULL) is fine, the error handling actually cleans up properly. Related: https://www.kernel.org/doc/html/v4.17/process/coding-style.html#centralized-exiting-of-functions .

Low: https://github.com/JeremiahCheatham/Super-Clock/blob/main/superclock-sdl.c#L189 Arrays should be const. Below the same.

Required: https://github.com/JeremiahCheatham/Super-Clock/blob/main/superclock-sdl.c#L256 const SDL_Color color = colors[i];, why the if...

Required: https://github.com/JeremiahCheatham/Super-Clock/blob/main/superclock-sdl.c#L266 SDL_Surface *const text_suft = TTF....(font, i == 2 ? "0" : "1", colors[1]). More coffee!!

High: https://github.com/JeremiahCheatham/Super-Clock/blob/main/superclock-sdl.c#L292 Remove static

Very low: https://github.com/JeremiahCheatham/Super-Clock/blob/main/superclock-sdl.c#L294 Consider localtime_r .

High: https://github.com/JeremiahCheatham/Super-Clock/blob/main/superclock-sdl.c#L299 Remove static

High: https://github.com/JeremiahCheatham/Super-Clock/blob/main/superclock-sdl.c#L300 Use snprrintf, do not use sprintf.

High: https://github.com/JeremiahCheatham/Super-Clock/blob/main/superclock-sdl.c#L306 Remove static. Use get_time(), it's right above.

Medium: https://github.com/JeremiahCheatham/Super-Clock/blob/main/superclock-sdl.c#L325 Remove global variables. Refactor to take a structure as an argument, and keep state in a structure.

High: https://github.com/JeremiahCheatham/Super-Clock/blob/main/superclock-sdl.c#L330 If ticks increment between the call to if and the call to next_time =, the value of next_time will increment and stall (in time). Just next_time += 1000.

Very high: Required: https://github.com/JeremiahCheatham/Super-Clock/blob/main/superclock-sdl.c#L327 Properly handle overflow/warparound. From https://wiki.libsdl.org/SDL_GetTicks This function is not recommended as of SDL 2.0.18; use SDL_GetTicks64() instead, where the value doesn't wrap every ~49 days. Use https://wiki.libsdl.org/SDL_GetTicks64 . Anyway, there are PCs (like mine) that work longer than 49 days. Properly handle warparound or use something else.

Medium: https://github.com/JeremiahCheatham/Super-Clock/blob/main/superclock-sdl.c#L338 Remove global variables. Refactor to take a structure as an argument and keep state in that structure.

High: Required: https://github.com/JeremiahCheatham/Super-Clock/blob/main/superclock-sdl.c#L341 SDL_GetTicks() returns an Uint32, not an int. Assigning to int will overflow when the value is > INT_MAX. As above, use 64 version.

High: Required: https://github.com/JeremiahCheatham/Super-Clock/blob/main/superclock-sdl.c#L350 This is odd that it has a minus. It's way more odd that carry_delay is a float. Why is it a float? It should be an UInt32.

Low: https://github.com/JeremiahCheatham/Super-Clock/blob/main/superclock-sdl.c#L372 = NULL; - what for? Just let it be, you are exit()ing below anyway.

Low: https://github.com/JeremiahCheatham/Super-Clock/blob/main/Makefile#L2 As above, add -Wall. c99 is 20 years old! - use -std=c11. Do CC ?= gcc CFLAGS ?= stuff, let users overwrite it. Overall, consider CMake or Meson. Nice Makefile.

Low: https://github.com/JeremiahCheatham/Super-Clock/blob/main/install-superclock-sdl.sh#L2 use $UID in Bash.

High: Check scripts with shellcheck .

High: https://github.com/JeremiahCheatham/Super-Clock/blob/main/install-superclock-sdl.sh#L29 Do not install stuff to /usr/bin! Manual scripts install stuff to /usr/local/*. See https://refspecs.linuxfoundation.org/fhs.shtml . If you want in /usr/bin, create a package for your linux distribution.

Low: https://github.com/JeremiahCheatham/Super-Clock/blob/main/install-superclock-sdl.sh#L29 use install instead of cp.

Hello Wow thanks for the huge amount of helpful information. I would like to walk though some if it's ok. This top lines i keep reading but am unsure of the meaning. Do you mean i should use multiple source files?

"If it would be a program for one time unrelated job and I wouldn't have time, I would merge it. If it would be a feature to a bigger database, I would require to sprinkle the code with static const and assert()."

When you say don't use global variables do you mean all of my global variable including the const or only the no const ones? From what i read about global variables i thought you should try to not use but if it's something global then it was ok? Should i move them all down to the main function? I did it so i could easily change those aspects of the program in 1 place at the top.

I think i read several times you mentioning using structs. Should i be placing those global variables and const in a struct at the top so as to 1 easily find and edit if needed and 2 reference them as a struct?

This top lines i keep reading but am unsure of the meaning

What I mean is that the code is not written in a modular way, because of the use of global variables and static state within functions and lack of structs grouping variables. As a lone program - it's totally fine, it can be as it is.

Do you mean i should use multiple source files?

No, one file is fine.

When you say don't use global variables do you mean all of my global variable including the const or only the no const ones?

Excluding const. Constants are nice on top of the file, it's nice.

i thought you should try to not use but if it's something global then it was ok?

This just strongly depends on the size of the project. For this one-file small project - it's totally fine,

The bigger project you have, the harder it is to keep track of everything. Global variables just straight lead to spaghetti code. Generally, you want object-oriented programming.

should i move them all down to the main function? ....

You can leave it as it is, it's totally fine for a short program with one file - the review is rather pedantic and subjective. I would do like:

struct superclock {
   SDL_Window *win;
   SDL_Renderer *rend;
   bool running;
   bool show_fps;
   bool show_time;
   unsigned short style;
   struct digits {
        unsigned short digit;
        SDL_Rect rect;
        SDL_Texture *text;
    } digits[TEXTS_LENGTH];
   /* etc... maybe multiple smaller structs? Dunno, depends on the project.  */
   /* related http://www.catb.org/esr/structure-packing/  */
};

static void superclock_init(struct superclock *t) {
  const struct superclock init = {
     .style = 1,
  };
  *t = init;
}
int superclock_setup_sdl(struct superclock *t) { ... }
int superclock_create_texts(struct superclock *t);
int superclock_something_else(struct superclock *t);

int main() {
   struct superclock sc;
   superclock_init(&sc);
   if (!superclock_setup_sdl(&sc)) { ... }
   superclock_create_texts(&sc);
}

But if the code works as it is now, leave it as it is.

i read several times

Och, please don't! The above are all suggesting. The Properly handle overflow/warparound -> displaying fps in terminal is not the main focus of the program, so it can stay as it is anyway (just move to 64). Rather, the above suggestions are pointers, topics to research about, or stuff that just caught my eye, nothing too much important.

The overall code looks really nice - there is error handling, cleanup, no obvious undefined behavior.

Also https://codereview.stackexchange.com/ ;)

Yeah i am googling a lot lol. I didn't know if initializing char[] actually included the null terminator. now i know lol.

SDL_Texture *texts[..] = { 0 };

So why should i use {0} instead of NULL. I did this because of the pointer created but wasn't creating the textures yet. So what is the difference with the 2? I though it was always NULL for pointers.

why should i use {0} instead of NULL.

Well, just because it's shorter ;) . https://en.cppreference.com/w/c/language/initialization . Generally = {0} is a way of initializing anything to zeros.

You can use SDL_Texture *texts[..] = { NULL };, but only the first element will be initialized with NULL and the rest with 0... which practically is the same anyway.

So what is the difference with the 2?

Ugh, och, this is a hard question. Practically nowadays, there is no difference in value, there is a difference with meaning - NULL is a null pointer constant, 0 is a int with value 0. Generally, have fun reading http://c-faq.com/null/index.html and overall http://c-faq.com/ .

Do you mean

SDL_Texture *texts[TEXTS_LENGTH]; for (int i = 0; i < TEXTS_LENGTH; i++) texts[i] = NULL;

to be changed to

SDL_Texture *texts[TEXTS_LENGTH] = {0};

I think i tried something like this when i first wrote it but used NULL and it gave me error about something so i used a loop to initialize pointers.

So the logic behind my FPS. I actually wrote this myself i don't know how it's supposed to be done. So this was out of my own brain and i tested it alot lol. This is also why there is a show FPS so i could see if it was working and accurate. This was a separate file in earlier coding project i was working on astroids. So here is the logic.

So if i want 60FPS i need a frame time of 16.666666666 for ever. But i don't want to delay my program for that much. Because time has already passed during this program loop. So i need to know how much time i have used already and subtract that from my target (Sort of). Now i need to delay the program from that long. Then i need to know if i went over or under my target and add that to the next frame. since it's a float to begin with and there is no way to delay for a fraction. Also the delay is not that accurate anyway. So i don't carry over the fraction say 0.6666 no. I actually carry over what ever plus or minus that was needed to most accurately reach that frame. But i don't let it carry to high of a number either way. So yes it needs to be a signed float.

example.

delay 16.6666 target (+ last frame carry) coding took 3.2 need to delay 13. 4666 actually try to delay 13 reality delays 14.5 so my total time was 17.5 -0.8334 is carried over to next frame.

This gives a very highly accurate FPS.

So this is very confusing you said to use SDL_GetTicks64 but that is not defined. And they say 2.0.18 however 2.0.16 is the current version of sdl2 on there website also the current version of SDL2 on archlinux servers. There is no 2.0.18 and SDL_GetTicks64 is not defined.

I see the int should be an Uint32. But i don't know how to go about wrap around handling as you said. if we exclude the whole SDL_GetTicks64 what is the correct way i should handle the 32 does it wrap around or what happens and does it matter? since i only need the time between frames?

JeremiahCheatham / Super-Clock-archived

review #1