John, your code is quite awkward, the place
and out
commands do not work as intended and neither out
or back to classroom
free
any of the memory you have allocated. Get in the habit now of tracking each allocation you make and then free
each before the program ends. Yes, the memory is freed when your program exits, but when you begin writing functions (as here) where you allocate memory, you must free
that memory or as your code grows, memory management will rapidly become unmanageable. (so do yourself a favor, learn to do it right from the beginning)
Next, in any interface you write where you expect the user to enter input, prompt the user for the input. Otherwise, the user is left looking at a blinking cursor on the screen wondering if the program is stuck??
Thinking though what data you need and logically providing prompts to the user will help you lay your code out in a less awkward fashion.
Do not typedef
pointers. (e.g. typedef Line *LinePtr;
) That will only serve to confuse the heck out of you as to the actual level of pointer indirection, and it also makes your code almost impossible for someone else reading through your code. (this becomes a much bigger problem as you code is spread between 10 different header and source files) Especially while you are learning C do not typedef
pointers
Let's turn to your main program loop. First, since you do not make use of int argc, char *argv[]
, the proper invocation of main
is int main(void)
making it explicit that no arguments are expected. Applying the prompting discussed above, the style changes from my comment and reordering to make a bit more sense, your main could look like:
#define ORDC 25 /* if you need constants, define them or use an enum */
#define NAMC 100 /* (don't use magic numbers in your code!) */
...
int main (void) {
char order[ORDC] = "", /* initialize all strings to zeros */
name1[NAMC] = "",
name2[NAMC] = "";
line *startptr = NULL;
/* provide an initial prompt showing valid orders to place */
printf ("orders [add, out, first, place, reverse, back to classroom]\n");
for (;;) { /* loop until done or user cancels input */
printf ("\norder: "); /* prompt for order: each iteration */
if (!fgets (order, sizeof order, stdin)) { /* use fgets for user input */
fprintf (stderr, "note: user canceled input.\n");
return 1;
}
rmcrlf (order); /* trim the trailing '\n' (returning the length) */
if (strcmp (order, "back to classroom") == 0) { /* are we done? */
backtoclassroom (startptr);
break;
}
if (strcmp (order, "reverse") == 0) { /* reverse takes no name */
reverse (&startptr);
continue;
}
printf ("name : "); /* every other function takes a student name */
if (!fgets (name1, sizeof name1, stdin)) {
fprintf (stderr, "note: user canceled input.\n");
return 1;
}
rmcrlf (name1);
if (strcmp (order, "add") == 0) /* use if, else if, else logic */
add (&startptr, name1);
else if (strcmp (order, "out") == 0)
out (&startptr, name1);
else if (strcmp (order, "first") == 0)
first (&startptr, name1);
else if (strcmp (order, "place") == 0) { /* place takes two names */
printf ("after: ");
if (!fgets (name2, sizeof name2, stdin)) { /* get name2 here */
fprintf (stderr, "note: user canceled input.\n");
return 1;
}
rmcrlf (name2);
place (startptr, name1, name2);
}
else /* handle the Bad Input case */
fprintf (stderr, "error: invalid order, try again.\n");
}
return 0;
}
When you pass an array to a function as a parameter, the first level of indirection (meaning the first [..]
following the array name) is converted to a pointer (in fact for any access of an array, other than with sizeof
, _Alignof
or the unary &
operator, or is a string literal used to initialize an array, this conversion takes place, see: C11 Standard - 6.3.2.1 Lvalues, arrays, and function designators (p3)). So your functions need only take a pointer to the array as a parameter, e.g.
void first (line **lptr, char *name);
void place (line *lptr, char *name1, char *name2);
(and use descriptive names for your variables, name
instead of array
makes a lot more sense)
For your out
function, you only have two cases you are dealing with, (1) am I deleting the first node? (if so, the list address will become that of the 2nd node), or (2) iterate until I find the node to delete and then prev->nextptr = current->nextptr; free (current);
. You could do something like:
void out (line **lptr, char *name)
{
line *iter = *lptr,
*prev = *lptr;
if (strcmp ((*lptr)->name, name) == 0) { /* name is first node */
iter = iter->nextptr; /* save pointer to next */
free (*lptr); /* free first */
*lptr = iter; /* set first = saved */
return;
}
while (iter && strcmp (iter->name, name)) { /* find node with name */
prev = iter; /* save previousptr */
iter = iter->nextptr;
}
if (!iter) { /* handle name not found */
fprintf (stderr, "error: %s not in list - can't remove.\n", name);
return;
}
prev->nextptr = iter->nextptr; /* previousptr = nextptr */
free (iter); /* free current */
}
For your place
function, you need 2 names, (1) name1
, the name of the new student, and (2) name2
, the name of the person to place them after. Adding the needed parameter to place
, you could do something like the following:
void place (line *lptr, char *name1, char *name2)
{
line *iter = lptr;
line *newptr = malloc (sizeof *newptr);
strcpy (newptr->name, name1);
newptr->nextptr = NULL;
while (iter && strcmp (iter->name, name2)) /* locate after: name */
iter = iter->nextptr;
if (!iter) { /* handle name2 not found */
fprintf (stderr, "error: %s not in list - can't place %s.\n",
name2, name1);
return;
}
newptr->nextptr = iter->nextptr;
iter->nextptr = newptr;
}
That solves your two pressing issues. There may be a corner-case or two you will need to tweak, but putting all the pieces together (and adding spaces between your function names and the (
so older eyes can read your code better, you could do something like the following:
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#define ORDC 25 /* if you need constants, define them or use an enum */
#define NAMC 100 /* (don't use magic numbers in your code!) */
typedef struct line {
char name[NAMC];
struct line *nextptr;
} line;
size_t rmcrlf (char *s);
void add (line **lptr, char *name);
void out (line **lptr, char *name);
int isempty (line *lptr);
void backtoclassroom (line *currentptr);
void first (line **lptr, char *name);
void place (line *lptr, char *name1, char *name2);
static void reverse (line **lptr);
int main (void) {
char order[ORDC] = "", /* initialize all strings to zeros */
name1[NAMC] = "",
name2[NAMC] = "";
line *startptr = NULL;
/* provide an initial prompt showing valid orders to place */
printf ("orders [add, out, first, place, reverse, back to classroom]\n");
for (;;) { /* loop until done or user cancels input */
printf ("\norder: "); /* prompt for order: each iteration */
if (!fgets (order, sizeof order, stdin)) { /* use fgets for user input */
fprintf (stderr, "note: user canceled input.\n");
return 1;
}
rmcrlf (order); /* trim the trailing '\n' (returning the length) */
if (strcmp (order, "back to classroom") == 0) { /* are we done? */
backtoclassroom (startptr);
break;
}
if (strcmp (order, "reverse") == 0) { /* reverse takes no name */
reverse (&startptr);
continue;
}
printf ("name : "); /* every other function takes a student name */
if (!fgets (name1, sizeof name1, stdin)) {
fprintf (stderr, "note: user canceled input.\n");
return 1;
}
rmcrlf (name1);
if (strcmp (order, "add") == 0) /* use if, else if, else logic */
add (&startptr, name1);
else if (strcmp (order, "out") == 0)
out (&startptr, name1);
else if (strcmp (order, "first") == 0)
first (&startptr, name1);
else if (strcmp (order, "place") == 0) { /* place takes two names */
printf ("after: ");
if (!fgets (name2, sizeof name2, stdin)) { /* get name2 here */
fprintf (stderr, "note: user canceled input.\n");
return 1;
}
rmcrlf (name2);
place (startptr, name1, name2);
}
else /* handle the Bad Input case */
fprintf (stderr, "error: invalid order, try again.\n");
}
return 0;
}
/** remove newline or carriage-return from 's'.
* returns new length. 's' must not be NULL.
*/
size_t rmcrlf (char *s)
{
char *p = s;
if (!*s) /* s is empty-string */
return 0;
/* find eol or nul-terminating char */
for (; *p && *p != '\n' && *p != '\r'; p++) {}
if (*p == '\n' || *p == '\r') /* validate eol & overwrite */
*p = 0;
else /* warn - no end-of-line */
fprintf (stderr, "rmcrlf() warning: no eol detected.\n");
return (size_t)(p - s);
}
int isempty (line *lptr)
{
return (lptr == NULL);
}
void backtoclassroom (line *currentptr)
{
printf ("\nline returning to classroom:\n\n");
if (isempty (currentptr)) {
printf ("line is empty.\n");
}
else {
while (currentptr != NULL) {
line *victim = currentptr; /* ptr to node to free */
printf (" %s\n", currentptr->name);
currentptr = currentptr->nextptr;
free (victim); /* free your memory! */
}
}
}
static void reverse (line **lptr)
{
line *previousptr = NULL,
*currentptr = *lptr,
*afterptr;
while (currentptr != NULL) {
afterptr = currentptr->nextptr;
currentptr->nextptr = previousptr;
previousptr = currentptr;
currentptr = afterptr;
}
*lptr = previousptr;
}
void out (line **lptr, char *name)
{
line *iter = *lptr,
*prev = *lptr;
if (strcmp ((*lptr)->name, name) == 0) { /* name is first node */
iter = iter->nextptr; /* save pointer to next */
free (*lptr); /* free first */
*lptr = iter; /* set first = saved */
return;
}
while (iter && strcmp (iter->name, name)) { /* find node with name */
prev = iter; /* save previousptr */
iter = iter->nextptr;
}
if (!iter) { /* handle name not found */
fprintf (stderr, "error: %s not in list - can't remove.\n", name);
return;
}
prev->nextptr = iter->nextptr; /* previousptr = nextptr */
free (iter); /* free current */
}
void first (line **lptr, char *name)
{
line *newptr = malloc (sizeof *newptr); /* set size on current *ptr */
strcpy (newptr->name, name);
newptr->nextptr = *lptr;
*lptr = newptr;
}
void place (line *lptr, char *name1, char *name2)
{
line *iter = lptr;
line *newptr = malloc (sizeof *newptr);
strcpy (newptr->name, name1);
newptr->nextptr = NULL;
while (iter && strcmp (iter->name, name2)) /* locate after: name */
iter = iter->nextptr;
if (!iter) { /* handle name2 not found */
fprintf (stderr, "error: %s not in list - can't place %s.\n",
name2, name1);
return;
}
newptr->nextptr = iter->nextptr;
iter->nextptr = newptr;
}
void add (line **lptr, char *name)
{
line *newptr = malloc (sizeof *newptr); /* set size on current *ptr */
line *lastptr = *lptr;
strcpy (newptr->name, name);
newptr->nextptr = NULL;
if (*lptr == NULL) {
*lptr = newptr;
return;
}
while (lastptr->nextptr != NULL) {
lastptr = lastptr->nextptr;
}
lastptr->nextptr = newptr;
}
Example Use/Output
$ ./bin/classline
orders [add, out, first, place, reverse, back to classroom]
order: add
name : john doe
order: add
name : mary smith
order: first
name : nancy first
order: place
name : sally second
after: nancy first
order: out
name : john doe
order: back to classroom
line returning to classroom:
nancy first
sally second
mary smith
Memory Use/Error Check
In any code you write that dynamically allocates memory, you have 2 responsibilities regarding any block of memory allocated: (1) always preserve a pointer to the starting address for the block of memory so, (2) it can be freed when it is no longer needed.
It is imperative that you use a memory error checking program to insure you do not attempt to access memory or write beyond/outside the bounds of your allocated block, attempt to read or base a conditional jump on an uninitialized value, and finally, to confirm that you free all the memory you have allocated.
For Linux valgrind
is the normal choice. There are similar memory checkers for every platform. They are all simple to use, just run your program through it.
$ valgrind ./bin/classline
==22574== Memcheck, a memory error detector
==22574== Copyright (C) 2002-2015, and GNU GPL'd, by Julian Seward et al.
==22574== Using Valgrind-3.11.0 and LibVEX; rerun with -h for copyright info
==22574== Command: ./bin/classline
==22574==
orders [add, out, first, place, reverse, back to classroom]
<snip>
line returning to classroom:
nancy first
sally second
mary smith
==22574==
==22574== HEAP SUMMARY:
==22574== in use at exit: 0 bytes in 0 blocks
==22574== total heap usage: 4 allocs, 4 frees, 448 bytes allocated
==22574==
==22574== All heap blocks were freed -- no leaks are possible
==22574==
==22574== For counts of detected and suppressed errors, rerun with: -v
==22574== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0)
Always confirm that you have freed all memory you have allocated and that there are no memory errors.
Look things over and let me know if you have further questions.