Assuming you are correct in saying that the file is opened successfully, your crash occurs because you set new_node = head; when head == NULL; and then attempt to read a number in &new_node->id, dereferencing a null pointer. In fact, that's probably when the crash occurs even if the file fails to open, but that's just a question of which null pointer is going to get dereferenced first.
There are quite a number of problems in this code. You are:
- leaking memory,
- not checking error returns,
- not limiting input strings to the size of the variables
- passing
char (*)[25] values to a function that expects char * values
- dereferencing null pointers,
- not putting spaces between name components,
- concatenating on the end of chunks of memory that are not null-terminated strings,
- not allocating new space for each student read, and
- vulnerable to input data errors leading to mishandled ID numbers.
That leads to a lot of annotations in a rewrite of the code.
struct studentInfo
{
char name[50];
int id;
struct studentInfo *next;
} *head = NULL;
static const char filename[] =
"C:\\Users\\Brent Rademaker\\Desktop\\COP3502C\\Assignment 1\\AssignmentOneInput.txt";
// Pass file name as argument so that multiple files can be processed
void createList(const char *file)
{
// I'd use ofp for an output file; fp when there's one file, or
// ifp for an input file
FILE *ofp;
// struct studentInfo *new_node = (struct studentInfo *)malloc(sizeof(struct studentInfo));
// struct studentInfo *temp = (struct studentInfo *)malloc(sizeof(struct studentInfo));
struct studentInfo *tail = head; // Major change
char firstName[25];
char lastName[25];
int id; // Added
// Originally:
// new_node = head; // Overwrites newly allocated memory with NULL (leak 1)
// temp = head; // Overwrites newly allocated memory with NULL (leak 2)
ofp = fopen(file, "r");
if (ofp == NULL)
{
fprintf(stderr, "failed to open file %s for reading\n", filename);
exit(1);
}
// Traverse to end of list (necessary on second call)
while (tail != NULL && tail->next != NULL)
tail = tail->next;
// Limit strings to size of variables (size - 1 specified)
// Test that you get 3 values; if you only get two, the ID is invalid.
while (fscanf(ofp, "%24s %24s %d", firstName, lastName, &id) == 3)
{
// Originally:
// strncat((new_node->name), firstName, 10);
// strncat((new_node->name), lastName, 10);
// These are appending to the end of a string that is not guaranteed
// to be null terminated. The names were not limited to 10 bytes.
// There is no space between the first and last names in the concatenated string.
// Allocate new node when student information read correctly.
// Cast left in place since compiler may be a C++ compiler compiling C
struct studentInfo *new_node = (struct studentInfo *)malloc(sizeof(*new_node));
if (new_node == NULL)
break;
// This sequence is safe because firstname contains up to 24 bytes plus null,
// lastName contains up to 24 bytes plus null, and new_node->name can
// hold 24 + 1 + 24 + 1 = 50 bytes.
strcpy(new_node->name, firstName);
strcat(new_node->name, " ");
strcat(new_node->name, lastName);
// If need be, use strcpy_s() and strcat_s()
// strcpy_s(new_node->name, sizeof(new_node->name), firstName);
// strcat_s(new_node->name, sizeof(new_node->name), " ");
// strcat_s(new_node->name, sizeof(new_node->name), lastName);
new_node->id = id;
new_node->next = NULL;
// Add new node to end of list
if (head == NULL)
head = new_node;
else
tail->next = new_node;
tail = new_node;
// Alternatively, and more simply, add new node to head of list
// Don't need the tail variable any more, or any special case code
// new_node->next = head;
// head = new_node;
}
fclose(ofp);
}
int main(void)
{
createList(filename);
return 0;
}
Alternative testing code
Instead of the existing stub main(), I used this code to test the createList() function:
static void check_file(const char *file)
{
struct studentInfo *node;
printf("Before %s\n", file);
createList(file);
printf("After %s\n", file);
node = head;
while (node != NULL)
{
printf("%.5d %s\n", node->id, node->name);
node = node->next;
}
printf("Done %s\n", file);
}
int main(void)
{
check_file("data.1");
check_file("data.2");
return 0;
}
Test data — data.1:
Firstname LastName 1234
Abby Holmes 2345
PersonWithVeryLongFirst AndWithVeryLongLastName 3456
Test data — data.2:
Firstname LastName 12784
Abby Holmes 27845
PersonWithVeryLongFirst AndWithVeryLongLastName 78456
Example output:
$ ./stud
Before data.1
After data.1
01234 Firstname LastName
02345 Abby Holmes
03456 PersonWithVeryLongFirst AndWithVeryLongLastName
Done data.1
Before data.2
After data.2
01234 Firstname LastName
02345 Abby Holmes
03456 PersonWithVeryLongFirst AndWithVeryLongLastName
12784 Firstname LastName
27845 Abby Holmes
78456 PersonWithVeryLongFirst AndWithVeryLongLastName
Done data.2
$
Middle names and other esoterica
Is there a way that I could make it variable? For instance if some people had middle names?
Yes, there are ways to do it, but they're harder, of course. Given the way that the royal family in the UK have a large number of names each (before you get into handling titles, etc), you might conclude that there is an upper limit on the number of names, such as 8. You will need to fall back onto fgets() plus sscanf() processing. You might then use:
typedef char Name[25];
Name n[9]; // At most 8 names plus an ID
char buffer[4096];
while (fgets(buffer, sizeof(buffer), ofp) != NULL)
{
int n_scan = fscanf(ofp, "%s %s %s %s %s %s %s %s %s",
n[0], n[1], n[2], n[3], n[4], n[5], n[6], n[7], n[8]);
if (n_scan < 3)
…format error…
else
{
…process elements n[0] to n[n_scan-2] as names…
…process n[n_scan-1] as id…with sscanf() probably…
…add the information to the list…
}
}
There are ways to detect whether you used everything in the line, but they're even more esoteric. You could also consider simply reading the line, then scanning backwards, ignoring trailing white space, then checking that the last 'word' is an ID, and simply using all the prior material as 'the name'. With this, though, you need to consider whether your structure should contain a char *name; element so that you copy (strdup()?) the name into it. It complicates the 'free()` process (you need to free the name before you free the node), but that's often a sensible penalty to pay.
You could also look up How to use sscanf() in loops? to iterate over the text in the line.
You could also consider using a C99 'flexible array member' (FAM) for the name; it would have to be the last part of structure. Since structures containing a FAM have to be dynamically allocated, using them with a list presents few problems. You might, or might not, need to record the length of the name — it depends on whether you'll ever change the name in an existing node or not. (If you won't, strlen(node->name) is sufficient; if you will, you need to know whether there's enough space for the new name, when the current name might be shorter than the available space.) On the whole, I think a FAM is too complex for you to manage reliably this week; you need to be more comfortable with C than you are at the moment.
