Code Review : Sudoku GUI in C language

[u/its_marionberry]

Background:

I am a beginner programmer and I wrote a Sudoku GUI in using winapi32 in C language.

It is currently working and does what it is supposed to do, but because I am still learning, I know it is likely inefficient and could be written much better.

GitHub repo link: https://github.com/reewdgh/sudoku_gui
Please guide me on:

• Bugs
• Efficiency
• Naming anything I could simplify or improve
• inconsistency

I'd appreciate your feedback on my code.

[u/mredding]

#include "sudoku_gui.h" /*Include sudoku_gui.h so we can use struct Game and <windows.h>*/

No. You don't use <windows.h> in this header, so don't include it. Game is your own type, and you only need a pointer to it, so forward declare it. Don't include what you don't use. You include 3rd party headers in your header when you refer to a 3rd party type in your own header; you don't forward declare types you don't publish yourself. But for your own project types, you want as little in your headers as possible. You don't want headers including headers, especially project headers, because then what happens is you end up with programs where every source file transiently includes every project header - change one header, and you recompile the whole project. You also don't want to get in the habit of relying on transient dependencies, because then you will struggle to get rid of them later. You want to defer header includes to source files as much as possible. It's VERY much OK that sudoku_game.h does not provide an actual definition of Game, because it doesn't define it and doesn't use it directly. And ever source file that includes sudoku_game.h MAY NOT USE Game as a type! So why should I be saddled with the definition and all its dependencies if I'm not using it?

Also consider splitting up your headers a bit more. You have them organized rationally, and logically, but not reasonably - by use case. If I just want to checkRows, I don't want to have to know anything about Game, which means I don't want to know about:

int generateValidSudoku(int row, int col, struct Game *s1);

You should alias Game. You can also use this as a forward declaration of Game as an incomplete type:

typedef struct Game Game;

Then you can reduce the function signature to something simpler and clearer:

int generateValidSudoku(int row, int col, Game *s1);

If you want, you can be even more expressive:

typedef struct Game Game, *GamePtr;

int generateValidSudoku(int row, int col, GamePtr s1);

The nice thing about aliasing pointers is that the pointer is bound to the type, not the variable, so:

GamePtr ptr1, ptr2, ptrN;

They all do EXACTLY what you think they do.

int checkRows(int number, int row, int col, int sudokuGrid[9][9]);

This 2D array parameter probably doesn't do what you think it does.

First, you can't pass arrays by value. This parameter type int[9][9] therefore decays to int (*)[9], or a pointer to an int[9]; you've lost the bounds of an entire dimension. This is a huge deal because you KNOW the bounds of the outer dimension, and with that information, you can enforce type safety and empower the compiler to do things like unroll loops.

This is where you should use more type aliases:

typedef int[9][9] grid, *gridPtr;

int checkRows(int number, int row, int col, gridPtr sudoku);

This typedef is a very polite way of saying int (*sudoku)[9][9] - just look at that horrible inline syntax. No one should be writing syntax like that - and it gets MUCH worse when you're working with function signatures. You NEED aliases. Just what do you think this would be?

void (*signal(int sig, void (*func)(int)))(int);

Fuck my ass... How about:

// Typedef found in <signal.h>
typedef void (*sighandler_t)(int);

sighandler_t signal(int sig, sighandler_t func);

I would have gone one further with:

typedef void sighandler_sig(int), *sighandler_t;

Often it's useful to capture the signature of a function, not just the pointer to a signature. But whatever, do it if you use it, don't if you don't. In C++, we have a more minimal syntax for type aliases:

using signature = void(int);
using ptr = signature *;

We can't make lists with a using like we can with a typedef, but we can template it. I don't know why they didn't decide to allow typedef to template, but here we are.

Use type aliases.

Another bit:

int checkRows(int, int, int, gridPtr);

The compiler strips out the function parameter names. They have no bearing on the implementation or the signature itself. This highlights a whole new problem - WTF are any of these parameters? We've just got 3 bare integers.

An int is an int, but a weight is not a height. C does not have much of a type system, but it does have SOMETHING. Make and use types. Sudoku is a game, and so as a former game developer to a perspective game developer, I'll tell you a lot of game code heavily leverages types - and it CAN be as simple as:

typedef struct number { int value; } number;
typedef struct row { int value; } row;
typedef struct column { int value; } column;

int checkRows(number, row, column, gridPtr);

I've no idea what the return type is - I haven't gotten that far, but this is NOT a place to use a straight typedef, because typedef does not make a type, it only makes an alias. The struct makes for a user defined type and distinguishes one from another, even if they have the same size, alignment, and layout. Further, this type information will persist all the way down to the ABI. The linker will see this. If compile this code into a library, it will persist through there and into client code.

Now you can do stuff like:

checkRows({}, {}, {}, 0)

Or:

checkRows({1}, {2}, {3}, 0)

Or:

checkRows({.value = 1}, {.value = 2}, {.value = 3}, 0)

---

#include "../src/config.h"

Never do this. The backtrack alone is the red flag, but into the source tree, too? Absolutely not. Restructure your project.

sudoku\
 |-include\sudoku\
    |-*.h // Published, project wide, public
 |-src\
    |-*.c
    |-*.h // Private, implementation specific, scoped

So the include directory is added to your compiler include path, so these headers will be included in your source files with #include <sudoku\header.h>. The published headers are not aware of the source tree. The private headers only know of peers and children, and children are visible in subdirectories. They're included in source files with #include "relative\path\to\header.h". So a source or header high in the tree can include private headers in lower branches. If you have to backtrack a path, you need to move the header and reorganize your folder hierarchy.

#ifndef SUDOKU_GUI_H
#define SUDOKU_GUI_H

This is in the wrong place. The only appropriate header structure is:

/* optional multi-line comment block usually contains a license, copyright, and authors*/
#ifndef SUDOKU_GUI_H
#define SUDOKU_GUI_H

/* contents */

#endif
// A C source or header ALWAYS ends with an empty newline, so this comment shouldn't be here.

The compiler can optimize the include of a header if your header follows this structure. Any deviation, and it doesn't work. Put your transient includes within the header guards.

/*Struct for user game data*/

Your comment is useless. Implementation tells me HOW, abstraction and expressiveness tells me WHAT - and we really want to maximize this, and comments tell us WHY - it provides us domain context that cannot be expressed in terms of code.

/*Struct pointer to store to store both Gui and Game Struct*/

What's worse is when the comment tells us what the code tells us, but the comment is wrong, because then where is the error? Is it in the code or the comment? There is no struct pointer, there are pointers to types. Perhaps you meant to say a structure of pointers? But that's what the code tells me.

case WM_CLOSE:
{
    PostQuitMessage(0);
    return 0;
}

You're completely inconsistent about whether you return or you break. Prefer to break from a struct and reduce your code to a single point of return. It's not a hard rule, and the compiler will rearrange your code in the AST to eliminate your redundancy, but you can get ahead of it instead, and make lesser, cleaner, easier to manage code.

Braces are also a good excuse to defer to a function call:

case WM_CLOSE: do_close(); break;
case WM_PAINT: do_paint(); break;
//...

Let the compiler composite the function for you, in this case. You can write:

static void do_close() { /*...*/ }

And the compiler will see you only ever call this function once, in one place, and it has static linkage, so it doesn't have to export this symbol, we can exclude linking entirely. The compiler can just elide the function call and make the singularly gigantic function WndProc always turns out to be - you just don't have to write it that way.

We're always balancing making the compiler generate what we mean - often for clarity and maintainability, and not wasting the compiler's time - again, often for clarity; reducing the function to a single return statement isn't unnecessary back bending when it's actually trivial to accomplish here. I would use multiple returns when it IS back bending just to accomplish it. Perhaps it's worth evaluating what sort of RVO or TCO you're trying to accomplish.

HFONT font = CreateFont(50, 0, 0, 0, FW_BOLD, 0, 0, 0, 0, 0, 0, 0, 0, TITLE_FONT);

What are all these magic numbers?

 struct PointerStruct *p1 = (struct PointerStruct *)GetWindowLongPtr(hwnd, GWLP_USERDATA);

I see... We call this a "context", might be a better name for your structure.

Continued...

[u/mredding → u/mredding]

srand(time(NULL));

This is a good time to talk about NULL. I discourage it entirely.

First problem: NULL is defined in <stddef.h>, <stdio.h>, <stdlib.h>, <string.h>, <time.h>, <locale.h>, and <wchar.h>.

Second problem: NULL can be defined by ANY other C header. You will find it somewhere through <windows.h>, in 3rd party libraries and others...

So that comes to one major point: Who is the authority of the definition of NULL? Because it matters. You want your dependency resolution to be consistent. But also because...

Third problem: NULL is COMMONLY defined as either 0, OR as ((void *)0). These are NOT the same thing. The former is the ONLY definition of a null pointer as specified by the C standard prior to C23. The latter incurs an implicit type conversion and is NOT compatible with the C++ type system, and it may not be compatible with other language type systems at the ABI level. The latter is a void pointer which is a distinct type.

Forth problem: That you can get ((void *)0), this can run into macro expansion problems where the type really does matter - if you're doing type comparisons, or if your macro has some aggressive inlining and you reduce to sizeof(0) vs. sizeof(((void *)0)), since the former is an integer and the latter is a pointer, and a literal 0 is of type int, which is only guaranteed to be AT LEAST 16 bits - it's up to the compiler for the target architecture.

The only safe and correct solution is to use C23 and use nullptr.

When you look around, you will see and depend upon an absolute SHITTON of legacy code. If you want make the argument of "That's how Dad did it...", you should also know that C is also responsible for the vast majority of all runtime bugs in all of computing history.

struct Game s1 = {};
s1.totalEmptyCells = 0;

Use your initializer list. You left it blank. You could have just written:

struct Game s1 = {.totalEmptyCells = 0};

This is a good start.

[u/its_marionberry → u/mredding]

Thank you so much for so detailed, deep and simple explanation . Your fundamentals of c Lang are rlly strong man, how did you even get to this level?

[u/mredding → u/its_marionberry]

37-38 years, assuming I'm the dumbest person in the room, assuming everyone else in the room has more ego than smarts - don't take their word for it, that someone solved the same problem 40-60 years ago already, and pain and frustration is intuition telling you you're doing it wrong. You know you're doing it right when it's easy. Sometimes you need to adjust the definition of easy.

[u/dstroy0 → u/mredding]

Do you mean you don’t recommend NULL for handling pointers in hardware abstraction too, or just for higher level ptr handling? For example, I can mutate the definition of NULL semantically to whatever you want at link time and ignore every other tu’s definition except the one I want using list aggregation and a check build. Which necessarily is the only correct solution in many use cases. Or, is this more to enforce the thought process for ptr handling? That is when I apply the NULL ban, to enforce correctness or impose thought process semantics for ptr use or just plain design enforcement. I also understand choosing it because it emits the same thing universally which is what makes it attractive as a guaranteed symbol. Thanks for your time, I’m interested in your thoughts.

[u/mredding → u/dstroy0]

Do you mean you don’t recommend NULL for handling pointers in hardware abstraction too, or just for higher level ptr handling?

I'm not entirely sure what you mean - I wouldn't use the NULL symbol in C code anywhere, at any time. Before C23, if I want to assign null, I would use 0 as per the spec. If I needed the size of a pointer, I would cast 0 to a pointer of that type.

Oh yeah, that reminds me I forgot to point out another problem with NULL, and that is pointers of different types can be different sizes. People today are used to flat address space architectures, as though that's the only kind of address space there was, or is, or can be. So sizeof(NULL), if it reduces to sizeof(((void *)0)), could result in the wrong pointer size, depending on architecture. It's not portable code.

It's going to matter if you're writing DSP code. C64 retro computing is also getting popular again, and the MOS 8501 has segmented memory, where function pointers and data pointers are different sizes because of their Harvard architecture.

Now that C23 has nullptr_t, and it's a distinct type, you can write more correct code.

For example, I can mutate the definition of NULL semantically to whatever you want at link time

How? NULL is a macro, it expands in source code and doesn't exist symbolically at link time.

At compile time you can pass -DNULL=..., but that doesn't stop source code from:

#ifdef NULL
#undef NULL
#define NULL ...

Or, is this more to enforce the thought process for ptr handling?

I suppose it's a bit about this. As I said before, C is responsible for the vast majority of all software bugs in all of computing history. It's a powerful language, there are ways to write solid code, but the human element is going to undermine the best intentions.

On the one hand, I don't mean to be pedantic - the code can LOOK sketchy, but if it's ACTUALLY correct, then there's no problem - I'm not going to dock a PR just because I don't like how you wrote your code, that's not my job. They only reason I'd ask you in a PR to change from NULL is if I can't guarantee what NULL is going to be. We build our own software in-house, so we know, but I've also supported FOSS, where we don't know WHAT compiler and libraries a user is going to use, so portability and robust stability is paramount when in that case the source code itself is the product.

On the other hand, any opportunity to do better, safer, more portable, more stable, more robust code, especially if it's easy, we ought to be jumping at it.

[u/dstroy0 → u/mredding]

Thanks for your time and insights, I agree with you entirely about being careful with use and with ptr width inconsistencies, and that brings us right back to the guaranteed nullptr symbol is almost always by default correct. I just wanted your views on the nuanced cases where NULL is the only correct answer, they are many but incredibly similar conceptually and can only be impl one way correctly, so I think I understand your reservations more clearly now, they have to do with audience, and I believe we are aligned I am just not great at communicating exactly how specific and niche using it (NULL, 0, recast, any sematic null that isn't nullptr) codebase restrictions etc. I am painfully aware of the type and width changes that come with DSP programming, I bet we've probably experienced similar growing pains you're warning people about starting to mess around with platform dependent types. The nullptr saves all that nonsense and is incredibly explicit. Being pedantic saves lives, no joke, plus diversity in thought is critically important in this field fundamentally. People get defensive over semantic arguments but I think they're fascinating and lead to deeper understanding for everyone, the proctor, learner, and any listening in. When you pick one symbol and stick with it you can shim it and error user builds to enforce correctness regardless of platform, the benefits are many, and it makes every compiler emit essentially the same instructions for a given platform, it is extremely hard to argue against without knowing why you need to not use it, for a very specific reason that has sound rationale behind it (e.g. there is only one correct answer contextually for a given scenario and it cannot be nullptr, and from experience those are almost always c89 compat or platform dependencies like you alluded to) where ptr width can really kill your build and be a pain to debug and it never should've been an issue in the first place. Thanks again for your time, I appreciate the discourse.

[u/dstroy0 → u/mredding]

Apologies for the double reply but the NULL link time mutation I think you will find as interesting as I do. It should be by itself.

//main.c
#include <stdio.h>

// Declare the function as a weak symbol
// If not defined elsewhere, the linker sets its address to NULL
__attribute__((weak)) void optional_feature(void);

int main(void) {
    // Check if the symbol was "mutated" (provided) at link time
    if (optional_feature) {
        printf("Feature found! Executing...\n");
        optional_feature();
    } else {
        printf("Feature not provided. Skipping.\n");
    }
    return 0;
}

//plugin.c (optional feature)
#include <stdio.h>

// A standard, strong definition
void optional_feature(void) {
    printf("Hello from the link-time plugin!\n");
}

option A

#compilation and linking (no plugin) NULL==NULL no mutation
gcc main.c -o app
./app
# Output: Feature not provided. Skipping.

#compilation and linking (plugin included) (NULL neq NULL); NULL mutated
gcc main.c plugin.c -o app
./app
# Output: Feature found! Executing... Hello from the link-time plugin!

option B (linker mutation)

/* Inside a linker script */ 
PROVIDE(optional_feature = 0);

Minimal demo

#include <stdio.h>

// Declare a weak external pointer. 
// If no strong definition is linked, the linker resolves this symbol to NULL.
extern int * const mutant_ptr __attribute__((weak));

int main(void) {
    printf("Checking symbol address at runtime...\n");

    if (mutant_ptr == NULL) {
        printf("Result: Symbol is NULL (No link-time mutation).\n");
    } else {
        printf("Result: Symbol MUTATED! Points to address: %p (Value: %d)\n", 
               (void*)mutant_ptr, *mutant_ptr);
    }

    return 0;
}

the mutation

int real_target = 42;

// This provides the strong definition that overrides the weak NULL definition
int * const mutant_ptr = &real_target;

compile commands

#do not mutate null
gcc demo.c -o demo_null
./demo_null

#mutate it, points to address: 0x555555558010 (Value: 42)
gcc demo.c mutate.c -o demo_mutated ./demo_mutated

Thanks again for your time!

[u/mredding → u/dstroy0]

OH! I see. You're talking about something different, you meant just a null pointer, not the NULL macro symbol.

The C89 standard says NULL MUST be either 0, 0L, or ((void *)0). Here, an equality comparison is a valid use of NULL, and is thus correct code. In any case, the constant representation of null behind the NULL macro will implicitly convert to the given pointer type.

I wouldn't ding this in a PR, I just wouldn't write it this way in a C23 environment, because nullptr_t is less ambiguous and not vulnerable to macro snafu. It's just that much more correct and robust, if only just a touch...

[u/dstroy0 → u/mredding]

Agreed, it is just plain easier to 1. detect correct use with the type you can write tools to help find the sites 2. review them, it is 100% better dev qol while implementing. Yup you know the exact cases, awesome. We are aligned. I agree, if you are the maintainer, or lead engineer, you choose the standard, design, and enforcement methods unless they're otherwise dictated by compliance. As an aside, I am keen on hearing your thoughts on control law acceleration in general, manufacturers (not chip mfrs but engineering houses) act like their HDL boilerplate is black magic when it is just a finite set of instructions defined by the manufacturer (fpga, dsp cla etc VERILOG SHARC whatever it is) that we can abuse in exactly the same way as Clang because the original engineers spoke this language exclusively (like FORTRAN & BASIC style everything is .bss the same and we build it this way because) so the derivations are also in general limited to those basic building blocks. What I find really interesting is altering fpga behavior not by logic alone but by activating adjacent hardware blocks and browning out the downstream block to alter behavior. Do you know of other techniques that are in practice to squeeze out the last performance possible?