A good program is like a well-written essay. It should be well-organized, clear, and where possible, concise. The logic should be straight forward and easy for another programmer to follow. Remember that your program will almost always be read by another human. In academia it will most likely be read by a TA and in industry it will most likely be extended and modified by other programmers. Like well-written papers, well-written programs go through multiple drafts, although the multiple revision process is infeasible in industry and has never been adopted as a teaching strategy in Computer Science. Nonetheless I have often observed that I could write a much cleaner piece of code if I had the time to re-write it.
I do not want to repeat what the other two essays on good program writing had to say so I will instead present a laundary list of items that contribute to better-written programs:
int temp; temp = a; a = b; b = temp;temp is a very non-descriptive name because it does not describe its purpose, which is to save the value of a. It is true that temp informs the reader that it represents temporary storage but it would be so much better to describe what it stores. Hence I would much prefer a variable name like save_a which is very descriptive of the variable's purpose.
I think that it is sometimes okay to use short names, like i and j, for variables used in iterative loops. However, even in these cases I prefer more descriptive names when possible. For example, when looping through the rows and columns of a matrix, it is much more descriptive to write:
for (row = 0; row < max_row; row++) { for (col = 0; col < max_col; col++) { printf("%d\n", matrix[row][col]); } }than to write:
for (i = 0; i < m; i++) { for (j = 0; j < n; j++) { printf("%d\n", matrix[i][j]); } }The first block of code is self-explanatory; the second block is not. Since we all hate writing comments and never write enough of them, the least we can do is choose meaningful variable names.
typedef struct node { int value; struct node *next; } list_node; list_node *new_node = (list_node *)malloc(sizeof(list_node)); int current_item; // insert each new item at the head of the list while (scanf("%d", ¤t_item) != EOF) { new_node = (list_node *)malloc(sizeof(list_node)); new_node->value = current_item; new_node->next = head_of_list; head_of_list = new_node; }Look at the two lines that I've bold-faced. The first line allocates a block of memory for new_node and the second line makes sure that 1) the block of memory is never used, hence wasting its allocation, and 2) the block of memory is not returned to the storage manager, thereby introducing a memory leak into your program. The bottom line: don't allocate memory until you're ready to use it. That way you won't accidentally write the type of inefficient code shown above.
// find the node whose value is equal to search_key for (current_node = head_of_list; current_node != NULL; current_node = current_node->next) { if ((current_node != NULL) && (current_node->value == search_key)) return current_node; }The first test in the if statement is a complete waste of time. You wouldn't be in this loop if current_node were NULL because that's the continuation test for the loop. Don't repeat checks that must be true!
boolean EOF_p = read_next_line(&line); if (EOF_p) return;All the programmer has accomplished with EOF_p is to obfuscate the code and increase its cognitive load on the reader of the code. Now the reader of the code must keep track of EOF_p in short-term memory and remember that it's been assigned the return status of read_next_line. It's so much better to simply write:
if (read_next_line(&line)) return;Now I immediately understand the pre-requisite for returning and I don't have to struggle to remember the meaning of a temporary variable.