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

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 *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;
}

-
Could you also include the corresponding header file? – ChrisWue Jan 13 '14 at 8:22

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.

-
Wow it looks a lot neater already! – Gingerbeer Jan 14 '14 at 4:46
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.

-
Just got home. printw and refresh are a part of the ncurses library. Ignore those. Thanks for the advice, I'll fix those up. – Gingerbeer Jan 13 '14 at 8:53

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.
-
Thank you very much for your help, the ERROR err variable is needed for another function. Already the code is looking a lot nicer. Thank you! – Gingerbeer Jan 14 '14 at 3:28