0
\$\begingroup\$

how could I optimize my code, or make it cleaner? See things to avoid. I receive data in my protocol and then analyze it in a message structure.

My protocol: {2 octet: Message ID}{? octets: CONTENT}\n

#include "pegasus.h"
# define ENDL '\n'

t_message
*create_message(int id, char *content) {
    t_message   *message;

    if (!(message = (t_message*)malloc(sizeof(t_message))))
        return (NULL);
    message->id = id;
    message->content = content;
    return (message);
}

static t_message
*parse_message(char *buffer) {
    int         id;
    char        *content;
    t_message   *message;

    id = *(short*)(buffer);
    content = (buffer + 2);
    if (!(message = create_message(id, content))) free(buffer);
    return (message);
}

static int
update(char **buffer, char *data, int count) {
    int     bytes;
    size_t  size;

    if (*buffer) {
        bytes = strlen(*buffer);
        size = (count + bytes) + 1;
        if (!(*buffer = realloc(*buffer, size)))
            return (ERROR);
        strcpy((*buffer + bytes), data);
    }
    else if (!(*buffer = strdup(data)))
        return (ERROR);
    return (SUCCESS);
}

static t_message
*get_message(char **buffer) {
    char            *message;
    size_t          size;
    int             bytes = strlen(*buffer);
    char            *delimiter = strchr(*buffer, ENDL);
    char            *rest;

    if (!delimiter) return (NULL);
    size = ((long unsigned int)delimiter - (long unsigned int)*buffer);
    if (!(message = (char*)memalloc(sizeof(char) * (size + 1))))
        return (NULL);
    strncpy(message, *buffer, size);
    while (*delimiter == ENDL) delimiter++;
    rest = strdup(delimiter);
    free(*buffer);
    *buffer = rest;
    return (parse_message(message));
}

t_message
*read_message(t_client *client) {
    char        data[BUFF_SIZE + 1] = { 0 };
    size_t      count;
    t_message   *message = NULL;
    //client->is is a file descriptor
    while ((count = recv(client->s, data, BUFF_SIZE, 0))) {
        data[count] = '\0';
        if (update(&client->buffer, data, count) != SUCCESS) return (NULL);
        if ((message = get_message(&client->buffer))) return (message);
    }
    return (NULL);
}
\$\endgroup\$
2
  • 2
    \$\begingroup\$ Post the header, so we know what that typedef actually is. \$\endgroup\$ Commented Aug 10, 2019 at 20:19
  • \$\begingroup\$ Does this code work? I suspect it doesn't. \$\endgroup\$ Commented Aug 10, 2019 at 20:34

1 Answer 1

Reset to default
3
\$\begingroup\$

Don't cast the result of malloc

Do I cast the result of malloc?

NEVER.

Safe usage of malloc

Malloc is easily misused. Problems that can arise using malloc are the following:

  • casting the result: As said above, never do this.

  • sizeof(type) vs sizeof(*foo):

foo = malloc(sizeof(*foo) * nmemb); is better because if you ever change the type of foo, this call will still be valid, while if not, you would have to change every line where malloc is called with foo. If you forget any of those lines, good luck.

Never use = inside an if, even if you mean it.

Lines should be as simple as possible, and limited to just one purpose.

This:

if (!(message = (t_message*)malloc(sizeof(t_message))))
        return (NULL);

Should be rewritten as

message = malloc(sizeof(*message));
if (!message)
        return NULL;

If that extra line is too much for you, I suggest you to use this macro around malloc() which also adds some nice extra safety: mallocs()

Unneeded code: No

Don't write absolutely useless code like this: return (a);. It just makes it less readable without improving safety or anything at all.

Just use return a; or content = buffer + 2; (or content = &buffer[2];)

Aliasing

For some reason you are aliasing a char as a short. Until you publish the header, I don't know if it's safe. What I anticipate is that it is not clear why, which is something bad in itself.

C99 types

We're in 2019, C89 shouldn't even be considered as an option when writing new code (it's acceptable for codebases of millions of lines of code which were started many decades ago, and it might be unsafe to port them to C99).

The definition of short is actually the same definition as int_least16_t. If you really want that type, use int_least16_t; if you want int16_t, which is probably what you intended to use, use it, but forget that short exists, it's dead.

Casting a pointer to an integer: NO!

(long unsigned int)delimiter: Why? NO! In the very unlikely case that you actually want to use a pointer as an integer, you have (u)intptr_t; that's the only acceptable type to use, and you need a very very good reason to do that.

memalloc()

What is that???

I'm sure there are other things, but you can start fixing those.

\$\endgroup\$

You must log in to answer this question.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.