Thread: code review...

  1. #1
    Registered User
    Join Date
    Jul 2008
    Posts
    5

    code review...

    Hi everyone,

    Lately I have started to learn C for an application I want to write with a friend. I have some experience programming in python especially web and network enabled applications. I picked "The C Programming Language Second Edition" which I already read a few months ago though I've forgot many aspects of the language by now.
    I've just finished the pointers part so to check how much I understood from the book I've written a small program that paints HTML source code and outputs it to HTML.

    main.c:
    Code:
    #include <stdio.h>
    #include <unistd.h>
    #include <ctype.h>
    #include "srcp.h"
    
    #define USAGE "Usage: %s -f file.html [-o output_file]\n"
    
    int main(int argc, char **argv)
    {
    	char c;
    	char *src;
    	char *file_name = NULL;
    	char *outp = NULL;
    
    	while ((c = getopt (argc, argv, "f:o:")) != -1) {
    		switch (c) {
    			case 'f':
    				file_name = optarg;
    				break;
    			case 'o':
    				outp = optarg;
    				break;
    			case '?':
    				return 1;
    			default:
    				fprintf(stderr, USAGE, argv[0]);
    		}
    	}
    	if (file_name == NULL) {
    		fprintf(stderr, USAGE, argv[0]);
    		return 1;
    	}
    
    	src = file_read(file_name);
    	if (src == NULL) {
    		fprintf(stderr, "Can't read file %s\n", file_name);
    		return 1;
    	}
    	srcp_push("<html><head>");
    	srcp_push("<meta http-equiv='content-type' content='text/html; charset=utf-8' />");
    	srcp_push("<style type='text/css'> body { color: #fff; background: #000; font-size: smaller; }; </style>");
    	srcp_push("</head><body>");
    	srcp_html(src);
    	srcp_push("</body></html>");
    
    	if (outp == NULL) {
    		printf("%s\n", srcp_getoutput());
    	} else {
    		if (file_write(outp, srcp_getoutput()) == NULL) {
    			fprintf(stderr, "Can't write to location\n");
    			return 1;
    		} else printf("Done!\n");
    	}
    	
    	srcp_clean();
    	return 0;
    }
    srcp.h
    Code:
    #include "file.h"
    
    #define true 1
    #define false 0
    
    #define BUFSIZE 1000
    char buf[BUFSIZE];
    unsigned bp = 0;
    char *output = NULL;
    
    unsigned in_tag = false;
    
    char *srcp_html(char *);
    char *tostr(char);
    void srcp_push(char *);
    void srcp_pushbuf(void);
    void srcp_color_brac(char *);
    char *srcp_getoutput(void);
    
    char *srcp_getoutput(void)
    {
    	return output;
    }
    
    void srcp_color_brac(char *brac)
    {
    	srcp_push("<span style='color: red;'>");
    	srcp_push(brac);
    	srcp_push("</span>");
    
    	in_tag = (strcmp(brac, "&gt;") == 0) ? false:true;
    	srcp_push(in_tag ? "<span style='color: #9d3;'>":"</span>");
    }
    
    
    
    char *srcp_html(char *src)
    {
    	while (*src) {
    		switch (*src) {
    			case '>':
    				srcp_color_brac("&gt;");
    				break;
    			case '<':
    				srcp_color_brac("&lt;");
    				break;
    			case '&':
    				srcp_push("&amp;");
    				break;
    			case ' ':
    				srcp_push("&nbsp;");
    				if (in_tag) {
    					srcp_push("</span>");
    					srcp_push("<span style='color: green;'>");
    				}
    				break;
    			case '\n':
    				srcp_push("<br />\n");
    				break;
    			case '\t':
    				srcp_push("&nbsp;&nbsp;&nbsp;&nbsp;");
    				break;
    			case '=':
    				in_tag ? srcp_push("<span style='color: #9b3;'>=</span>"):srcp_push("=");
    				break;
    			default:
    				srcp_push(tostr(*src));
    		}
    		src++;
    	}
    	
    	srcp_pushbuf();
    
    	return output;
    }
    
    void srcp_push(char *str)
    {
    	int strsize = strlen(str);
    	
    	if (bp + strsize < BUFSIZE) {
    		strcat(buf, str);
    		bp += strsize;
    	} else {
    		srcp_pushbuf();
    		srcp_push(str);
    	}
    }
    
    void srcp_pushbuf(void)
    {
    	if (output == NULL) {
    		output = (char *) malloc(BUFSIZE);
    		strcpy(output, buf);
    	} else {
    		output = realloc(output, strlen(output) + BUFSIZE);
    		strcat(output, buf);
    	}
    	buf[bp = 0] = 0;
    }
    
    char *tostr(char c)
    {
    	static char ch[2];
    	ch[0] = c;
    	ch[1] = 0;
    	return ch;
    }
    
    void srcp_clean(void)
    {
    	if (output != NULL) free(output);
    }
    file.h
    Code:
    #include <stdio.h>
    #include <stdlib.h>
    #include <string.h>
    
    #define STATUS_OK "1"
    
    char *file_read(const char *name)
    {
    	char *ptr = NULL;
    	FILE *fp = fopen(name, "r");
    
    	if (!fp) return NULL;
    
    	fseek(fp, 0, SEEK_END);
    	int size = ftell(fp);
    	fseek(fp, 0, SEEK_SET);
    	
    	if ((ptr = calloc(size+1, 1)) == NULL)
    		goto error_exit;
    
    	if (fread(ptr, 1, size, fp) != size)
    		goto error_exit;
    	
    	fclose(fp);
    	return ptr;
    	
    	error_exit:
    		fclose(fp);
    		free(ptr);
    		return NULL;
    }
    
    char *file_write(const char *name, char *text)
    {
    	FILE *fp = fopen(name, "w");
    	if (!fp) return NULL;
    	
    	int size = strlen(text);
    	if (fwrite(text, 1, size, fp) != size) {
    		fclose(fp);
    		return NULL;
    	}
    
    	fclose(fp);
    	return STATUS_OK;
    }
    Any suggestions or corrections will be welcomed.

  2. #2
    and the Hat of Guessing tabstop's Avatar
    Join Date
    Nov 2007
    Posts
    14,336
    Notes I made as I was going along:
    • Maybe this is my unfamiliarity with getopt speaking, but shouldn't -? display the usage message before it quits? And why wouldn't you quit in the default case there?
    • So file_read malloc's a buffer, which is good. However, that buffer doesn't ever appear to be free'd. My suggestion would be that you should pass src to srcp_clean, so it can be free'd.
    • buf[bp=0]=0 is cute. I can almost guarantee that in two weeks you'll look at it, see an assignment inside an array index, and go "wtf" and delete before you realize what's going on.
    • Gotos are evil. Gotos are especially evil when they do nothing other than be the word "else". No really -- just check both conditions in the same if-condition with &&. In the true case, free ptr and set it to NULL. Afterwards, close fp (since you do that in both cases anyway) and then return ptr (since you do that in both cases anyway).

    Of course, I didn't test it or anything; that's just what I see from reading.

  3. #3
    Registered User
    Join Date
    Jul 2008
    Posts
    5
    First of all thanks.

    1) I didn't really read the documentation for C's getopt. I used an example I've found on google. Here is the fixed code:
    Code:
    while ((c = getopt (argc, argv, "f:o:")) != -1) {
    		switch (c) {
    			case 'f':
    				file_name = optarg;
    				break;
    			case 'o':
    				outp = optarg;
    				break;
    			default:
    				fprintf(stderr, USAGE, argv[0]);
    				return 1;
    		}
    	}
    2)
    Code:
    srcp.h
    void srcp_clean(char *src)
    {
    	if (output != NULL) free(output);
    	free(src);
    }
    
    main.c
    srcp_clean(src);
    3) I think in this case it's pretty obvious. I don't really see a difference in readability if the assignment was on its own line.
    4) The use of goto is based on "kernel coding style". Which recommend it for cleanups. And besides I didn't write that function but a really good programmer, it replaces another function I wrote which didn't take care of errors.

  4. #4
    Woof, woof! zacs7's Avatar
    Join Date
    Mar 2007
    Location
    Australia
    Posts
    3,459
    > And besides I didn't write that function but a really good programmer
    Doesn't look like the work of a really good C programmer...

    Perhaps pass a pointer to a pointer,
    Code:
    void srcp_clean(char ** src)
    {
       if(output != NULL)
          free(output);
    
       if(src != NULL)
       {
          free(*src);
          *src = NULL;
       }
    }
    That way you won't / can't use src from the caller after it's been free'd.

    Oh and, don't use goto . Especially not in this case.
    Last edited by zacs7; 10-26-2008 at 12:41 AM.

  5. #5
    and the Hat of Guessing tabstop's Avatar
    Join Date
    Nov 2007
    Posts
    14,336
    Kernel is kernel; if you're good enough to contribute code to the linux kernel, we'll let you use goto. When the difference is exactly one line of code, using goto is just silly. Yes, the compiler will optimize it away, probably, but still -- we know what error-checking looks like, we don't need "error_exit" to tell us what's going on.

    (Hooray for daylight saving, where the reply came in six minutes before the post!)

  6. #6
    Registered User
    Join Date
    Jul 2008
    Posts
    5
    Quote Originally Posted by zacs7 View Post
    > And besides I didn't write that function but a really good programmer
    Doesn't look like the work of a really good C programmer...
    He's the developer of CppCMS.

    Perhaps pass a pointer to a pointer,
    Code:
    void srcp_clean(char ** src)
    {
       if(output != NULL)
          free(output);
    
       if(src != NULL)
       {
          free(*src);
          *src = NULL;
       }
    }
    That way you won't / can't use src from the callee after it's been free'd.

    Oh and, don't use goto . Especially not in this case.
    I didn't really understand. Why use a pointer to a pointer can't i just write:
    Code:
    void srcp_clean(char *src)
    {
      if (output != NULL) free(output);
    
      if (src != NULL) {
         free(src);
         src = NULL;
      }
    }
    And I don't think I really need to do it because srcp_clean is called only once at the end of main.

    About the goto I guess here it is really unnecessary.

  7. #7
    and the Hat of Guessing tabstop's Avatar
    Join Date
    Nov 2007
    Posts
    14,336
    Quote Originally Posted by dar32 View Post

    I didn't really understand. Why use a pointer to a pointer can't i just write:
    Code:
    void srcp_clean(char *src)
    {
      if (output != NULL) free(output);
    
      if (src != NULL) {
         free(src);
         src = NULL;
      }
    }
    And I don't think I really need to do it because srcp_clean is called only once at the end of main.
    In this case, you can get away without it, since it is the last line of main. (Most people pretty quickly write a free-and-NULL function since it happens all the time.) Anyway, things are passed to functions by value, so changing the value of src (as in src=NULL) won't actually change the "real" value of src back in the calling function.

  8. #8
    Registered User
    Join Date
    Jul 2008
    Posts
    5
    Got it. Thanks .

  9. #9
    C++ Witch laserlight's Avatar
    Join Date
    Oct 2003
    Location
    Singapore
    Posts
    28,413
    It might be safer to allow srcp_clean to be called twice in a row. In that case, I would write:
    Code:
    void srcp_clean(char **src)
    {
      free(output);
      output = NULL;
    
      free(*src);
      *src = NULL;
    }
    Note that you do not need to check if a pointer is not a null pointer before using free on it since using free on a null pointer is perfectly fine.

    If you are really very sure that srcp_clean is only called exactly once, at the end of main, then you could write:
    Code:
    void srcp_clean(char *src)
    {
      free(output);
      free(src);
    }
    Oh, and you should #include <stdlib.h>
    Last edited by laserlight; 10-26-2008 at 01:06 AM.
    Quote Originally Posted by Bjarne Stroustrup (2000-10-14)
    I get maybe two dozen requests for help with some sort of programming or design problem every day. Most have more sense than to send me hundreds of lines of code. If they do, I ask them to find the smallest example that exhibits the problem and send me that. Mostly, they then find the error themselves. "Finding the smallest program that demonstrates the error" is a powerful debugging tool.
    Look up a C++ Reference and learn How To Ask Questions The Smart Way

  10. #10
    Woof, woof! zacs7's Avatar
    Join Date
    Mar 2007
    Location
    Australia
    Posts
    3,459
    > Note that you do not need to check if a pointer is not a null pointer before using free on it since using free on a null pointer is perfectly fine.
    Maybe so, but dereferencing a NULL pointer is not .

  11. #11
    Kernel hacker
    Join Date
    Jul 2007
    Location
    Farncombe, Surrey, England
    Posts
    15,677
    Code:
    		output = realloc(output, strlen(output) + BUFSIZE);
    		strcat(output, buf);
    If you run out of memory here, output will be NULL, which:
    1. leads to strcat() most likely crashing.
    2. The memory previously pointed to by output will be leaked, since you no longer have a pointer to it.

    Code:
    char *file_read(const char *name)
    {
    	char *ptr = NULL;
    	FILE *fp = fopen(name, "r");
    
    	if (!fp) return NULL;
    
    	fseek(fp, 0, SEEK_END);
    	int size = ftell(fp);
    	fseek(fp, 0, SEEK_SET);
    	
    	if ((ptr = calloc(size+1, 1)) == NULL)
    		goto error_exit;
    
    	if (fread(ptr, 1, size, fp) != size)
    		goto error_exit;
    	
    	fclose(fp);
    	return ptr;
    	
    	error_exit:
    		fclose(fp);
    		free(ptr);
    		return NULL;
    }
    Several points:
    * In the other code, we have checks to see if a pointer is NULL before free - consistency is important here, so you should do EITHER checking everywhere, or not anywhere. [There is really no reason to check in modern implementations of the C library - in the old days, it was undefined behaviour, but modern implementations should allow NULL and just return].

    * You do almost the same thing all the time, so using a goto seems pretty pointless. Just use a nested set of if-statements instead, and the code will be clearer. This is NOT kernel code where you have 11 different ways that the function can fail (only two that require cleanup, and one that is "immediate" failure).

    * size probably should be an unsigned. However, trying to allocate memory for an entire file in memory is generally not a good idea. Reading the file in blocks (say 4KB-32KB) is generally a better approach. What would you do in a 32-bit OS if the file is 5GB? Or even 2 GB, but there is only 1.8GB of free space?

    * Using calloc() is not particularly clever as you are immediately after filling all but the last character of the buffer with non-zero values [one would assume they are non-zero at least]. The only byte that needs to be zero is the ptr[size].

    Code:
    char *file_write(const char *name, char *text)
    {
    	FILE *fp = fopen(name, "w");
    	if (!fp) return NULL;
    	
    	int size = strlen(text);
    	if (fwrite(text, 1, size, fp) != size) {
    		fclose(fp);
    		return NULL;
    	}
    
    	fclose(fp);
    	return STATUS_OK;
    }
    * Why are you returning a pointer to char, when all you actually want is a "true or false"? Use either bool or int.

    --
    Mats
    Compilers can produce warnings - make the compiler programmers happy: Use them!
    Please don't PM me for help - and no, I don't do help over instant messengers.

Popular pages Recent additions subscribe to a feed

Similar Threads

  1. Code Review for upcoming contest
    By Darryl in forum Contests Board
    Replies: 2
    Last Post: 02-28-2006, 02:39 PM
  2. Problem : Threads WILL NOT DIE!!
    By hanhao in forum C++ Programming
    Replies: 2
    Last Post: 04-16-2004, 01:37 PM
  3. True ASM vs. Fake ASM ????
    By DavidP in forum A Brief History of Cprogramming.com
    Replies: 7
    Last Post: 04-02-2003, 04:28 AM
  4. Interface Question
    By smog890 in forum C Programming
    Replies: 11
    Last Post: 06-03-2002, 05:06 PM
  5. Replies: 0
    Last Post: 02-21-2002, 06:05 PM