2
\$\begingroup\$

I made a function that writes all the data from a hashtable to a .txt file and another to read the said data.

The data comes from a this type of struct and it should be read into it when called

typedef struct user{
    char nick[6];
    char name[26];
    int n_messages;
    int n_following;
    int n_followers;
    hashtable2 *arr_following;
    hashtable2 *arr_unfollowed;
}user;

It`s basically an hashtable with that struct as an element and each element can hold two more hashtables each one holding a element of this type:

typedef struct following{
    char nick[6];
    int last_message;
    bool first_time;
}following;

The resulting file is:

zero7;James Bond;2;1;0[MrPym;1;true](zero7;0;false)
MrPym;A Perfect Spy;1;0;1
zsq4r;Pseu Donym;3;1;2[zero7;2;true]
zero7;James Bond;4;3;3[zsq4r;3;true][zero7;3;false][MrPym;1;true]
MrPym;A Perfect Spy;1;3;1[zsq4r;3;true][zero7;4;true][AlecS;1;true]
AlecS;He Who Came from the Cold;1;0;1

Where zero7;James Bond;2;1;0 is the user info in order, [MrPym;1;true] is the information of the struct following and (zero7;0;false) the information also of the struct following but from the second hashtable.

Note: He Who Came from the Coldis a valid name for the user struct and names can also contain ints if that is important.

The function that writes the file:

void write_to_file(hashtable *active_users, FILE *fp_active){
    for(int i = 0; i < active_users->size; i++){
        if(active_users[i].buckets != NULL){
            fprintf (fp_active,"%s;%s;%d;%d;%d",active_users[i].buckets->nick, active_users[i].buckets->name,
                     active_users[i].buckets->n_messages, active_users[i].buckets->n_following,
                     active_users[i].buckets->n_followers);
            hashtable2 *current = active_users[i].buckets->arr_following;
            for(int j = 0; j<current->size; j++){
                if(current[j].buckets != NULL)
                    fprintf (fp_active,"[%s;%d;%s]",current[j].buckets->nick, current[j].buckets->last_message, current[j].buckets->first_time ? "true" : "false");
            }
            hashtable2 *current2 = active_users[i].buckets->arr_unfollowed;
            for(int j = 0; j<current2->size; j++){
                if(current2[j].buckets != NULL)
                    fprintf (fp_active,"(%s;%d;%s)",current2[j].buckets->nick, current2[j].buckets->last_message, current2[j].buckets->first_time ? "true" : "false");
            }
            fprintf(fp_active, "\n");
        }
    }
    fclose (fp_active);
}

The function that reads the file:

void read_from_file(hashtable *active_users, FILE *fp_active){
    char m_bool[6];
    char *first_token;
    char *last_token;
    char buffer[1540];
    char buffer2[1540];
    while(fgets(buffer, 1540, fp_active)) {
        strcpy(buffer2, buffer);
        user *new_user = malloc(sizeof(user));
        new_user->arr_following = create_hash2();
        new_user->arr_unfollowed = create_hash2();
        last_token = strtok( buffer, "[");
        sscanf(last_token,"%[^;]; %[^;]; %d; %d; %d", new_user->nick, new_user->name, &new_user->n_messages, &new_user->n_following,
               &new_user->n_followers);
        last_token = strtok( NULL, "[");
        while(last_token != NULL){
            following *tmp_following = malloc(sizeof(following));
            sscanf(last_token," %[^;]; %d; %5s", tmp_following->nick, &tmp_following->last_message, m_bool);
            if(strcmp(m_bool, "true]") == 0)
                insert_hash2(new_user->arr_following, tmp_following->nick, tmp_following->last_message, true);
            else
                insert_hash2(new_user->arr_following, tmp_following->nick, tmp_following->last_message, false);
            last_token = strtok( NULL, "[");
        }
        first_token = strtok( buffer2, ")");
        while(first_token != NULL && strcmp(first_token, buffer2) != 0){
            following *tmp_following = malloc(sizeof(following));
            sscanf(first_token," %[^;]; %d; %5s", tmp_following->nick, &tmp_following->last_message, m_bool);
            if(strcmp(m_bool, "true]") == 0)
                insert_hash2(new_user->arr_unfollowed, tmp_following->nick, tmp_following->last_message, true);
            else
                insert_hash2(new_user->arr_unfollowed, tmp_following->nick, tmp_following->last_message, false);
            first_token = strtok( NULL, "(");
        }
        insert2(active_users, new_user);
    }
}

What i want to be reviewed is essentially the read function but i provided the write function also in case a change to the style the file is made into needs to be improveded to make reading easier, because i think the read function is quite slow and sloppy, even if it works.

The file can have thousands of lines each holding up to 1540 chars.

Note2: i cant use a method such as strtok_r(), or any other that arent from the C standard libraries

\$\endgroup\$
3
\$\begingroup\$

What i want to be reviewed is essentially the read function

void read_from_file(hashtable *active_users, FILE *fp_active){

Crap happens

void read_from_file() lacks a way to indicate something bad happened like out-of-memory or error is reading data. Consider returning something to indicate failure or not. In general, the entire function assumes "no problems". To me, this is not robust. Error checking pays for itself as good code is expanded to ever more applications - and things happen.

#include <stdbool.h>

// Return error state
bool read_from_file(hashtable *active_users, FILE *fp_active){
  ...
  if (*new_user == NULL) {
    return true;
  }
  new_user->arr_following = create_hash2();
  if (new_user->arr_following == NULL) {
    free(new_user);
    return true;
  }
  ...
  if (ferror(fp_active)) {
    return true;
  }
  return false;
}       

Unclear code

The approach of a copy with strcpy(buffer2, buffer); is unclear. Certainly code could use a single pass. Look into using strcspn() and strspn() to find the '[' and ']' delimiters first and then parse that token, perhaps in a helper function.


Minor

Format to presentation width

With an auto formatter, this should be trivial to shorten lines past presentation.

    sscanf(last_token,"%[^;]; %[^;]; %d; %d; %d", new_user->nick, new_user->name, &new_user->n_messages, &new_user->n_following,
    //vs 
    sscanf(last_token,"%[^;]; %[^;]; %d; %d; %d", 
        new_user->nick, new_user->name, &new_user->n_messages, &new_user->n_following,

Buffer a bit too small

" file can have thousands of lines each holding up to 1540 chars" implies a line is up to 1540 characters and then an end-of-line or 1540 characters including an end-of-line. In either case, buffer[1540] is at least 1 or 2 char too small as fgets() needs a char to append a null character.

// char buffer[1540];
// while(fgets(buffer, 1540, fp_active)) {
#define LINE_CHAR_SIZE 1540
char buffer[LINE_CHAR_SIZE + 1 + 1];  // +1 for \n +1 for \0
while(fgets(buffer, sizeof buffer, fp_active)) {

The magic number 1540 is unfounded. Just some value? It deserves a comment to explain its rational - even as terse as system requirement.

No out-of-memory detection

Code lacks OOM detection. Further, recommend to allocate to the sizeof the de-referenced pointer, rather than to the size of the type. Easier to code, review and maintain.

// user *new_user = malloc(sizeof(user));
user *new_user = malloc(sizeof *new_user);
if (*new_user == NULL) {
  return true;
}

Asymmetric file handling

write_to_file() ends with the file closed. read_from_file() returns with the file still open.

Inconsistent width handling

sscanf(last_token," %[^;]; %d; %5s"... limits the input width with %5s (good) yet no limit with %[^;] (bad). I'd expect limits on both

// " %[^;]; %d; %5s"
" %5[^;]; %d; %5s" 

No error check when scanning

This is part of "Crap happens". Check the return value of scans.

// sscanf(last_token," %[^;]; %d; %5s", ....
if (sscanf(last_token," %5[^;]; %d; %5s", ...) != 3) {
  Handle_Bad_Input();
}

Memory leak?

It is not clear following *tmp_following = malloc(sizeof(following)); has a corresponding free(tmp_following);

\$\endgroup\$
  • \$\begingroup\$ thanks for the tips, i will try to improve the code \$\endgroup\$ – MiguelD Jun 2 '18 at 23:52

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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