3
\$\begingroup\$

I have a function that gets a string and prints it as a message. Then, it reads the text from stdin, saves it in a pointer and sends it back:

char *getString(char *queryText){
char buffer[BUFFERSIZE];
char *temp;
printf("%s", queryText);

fgets(buffer, BUFFERSIZE, stdin);

//clears \n at the end of the line
if ((strlen(buffer) > 0) && (buffer[strlen(buffer) - 1] == '\n'))
    buffer[strlen(buffer) - 1] = '\0';

temp = (char*)malloc(strlen(buffer));
//if(temp == NULL)? , checked each time in the calling function
strcpy(temp, buffer);
return temp;}

I use it every time I need to get an input from the user that needs to be saved dynamically. For example, in this function that adds a name to the char* contacts (that's just a part of it):

int add(char *contacts[], int *nextAvailableP){
if (*nextAvailableP > N-3){
    printf("Unable to add a contact, not enough memory\n");
    return FALSE;
}

int numberOfMails = 0;

//receives the name, and saves it at the right cell
contacts[*nextAvailableP] = getString("Enter the contact name\n");
if(contacts[*nextAvailableP] == NULL){
    printf("Malloc failed\n");
    return FALSE;
}
(*nextAvailableP)++;

My questions are:

  1. Is this really doing what i think it does?
  2. Do I need to check if malloc succeeded in char *getString(char *queryText) or the way it is right now?
  3. I'm just changing the pointer to the memory I've allocated, right? So I don't have to free it until I'm deleting the name from the array.
  4. Is it a good habit at all?

Whole Program

For reference, here is the whole program.

My assigment was to write a program that maintains a list of contacts and their mail addresses. I had to use char *contacts[N]; and a variable that saves the index of the next available cell in the array.

For each contact the cell at index:

  • i will save his name
  • i+1 will save the number of mails (not known in advance)
  • i+2 and beyond the mail addresses (not any format required)

I also had to

#define TRUE   1
#define FALSE  0

The program will present the user the following menu:

Choose an option:

1. Enter new contact (Had to check that there is enough room for the minimal contact)
2. Search contact
3. Delete mail from contact (only if he has more than 1 email adress)
4. Delete Contact (Delete everythings about him, and rearrange the array)
5. Print
6. Exit

We can assume that the maximum length of a name or a mail will be 50, and that the inputs are valid.

#include <stdio.h>
#include <stdlib.h>
#define TRUE   1
#define FALSE  0

const int N = 100;
const int BUFFERSIZE = 50;

void printMenu();
int add(char* contacts[], int* nextAvailableP);
char *getString(char *queryText);
void deleteMailAddress(char* contacts[], int* nextAvailableP);
int existReturnIndex(char* contacts[], int* nextAvailableP);
void deleteContact(char* contacts[], int* nextAvailableP);
void printArray(char* contacts[], int* nextAvailable);
void freeMemory(char* contacts[], int* nextAvailable);
int main()
{
    char *contacts[N];
    int nextAvailable = 0;
    int quit = FALSE;
    int option;
    printMenu();
    while(quit == FALSE){
        scanf(" %d", &option);
        getchar();
        switch(option){
        case 1:
            add(contacts, &nextAvailable);
            break;
        case 2:
            find(contacts, &nextAvailable);
            break;
        case 3:
            deleteMailAddress(contacts, &nextAvailable);
            break;
        case 4:
            deleteContact(contacts, &nextAvailable);
            break;
        case 5:
            printArray(contacts, &nextAvailable);
            break;
        case 6:
            freeMemory(contacts, &nextAvailable);
            quit = TRUE;
            break;

        default:
            printf("Enter a valid number between 1 to 6\n");
        }
        printMenu();
    }
}

//Prints the menu
void printMenu(){
    printf("Choose an option \n1. Enter new contact.\n2. Search contact.\n3. Delete mail from contact.\n4. Delete Contact.\n5. Print.\n6. Exit.\n\n");
}

void freeMemory(char *contacts[], int* nextAvailableP){
    int i;
    for(i=0; i<*nextAvailableP; i++){
        free(contacts[i]);
    }
    *nextAvailableP = 0;
}
void printArray(char *contacts[], int* nextAvailableP){
    int i;
    for(i=0; i<*nextAvailableP;){
        printf("%s:\n", contacts[i++]);
        int tempNumOfMails = atoi(contacts[i]);
        printXmailsNumberd(contacts, tempNumOfMails, ++i);
        i+=tempNumOfMails;
    }
}
//Gets a contact name and deletes it from the list if he exists, rearranging the array afterwards
void deleteContact(char *contacts[], int *nextAvailableP){
    int i = existReturnIndex(contacts, nextAvailableP);
    if(i != -1){
        int numberOfMails = atoi(contacts[i+1]);
        for(i; i<*nextAvailableP-(numberOfMails+2); i++){
            strcpy(contacts[i], contacts[i+numberOfMails+2]);
        }
        (*nextAvailableP)--;
        while(*nextAvailableP>=i){
            free(contacts[*nextAvailableP]);
            (*nextAvailableP)--;
        }
    }
    else{
        printf("Not on the list\n");
    }
}

//Gets the array, the number of mails to print and their first index and prints them numberd
void printXmailsNumberd(char* contacts[], int X, int beginIndex){
    int j;
    for(j=0; j<X; j++,beginIndex++){
        printf("%d. %s\n",j+1, contacts[beginIndex]);
    }
    printf("\n");
}
//Deletes a single mail address from a given user, if he exists, and rearrange the array afterwards
void deleteMailAddress(char* contacts[], int* nextAvailableP){
    int i = existReturnIndex(contacts, nextAvailableP);
    if(i != -1){
        i++;
        int numberOfMails = atoi(contacts[i]);
        int firstMailIndex = ++i;
        if(numberOfMails > 1){
            printXmailsNumberd(contacts, numberOfMails, i);
            printf("Choose the number of one of the above\n");
            int mailToDelete;
            scanf(" %d", &mailToDelete);
            for(i=mailToDelete+firstMailIndex-1; i<*nextAvailableP-1; i++){
                strcpy(contacts[i], contacts[i+1]);
            }
            itoa((numberOfMails-1),contacts[(firstMailIndex-1)], 10);
            (*nextAvailableP)--;
            free(contacts[*nextAvailableP]);
        }
        else{
            printf("Only one address available, you're not allowed to delete it\n");
        }
    }
    else{
        printf("Not on the list\n");
    }
}
//Checks if a name exits in the array and return his index
int existReturnIndex(char* contacts[], int* nextAvailableP){
    char *name = getString("Enter the contact name\n");
    if(name == NULL){
            printf("Malloc failed\n");
            return FALSE;
        }
    int i;
    for(i=0; i<*nextAvailableP; i++){
        if(strcmp(contacts[i],name) == 0){
           return i;
        }
    }
    free(name);
    return -1;
}
//prints the name of the person the user was looking for, and his mails, if he is on the list
void find(char *contacts[], int *nextAvailableP){
    int numberOfMails;
    int i = existReturnIndex(contacts, nextAvailableP);
    if(i != -1){
        i++;
        numberOfMails = atoi(contacts[i++]);
        printf("\n%s:\n", contacts[i-2]);
        printXmailsNumberd(contacts, numberOfMails, i);
    }
    else{
        printf("Not on the list\n");
    }
}

//Adds a contact to the list
int add(char *contacts[], int *nextAvailableP){
    if (*nextAvailableP > N-3){
        printf("Unable to add a contact, not enough memory\n");
        return FALSE;
    }

    int numberOfMails = 0;

    //receives the name, and saves it at the right cell
    contacts[*nextAvailableP] = getString("Enter the contact name\n");
    if(contacts[*nextAvailableP] == NULL){
        printf("Malloc failed\n");
        return FALSE;
    }
    (*nextAvailableP)++;

    //allocates the memory needed for the number of email addresses
    int numberOfMailsIndex = (*nextAvailableP);
    contacts[numberOfMailsIndex] = (char*)malloc(5);
    itoa(numberOfMails, contacts[numberOfMailsIndex], 10);
    (*nextAvailableP)++;

    //receives the name, and saves it at the right cell
    printf("Enter the mail/s, use enter to enter a mail and '-1' to signal you finished\n");
    while(1){
        if (*nextAvailableP > N-1){
            printf("Unable to add another mail, not enough memory\n");
            return FALSE;
        }
        char *tempMail = getString("Enter mail or '-1' to stop\n");
        if(tempMail == NULL){
            printf("Malloc failed\n");
            return FALSE;
        }
        if(strcmp(tempMail,"-1") == 0){
            break;
        }
        contacts[*nextAvailableP] = tempMail;
        printf("\nAdded!\n");
        (*nextAvailableP)++;
        numberOfMails++;
        itoa(numberOfMails, contacts[numberOfMailsIndex], 10);
    }
}

//Gets a string and prints it as a message.
//Then, read the text from stdin, saves it in a pointer and sends it back
char *getString(char *queryText){
char buffer[BUFFERSIZE];
char *temp;
printf("%s", queryText);

fgets(buffer, BUFFERSIZE, stdin);

//clears \n at the end of the line
if ((strlen(buffer) > 0) && (buffer[strlen(buffer) - 1] == '\n'))
    buffer[strlen(buffer) - 1] = '\0';

temp = (char*)malloc(strlen(buffer));
//if(temp == NULL)? , checked each time in the calling function
strcpy(temp, buffer);
return temp;}
\$\endgroup\$
  • \$\begingroup\$ @Reinderien I'll be happy to, but it's quite (realtivly). Do I just post it here the same way or it's better to uplaod it to somewhere else? \$\endgroup\$ – RedYoel Nov 25 '20 at 20:15
  • 2
    \$\begingroup\$ Please do not edit the question, especially the code after an answer has been posted. Everyone needs to be able to see what the reviewer was referring to. What to do after the question has been answered. \$\endgroup\$ – Mast Nov 25 '20 at 20:44
  • \$\begingroup\$ CC @Reinderien ^^ \$\endgroup\$ – Mast Nov 25 '20 at 20:44
  • \$\begingroup\$ @Mast ho sorry I just did, but I left the old part of code too. I'll upload a new question, can you edit the question back to the original? \$\endgroup\$ – RedYoel Nov 25 '20 at 20:51
  • 2
    \$\begingroup\$ The current question title, which states your concerns about the code, applies to too many questions on this site to be useful. The site standard is for the title to simply state the task accomplished by the code. Please see How do I ask a good question?. \$\endgroup\$ – BCdotWEB Nov 25 '20 at 21:02
5
\$\begingroup\$

Your code is not correct. It can crash.

First, look at the line

fgets(buffer, BUFFERSIZE, stdin);

If an error occurs while reading, the state of buffer is indeterminate. You should not continue processing it in that case. You need to detect this by the appropriate means and signal a failure.

Second, if the malloc() fails, what will be the effect of the next line which is

strcpy(temp, buffer);

hint: it's not good

To answer your specific questions:

  1. Mostly
  2. Yes
  3. Your wording is imprecise. You're not changing the pointer, you're changing the memory pointed to by the pointer.
  4. It is somewhat frowned upon to have a function allocate memory and then return a pointer to, because the caller may not know how the memory was allocated and how it needs to be deallocated (is it static? is it on the malloc heap? is it in a private buffer? is it in an OS-specific heap?). For a simple example what you have is probably okay but Best practice is for the caller to allocate it and then pass it in.
\$\endgroup\$
  • \$\begingroup\$ About the fgets(buffer, BUFFERSIZE, stdin); - I can assume at this point that the input is correct. About the malloc() - understood, (I edited the post). So do I have to check it again afterwards? About 3, That's probably my english - but just to make sure again - in terms of freeing the memory I'm ok, right? Thank you \$\endgroup\$ – RedYoel Nov 25 '20 at 20:11
  • \$\begingroup\$ Why can you assume that "the input is correct" but can't assume "there's enough memory so that malloc() will succeed"? What is the source of those assumptions? If you're developing in a situation where malloc() could fail, how can you possibly assume that fgets() will have enough memory available to do what it needs to do? \$\endgroup\$ – Snowbody Nov 25 '20 at 20:55
  • \$\begingroup\$ I'm a student, that's my first assigment about pointers and malloc. The source is my proffesor \$\endgroup\$ – RedYoel Nov 25 '20 at 20:58
  • 2
    \$\begingroup\$ What do you mean by "check it again"? What "it" are you talking about? The rule of malloc() is, you have to check that its return value is not NULL. Once you have determined that it is NULL, you cannot proceed as intended, you have to abort and/or signal an error. How you signal that error is up to you. Whoever calls your function then needs to detect that error and respond appropriately. So if you signal your error by returning NULL, then... (finish the thought yourself) \$\endgroup\$ – Snowbody Nov 25 '20 at 20:59
  • 1
    \$\begingroup\$ Then you need to check with your professor exactly what you can and cannot assume, and make sure you include all assumptions when you ask about the code. \$\endgroup\$ – Snowbody Nov 25 '20 at 21:00

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.