4
\$\begingroup\$

This is for a college assignment, it has been submitted but I'd like some general feedback on how to improve it or whether I should be using more functions or less functions.

Also, is my commenting too extreme?

/**
 * This program takes two binary integers and performs one of three operations
 * on them. 
 *
 * It will either multiple, divide or get the modulo of the two
 * numbers. 
 *
 * It ignores whitespace and accepts input in the following format 
 * <number><operation><number> = <enter>
 *
 * The program will then convert the binary integer to decimal, perform it's
 * operation, convert the output back to binary and print the result.
 *
 * An error will occur the incoming input is in the wrong format or if the
 * binary intergers are not unsigned (positive).
 * 
 * @author Sam Dunne <sam.dunne@ucdconnect.ie> 10308947
 */


/**
 * Include standard libraries
 */
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <math.h>
#include <limits.h>


/**
 * This function checks the string to ensure it's valid
 *
 * @param char input            The string input to be checked
 *
 * @var int i 
 * @var char operation          The operation to be performed
 *
 * @return char operation
 */

static char check_string(char *input) {

    char operation;
    unsigned int i = 0;

    /**
     * Check to see which operation is contained within the string.
     * Assign the correct operation to the 'operation' variable.
     */
    operation = strchr(input, '*') ? '*' : ( strchr(input, '/') ? '/' : ( strchr(input, '%') ? '%' : '0'));


    /**
     * Ensure a valid operator was found
     */
    if(operation == '0') {
        printf("\n\nNo operator found. Closing\n\n");
        exit(1);
    } 

    /**
     * Ensure there are no illegal characters in the string
     */
    for(i = 0; i < strlen(input); ++i) {
        if(input[i] != '0' && input[i] != '1' && input[i] != '*' && input[i] != '/' && input[i] != '%' && input[i] != ' ') {
            printf("\n\nInvalid Input. Closing\n\n");
            exit(1);
        }
    } 

    return operation;
}


/**
 * This function parses the input into two numbers and an operation and
 * performs the operation
 *
 * @param char input            The string input to be parsed
 * @param char operation        The operation being performed
 * @param int dec_output        The pointer that will 'return' the decimal value
 * @param int bin_output        The pointer that will 'return' the binary value
 *
 * @var int dec[]               The decimal representations of the binary strings
 * @var int dec_result          The decimal result of the operation
 * @var int mask                This is the mask for the decimal to binary conversaion
 * @var char bin[]              The binary strings
 * @var char rest               The rest of the string after it has been tokenised
 * @var char junk               Junk left over from strtol()
 *
 * @return int 0
 */
static void parse(char *input, char operation, unsigned int *dec_output, char *bin_output){

    unsigned int dec[2], dec_result = 0, mask = 0;
    char *bin[2], *rest, *junk;

    /**
     * Tokenise the string and assign the two tokens to variables
     */
    bin[0] = strtok_r(input, &operation, &rest);
    bin[1] = rest;

    /** 
     * Convert binary number to decimal 
     */
    dec[0] = strtol(bin[0], &junk, 2);
    dec[1] = strtol(bin[1], &junk, 2);

    if(dec[1] == 0){
        printf("\n\nDividing by zero and modulus with zero is impossible. Destroy the Universe elsewhere. Good day.\n\n");
        exit(1);
    }

    /**
     * Perform correct operation
     */
    dec_result = (operation == '*') ? (dec[0]*dec[1]) : ( (operation == '/') ? (dec[0]/dec[1]) : (dec[0]%dec[1]) );

    /** 
     * Convert result back to binary
     */
    *(bin_output+16) = '\0';
    mask = 0x10 << 1;
    while(mask >>= 1)
        *bin_output++ = !!(mask & dec_result) + '0';


    *dec_output = dec_result;
}


/**
 * This is the main function that calls all other functions
 *
 * @var char line[]         The array for storing the incoming string
 * @var int i
 * @var int dec_output      The decimal result to be printed
 * @var int bin_output      The binary result to be printed
 *
 * @return int 0
 */
int main(void) {

    char operation, line[256], bin_output[256];
    unsigned int i = 0, dec_output = 0;

    /** 
     * Read in the input to 'char line[]'
     */
    printf("Enter two binary integers seperated by one of [ * / %% ] and press enter\nCALC: ");
    fgets(line, sizeof(line), stdin);

    /** 
     * Remove newline from string if present 
     */
    i = strlen(line) - 1;
    line[i] = '\0';

    /**
     * Check validity of the input
     */
    operation = check_string(line);

    /**
     * Call the parser for results
     */
    parse(line, operation, &dec_output, bin_output);

    /**
     * Print the final answer
     */
    printf("\n\n|*---|Result = {DECIMAL:%d || BINARY:%s}|---*|\n\n", dec_output, bin_output);

    return 0;
}
\$\endgroup\$
  • 1
    \$\begingroup\$ Typos in file comment: - "multiple" should be "multiply" - "modulo of" should be "result of applying modulo to" - "it's" (the contraction for "it is") should be "its" (the possessive form of "it") - "interger" should be "integer" - "occur" should be "occur if" - "positive" should be "positive or zero" \$\endgroup\$ – minopret Mar 21 '12 at 12:31
6
\$\begingroup\$

In general the code is very clear.

That said, you use way too many comments. Do not paraphrase the code in comments, and instead use them to say things you cannot express in code: “code tells you how, comments tell you why.”

Comments such as “This is the main function that calls all other functions” or “ This function checks the string to ensure it's valid” are useless. They are only useful if the reader doesn’t understand C, and this isn’t the point of comments.

In particular, the main function needs no comment. Its use is clear to everybody. The check_string function, on the other hand, needs an improved comment: use it to explain what constitutes a valid string (instead of just saying that it checks validity).

What’s more, a multi-line comment in C starts by /*, not by /**. Use the latter only for documentation comments. By convention, they are then used by documentation systems to parse the code and generate documentation. For all other comments (in functions etc.) use normal single-line comments or multi-line comments that start with a single star (/*).

A word on the conditional operator: I believe the other commenters are wrong in their assessment of this operator. It’s safe, readable and should absolutely be used when necessary. However, a slightly different formatting can make chained conditions much more readable:

operation = strchr(input, '*') ? '*' :
            strchr(input, '/') ? '/' :
            strchr(input, '%') ? '%' : '0';

Note that thanks to the precedence rules of the conditional operator, the original parentheses aren’t necessary. Chaining conditionals in this way is a pretty well established idiom and certainly improves readability over using if.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ +1 for comments about the ternary operator, although I prefer to put the very last operand (the else) on its own line so the entire thing reads like a series of questions with answers. \$\endgroup\$ – seand Mar 21 '12 at 17:33
3
\$\begingroup\$

In addition to minopret's comments:

Functionality

  • Avoid using exit(), it is frowned upon in most implementations. For example, see this explanation of why it is bad in Windows. Instead, use return EXIT_FAILURE; (found in stdlib.h). And if you try to convert your program to use return instead, you will find out that you need to (and should) leave all error handling to the caller. Return an error code or similar.

  • !!(mask & dec_result) is this a typo? Why would you want to call the boolean NOT operator twice in a row?

  • Ensure that all user input strings are null terminated before passing them to string handling functions like strlen, or the program might go haywire. I suspect that this the row i = strlen(line) - 1; is a severe bug, but you get away with it because the debug executable build in your compiler happens to set line to all zeroes, which the standard does not guarantee.

Style

  • A function that does not modify the contents of a parameter passed by reference (pointer), should declare that pointer as const. For example static char check_string(const char *input);. This is referred to as "const correctness" among C and C++ programmers and is considered good practice.

  • Avoid declaring several variables on the same line, it leads to bugs and confusion. For example char* ptr1, ptr2;, ptr1 is a pointer, ptr2 is a char, which was perhaps not the intention. Instead, declare every variable on a line of its own:

-

   char operation;
   char line[256];
   char bin_output[256];

Performance

  • for(i = 0; i < strlen(input); ++i). Avoid calling functions from a loop condition. Code like this is not always optimized, so you end up calling strlen() over and over, even though it performs the very same calculation each time. A better way is this:

-

   int length = strlen(input);
   for(i = 0; i < length; ++i)

(or to be pedantic: size_t length = strlen(input);)

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ "!!(mask & dec_result) is this a typo?" I don't think it's a typo. !! is a common idiom for converting any non-zero value into 1. \$\endgroup\$ – Joey Adams Mar 21 '12 at 17:34
  • 1
    \$\begingroup\$ @JoeyAdams It won't convert anything to 1, it will convert it to non-zero. Though of course in practice, it will most likely be 1. A better way would be to write (mask & dec_result) > 0 which will evaluate directly to boolean. \$\endgroup\$ – Lundin Mar 21 '12 at 17:57
  • \$\begingroup\$ The expression !a yields 0 if a is non-zero, and 1 if a is zero. Thus, !!a yields 0 if a is 0, and 1 if a is non-zero. Link me to the standard if it says otherwise. \$\endgroup\$ – Joey Adams Mar 21 '12 at 18:57
  • \$\begingroup\$ from documentation of fgets: If any characters are read and there is no error, a \0 character is appended to end the string. So there is no mistake in strlen but one in fgets it should be fgets(line, sizeof(line)-1, stdin); \$\endgroup\$ – Xavier Combelle Mar 23 '12 at 9:00
  • \$\begingroup\$ Clearly !!(mask & dec_result) is not that common because Lundin didn't know what it was for and neither did I, I think (mask & dec_result) ? '1' : '0' would have been a better version of that whole expression. \$\endgroup\$ – JeremyP Apr 20 '12 at 11:07
1
\$\begingroup\$

Good work delineating the functions. You have met several relevant guidelines for each one of your functions. It is reasonably short. It performs one clearly defined operation. It uses a limited amount of conditional logic. Each of its conditional statements has a purpose that is clearly related to the rest. Any conditional statement whose purpose is loosely related to the rest is factored out as a separate function.

I would generally suggest to eliminate the use of the ternary operator (? :) despite that it is much more compact than if ... else. The ternary operator was a good hint to earlier compilers, but now people need more help than the compiler does. When branching on several values of the same data element you can generally arrange your code to use switch effectively.

I agree that you have incorporated more comments than are really useful. A good example of a comment that is better to remove is "Check validity of the input". The code uses the function name "check_string" that already expresses that purpose clearly. You can also assume that anyone reading your code knows the programming language (or is willing to learn it). Comments equivalent to identifiers or programming language constructs are good placeholders for your own use while the code is incomplete. When the code is complete and it expresses just what those comments say, then the comments are only clutter and are better removed.

On the other hand, your comments introducing each function are good. They are useful definitions of the contract (the API) that each function is intended to fulfill.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ I agree with everything said. An additional remark about the ?: operator - it is not only superfluous, it is dangerous too. If you for example have code like this: memcpy(dest, src, sizeof(type==INT ? my_int : my_double));. Lets say this code is supposed to copy generic data based on some enum "type" which keeps track of the data type. The code will not work, it will always copy 8 bytes no matter what type is set to. The culprit is the illogical behavior of the ?: operator, which always performs balancing between its 2nd and 3rd operand, silently converting the int to a double in this case. \$\endgroup\$ – Lundin Mar 21 '12 at 14:47
  • \$\begingroup\$ Try to compile & run this snippet for example: int my_int; double my_double; enum {INT, DOUBLE} type = INT; printf("%d", sizeof(type==INT ? my_int : my_double));. Always prints 8, assuming double has size 8 bytes. \$\endgroup\$ – Lundin Mar 21 '12 at 14:50
  • \$\begingroup\$ The hint about ?: is completely false, sorry. Its use is entirely appropriate here. An if would only make the code longer and more convoluted, not clearer. And the conditional operator was not a “hint at earlier compilers”, that’s nonsense. It’s a normal operation. \$\endgroup\$ – Konrad Rudolph Mar 21 '12 at 14:55
  • \$\begingroup\$ @KonradRudolph Even still, the ?: is unpredictable and unsafe, as I demonstrated. if-else is perfectly safe. Why use a language feature that is both superfluous and unsafe? \$\endgroup\$ – Lundin Mar 21 '12 at 15:21
  • \$\begingroup\$ @Lundin Wrong. The conditional operator is predictable and safe. Your code just fails to take into account fundamental rules of expression type and conversion. That’s not partial to ?:, the same happens for other operators (my_int + my_double …) and should be obvious. Failure to understand the language doesn’t obviate the usefulness of this operator. \$\endgroup\$ – Konrad Rudolph Mar 21 '12 at 15:28
0
\$\begingroup\$

Appearance is very nice, good attention to readability. Good consistency.

  • Too many comments for my liking. Comments that are obvious from the context are just noise. For example a call to a function called 'parse' is not made any clearer by telling the reading that you are "Calling the parser for the results" - the reader can see that. However, my guess is that you have been instructed to insert such comments and would lose marks if you omitted them, so maybe you should ignore me.

  • Opening braces in functions are normally at column 0.

  • Variables are better declared on their own line (ie. one per line) - with a short comment on their purpose if necessary.

  • Use an int for the operation type; using char saves nothing and may even cost extra cycles.... I see later that you have char for a purpose - misused.

  • Arguably, it is better to return (eg) 0 on failure from your functions, letting the caller handle it, rather than to exit. If you exit, use EXIT_FAILURE, not 1.

  • Excess comments in function headers. Repeating the types in the header is unwise; as programs are maintained, people forget to update headers when they change the code and so the types will become mismatched.

  • Including local variables in headers seems excessive (see above too).

  • Drop the text "This function " from headers - it can be assumed that you are describing the function.

Comments on functions:

check_string:

  • Doesn't check for multiple operators or for the number of operands;

  • for-loop could be replaced by strspn(input, "01*/% ") != strlen(input)

  • Input can be const

parse:

  • You assume bin_output size to be 17 bytes - you might at least state this in the header. Better to add a size parameter.

  • Input can be const

  • Call to strtok_r pretends that 'operation' is a nul-terminated string when it is really just a char. This is not reliable. Perhaps use strtol/strspn/strtol to separate parameters and operator?

  • check_string() and parse() might be combined, as check_string does very little checking

  • Overusing the ?: operator. A switch is more readable.

  • Conversion to binary better extracted into a separate function. Then do it correctly - your conversion is junk (sorry, but it is true).

  • Why not return dec_output rather than have an output parameter?

main:

  • Missing check for success of call to fgets

  • Newline is removed without checking that it is there (if user ended input with ctrl-d, it will not be) and if line length is zero, you will zero line[-1]

  • Return EXIT_SUCCESS rather than 0

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ "Use an int for the operation type; using char saves nothing and may even cost extra cycles.... I see later that you have char for a purpose - misused.". You are making assumptions about his hardware. On a 8-bit CPU char will be far more efficient than int. \$\endgroup\$ – Lundin Mar 21 '12 at 15:52
  • \$\begingroup\$ @Lundin SCD used char so that he could misuse it later in the call to strtok_r in parse(). However, you are of course correct - but it is a realistic assumption :-) \$\endgroup\$ – William Morris Mar 21 '12 at 16:07

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.