HiveBrain v1.2.0
Get Started
← Back to all entries
patterncMinor

Reading and printing sudoku input

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
readinginputprintingandsudoku

Problem

I have seen programs to solve sudoku puzzles but to be honest.. They were badly written.
First of all, readability of the program itself was horrible and the code was really long and confusing.

So I decided to start from scratch, using my own style to write really good solver.
What I have this far is reading a board input from the user and outputting the board representation:

#include 
#include 
#include 
#include 
#include 

#define FOREGROUND_LBLUE 9
#define COLORSET_MAIN 3

char sudoku [9][9];

int chrton (unsigned char chr)
{
    if(chr >= '0' && chr <= '9')    { return chr - '0'; }
    else                            { return 0;         }
}

int main()
{
    char num = 0;
    char x = 0, y = 0;

    SetConsoleTextAttribute (GetStdHandle(STD_OUTPUT_HANDLE), FOREGROUND_LBLUE | BACKGROUND_BLUE);
    printf("Validate numbers:\t\t\t\t\n\n");
    SetConsoleTextAttribute (GetStdHandle(STD_OUTPUT_HANDLE), COLORSET_MAIN);

    // Part 1 : validating hint numbers
    while(y != 9)
    {
        if(kbhit() && isdigit(num = _getch()))
        {
            printf("%c"" ", num);

            sudoku[x][y] = chrton(num);

            if( ++x == 9 )                  { putchar('\n'); x = 0; y++; }
            if( not((x * x) % 3) and x)     { putchar('\t');             }
            if( not((y * y) % 3) and !x)    { putchar('\n');             }
        }
    }
    system("PAUSE");

    return 1;
}


Obviously it is written for Windows compatibility but I use it as for advantage to exploit some rules of commonly-accepted IO.

Solution

I have found a couple of things that could help you improve your code.

Avoid the use of global variables

I understand that it's just partial code so far, but it's generally better to explicitly pass variables your function will need rather than using the vague implicit linkage of a global variable as with sudoku in this code.

Don't use system("pause")

There are two reasons not to use system("cls") or system("pause"). The first is that it is not portable to other operating systems which you may or may not care about now. The second is that it's a security hole, which you absolutely must care about. Specifically, if some program is defined and named cls or pause, your program will execute that program instead of what you intend, and that other program could be anything. First, isolate these into a seperate functions cls() and pause() and then modify your code to call those functions instead of system. Then rewrite the contents of those functions to do what you want using C++. In this case, a better alternative would be getchar().

Be careful with char and int

Your chrton function returns an int and then stores it in a char-sized location. Better would be to return char, since that's how sudoku is defined.

Consider using unsigned char for subscripts

It's not technically wrong, but you may wish to use unsigned char for the type of x and y. It eliminates a class of errors that can occur if the array subscript is a negative number. This is a fairly minor point, however.

Consider using more idiomatic C

While your use of and and not is not techncially wrong, because you have correctly included the iso646.h header, you should be aware that many experienced C programmers will be unfamiliar with these operator alternatives, and so if others read your code, it might be an impediment to their understanding of the code. Similarly, your unusual formatting of if statements as one-liners is likely to seem a bit strange to experienced programmers.

Consider separating input and output

Right now, the main function issues a prompt, reads in the sudoku board and then prints it out as each character is read. A more modular (and likely more maintainable) approach would separate these into separate functions.

Consider returning 0 instead of 1

A non-zero return value is interpreted as an error condition on most operating systems. Normal program exit should return 0 or EXIT_SUCCESS or simply omit it, because the compiler will automatically insert the equivalent of return 0; at the end of main.

Context

StackExchange Code Review Q#69720, answer score: 6

Revisions (0)

No revisions yet.