Review C Implementation

[hohmannr]

Disclaimer: I have not built the code yet, since I am only on an iOS device atm.

Warning: This is an opinionated review. This means, some of the review comments (mainly about conventions, are my preference). They are marked with a [opinion] tag.

C Implementation

Conventions

• [opinion] Normally in C you would use snake case for file names, ergo superclock_sdl.c.
• [opinion] Do not name variables/functions MY_... or my_.... It does not state clearly what it means. Does my_exit() exit the program? What would a normal exit() do. Try to stick to concise, but descriptive names.
• [opinion] This one is really nit picky and does not make any difference in real programs^^: Declare constant variables of atomic types (int, unsigned, long, bool, ...) as a preprocessor #define, then they do not need to have memory associated to them, since the preprocessor just replaces them in your source code.
• Good job forward declaring! In a larger program you would separate the declarations into their own superclock_sdl.h header file. But this is fine.

Code

• Be careful when using preprocessor statements, basically all statements that are not atomic types, should be in parenthesis. Here you are fine, but this should become a habit. I give you an example:

  
  /* FOO and BAR are unsigned types with only one bit on 1 and others on 0 (similar to SDL_INIT_XXX) */
  #define FLAGS FOO | BAR

unsigned flags = FLAGS & 0;

You expect that `flags == 0`, but it is `flags == FOO`, since bit operations get evaluated right to left. This would not have happened when doing `#define FLAGS (FOO | BAR)`

- There is no need for `main(void)`, you can just leave it blank or accept input with `main(int argc, char *argv[])`. I get why you did it there. You know that when forward declaring a function `int foo();`, the function interface can accept an arbitrary amount of arguments. So when forward declaring, you would need to use `int foo(void);` to make sure that no arguments can be given to the function. On the function implementation though, leaving blank will declare that it takes no arguments. So `int foo() { /* code */ }` is the same as `int foo(void) { /* code */ }`, but when declaring `int foo(void);` is not the same as `int foo();`, since the first one accepts none arguments, while the second one accepts an arbitrary amount of arguments.
- Very nice of you to declare all variables on top, this is a convention a lot of C programmers do, but it is not necessary anymore in C99 (the C dialect you use). But basically all of my colleagues and me still do it this way. Also good to initialize the pointers to `NULL`, which is not needed here, since SDL does that too when it fails, but its fine practice.
- Error handling: When initializing SDL in `setup_sdl()`, you return a `bool` which tells if it was succesful or not. For the failures, you print out why it failed in the same function. This is not good practice, since you deal with the error twice, once in `setup_sdl()` (printing) and then again in `main()`. You should handle an error once and not at multiple places, this becomes frustratingly complex on larger projects. A better way would be to have the function return an `int` which acts as an error code. This decouples the the error handling from the function itself and forces the caller (in this case `main()`) to handle the error. The print statements could go either into `my_exit()` along with a switch statement that analyzes the error code or into `main()`. I would put them into `my_exit()` though.
- Then you populate your rects. In `populate_rects_res()` you have a switch statement. First of all, please always write a comment when you do a fall through, e.g. for `case 1: /* fallthrough */`. This makes it more obvious in large swtich statements, that you intended to have a fallthrough there and not simply just forgot the `break;`. Here it is pretty obvious, but I have had bugs that haunted me for days just because someone had a implicit fallthrough. The `default` case should always be last as for convention. Here you would put cases 1, 2 and default at the bottom of the switch. You do not need curly braces after a case-statement. Switch is `goto` in disguise, once `break;` is reached, it jumps out of the switch, no curly braces needed.
- `[opinion]` A big no-no for me is declaring variables inside a switch statement. In my understanding, every case statement should have the same variables and should not spawn in magical variables from somewhere to do something with them inside this specific switch block. Here they all have the same variables, declare them out side and initialize them inside the switch. I see that in some cases the arrays have different dimensions, this also seems odd to me, but I cannot further reason why. Comments would be nice.
- Also it would be nice to declare the function argument `SDL_Rect *rects` as `SDL_Rect rects[]`. This makes it clear to the reader that its not a single pointer but an array of rects. Also please always provide the length of the array when you pass it into a function, since the function cannot know its length other wise. I would declare it as follows `void populate_rects_res(SDL_Rect rectv[], unsigned rectc, SDL_Window *win) { /* ... */ }`.
- Also: When I saw what you do with `rects[i]`, in my head it screamed: *Segmentation Fault Error*. You implicitly assume that `rects[i]` is never out of bounds, without passing the length of `rects[]`. You should NEVER do that. A better solution is to pass the length and add another for loop that iterates over the length. Or you add a comment why the array will never be out of bounds (and you should only do that if you know it for sure, which normally happens in 0.1% of the cases). Take-away: Never assume the length of an array when it is passed to a function, pass the length as a separate argument.
- Then you start the main loop and poll for events. I think you have missed a `break;` (see the stuff from above already comes in handy) at `case SDL_USEREVENT`, this means an implicit fallthrough and when an `SDL_USEREVENT` is registered, then it will also run the Code for `SDL_KEYDOWN` until the first `break;` is hit. The rest is very good!
- Afterwards you get the time via `get_time()`. There is no need to declare `rawtime` to be `static` since it changes everytime when you call the function anyway. Static types in function keep their value in between function calls, but you set the value here to a new one by calling `time(&rawtime);` which changes the value, so persistence is not needed in between function calls. Static variables inside functions are very very raw and are usually used to count the function calls or encode a state (such as FPS timings, as you use later). Also, you pass `timeinfo` as a reference argument and return it as a value, chose one of two.
- In `time_in_title()` same applies for the static variable there. Also, when passing a pointer to a struct that you do not modifiy in the function, mark the argument with `const`. This makes clear to the reader that you do not intend to change anything in the object, ergo `void time_in_title(const struct tm *timeinfo, SDL_Window *win) ...`.
- `convert_to_binary()`: Same for static variable applies here. Also you pass `timeinfo` as an argument, but set it again shortly after (this you have already done in `get_time()` and it is the exact same code, it is not needed, you passed it already as an argument. Also please specify `digits[]` as an array in the arguments rather than a pointer and pass its length, like I discussed above, already. Also some comments would be nice for the magical numbers for `i`. Also I would not use an int array to store binary, you could look into using a single `unsigned`, but it seems to be connected to the way you get the later in `main()`, so I give you the benefit of the doubt.
- In `print_fps()` and `fps_delay()` the static variables are fine, since they encode a state of the function.
- In `show_time_timer` `SDL_PushEvent()` can return an error which is not checked for.

## General
- `[opinion]` I would name functions like `noun_verb`, e.g. `setup_sdl` to `sdl_setup` or `populate_rects_res` to `rects_populate_res`. This makes it clearer what object the function handles and what it does to that object.
- `[opinion]` Your one line if-statements would look better if you put them into two lines like:

if (condition) statement;

- `[opinion]` It is always beautiful to have new line before the `return` at the end of the function.
- You should stick to a coding style. I quite like the one from the linux kernel: https://www.kernel.org/doc/html/v4.10/process/coding-style.html

# Summary

All in all, this is a fine project! Great job. You are beginning a great journey into C. Do not be discouraged of the comments I gave. They are there to help you and sometimes I also make mistakes! No program is bugfree! Keep on learning! :+1:

[JeremiahCheatham]

Hello I have been working on the code when i have time. I did changed the fps_delay and fps_show functions a lot because of fear of over flow so it's much bigger now. I also changed the names of the functions as per what you said about noun verb. I did change the default in the switching case to the bottom and removed the break. But I didn't change how it works because it's actually changing the geometry of the clock based on the style. So this clock displays the time in 3 groups of 8. The default compact style the 8 bits are broken into 2 vertical columns of 4. So the first 2 columns of 4 is hours on the left and center 2 is minutes and right 2 is seconds. However pressing S key will change it into a single column of 8 for each either vertically or horizontally so that is 3 styles. But there is also dark box for nothing and light box for 1. so you see light boxes representing the bits. But if you keep pressing S you will come to dark boxes with a light 0 or light 1 written on them. So there is 3 layouts and 2 styles of showing 0 or 1. This is why there is fall through in the case. Also why in creating the textures only surfaces index 1 is light and all else darker. because 2 and 3 are dark with light 0 and 1 on them. Hope that makes sense. I will upload some screen shots later.

[JeremiahCheatham]

There is a screenshot at the top now and the rest at the bottom of the read me file.

[JeremiahCheatham]

Also: When I saw what you do with rects[i], in my head it screamed: Segmentation Fault Error. You implicitly assume that rects[i] is never out of bounds, without passing the length of rects[].

So i was really unsure how to do this. But what i did was make a preprocesser directive.

#define DIGITS_LENGTH 24
#define TEXTS_LENGTH 4

I then used those to make the rects and texts and also used those in the function to make sure i didn't go out of bounds.

int digits[DIGITS_LENGTH];
SDL_Rect rects[DIGITS_LENGTH];

Then forgot to use it here instead used magic number 24

    // Draw the images to the renderer.
    for (int i = 0; i < 24; i++) {
        int offset = (style < 4 ) ? 0: 2;
        SDL_RenderCopy(rend, texts[digits[i] + offset], NULL, &rects[i]);

To be honest i didn't know how to go about this. At first i did a sizeof array with sizeof item. But then i switched to the define. How is the right way to do this.

For the switching case it's more complicated because i am guarding that it stays under 24 with < 3 and < 8 kind of thing.

I was also very confused about using perprocessors since everything thing i learned about it said they are OLD way to do things and should not be used anymore. and that global const was the current correct way. But everyone replied i should be using preprocessors so I'm confused.

I had to use the define for the SDL switches because i just simply couldn't figure out how to make a const for them.

[JeremiahCheatham]

Also since i am defining and array in the switching statement i must use the curly braces or it will not work.

[hohmannr]

Sounds good. What you can do to decouple the function from the array and its size is something like this

#define FOO_MAX 512

int foo[FOO_MAX];

void some_array_function(int arrv[], size_t arrc) {
        /* do something with `arrv` */
}

int main() {
        some_array_function(foo, FOO_MAX);

        return 0;
}

[JeremiahCheatham]

#define FOO_MAX 512

int foo[FOO_MAX];

void some_array_function(int arrv[], size_t arrc) {
    /* do something with `arrv` */
}

int main() {
    some_array_function(foo, ( sizeof(foo) / sizeof(foo[0]) ));

    return 0;
}

So what i would love to know is is the method you wrote in your reply the one i was trying to do the best method or should i be doing the above with sizeof? Which is the most correct way so when i write code going forward. Also I'm so confused about size_t I have looked it up several times but still confused. Its a typedef to some unknown defined unsigned? What, why, when how?

[hohmannr]

What I would do is

#define LEN(x) (sizeof(x)/sizeof(x[0])))

int main() {
        some_array_function(foo, LEN(foo));
}

[JeremiahCheatham]

I guess what i mean is having all using 1 number. or having the function call using what ever was actually made from that array statement to guard against if not actually making the correct size. Maybe i'm chasing my tail here.

[JeremiahCheatham]

#define LEN(x) (sizeof(x)/sizeof(x[0])))

is this saying where ever in the code i use LEN(anyarray) it will automatically take that array and replace its size? But it's preprocessor so it doesn't actually know if that array was actually create that size at run time right?

[hohmannr]

sizeof(x) is an operator (so it is built into the language, meaning it is implemented by the compiler). If x is an array, it will give you the size in bytes that the whole array takes. Then you divide it by the size of one item, in this case sizeof(x[0]), which is the amount of bytes that one item has and you get the array length for any array.

[JeremiahCheatham]

yes but when you do the #define its implemented first and not at runtime right? so if the array[4] the preprocessor will answer 4 but if the code runs and the array of 4 doesn't get created some how then that preprocessor doesn't know this and still reports 4 correct meaning out of bounds? Meaning anytime an array doesn't get created correctly durring runtime will always go out of bounds? I thought that was what everyone was telling me not to do. My code works correctly it's only if that array doesn't get created then it will go out. But all those other methods except the one i posted above will all cause the exact same error right?

[hohmannr]

No, all the preprocessor does is change anything that looks like LEN(arr) in the source code to (sizeof(x)/sizeof(x[0])).

Here is a quick snippet. The command cpp stands for C PreProcessor. Check what it gives. It should get rid of all defines and replace the A with 11.

> cat main.c
#define A 11

int main() {
    int a = A;

    return 0;
}

> cpp main.c

[JeremiahCheatham]

so what you wrote

#define LEN(x) (sizeof(x)/sizeof(x[0])))

int main() {
    some_array_function(foo, LEN(foo));
}

gets expanded into this.

int main() {
    some_array_function(foo, (sizeof(foo)/sizeof(foo[0]))));
}

If so that sounds perfect.

[hohmannr]

Yes. That is the trick with the preprocessor. Catch up on K&R, it is really a good book.

[JeremiahCheatham]

yeah i actually have digital copy of it and reading it now too. But these things sometimes swim around in your brain and the books just leave questions. So what about size_t what is it and when to use? I really have looked this one up several different times on different dates without finding answer. Also i was going to change over all my global const to #define is that ok for the TITLE string and ICON fine string? I tried it and no errors but didn't know what's best. The title and hight width do get changed later but this is only use for initial window creation. after that the style function changes them. and time_in_title

[hohmannr]

Its fine to have any atomic type (int, unsigned, float, ...) as defines.

size_t is normally a typedef for unsigned long. You use it to indicate the size of something, the sizeof() operator returns size_t. Whenever you deal with sizes that are limited, just use size_t. It indicates to the reader, to be careful and that this must not exceed a certain size. I use it when iterating over an array e.g.

for(size_t i = 0; i < LEN(arr); ++i) {
        /* do something */
}

[JeremiahCheatham]

so sizeof returns size_t and it will never go beyond that. I don't know why no one says this online. when ever i read a book or watched a video and was wondering i would look it up and read all these websites and it's 50/50 whether i would get an answer.

The time function i was really lost at how to use this.

struct tm *timeinfo;
time_t rawtime;
time( &rawtime );
struct tm localtime( &rawtime );

Since i needed this struct for my function i defined it in main. I didn't understand what time_t was i thought it was also struct so i made it static thinking it's faster but now i get it's a special type like size_t. Since i need that localtime ever frame i tried to put it into a function that retured the local time struct each frame. Both reviews complained about this. This was not so much as my design decision as to what i could understand online how to get time and make it work. I don't know what i should be doing here.

[JeremiahCheatham]

https://www.tutorialspoint.com/c_standard_library/c_function_localtime.htm was the source of my information.

#include <stdio.h>
#include <time.h>
int main () {
   time_t rawtime;
   struct tm *info;
   time( &rawtime );
   info = localtime( &rawtime );
   printf("Current local time and date: %s", asctime(info));
   return(0);
}

[JeremiahCheatham]

I think i keep hitting this close with comment why is that option there grrrrrr.

[hohmannr]

No, size_t is not only returned by sizeof(). It is a type that you should use whenever you want to indicate, that a size is limited by something. E.g. an array length.

The C stdlib is not really a modern stdlib, it is a collection of code that everyone wrote their own implementations of back in the 80s. So there is not really consistent in terms of style.

time_t tt;
struct tm *tt_local;

time(&tt); /* this puts a value into `tt` */
tt_local = localtime(&tt); /* this uses `tt` and returns the local time as a `struct tm *` */

Passing a pointer to a function, ergo &tt, is called passing by reference, while as passing a value, ergo tt, is called passing by value. When passing by reference, your function can modify tt persistently. If you pass by value and you modify tt inside the function, it will not be persistent out side of it.

[JeremiahCheatham]

I have implemented that LEN(X) for functions using texts[] and rects[] I also used it internally in the switching statement for each x and y array. I also added a catch for the i++ I check if it's in bounds at the top of each loop. if it's not I send the error message and shut the program down.

[JeremiahCheatham]

Yeah did know about the passing pointers i just didn't really understand how the whole time thing worked. I did come across the time being changed in my function but i changed the order or something. I will dig into the time issue when i get home.

I have made a lot of changes to the code of you see all my comments lol. I still have some to do list in my head but i have been trying to tackle things one at a time to understand them. I also tried to change my switching statement to a compressed if statement but i ran into the issue of having the array being declared in the if statement and not seen after it. but if defined first that seems like it has all kinds of bound issues so i left it as switch but tried to add in all the error checking.

[JeremiahCheatham]

I have tried to decouple the error messages but i still have to pass them back into main then exit. I think this would be easier if I used structs to pass data around. But i was saving that for future work. As i am not sure how to implement the struct and how much data should be in it.

[JeremiahCheatham]

The reason why the timer_show_time is returning a Uint32 is because it's a strange SDL function.

timer = SDL_AddTimer(5000, timer_show_time, NULL);

it called the function but it also reads the return as the next time input. So when it's called it runs timer_show_time after 5 seconds but the return of that function is what sets the next time. Usually if you want it to repeat you pass the 5000 though. and if you want it once then you return 0. I find it very strange but it is how it's supposed to work. Also i needed the timer variable to remove the timer if a new one is created before the other one has hit it's time.

[JeremiahCheatham]

I have tried to encapsulate data into a struct that i pass by reference to functions and added a function that checks the length of the 3 arrays to verify they are correct.

If you have time could you look over my code and let me know if i have made a major mistake. Thank you so much.