4
\$\begingroup\$

The objective is to get a list of number and then switch the last person surname with the first person surname; when I type '$' it do the changes and finish the program! (POINTERS and DYNAMIC ALLOCATION).

I have to use pointers and dynamic allocation, my code is nice, but I know that it can be better.

How can I start to identify parts of my code that could be extracted to functions?

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

int main()
{
    char ***names=NULL;
        int i, j, k, letter, totalNames, auxInt;
        int *wordsbyname= NULL;
        char *auxWord;
        i=j=k=0;

        do
        {
                names=realloc(names, (i+1)*sizeof(char**));
                names[i]=NULL;
                wordsbyname=realloc(wordsbyname, (i+1)*sizeof(int));
                j=0;
            do
            {
                names[i]=realloc(names[i],(j+1)*sizeof(char*));
                names[i][j]=NULL;
                k=0;
                    do{
                        letter=getchar();
                        names[i][j]=realloc(names[i][j], (k+1)*sizeof(char));
                        names[i][j][k]=letter;
                        k++;
                    }while ((letter!= 10) && (letter!=32) && (letter!=36));
                    names[i][j][k-1]='\0';
                    j++;
            }while((letter!= 10)&&(letter!=36));
            wordsbyname[i]=j;
            i++;
        }while (letter!=36);
        totalNames=i;
        printf("\n");

        for(i=0;i<totalNames/2;i++){
            auxInt=totalNames-i-1;
            auxWord=malloc(sizeof(char)*(strlen(names[i][wordsbyname[i]-1])+1));
            strcpy(auxWord,names[i][wordsbyname[i]-1]);
            names[i][wordsbyname[i]-1]=realloc(names[i][wordsbyname[i]-1], sizeof(char)*strlen((names[auxInt][wordsbyname[auxInt]-1])+1));
            strcpy(names[i][wordsbyname[i]-1],names[auxInt][wordsbyname[auxInt]-1]);
            names[auxInt][wordsbyname[auxInt]-1]=realloc(names[auxInt][wordsbyname[auxInt]-1],sizeof(char)*(strlen(auxWord)+1));
            strcpy(names[auxInt][wordsbyname[auxInt]-1],auxWord);
            free (auxWord);
        }

        for(i=0 ; i<totalNames; i++){
            for (j=0; j<wordsbyname[i];j++){
                printf("%s ", names[i][j]);
                free(names[i][j]);
            }
            free(names[i]);
            printf("\n");
        }
    free(wordsbyname);
    free(names);

    return 0;
}

\$\endgroup\$
3
  • 1
    \$\begingroup\$ This post has been un-deleted by the community, with the following rationale: it was on-topic; and any concerns about language were intended to communicate that the descriptive bodies of answers are in English on StackExchange. I apologise that I did not make that clear. \$\endgroup\$
    – Reinderien
    Jul 2, 2020 at 14:20
  • 2
    \$\begingroup\$ @Miguel Avila I noticed you formatted code. Suggest reviewing Should you edit someone else's code in a question?, rollback your edit and apply reinderien changes. \$\endgroup\$ Jul 2, 2020 at 16:39
  • 1
    \$\begingroup\$ @MiguelAvila Please stop editing / formatting code we generally don't approve code edits unless they are made by the original poster. Format is part of what gets reviewed, especially certain languages such as python. \$\endgroup\$
    – pacmaninbw
    Jul 2, 2020 at 17:50

1 Answer 1

2
\$\begingroup\$

I can't comment on variable names because I don't know your language, this will limit the scope of the review I can do and I hope you understand everything in this review / answer. The memory management is good, I don't see any memory leaks.

Test for Possible Memory Allocation Errors
In modern high level languages such as C++, memory allocation errors throw an exception that the programmer can catch. This is not the case in the C programming language. While it rare in modern computers because there is so much memory, memory allocation can fail, especially if the code is working in a limited memory application such as embedded control systems. In the C programming language when memory allocation fails, the functions malloc(), calloc() and realloc() return NULL. Referencing any memory address through a NULL pointer results in unknown behavior (UB).

Possible unknown behavior in this case can be a memory page error (in Unix this would be call Segmentation Violation), corrupted data in the program and in very old computers it could even cause the computer to reboot (corruption of the stack pointer).

To prevent this unknown behavior a best practice is to always follow the memory allocation statement with a test that the pointer that was returned is not NULL.

Example of Current Code:

        nomes = realloc(nomes, (i + 1) * sizeof(char **));
        nomes[i] = NULL;
        palavraspornome = realloc(palavraspornome, (i + 1) * sizeof(int));
        j = 0;
        do

Example of Current Code with Test:

    do
    {
        nomes = realloc(nomes, (i + 1) * sizeof(char **));
        if (nomes != NULL)
        {
            nomes[i] = NULL;
            palavraspornome = realloc(palavraspornome, (i + 1) * sizeof(int));
            if (palavraspornome == NULL)
            {
                fprintf(stderr, "Memory allocation error when allocating palavraspornome.\nUnable to recover, program exiting.\n");
                return EXIT_FAILURE;
            }
        }
        else {
            fprintf(stderr, "Memory allocation error when allocating nomes.\nUnable to recover, program exiting.\n");
            return EXIT_FAILURE;
        }
        j = 0;
        do

A good way to get rid of some of the code repetition would be to create a function that allocates memory, does the test, reports the error when it happens and exits the program.

void *safe_memory_allocation(size_t size_to_allocate)
{
    void *return_pointer = malloc(size_to_allocate);
    if (return_pointer == NULL)
    {
        fprintf(stderr, "Memory allocation error .\nUnable to recover, program exiting.\n");
        exit(EXIT_FAILURE);
    }
    
    return return_pointer;
}

Convention When Using Memory Allocation in C
When using malloc(), calloc() or realloc() in C a common convetion is to sizeof(*PTR) rather sizeof(PTR_TYPE), this make the code easier to maintain and less error prone, since less editing is required if the type of the pointer changes.

    do
    {
        nomes = realloc(nomes, (i + 1) * sizeof(*nomes);
        if (nomes != NULL)
        {
            nomes[i] = NULL;
            palavraspornome = realloc(palavraspornome, (i + 1) * sizeof(*palavraspornome));
            if (palavraspornome == NULL)
            {
                fprintf(stderr, "Memory allocation error when allocating palavraspornome.\nUnable to recover, program exiting.\n");
                return EXIT_FAILURE;
            }
        }
        else {
            fprintf(stderr, "Memory allocation error when allocating nomes.\nUnable to recover, program exiting.\n");
            return EXIT_FAILURE;
        }
        j = 0;
        do

Complexity
The function main() is too complex (does too much). As programs grow in size the use of main() should be limited to calling functions that parse the command line, calling functions that set up for processing, calling functions that execute the desired function of the program, and calling functions to clean up after the main portion of the program.

There is also a programming principle called the Single Responsibility Principle that applies here. The Single Responsibility Principle states:

that every module, class, or function should have responsibility over a single part of the functionality provided by the software, and that responsibility should be entirely encapsulated by that module, class or function.

There are at least 3 sub functions that can be extracted from main(), each of the major loops is a candidate for a function, some of the smaller loops may also be good candidates. A sample function that you could implement if you are not on a POSIX where this is a C library function is char *strdup(const char *str1) (this function was merged into the C ISO standard in 2019).

The implementation of strdup() would look something like this:

char *strdup(const char *original_string)
{
    size_t new_size = strlen(original_string) + 1;
    char *duplicate = calloc(sizeof(*original_string), new_size);
    if (duplicate != NULL)
    {
        memcpy(&duplicate[0], original_string, new_size);
    }
    return duplicate;
}

Whether you write or own or use the library version strdup() returns NULL if the memory allocation failed so the value must be tested before use.

Possible Performance Improvements
Rather than use character input (letra = getchar();) get a line of text at a time using fgets(char *buffer, size_t buffer_size, FILE *filename). This reduces the amount of code necessary and it will be faster than character input.

Instead of

        do
        {
            nomes[i] = realloc(nomes[i], (j + 1) * sizeof(char *));
            nomes[i][j] = NULL;
            k = 0;
            do
            {
                letra = getchar();
                nomes[i][j] = realloc(nomes[i][j], (k + 1) * sizeof(char));
                nomes[i][j][k] = letra;
                k++;
            } while ((letra != 10) && (letra != 32) && (letra != 36));
            nomes[i][j][k - 1] = '\0';
            j++;
        } while ((letra != 10) && (letra != 36));

Use

        do
        {
            nomes[i] = realloc(nomes[i], (j + 1) * sizeof(char *));
            nomes[i][j] = NULL;
            k = 0;
            char buffer[1024];
            if (fgets(buffer, 1024, stdin) != 0)
            {
                nomes[i] = strdup(buffer);
            }
            j++;
        } while ((letra != 10) && (letra != 36));
        palavraspornome[i] = j;

Rather than calling realloc() for every new string create an array of an arbitrary number of names and then reallocate that array when the capacity is reached. the following code is a basic idea, it may not compile properly or run

char ***resize_nomes_array(char ***nomes, size_t *current_capacity, int i)
{
    char ***new_nomes = NULL;

    if (current_capacity == 0)
    {
        *current_capacity = 10;
    }
    else
    {
        *current_capacity = (size_t)(*current_capacity * 1.5);
    }

    if (nomes == NULL)
    {
        new_nomes = calloc(*current_capacity, sizeof(*new_nomes));
    }
    else
    {
        new_nomes = realloc(nomes, *current_capacity * sizeof(*nomes));
    }

    if (new_nomes == NULL) {
        fprintf(stderr, "Memory allocation error when allocating nomes.\nUnable to recover, program exiting.\n");
        exit(EXIT_FAILURE);
    }

    for (size_t index = i; index < *current_capacity; index++)
    {
        new_nomes[index] = NULL;
    }
    return new_nomes;
}
\$\endgroup\$
8
  • \$\begingroup\$ It's called undefined behavior, not unknown. \$\endgroup\$ Jul 2, 2020 at 23:10
  • \$\begingroup\$ Why calloc in strdup? \$\endgroup\$ Jul 2, 2020 at 23:12
  • \$\begingroup\$ @RolandIllig to automatically set the last character to \0. \$\endgroup\$
    – pacmaninbw
    Jul 3, 2020 at 0:00
  • \$\begingroup\$ That's a useless argument since the code already copies n + 1 bytes. \$\endgroup\$ Jul 3, 2020 at 5:53
  • \$\begingroup\$ @RolandIllig suppose you explain why calloc() rather than malloc() is a bad idea? \$\endgroup\$
    – pacmaninbw
    Jul 3, 2020 at 19:54

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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