Can you criticize this code?

Question

I was wondering if anyone would be so kind as to look at this code I've written for practice. Besides a little php, c if my first language and I'm trying to teach myself. so pretty much, what am I doing wrong, where can I optimize, anything.

What this code does is (in terminal) draw a bunch of random 1's and 0's, but I added an effect kinda like a lattice. the bulk of this I got help on here.

#include <stdio.h>
#include <stdlib.h>
#include <sys/ioctl.h>

int main (int argc, char *argv[]){   //check for whitch effect to print
    switch(*argv[1]){
     case '1': lattus();break;
     case '2': normal();break;
     default: printf("1 = lattus effect | 2 = static effect\n");break;
    }
}

char *randstring (char *buffer, int length){     //genertate a random number
    int i = length;
    while(--i >= 0) {
     buffer[i] = (rand() % 2) ? '1' : '0';
    }
    buffer[length] = 0;
    return buffer;
}

int normal(){     // normal drawing of 1's and 0's
    struct winsize w;
    ioctl(0, TIOCGWINSZ, &w);
    int width = w.ws_col;
    int height = w.ws_row;  //get terminal width and height
    char buffer[width*height + 1]; //create a buffer big enough to hold one draw to the screen
    int i = 25;
    while(i-- >= 0) {
     printf("%s\n", randstring(buffer, width*height)); //draw to screen
     usleep(90000);
    }
    system("clear");  //clear screen
}

int lattus (void){
    struct winsize w;
    ioctl(0, TIOCGWINSZ, &w);
    int width = w.ws_col;  //get the terminal width
    char buffer1[width + 1]; //create 3 buffers for each segment
    char buffer2[width + 1]; //each big enough to hold the width of the terminal
    char buffer3[width + 1];
    int first = 1;   //how many before the space
    int second = width - 8;  //how many in the middle of the space
    int third = 1;   //how many at the end of the space
    int i = 1000;   //draw 1000 lines
    int on = 0;   //switch for growing and shrinking
    while(i-- >= 0){
     usleep(9000);
     if(first == 1 && third == 1 && second == width - 8 | second == width - 9){ //is it at min?
      if(second % 2 == 0){  //is it an even number (had problems with buffer if it was odd)
       second = second - 2;
      }else{
       second = second - 3;
      }
      first ++;
      third ++;
      on = 0;  //keep growing
     }else if(first == (width - 8) / 2 && third == (width - 8) / 2 && second == 2){ //untill it gets to max
      if(second % 2 == 0){
       second = second + 2;
      }else{
       second = second + 1;
      }
      third --;
      first --;
      on = 1;  //start shrinking
     }else if(on == 0){ //else if suppost to grow, grow
      second = second - 2;
      third ++;
      first ++;
     }else if(on == 1){ //else if suppost to shrink shrink
      second = second + 2;
      third --;
      first --;
     }else{
      break;
     }
     printf("%s   %s   %s\n", randstring(buffer1, first), randstring(buffer2, second), randstring(buffer3, third)); //print it out
    }
    system("clear"); //clear screen
}

Answer 1

A few pointers:

First, variable length arrays are only allowed in C99:

char buffer1[width + 1];

If the code is to be strictly ANSI C, one would make an pointer to an array and use a function like malloc or calloc to allocate some memory for the array:

char* buffer = malloc(sizeof(char) * (width + 1));

(Then, if necessary, the buffer would be initialized, or calloc used instead to initialize and allocate at once.)

Second, code readability:

if(second % 2 == 0){
    second = second - 2;
}else{
    second = second - 3;
}

Could be better written as:

if (second % 2 == 0) {
    second = second - 2;
} else {
    second = second - 3;
}

It may seem like nitpicking, but it does make it easier for people to read your code and understand what is going on. Coding is a form of communication as well -- make it easier for others to read, and it others will be able to better understand what you're trying to achieve.

The switch statement could be similarly improved:

switch(*argv[1]){
    case '1': lattice();break;
    case '2': normal();break;
    default: printf("1 = lattice effect | 2 = static effect\n");break;
}

To:

switch(*argv[1]) {
    case '1':
        lattice();
        break;
    case '2':
        normal();
        break;
    default:
        printf("1 = lattice effect | 2 = static effect\n");
}

Also, adding line breaks between certain blocks of statement which perform one task may improve readability.

There seems to be copious use of while loops, but I don't see a single for loop -- for loops are probably more useful for performing "counter"-type activities, such as performing repetitions for a certain number of iterations:

int i;
for (i = 0; i < 100; i++) {
    /* Action to perform 100 times */
}

The basic structure of a for loop is as follows:

for (initial-condition; loop-condition; expression-on-each-iteration) {
    /* Action to perform on each iteration. */
}

(Note: The name for the conditions are not official names -- I just made them up to describe what they're for.)

The initial-condition is what to do when the for loop first begins. Many times, this is used to initialize a counter variable to some value. For example, i is set to 0.

The loop-condition is an expression that is evaluated at the beginning of each iteration which determines whether the loop should continue. If the expression is true, then the loop is performed, but if false, then the loop terminates.

The expression-on-each-iteration is something that is performed at the beginning of each iteration of the loop, if that iteration is to take place. Commonly, this is part is used to increment the counter used to control the loop.

Answer 2

I would suggest to check if argc is indeed 1 or greater, before trying to refer to it in argv[1]

int main (int argc, char *argv[])
{
    if(argc != 1)
    {
         printf("1 = lattice effect | 2 = static effect\n");
         return 1;
    }
    switch(*argv[1]) {

I have bad memories of unchecked pointers :-)

Answer 3

I'd suggest that refactormycode.com would be a more suitable place for this sort of question.

Answer 4

Also, avoid system(). It's evil. Instead of system("clear"), provide your own. Use

write (1, "\033[2J", 4);

Answer 5

ANSI C89 requires procedures to be declared before they are used. Which compiler are you using? gcc at least will complain about this. If you are using gcc and aiming for C89 compatibility try using the compiler options "-Wall -ansi -pedantic" to make the compiler more verbose.

If you move your "main" procedure to the bottom of the code, you will solve that problem. Alternatively, you could provide signatures for lattuce and normal above main like so

int lattus (void);

Looking at this line

 if(first == 1 && third == 1 && second == width - 8 | second == width - 9){      //is it at min?

Should that be a boolean OR after -8?

 if(first == 1 && third == 1 && second == width - 8 || second == width - 9){      //is it at min?

Some parenthesis around the boolean operators would make things clearer there.

You are using C++-style // comments which are not supported by C89. They can be re-written like this:

 if(first == 1 && third == 1 && second == width - 8 || second == width - 9){      /* is it at min? */