Thread: Review a C program

  1. #1
    Registered User
    Join Date
    Nov 2020
    Posts
    2

    Review a C program

    Hi,
    I have a project to do. I did it but I need to organize it better and I am not so good at it. I would be very grateful for any suggestion.
    The project consists in a 3D box ordering. The program takes as input a file.in which contains a number n of boxes to order and a bunch of sizes x, y, z respectively for length, width, height and the program must find the longest sequence of boxes that can be nested in one another. So, say the biggest box is 'n' the next box must have sizes strictly minor to n.

    Code:
    #include <stdio.h>
    #include <stdlib.h>
    
    
    
    
    #define FIT_TEST(a,b) (a.x > b.x && a.y > b.y && a.z > b.z)
    
    
    struct box {
        float x, y, z;
        //Registers the best sequence
        int score;
        //Processed boxes
        int processed;
        //Registers the next box (in descending order) of the sequence
        int prev;
    };
    
    
    
    
    int main(int argc, char* argv[]) {
        int n, i = 0;
        
        FILE* filein = stdin;
        if (argc > 1) {
            filein = fopen(argv[1], "r");
            if (filein == NULL) {
                fprintf(stderr, "Can not open %s\n", argv[1]);
                return EXIT_FAILURE;
            }
        }
    
    
        if(fscanf(filein, "%d", &n)==1)
           if (n <= 0) {
            fprintf(stderr, "ERROR: arguments number equal to 0\n");
            exit(EXIT_FAILURE);
           }
    
    
        
        // allocation and value loading
        struct box* B = calloc(n, sizeof * B);
        
        if (B != NULL) {
            for (int i = 0; i < n; ++i) {
                if(fscanf(filein, "%f %f %f", &B[i].x, &B[i].y, &B[i].z)==3)
                   B[i].prev = -1;
            }
    
    
         
            //Loop that find how many boxes fit in box i and registers the result in processed
                while (i < n) {
                  for (int j = 0; j < n; ++j)
                    if (FIT_TEST(B[i], B[j]))
                        B[i].processed++;
                  i++;
                 }
               
                
               for(;;) {
    
    
                int i=0;
    
    
                // Find not processed boxes
                while(i<n) {
                    if (B[i].processed == 0 && B[i].score == 0)
                        break;
                    i++;
                }
    
    
                 //Loop will stop from this condition
                if (i == n)
                   break;
    
    
                Score of boxes
                B[i].score = 1;
    
    
      
               //Find the box with the longest chain which can be put in i
                for (int j = 0; j < n; ++j) {
                    if (FIT_TEST(B[i], B[j]) && B[j].score + 1 > B[i].score) {
                        B[i].score = B[j].score + 1;
                        B[i].prev = j;
                    }
                }
    
    
                // Reduce processed of j
                for (int j = 0; j < n; ++j) {
                    if (FIT_TEST(B[j], B[i]))
                        B[j].processed--;
                }
            }
    
    
            //Find the box with the best result
            int best = 0;
               for (int i=0; i < n; i++) {
                 if (B[i].score > B[best].score)
                    best = i;
                }
                
            //Print number of boxes
            printf("%d boxes\n", B[best].score);
    
    
            //Loop that prints in descending order starting from box best
            for (int i = best; i != -1; i = B[i].prev)
                printf("box %d: %f %f %f\n", i, B[i].x, B[i].y, B[i].z);
    
    
            free(B);
        }
     
    }
    Last edited by everythingpro; 09-04-2021 at 12:56 PM.

  2. #2
    and the hat of int overfl Salem's Avatar
    Join Date
    Aug 2001
    Location
    The edge of the known universe
    Posts
    38,734
    1. Indentation, apply one style consistently.
    Code:
    #include <stdio.h>
    #include <stdlib.h>
    
    #define FIT_TEST(a,b) (a.x > b.x && a.y > b.y && a.z > b.z)
    
    struct box {
      float x, y, z;
      //Registers the best sequence
      int score;
      //Processed boxes
      int processed;
      //Registers the next box (in descending order) of the sequence
      int prev;
    };
    
    int main(int argc, char *argv[])
    {
      int n, i = 0;
    
      FILE *filein = stdin;
      if (argc > 1) {
        filein = fopen(argv[1], "r");
        if (filein == NULL) {
          fprintf(stderr, "Can not open %s\n", argv[1]);
          return EXIT_FAILURE;
        }
      }
    
      if (fscanf(filein, "%d", &n) == 1)
        if (n <= 0) {
          fprintf(stderr, "ERROR: arguments number equal to 0\n");
          exit(EXIT_FAILURE);
        }
    
      // allocation and value loading
      struct box *B = calloc(n, sizeof *B);
    
      if (B != NULL) {
        for (int i = 0; i < n; ++i) {
          if (fscanf(filein, "%f %f %f", &B[i].x, &B[i].y, &B[i].z) == 3)
            B[i].prev = -1;
        }
    
        //Loop that find how many boxes fit in box i and registers the result in processed
        while (i < n) {
          for (int j = 0; j < n; ++j)
            if (FIT_TEST(B[i], B[j]))
              B[i].processed++;
          i++;
        }
    
        for (;;) {
          int i = 0;
    
          // Find not processed boxes
          while (i < n) {
            if (B[i].processed == 0 && B[i].score == 0)
              break;
            i++;
          }
    
          //Loop will stop from this condition
          if (i == n)
            break;
    
          //!! This was not a comment in your post
          //!! Score of boxes
          B[i].score = 1;
    
          //Find the box with the longest chain which can be put in i
          for (int j = 0; j < n; ++j) {
            if (FIT_TEST(B[i], B[j]) && B[j].score + 1 > B[i].score) {
              B[i].score = B[j].score + 1;
              B[i].prev = j;
            }
          }
    
          // Reduce processed of j
          for (int j = 0; j < n; ++j) {
            if (FIT_TEST(B[j], B[i]))
              B[j].processed--;
          }
        }
    
        //Find the box with the best result
        int best = 0;
        for (int i = 0; i < n; i++) {
          if (B[i].score > B[best].score)
            best = i;
        }
    
        //Print number of boxes
        printf("%d boxes\n", B[best].score);
    
        //Loop that prints in descending order starting from box best
        for (int i = best; i != -1; i = B[i].prev)
          printf("box %d: %f %f %f\n", i, B[i].x, B[i].y, B[i].z);
    
        free(B);
      }
    }
    2. Use functions.
    Your main() is the thick end of 100 lines long.
    As a guide, if you can't see the opening and closing brace of a function on-screen at the same time (20 to 30 lines), then you need to split the function up.
    The only 'function' you have is a macro, but that should be written as a function anyway.

    3. Use meaningful variable names.
    Code:
    $ gcc -Wall -Wshadow foo.c
    foo.c: In function ‘main’:
    foo.c:39:14: warning: declaration of ‘i’ shadows a previous local [-Wshadow]
         for (int i = 0; i < n; ++i) {
                  ^
    foo.c:18:10: note: shadowed declaration is here
       int n, i = 0;
              ^
    foo.c:53:11: warning: declaration of ‘i’ shadows a previous local [-Wshadow]
           int i = 0;
               ^
    foo.c:18:10: note: shadowed declaration is here
       int n, i = 0;
              ^
    foo.c:87:14: warning: declaration of ‘i’ shadows a previous local [-Wshadow]
         for (int i = 0; i < n; i++) {
                  ^
    foo.c:18:10: note: shadowed declaration is here
       int n, i = 0;
              ^
    foo.c:96:14: warning: declaration of ‘i’ shadows a previous local [-Wshadow]
         for (int i = best; i != -1; i = B[i].prev)
                  ^
    foo.c:18:10: note: shadowed declaration is here
       int n, i = 0;
              ^
    not 'i' all over the place.
    This is easier to solve when you have lots of small functions to do one thing well.

    4. I would argue that these should be in a different structure, maybe 'struct nested_box'
    Code:
        //Registers the best sequence
        int score;
        //Processed boxes
        int processed;
        //Registers the next box (in descending order) of the sequence
        int prev;
    Otherwise you have 'struct box' is 'struct box_and_some_other_stuff'
    If you dance barefoot on the broken glass of undefined behaviour, you've got to expect the occasional cut.
    If at first you don't succeed, try writing your phone number on the exam paper.

Popular pages Recent additions subscribe to a feed

Similar Threads

  1. program review
    By melestorm in forum C++ Programming
    Replies: 1
    Last Post: 11-12-2009, 05:30 AM
  2. Program review
    By MikeyVeeDubb in forum C Programming
    Replies: 1
    Last Post: 09-24-2009, 01:29 PM
  3. program review
    By shuaib.akram in forum C Programming
    Replies: 3
    Last Post: 08-16-2007, 11:59 PM
  4. Review my Program
    By Sentral in forum C++ Programming
    Replies: 3
    Last Post: 07-27-2005, 05:07 PM
  5. Please Review my Program
    By Unregistered in forum C Programming
    Replies: 2
    Last Post: 04-20-2002, 07:19 PM

Tags for this Thread