2
\$\begingroup\$

I'm making a database program that reads from a file info on tennis players. Everything works, but is there a simpler way of doing this? Is there anything I can improve here?

#include <stdio.h>
#include <string.h>
#include <stdlib.h>

const int MAX_PLAYERS = 5;

struct player {
    char *fullname[2];
    int age;
    double rating;
    char *racketBrand;   
}; typedef struct player tPlayer;

void readData(tPlayer *player){
    FILE *file = fopen("database", "r");
    if(!file){
        perror("File not found");
        return;    
    }

    int idx = 0;
    char buffer[50]; 
    while(fgets(buffer, sizeof(buffer), file)){
        player[idx].fullname[0] = malloc(20);
        player[idx].fullname[1] = malloc(20);
        player[idx].racketBrand = malloc(20); 
        sscanf(buffer, "%s %s %d %lf %s", player[idx].fullname[0], player[idx].fullname[1], 
            &player[idx].age, &player[idx].rating, player[idx].racketBrand);   
        idx++;
    }    
    fclose(file);
}

void printInfo(tPlayer *player){
    int idx;
    for(idx = 0; idx < MAX_PLAYERS; idx++){
        printf("***Name: %s %s ***Age: %d ***Rating: %.1lf ***Racket Brand: %s\n", player[idx].fullname[0], 
            player[idx].fullname[1], player[idx].age, player[idx].rating, player[idx].racketBrand);    
    }    
}

int main(){
    tPlayer player[MAX_PLAYERS];
    readData(player);
    printInfo(player);
}
\$\endgroup\$
3
\$\begingroup\$

This part hardly makes sense to me:

    player[idx].fullname[0] = malloc(20);
    player[idx].fullname[1] = malloc(20);
    player[idx].racketBrand = malloc(20);

If you are always allocating 20 bytes for your strings, why bothering with a dynamic allocation? Simply declare your strings as char whatever[20];. If you want your dynamically allocated strings to actually allocate as much memory as is needed, things will go far more complicated (see my old answer here, although you'll need to adapt it).

Your code does not allow for a tennis player to have more than one name and one surname, but I'll assume that this is on purpose. However, if we assume that your data file is not broken in a very specific way (i.e., having one player's data spread across more than one line), we could simply do this (assuming we also abandon dynamic memory allocation):

while (fscanf(file, "%19s %19s %d %lf %19s", player[idx].fullname[0], player[idx].fullname[1], &player[idx].age, &player[idx].rating, player[idx].racketBrand) == 5) idx++;

Unless there is a reason to assume that there are at most MAX_PLAYERS in the file, it would be better to dynamically allocate player (the main array), using realloc. This is, of course, more complicated than your attempt, but it is also less limiting for the user.

If you want to keep the limit on the number of players, you should also check it when reading the data, i.e., you should replace idx++ with

if (idx++ >= MAX_PLAYERS) break;

And a minor technical notion. You can replace

struct player {
    char *fullname[2];
    int age;
    double rating;
    char *racketBrand;   
}; typedef struct player tPlayer;

with

typedef struct player {
    char *fullname[2];
    int age;
    double rating;
    char *racketBrand;   
} tPlayer;

I am all for using typedef, but it is fair to warn you that there are some disagreements about it.

\$\endgroup\$

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.