Is my MySQL library wrapper neat? How can I do better?

Question

I'm moderately new to programming in C and I'm not sure about programming practices with code; suddenly I have had fears that my code is messy and disorganized. I can read it when I come back a month later yet I'm still nervous if I'm doing anything that's considered taboo.

This isn't all the code that I've done with this project, I think my other code is fine.

#include <stdio.h>
#include <stdlib.h>
#include <signal.h>
#include <unistd.h>
#include "functions.h"
#include "errh.h"
#include "mysqlerr.h"

void FIRST_TIME_HANDLER(MYSQL *conn) 
{
    printw("[+] Doing initialization checks...\n"); 
    SEND_MYSQL_QUERY(conn, "create table IF NOT EXISTS user_2(username varchar(25) not null, passwd varchar(25) not null)");
    printw("[+] Handler has checked tables.\n"); 
    refresh();
}

void INThandler(int sig)
{
    signal(sig, SIG_IGN);
    printw("Closing server...\n");
    refresh();
    sleep(5);
    endwin();
    exit(0);
}

MYSQL_INFO *CONNECTION_INIT()
{
    MYSQL_INFO *info = malloc(sizeof(*info));
    info->IP = "IPADDRGOHERE"; info->user = "Hello"; 
    info->pass = "World"; info->DB = "DBGOHERE";
    return info;
}

int EXIT_SERVER(MYSQL *connection)
{
    mysql_close(connection);
    printw("MySQL session has now closed.\n"); 
    refresh();
    return KILL_SIGNAL;
}

MYSQL *MYSQL_CONNECT_DB(MYSQL_INFO *info)
{
    MYSQL *connection;
    const char *server = info->IP; 
    const char *user = info->user;
    const char *password = info->pass;
    const char *database = info->DB;

    connection = mysql_init(NULL);

    if(!mysql_real_connect(connection, server, user, password, database, 0, 
        NULL, 0))
    {
        //printw("[-] Error: %s\n", mysql_error(connection));
        MYSQL *f = KILL_SIGNAL;
        refresh();
        return f;
    }

    printw("[+] Connected to MySQL server.\n");
    FIRST_TIME_HANDLER(connection);
    return connection;
}

void SEND_MYSQL_QUERY(MYSQL *conn, const char *query)
{
    refresh();
    if(mysql_query(conn, query))
    {
        ERROR err;
        throw_error_init(err, 0, 1, NONFATAL, "MySQL QUERY ERROR", 
            "ERROR IN THE PROCESS OF QUERY.", MySQL_query_error);
    }
    else 
    {
        printw("Query sent.\n");
    }
}

int init_connection() 
{
    MYSQL *conn;
    MYSQL_INFO *info = CONNECTION_INIT();

    if((conn = MYSQL_CONNECT_DB(info)) == (MYSQL *) 0)
    {
        ERROR err;
        throw_error_init(err, 1, 1, FATAL, "INIT CONNECTION ERROR", 
            "Failed to connect to server.", MySQL_init_failure);
    } 
    else {   
        int key;
        nodelay(stdscr,TRUE);

        signal(SIGINT, INThandler);

        while(1)
        {
            if((key = getch()) == ERR)
            {
                if(mysql_ping(conn) == 1)
                { 
                    printw("Error!\n");
                    refresh(); 
                    return 1;
                } 
            } 
            else 
            {
                if(key == EOF) break;
            }
        }

        free(info);
        EXIT_SERVER(conn);
        return 0;
    }
    return 0;
}

Answer 1

You should generally avoid while(1) loops. Maybe rewrite that piece of code like this:

    while((key = getch()) != EOF)
    {
        if(key == ERR && mysql_ping(conn) == 1)
        { 
            printw("Error!\n");
            refresh(); 
            return 1;
        } 
    }

while(1) is conceptually "go on and on forever", but this is not your case. This way you don't even need to call a break.

Answer 2

1. In C the naming conventions typically are lower_case_with_underscore for function names and types (although for types some people also use PascalCase). UPPERCASE is only really used for macro definitions.

2. In CONNECTION_INIT you have a bunch of hard-coded settings. These should come from a config file or via command line arguments.

3. In init_connection you write:

   if((conn = MYSQL_CONNECT_DB(info)) == (MYSQL *) 0)
   

   • There is no reason to cast in order to compare it to NULL, simply == NULL works and is more idiomatic.

   • There is no reason to do the assignment and compare in one statement. Splitting it up makes the if statement less crammed:

     conn = MYSQL_CONNECT_DB(info);
     if (conn == NULL)
     {
         ...
     

4. I assume printw is some kind of way to print debug/info/tracing messages. If so then this is not a good name for the function. Better would log_info or similar (depending on the exact purpose).

5. There is this magic refresh method called everywhere but it doesn't seem to follow any particular pattern. Seems suspicious to me.

Answer 3

All of what @ChrisWue is saying in his answer, plus:

• Avoid putting multiple statements on a single line (your CONNECTION_INIT method)
• To increase readability, decrease your indent level : in your SEND_MYSQL_QUERY and init_connection methods, just add a return statement to your error management "ifs", and get rid of the else, as you do in your MYSQL_CONNECT_DB
• I don't see the ERROR err variable utility, as it seems only to be used inside the throw_error_init method
• In your MYSQL_CONNECT_DB, you return a KILL_SIGNAL constant if something went wrong, but when you call this method, you test for a (MYSQL *) 0 value (or NULL as @ChrisWue suggested). Be coherent: either return a NULL value if something goes wrong, or test for a KILL_SIGNAL constant.