Anyone want to check some C code? :)
» >
»
» View previous topic :: View next topic  
I'm busy on modifying Luna (a program that generates documents for the TI-Nspire).

I don't have that much experience with C, but I managed to do what I wanted. But I'm just wondering if I'm doing everything the right way.
It's the first time I use malloc and co, so you never know Smile

Here is my code:

Code:

   //Extension by Jim Bauwens
   if (argc>3){
      FILE *FP;
      int ifile;
      fpos_t pos;
      char *buf ;
      
      for (ifile=3; ifile<argc; ifile++){
         if ((FP=fopen(argv[ifile], "r")) == NULL) {
            printf("Couldn't open %s for reading, ignoring it\n", argv[ifile]);
            break;
         }

         if (fseek(FP, 0, SEEK_END)==-1){
            printf("Couldn't jump to end of file\n");
            break;
         }

         fgetpos(FP, &pos);
         rewind(FP);
         
         buf = (char *) malloc(pos.__pos);
         if (buf == NULL) {
            printf("Could not allocate memory!\n");
              break;
           }
           
           fread(buf, sizeof(char), pos.__pos-1, FP);
         
         if (ferror(FP)){
            printf("Error reading %s!\n", argv[ifile]);
            clearerr(FP);
            break;
         }
         
         if (zipOpenNewFileInZip2(zf, argv[ifile], &zi, NULL, 0, NULL, 0, NULL, 0xD, 0, 1) != ZIP_OK) {
            printf("can't zip %s", argv[ifile]);
            goto close_quit;
         }         
         if (zipWriteInFileInZip(zf, buf, pos.__pos) != ZIP_OK) {
            puts("can't add file to zip file");
            goto close_quit;
         }         
         if (zipCloseFileInZipRaw(zf, pos.__pos, 0) != ZIP_OK) {
            puts("can't close file in zip file");
            goto unlink_quit;
         }   
               
         fclose(FP);
         free(buf);      
      }
   }
   //End Extension


Any bad errors in that?
TIA Smile
TI-Nspire projects of me:
nCreator | PCspire | Klondike Lua | LogoMagic | EEPro for the TI-Nspire | Pegs | General math definitions
I'm assuming you need those gotos for jumping around? Otherwise, why not use functions? It's a bit cleaner. Other than that, looks fine.
R O F L M A O
O
F
L
M
A
O
The labels I'm jumping to are part of the original program, and I'm not going to start to rewrite that Smile

So, my malloc stuff is good?
Very Happy
TI-Nspire projects of me:
nCreator | PCspire | Klondike Lua | LogoMagic | EEPro for the TI-Nspire | Pegs | General math definitions
The malloc() stuff is fine. There are a few things that you could express a bit more cleanly by combining statements, and a few bits of tabbing that are aligned counter-intuitively, but the code is solid. Smile

Code:
    fpos_t pos;
    char *buf ;

    fgetpos(FP, &pos);
    buf = (char *) malloc(pos.__pos);

man fpos_t wrote:
The content of a fpos_t is not meant to be read directly, but only to use its reference as an argument in a call to fsetpos.

Use ftell.

Usually, if something's name has one or more underscores leading, you shouldn't use it without good reason.
タリ ·· Ask questions the smart way ·· digipres.club
I don't see any big issues besides what others have pointed out, but there's the little issue of writing error messages to stdout. Error messages should be written to stderr; after all, that's why stderr exists (stderr and stdout both typically output to the terminal, but they can be redirected separately, which is pretty useful). If the existing code writes error messages to stdout already, then unless you plan to change the rest of the code to use stderr, you should leave your extension as-is because consistency is pretty important too.

Regarding goto: it's one of the best and simplest ways to implement exception/error handling in C. You can easily make error handlers fall through to "unwind" or "undo" actions you've done before the error happened. For a good example of using goto for error handling, see the Linux kernel (http://kerneltrap.org/node/553/2131 is a good discussion of goto in Linux).
Christopher Williams
KermMartian wrote:
There are a few things that you could express a bit more cleanly by combining statements

Yeah, I know. But I first want to get everything working before I group the file reading and zipping part Smile

KermMartian wrote:

and a few bits of tabbing that are aligned counter-intuitively, but the code is solid. Smile

That was because of a copy/paste fail + tab/space mixing at one spot.
Looks good in my editor Razz

Tari wrote:

Use ftell.

Usually, if something's name has one or more underscores leading, you shouldn't use it without good reason.

Thanks Smile
I originally was going to use ftell, but I don't know why I used this in the end. I found the __pos thing online, didn't know it was not supposed to be used. I'll remember that, thanks Smile


Quote:
I don't see any big issues besides what others have pointed out, but there's the little issue of writing error messages to stdout. Error messages should be written to stderr; after all, that's why stderr exists (stderr and stdout both typically output to the terminal, but they can be redirected separately, which is pretty useful). If the existing code writes error messages to stdout already, then unless you plan to change the rest of the code to use stderr, you should leave your extension as-is because consistency is pretty important too.

Good point Smile
The original code uses puts for its errors, so the same style as stdout (I think).
I could easily change the other code, but I don't know if its best if I wish to keep up with updates.

Thanks everyone for your interesting and smart answers Smile
TI-Nspire projects of me:
nCreator | PCspire | Klondike Lua | LogoMagic | EEPro for the TI-Nspire | Pegs | General math definitions
  
Register to Join the Conversation
Have your own thoughts to add to this or any other topic? Want to ask a question, offer a suggestion, share your own programs and projects, upload a file to the file archives, get help with calculator and computer programming, or simply chat with like-minded coders and tech and calculator enthusiasts via the site-wide AJAX SAX widget? Registration for a free Cemetech account only takes a minute.

» Go to Registration page
Anyone want to check some C code? :)
»
» View previous topic :: View next topic  
Page 1 of 1
» All times are UTC - 5 Hours
 
You cannot post new topics in this forum
You cannot reply to topics in this forum
You cannot edit your posts in this forum
You cannot delete your posts in this forum
You cannot vote in polls in this forum

 

Advertisement