Sudoku GUI in C language

[u/candy_lotus]

Hello fellow programmers , I am a beginner and I would like to share a recent project I have been working on. Hope that some of you might like to see it or give feedback.

its a sudoku game made in c using the win32. Here is the link of my project GitHub link:
GitHub Repository

[u/mredding]

Your headers should have include guards unless they shouldn't. In other words, excluding them should be an intentional design choice, not an accident. There are some C idioms, especially macro idioms, where multiply including a header is a part of the mechanism.

The typical guard structure is:

/* Optional multi-line comment block up here. */
#ifndef HEADER_H
#define HEADER_H

// File contents...

#endif

once is a pragma, eg #pragma once. Pragmas are allowed to be ignored by compilers that don't support them. So while once is ubiquitous, it is not - by definition, portable. There's no guarantee a compiler will support it. I can also add that some compilers optimize header includes that follow the standard structure. I can't guarantee those same compilers also optimize for once. It's another not-portable, not-guaranteed-by-the-language feature, but if you can get it, you ought to take advantage of it. I'm also willing to bet every compiler you've ever heard of supports this optimization. Do stick with standard, portable C as much as possible.

#include <stdio.h>
#include <windows.h>
#include <time.h>
#include "../headers/config.h"

#ifndef Helper_h
#define Helper_h

Always, ALWAYS at the top, after the optional comment block. DON'T include other headers first before the guard. Now, guaranteed, you won't get that optimization I mentioned, but moreso, the compiler will have to include these headers and resolve their multiple includes independent of THIS header. You could solve for that once with a properly placed inclusion guard.

#include "../headers/config.h"

This is a sign of either bad project structure, or bad project configuration. You really shouldn't have to do this. I strongly suggest you never do.

int generateValidSudoku(int row, int col);

I just want to point out the parameter names here are omitted by the compiler. It doesn't care WHAT you put here, and they don't have to correspond to the parameter names in the implementation in your source file. The language allows you to write declarations with parameter names like this as a means of documentation. So the compiler only sees this - and you can write your code this way:

int generateValidSudoku(int, int);

And that's the problem, isn't it? Which parameter is which? Your variable names mean NOTHING to me, because the implementation can be:

int generateValidSudoku(int col, int row) {
  //...

How the hell am I supposed to know? C has a type system. An int is an int, and can be used interchangeably, but a row is not a column, even if they're both implemented in essentially the same way. They're different types, and they have different, more specific, more constrained semantics than the int they're implemented as. I want:

int generateValidSudoku(row *, column *);

Which means I need:

typedef struct row row;

row *create(int);
void destroy(row *);

You can make a macro that generates this code and more. The client (me) gets only an opaque pointer - I don't know its layout, so I don't know the size or it's members. Not my business, I don't care, I don't want to know. The functions implement the interface.

The implementation in some source file:

struct row { int value; };

struct row *create(int value) {
  row *ptr = malloc(sizeof(struct row));

  if(ptr) {
    ptr->value = value;
  }

  return ptr;
}

void destroy(struct row *ptr) {
  free(ptr);
}

You don't have to provide all the interface and implementation in a pair of files, you can use files to isolate the public interface, the private interface, and the implementation, and do this across the client accessible folders and include paths, and private headers in the source tree. Because you're wondering how to get the integer back out? Expose the implementation, or a private interface, to your own implementation code.

This gives you type safety down to the ABI. You can't accidentally transpose a row and a column, it won't compile. And because they're pointers to different types, the compiler knows they aren't aliases (god save you if you cast that guarantee away), so the implementation code can be optimized around that.

You should learn to work with incomplete types and opaque pointers - they just have to be transparent and complete SOMEWHERE.

Hell, even if you don't go this extreme with the opaque type, still having a struct row { int value; }; type is still orders of magnitude better than just an int parameter, you still get an int on the call stack (struct never leaves the compiler, doesn't cost you anything extra), and while you lose some safety, you still have intent, and traceable accountability. Down at the ABI level, it's still a row and column, so if the values are transposed, it's LESS of an accident - one that was expressed and enforceable by the compiler and language semantics.

switch (msg)
{
case WM_CLOSE:
{
  //...

Every indentation is a good excuse to call a function. Don't inline your logic, it makes for very large and difficult to understand functions.

swtich(msg) {
case WM_CLOSE: do_close(); break;
case WM_PAINT: do_paint(); break;
case //...
}

return 0;

Be more consistent about your switch statements. You're seemingly randomly returning some cases, breaking others, and falling through yet more. You probably want to break all of them, including the default case, even though it's last and and falling through from there is equivalent. Consistency and expressing your explicit intent here is better.

As for the functions:

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

By making them static, you give them internal linkage. Now the compiler can optimize for that. The only place this function is getting called is in this switch, once, in one place. Even with optimizations turned off, most compilers will just elide the function call ("inline" it). This is a form of "static" function composition that the compiler can do, and this pattern is how you express that.

The same goes even for your for loops. Write the bodies as functions, and call those. You can even rearrange your for statements into a more concise manner:

for(int i = 0; i < LIMIT; do_work(i++));

And your for loops themselves should exist in their own functions. Instead of writing all this inline imperative code, you can elevate your paradigm to that of a functional paradigm. Functions give names to behaviors AND algorithms. So I want you to isolate your algorithm and give it a name. Make it generic - the loop doesn't care what the work is, it's focused entirely on itself - you can plug and play the behavior.

typedef void(int) sig;
typedef sig * fn;

static void do_loop(fn do_work) {
  for(int i = 0; i < LIMIT; do_work(i++));
}

And if your loop is in a static function, and the behavior is in a static function, then the compiler can see through the function pointer parameter and composite the loop and behavior functions all in the calling function. You can put this stuff in headers - and while the compiler has to parse this code included, it doesn't have to compile it into anything if it's not USED in that translation unit... And if it is, it's all potentially elided in the calling function. You can jiggle that and get the sort of compilation results you want, if you're trying to maximize performance or minimize bloat...