Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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;
}
share|improve this question
    
Could you also include the corresponding header file? –  ChrisWue Jan 13 at 8:22

3 Answers 3

up vote 3 down vote accepted

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.

share|improve this answer
    
Wow it looks a lot neater already! –  Gingerbeer Jan 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.

share|improve this answer
    
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 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.
share|improve this answer
    
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 at 3:28

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.