I am currently working on a game, and I get the configuration in an .ini file, and I get information with this library https://github.com/benhoyt/inih I would like to improve the readability / performance of this code, as I do not know much about good practices.
include "project.h"
// typedef struct s_server_conf {
// const char *ip;
// unsigned int port;
// } t_server_conf;
// typedef struct s_db_conf {
// const char *host;
// const char *user;
// const char *passwd;
// const char *db;
// unsigned int port;
// } t_db_conf;
// typedef struct s_conf {
// t_server_conf server;
// t_db_conf conf;
// } t_conf;
static int parse_server(t_conf *conf, const char *name, const char *value)
{
# define MATCH(n) (strcmp(name, n) == 0)
if (MATCH("ip"))
conf->server->ip = strdup(value);
else if (MATCH("port"))
conf->server->port = atoi(value);
else
return (0);
return (1);
}
static int parse_database(t_conf *conf, const char *name, const char *value)
{
# define MATCH_NAME(n) (strcmp(name, n) == 0)
if (MATCH_NAME("host"))
conf->db->host = (strdup(value));
else if (MATCH_NAME("user"))
conf->db->user = (strdup(value));
else if (MATCH_NAME("passwd"))
conf->db->passwd = (strdup(value));
else
return (0);
return (1);
}
static int handler(void *conf, const char *section, const char *name, const char *value)
{
# define MATCH_SECTION(s) (strcmp(section, s) == 0)
printf("Le nom du champ est %s, la valeur est %s (La section est %s)\n", name, value, section)
;
if (MATCH_SECTION("database"))
return (parse_database((t_conf*)conf, name, value));
if (MATCH_SECTION("server"))
return (parse_server((t_conf*)conf, name, value));
return (0);
}
void clear_conf(t_conf *conf)
{
if (conf->db)
free(conf->db);
if (conf->server)
free(conf->server);
free(conf);
}
static t_conf *create_conf(void)
{
t_conf *conf;
if (!(conf = (t_conf*)malloc(sizeof(t_conf))))
return (NULL);
memset(conf, '0円', sizeof(t_conf));
if (!(conf->db = (t_db_conf*)malloc(sizeof(t_db_conf))))
{
clear_conf(conf);
return (NULL);
}
conf->db->port = 3306;
if (!(conf->server = (t_server_conf*)malloc(sizeof(t_server_conf))))
{
clear_conf(conf);
return (NULL);
}
return (conf);
}
t_conf *get_conf(void)
{
t_conf *conf;
if (!(conf = create_conf()))
return (NULL);
if (ini_parse("settings.ini", handler, conf) < 0)
{
printf("Unable to load configuration\n");
exit (1);
}
return (conf);
}
1 Answer 1
Design
Rather than bury a magic filename, consider passing it in.
//t_conf *get_conf(void) {
// ...
// if (ini_parse("settings.ini", handler, conf) < 0)
t_conf *get_conf(const char *ini_file_name) {
...
if (ini_parse(ini_file_name, handler, conf) < 0)
Maybe add before the if()
if (ini_file_name == NULL) {
ini_file_name = "settings.ini";
}
Devoid of overall comments
Consider comments to at least convey the coding goals.
Allocate to the size of the de-referenced object
... rather than allocate to the size of the type - easier to code right, review and maintain. Cast not needed.
//if (!(conf = (t_conf*)malloc(sizeof(t_conf))))
// return (NULL);
conf = malloc(sizeof *conf);
if (conf == NULL) {
return NULL;
}
// like-wise
// memset(conf, '0円', sizeof(t_conf));
memset(conf, 0, sizeof *conf);
Use auto formatting
Life is too short for manual formatting. Find and use an auto formatter - customized as needed.
Comments of typedefs not needed
As is now, this code has comments that is not required to match what is truly in "project.h" and as time goes on, can diverge. For code posting here, post a portion of "project.h" instead.
Invalid code
include "project.h" should be #include "project.h"
Hiding objects here is bad
# define MATCH(n) (strcmp(name, n) == 0) brings in name, but not obvious from the call if (MATCH("ip")).
Alternative:
static bool streq(const char *a, const char *b) {
return strcmp(a,b) == 0;
}
...
if (streq(name, "ip"))
Naked magic number
Buried in code is
conf->db->port = 3306;
Better to have
// near top of code
#define PORT_DEFAULT 3306
// later on
conf->db->port = PORT_DEFAULT;
Unneeded if
Just call free(). free(NULL) is OK.
//if (conf->db)
// free(conf->db);
free(conf->db);
Like free(), I'd expect clear_conf(NULL) to not error
void clear_conf(t_conf *conf) {
if (conf) { // add
() not needed in return and others
Many places
// return (conf);
return conf;
// conf->db->host = (strdup(value));
conf->db->host = strdup(value);
Non-standard C
Note: strdup() is not standard, yet ubiquitous.
Missing #include <>
Do not rely on "project.h" to provide them unless that is clearly a project rule.
NULL as name
Given the potential generic use of handler, I'd guard against NULL.
// printf("Le nom du champ est %s, la valeur est %s (La section est %s)\n", name, value, section);
if (name === NULL || value == NULL || section == NULL) {
tbd();
} else {
printf("Le nom du champ est %s, la valeur est %s (La section est %s)\n", name, value, section);
}
Further I'd bracket such strings to help detect leading/trailing white spaces in them
printf("Le nom du champ est \"%s\", la valeur est \"%s\" (La section est \"%s\")\n",
name, value, section);
Lack of error checking
// what if `value` is "", "abc", "123xyz", "12345678901234567890", ...?
conf->server->port = atoi(value);
// what if allocation returns NULL?
conf->server->ip = strdup(value);
Respect presentation width
printf("Le nom du champ est %s, la valeur est %s (La section est %s)\n", name, value, section)
;
vs.
printf("Le nom du champ est %s, la valeur est %s (La section est %s)\n",
name, value, section);
Another reason for an auto formatter.
Curious dual language
I'd expect one.
printf("Le nom du champ est %s, la valeur est %s (La section est %s)\n", ...
printf("Unable to load configuration\n");
Conversion changes sign-ness
unsigned port;
...
conf->server->port = atoi(value);
Perhaps a helper function
// return error status
bool to_unsigned(unsigned *u, const char *s) {
if (s == NULL) {
return true;
}
char *endptr;
errno = 0;
unsigned long = strtoul(s, &endptr, 10);
if (errno || s == endptr || *endptr || value > UINT_MAX) {
return true;
}
if (u) {
*u = (unsigned) value;
}
return false;
}
if (to_unsigned(&conf->server->port, value)) {
hanlde_oops();
}