HiveBrain v1.2.0
Get Started
← Back to all entries
patterncMinor

Calculator supporting multiplication, division, or modulo of two numbers

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
divisionmultiplicationnumberstwocalculatorsupportingmodulo

Problem

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
* =
*
* 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 10308947
*/

/**
* Include standard libraries
*/
#include
#include
#include
#include
#include

/**
* 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 >= 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

Solution

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.

Code Snippets

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

Context

StackExchange Code Review Q#10186, answer score: 6

Revisions (0)

No revisions yet.