2

When I print char** surname and char** first, I get some strange outputs. I am not sure if I am doing the malloc correctly or if I'm doing something else incorrectly.

The Input -> names1.txt

The outputs

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

int main ()
{
  int size, i;
  char **surname, **first, *middle_init, dummy, str[80];
  FILE *fp_input = fopen("names1.txt", "r");

  fscanf(fp_input, "%d%c", &size, &dummy); // gets size of array from file

  /* dynamic memory allocation */

  middle_init = (char*)malloc(size * sizeof(char));
  surname = (char**)malloc(size * sizeof(char*));
  first = (char**)malloc(size * sizeof(char*));

  for (i = 0; i < size; i++)
  {
    surname[i] = (char*)malloc(17 * sizeof(char));
    first[i] = (char*)malloc(17 * sizeof(char));  
  } // for

  /* reads from file and assigns value to arrays */

  i = 0;
  strcpy(middle_init, "");

  while (fgets(str, 80, fp_input) != NULL)
  { 
    surname[i] = strtok(str, ", \n");
    first[i] = strtok(NULL, ". ");
    strcat(middle_init, strtok(NULL, ". "));
    i++;
  } // while

  /* prints arrays */      

  for (i = 0; i < size; i++)
        printf("%s %s\n", surname[i], first[i]);

  return 0;
} // main
3
  • 1
    This code leaks all dynamic allocations in the initial for-loop for surname[i] and first[i] as it process the second for-loop, then leaks the remainders before exiting main(). Commented Nov 11, 2012 at 6:54
  • 2
    You allocate memory for surname[i], then set surname[i] to a different pointer, thus losing the allocated memory block. You should be copying the tokenized parts of str into surname[i] instead. Commented Nov 11, 2012 at 6:54
  • You never check for failure from the strtok functions. Commented Nov 11, 2012 at 7:02

2 Answers 2

1

A casual look at the code suggests:

  • You must use strcpy() or a variant on the theme to copy the string found by strtok() into the surname, etc.

  • The way you've written it, you throw away your allocated memory.

  • You get the repeated output because you're storing pointers to the string you use to hold the line in the surname and first arrays. That string only holds the last line when you do the printing. This and the previous point are corollaries of the first point.

  • You only allocate a single character for the middle initials. You then use strcat() to treat them as strings. I recommend treating middle initials as strings, much like the other names. Or, since you aren't required to print them, you might decide to ignore middle initials altogether.

  • Using 17 instead of enum { NAME_LENGTH = 17 }; or equivalent is not a good idea.

There are undoubtedly other issues too.

I guess you've not reached structures in your course of study yet. If you have covered structures, you should probably use a structure type to represent a complete name, and use a single array of names instead of parallel arrays. This will likely simplify memory management too; you'd use fixed size array elements in the structure, so you'd only have to make one allocation for each name.

The code below produces the output:

Ryan Elizabeth
McIntyre O
Cauble-Chantrenne Kristin
Larson Lois
Thorpe Trinity
Ruiz Pedro

In this code, the err_exit() function is vastly valuable because it makes error reporting into a one-line call, rather than a 4-line paragraph, which means you're more likely to do the error checking. It is a basic use of variable length argument lists, and you may not understand it yet, but it is extremely convenient and powerful. The only functions that could be error checked but aren't are the fclose() and printf(). If you're reading a file, there's little benefit to checking fclose(); if you're writing and fclose() fails, you may have run out of disk space or something like that and it is probably appropriate to report the error. You could add <errno.h> to the list of headers and report on errno and strerror(errno) if you wanted to improve the error reporting more. The code frees the allocated memory; valgrind gives it a clean bill of health.

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

static void err_exit(const char *fmt, ...);

int main(void)
{
    enum { NAME_SIZE = 25 };
    const char *file = "names1.txt";
    int size, i;
    char **surname, **first, str[80];
    FILE *fp_input = fopen(file, "r");

    if (fp_input == NULL)
        err_exit("Failed to open file %s\n", file);
    if (fgets(str, sizeof(str), fp_input) == 0)
        err_exit("Unexpected EOF on file %s\n", file);
    if (sscanf(str, "%d", &size) != 1)
        err_exit("Did not find integer in line: %s\n", str);
    if (size <= 0 || size > 1000)
        err_exit("Integer %d out of range 1..1000\n", size);

    if ((surname = (char**)malloc(size * sizeof(char*))) == 0 ||
        (first = (char**)malloc(size * sizeof(char*))) == 0)
        err_exit("Memory allocation failure\n");
    for (i = 0; i < size; i++)
    {
        if ((surname[i] = (char*)malloc(NAME_SIZE * sizeof(char))) == 0 ||
            (first[i] = (char*)malloc(NAME_SIZE * sizeof(char))) == 0)
            err_exit("Memory allocation failure\n");
    }

    for (i = 0; i < size && fgets(str, sizeof(str), fp_input) != NULL; i++)
    { 
        char *tok_s = strtok(str, ",. \n");
        char *tok_f = strtok(NULL, ". ");
        if (tok_s == 0 || tok_f == 0)
            err_exit("Failed to read surname and first name from: %s\n", str);
        if (strlen(tok_s) >= NAME_SIZE || strlen(tok_f) >= NAME_SIZE)
            err_exit("Name(s) %s and %s are too long (max %d)\n", tok_s, tok_f, NAME_SIZE-1);
        strcpy(surname[i], tok_s);
        strcpy(first[i], tok_f);
    }
    if (i != size)
        err_exit("Only read %d names\n", i);

    fclose(fp_input);

    /* prints arrays */      
    for (i = 0; i < size; i++)
        printf("%s %s\n", surname[i], first[i]);

    for (i = 0; i < size; i++)
    {
        free(surname[i]);
        free(first[i]);
    }
    free(surname);
    free(first);

    return 0;
}

static void err_exit(const char *fmt, ...)
{
    va_list args;
    va_start(args, fmt);
    vfprintf(stderr, fmt, args);
    va_end(args);
    exit(1);
}
Sign up to request clarification or add additional context in comments.

Comments

0

here:

surname[i] = (char*)malloc(17 * sizeof(char));
first[i] = (char*)malloc(17 * sizeof(char));  
..
surname[i] = strtok(str, ", \n");
first[i] = strtok(NULL, ". ");

you allocate memory for surname and first and you don't use that memory because you assign to it the string returned from strtok which you should not do anyway because it points to a static buffer used by the function for parsing, you could use strdup instead:

  while (fgets(str, 80, fp_input) != NULL)   { 
    surname[i] = strdup(strtok(str, ", \n"));
    first[i] = strdup(strtok(NULL, ". "));
    middle_init[i] = strtok(NULL, ". ")[0];
    i++;  
  } // while

  /* prints arrays */          
  for (i = 0; i < size; i++)
        printf("%s %s %c\n", surname[i], first[i], middle_init[i]);

strdup will allocate memory and copy the string, this way you avoid hard coding the string length too, you should free that memory when you're done, also note that middile_init is a char array, so I just assign 1 char.

Comments

Your Answer

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

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.