How to make a program bettter. Does it come with experience?

Question

Hi guys look at the code given below. It is from my class work. It is a simple demonstration of a singly linked list.

#include <stdio.h>
#include <stdlib.h>
#include <wctype.h>
#include <errno.h>
#include <assert.h>
#include <ctype.h>
#include <stddef.h>
#include <wchar.h>
#include <locale.h>
#include <setjmp.h>
struct node
{
    int          info;
    struct node *link;
};

typedef struct node* NODE;

NODE main()
{
    NODE first = NULL;
    int  item, choice;

    for(;;)
    {
        printf("1.INSERT AT FRONT\n2.DISPLAY\n3.QUIT\n\nPLEASE ENTER YOUR CHOICE:");
        scanf("%d",&choice);

        switch (choice)
        {
            case 1:
            printf("\nEnter the item to be inserted:");
            scanf("%d", &item);

            first = insert_front(item,first);
            break;

            case 2:
            display(first);
            break;

            case 3:
            exit(0);
        }
    }
}

NODE getnode()
{
    NODE x;

    x = (NODE) malloc(sizeof(struct node));
    if(x == NULL)
    {
        printf("Out of memory");
        exit(0);
    }
    return x;
}

int insert_front(int item, NODE first)
{
    NODE temp;
    temp = getnode();
    temp->info = item;
    temp->link = first;
    return temp;
}

void display(NODE first)
{
    NODE temp;

    if(first == NULL)
    {
        printf("Empty");
        return;
    }

    printf("\nThe contents are: ");

    temp = first;
    while(temp != NULL)
    {
        printf("%d ", temp->info);
        temp = temp->link;
    }
    printf("\n");
}

As you can see this code demonstrates the working of a linked list. I wanna ask this question to all those hardcore C programmers that how to make this program more smaller, better, more professional and much more good. If you were given a chance to modify this program how would you do. I wanna know how to think and how to train myself to modify programs like these. And as you see I have learn't Most of C. I referred to and learn't C from K.N Kings "The C programming language: A modern approach" and "Computer science: A structured approach using C" and Data structures from "Data structures using C" by Tenenbaum.

Now I have stretched my legs in learning C what can I do to create application that might be useful to me or to others. Say I want to create a small game like "snake" or create a small application that my father might use to do his buisness. something like that. Were to go from here. How to get better at cod

Answer 1

I'm not a C-guru at all but I believe I can give you a few advices to improve your code:

1. Review your included libraries. Do you really need all those ones? Nope. Chuck Testa You only use stdio.h and stdlib.h.

2. main() should return int and you specify it actually returns NODE... but it isn't returning nothing at all! Double fail x( So... int main() { return 0; } FTW

3. Naming consistency. You have written getnode() and insert_front(). You can name your members whatever you want (getnode(), getNode(), get_node()) but keep the patter for all of them (insertfront(), insertFront(), insert_front()). IMO, underscores are much C-style.

4. Blank lines. Avoid inserting blank lines. They don't clarify the reading of your code, instead of that they expand your code unnecessarily. You can insert blank lines between your function definitions. This helps to delimit and group your functions and eases their location.

5. What if I type 4 in your menu? The golden rule in programming is: Users provide bad inputs. So you always must to validate inputs. Don't matter if it seems trivial. Always.

6. And for last and getting a bit cranky, you can get a temp node in one step within insert_front():

   NODE temp = get_node(); // I've changed `getnode()` name for you. You are welcome.
   

Answer 2

Some more (append to jackflash's answer!):

1. The printf() function has to scan the format string, and is therefore less efficient than puts(). In a similar way, printf("\n"); could be puts("\n"); but really should be putchar('\n');.

2. It's hard to use scanf() while also implementing proper error handling. It's impossible to use "scanf()" properly without checking its ("number of successful assignments") returned value. Failure to check the return value is always a bug.

3. Your switch(choice) needs a default: puts("Bad option!\n");.

4. The typedef struct node* NODE; should probably be typedef struct node NODE; instead (with corresponding changes everywhere to suit). This makes it much easier for programmers to see when something is a pointer and when it's not. For example, consider int foo(NODE *destNode, NODE srcNode); where it's easy to see what is happening; and compare it to int foo(NODE destNode, Um?? ); where the first parameter looks like "pass by value".

5. getnode() doesn't get/find a node, but creates a node instead. It should be named appropriately (e.g. create_node()).

6. getnode() could/should initialise the structure. E.g. NODE *create_node(int info) { ...; x->link = NULL; x->info = info; return x; }.

7. int insert_front() is buggy, because it returns a pointer to struct node and doesn't return an int. Your compiler should have complained loudly about this bug.

8. insert_front() could/should insert a previously created node, instead of mixing different operations ("create" and "insert"). Basically it'd be NODE *insert_front(NODE *newNode, NODE *firstNode) or perhaps void insert_front(NODE **firstNode, NODE *newNode).

Answer 3

Siddarth, here are some extra points to add to those from @JackFlash and @Brendan.

• Turn on more compiler warnings, for example with gcc I use the following: -Wall
  -Wextra
  -Wshadow
  -Wstrict-prototypes -Wmissing-prototypes
  -Wundef
  -Wunreachable-code
  -Wunused
  -Wcast-qual

• With compiler warnings enabled you will get some warnings from your code because of missing prototypes. You have no prototypes for functions. There are two options: either define all functions before (above) where they are used; or declare prototypes for functions that are used before they are defined. I usually take the former approach which means that main() is always defined at the end of the file.

• All functions, except main() that are used only within the file where they are defined should be defined as 'static': static void display(NODE first)

• In a linked list, the pointer to the next node is perhaps more logically called next

• main() has the prototype int main(int argc, char ** argv).

• Functions that take no parameters should have a 'void' parameter list, eg static NODE get_node(void) {...}

• As stated by others, don't define pointer types.

• Upper case identifiers are normally reserved for preprocessor constants (defined with #define). For your node type, use node or perhaps Node, but not NODE.

• getnode (better get_node), being so small, is sensible as a separate function only if it is to be used more than once. In your code it is not, but I guess it might be if you added an insert_end to your existing insert_front. Otherwise I would rename insert_front to insert and integrate the malloc there.

• the value returned from malloc should not be cast.

• 'x' is a poor choice of name for a 'node'. 'n' would make more sense - whatever you choose, be consistent. In display you use temp.

• combine the memory allocation and the declatation of the variable to hold the pointer: node *n = malloc(sizeof(node));

• Put a space after keywords (if, else, while, for etc) (my personal preference)

• Print errors to stderr not stdout. Use perror where you can (as in failure of malloc): perror("malloc");

• exit should use EXIT_FAILURE or EXIT_SUCCESS: exit(EXIT_FAILURE);

• insert_front should return a node * (or 'Node *)

• in display your while loop could perhaps be done as a for

  for (node *n = first; n != 0; n = n->link) 
  {
      printf("%d ", n->info);
  }