Thread: Project/code review requested

  1. #1
    Registered User
    Join Date
    Feb 2011
    Posts
    144

    Project/code review requested

    Good morning, gentlemen and ladies.

    I have written a small project in the C89 dialect of C. I am requesting a sizeable code review - in fact a review of the entire project - for anyone intrepid enough to examine my code.

    The code is here: BASICloader/BASICloader.c at master * richardcavell/BASICloader * GitHub

    It is 1400 lines of C, and it outputs BASIC source code.

    My goals are:

    * Coding to a professional standard
    * Strict C89 standards compliance
    * Correct use of the standard library (and no other library/API)
    * Code correctness
    * Detection and graceful handling of all possible errors

    Specific issues for this project are:

    * Wise choice of data types, to accommodate range requirements
    * Detection of overflow and other arithmetic problems
    * It should be easy to add other target architectures, and easy to see
    where the additional code needs to go
    * Someone might try to compile this with a compiler or machine that
    was created before the C89 standard even existed.

    I would be enormously grateful for anyone who could give me feedback. Thanks.

    Richard

  2. #2
    Registered User
    Join Date
    May 2010
    Posts
    4,553
    * Coding to a professional standard
    Fail, big time.

    1400 lines in a single file, really?

    Cryptic variable names, learn to use meaningful variable and function names.

    Be careful of mixing typedef with #define, typedef happens at compile time, #define happens at pre-processor time so typedefs are probably not available to #define.

    * Strict C89 standards compliance
    This is 2017 why are you still stuck in 1990?

    * Correct use of the standard library (and no other library/API)
    Too much cryptic code for me to even try to see if you're making any mistakes. Did you carefully test each of your functions as you made them to see if they meet your design constraints?

    * Code correctness
    See above.

    * Detection and graceful handling of all possible errors
    It doesn't seem like you're very consistent. Look at this snippet:
    Code:
    internal_error(const char *fmt, ...)
    {
      va_list ap;
    
      (void) fprintf(stderr, "Internal error: ");
    
      va_start(ap, fmt);
      (void) vfprintf(stderr, fmt, ap);
      va_end(ap);
    
      fprintf(stderr, "\nPlease report this to Richard Cavell\n"
                      "at richardcavell@mail.com");
    Why are you specifically ignoring the return values from the first two print statements by using the cast, but fail to either check the final return value or cast the return value to a void?

    IMO, several of your functions have too many parameters, consider using structures to simplify if possible.


    Jim

  3. #3
    misoturbutc Hodor's Avatar
    Join Date
    Nov 2013
    Posts
    1,795
    Things like this confuse me:
    Code:
    static void
    check_line_number(line_type *line)
    Why are you passing this as a pointer when: a) it is a simple type (currently unsigned short int); and b) you are not modifying the object it's pointing to?

    Edit:
    I haven't compiled the code, but:
    Code:
    if (*line < old_line || *line + *step > LINE_TYPE_MAX)
    Can *line + *step ever be greater than LINE_TYPE_MAX? To me it doesn't seem possible.
    Last edited by Hodor; 08-27-2017 at 12:01 AM.

  4. #4
    Programming Wraith GReaper's Avatar
    Join Date
    Apr 2009
    Location
    Greece
    Posts
    2,731
    Let's see... The things I checked:
    *) Your indentation is almost consistent but too compressed for my taste. That's a personal preference of course, I prefer 4 spaces per level.
    *) Don't explicitly ignore return values, that just bloats the code up in my opinion. Reviewers can figure out for themselves whether that function returns something or not.
    *) You don't need to explicitly state that the first constant of every enum is zero, that behavior is already standard.
    *) Why are you using static everywhere? This isn't a linkable library.
    Devoted my life to programming...

Popular pages Recent additions subscribe to a feed

Similar Threads

  1. c code requested- really hard problem
    By amnakhan786 in forum C Programming
    Replies: 4
    Last Post: 11-13-2011, 03:21 AM
  2. Code Check Requested! (Alphabetize entries?)
    By theonetruehero in forum C Programming
    Replies: 6
    Last Post: 04-15-2008, 04:43 PM
  3. Advice requested, Code makes sense to me, not compiler
    By andrew.bolster in forum C Programming
    Replies: 53
    Last Post: 01-06-2008, 01:44 PM
  4. review this code
    By KIBO in forum C Programming
    Replies: 12
    Last Post: 08-14-2007, 02:28 PM
  5. Code Review
    By Thantos in forum C Programming
    Replies: 8
    Last Post: 03-06-2004, 06:20 PM

Tags for this Thread