current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 6 down vote favorite

Here is my code where I have 3 functions that do simple arithmetic:

#include <stdio.h>

/* The following Code Contains 3 functions
 * that compute simple arithmetic such as
 * computing the sum of 3 integers and divison
 * of three floating integers.
 */
 void getSum(int x, int y, int z);
 double doDiv(float one, float two, float three);
 void getDiv(float a, float b, float c);

 int main()
 {  
    getSum(100,242,62); 
    getDiv(100.0,25.0,2.0);
    return 0;
 }

/*The function getSum accepts three int
 *values and computes the sum of all three and prints
 *the result via standard output.
 */

 void getSum(int x, int y, int z)
 {
    int sum = x + y + z;
    printf("%i\n",sum);

 }

/*The function doDiv accepts three floating
 *values and computes the divison of first two.
 *Finally it divides the result by the third 
 * floating value and returns the final answer.
 */

 double doDiv(float one,float two, float three)
 {
    float answer; 
    answer = one/two;
    answer = answer/three;
    return answer;
 }

/* The function getDiv accepts three floating values
 * and then calls the doDiv function. The three accepted 
 * values are passed as parameters in the function call.
 */

 void getDiv(float a, float b, float c)
 {
    float finalAnswer;
    finalAnswer = doDiv(a,b,c);
    printf("%f\n", finalAnswer);
 }

I have tried my best to comment each function but I am not sure if I am doing it correctly. Can someone please advise me on making my comments better, naming variables better and anything else that can help me utilize coding standards correctly?

I would like to pick up the habit now while I am still writing small lines of code so I can take it forward with me when I write bigger codes.

Any suggestions on how I can comment better, name variables better, write code more elegant are welcome. Also, do I comment the method declarations on top?

share|improve this question

3 Answers 3

up vote 10 down vote accepted

There's a bit to say about this code, so I'll try to take it from the top and work my way down:

  • Declare main() after your functions so you don't have to declare those function prototypes at the beginning of your source code.

  • You shouldn't be printing anything from the functions other than main() here. You should be returning the values to main() and handle them there.

  • You are restricting yourself by only accepting int parameters into your getsum() function. I would use intmax_t from <stdint.h> instead.

  • You don't protect yourself from integer overflow within any of your functions. Take a look at this question and answer for an idea of how to implement that protection using the latest standards.

  • Why does your doDiv() function accept float values, and then return a double value? Inconsistent and not what a programmer would expect. Either return a float, or accept doubles as parameters.

  • Always declare what parameters your function takes in, even if nothing.

    int main(void)

    You might wonder why we have to do this. Imagine we have the function foo() declared as such:

    int foo()

    In C, this is known as an identifier list and means that it "can take any number of parameters of unknown types". We can actually pass values to the function even though we don't mean to or intend to. If the caller calls the function giving it some argument, the behavior is undefined. The stack could become corrupted for example, because the called function expects a different layout when it gains control.

    Using identifier lists in function parameters is depreciated. It is much better to do something like:

    int foo(void)

    In C, this is known as a parameter type list and defines that the function takes zero arguments (and also communicates that when reading it) - like with all cases where the function is declared using a parameter type list, which is called a prototype. If the caller calls the function and gives it some argument, that is an error and the compiler spits out an appropriate error.

    The second way of declaring a function has plenty of benefits. One of course is that amount and types of parameters are checked. Another difference is that because the compiler knows the parameter types, it can apply implicit conversions of the arguments to the type of the parameters. If no parameter type list is present, that can't be done, and arguments are converted to promoted types (that is called the default argument promotion). char will become int, for example, while float will become double.

  • Use %g when printing float values. It also allows you to print double if you decide to "upgrade" the type later.

  • You don't have to return 0 at the end of main(), just like you wouldn't bother putting return; at the end of a void-returning function. The C standard knows how frequently this is used, and lets you not bother.

    C99 & C11 §5.1.2.2(3)

    ...reaching the } that terminates the main() function returns a value of 0.

  • You asked me to comment on your formatting and documentation. I find leading by example is a good way to learn something, so look at how I do it in this question of mine. Also, I use Doxygen for my documentation generation. I think it is a good habit that you should get into as well.

share|improve this answer
    
Thank you for your detailed response. can you also suggest if my comment formatting is correct if not can u show a small example in ur answer of how to correctly comment each function and also write a overall comment for entire code. –  user2733436 2 days ago
    
@user2733436 Edit made as my last point. There is nothing that looks glaringly wrong, but I pointed you to some of my code for reference. –  syb0rg 2 days ago
    
oh okay. I had an idea that there was a specific format for commenting the code such as you specify parameters , value to be returned or something. –  user2733436 2 days ago
    
Agree with most (+1) except 'Declare main() after your functions ...". Best would be to use f.h, f.c, main.c. But if all the are combined in 1 file for review, the OPs' order readily translates into the the 3 files list. In essence, the void getSum(); void getDiv(...); part is the header file and void doDiv(...); s/b a static function below main()`. –  chux 2 days ago
    
@chux I agree, but for such a small file as this I didn't think a header was completely necessary. –  syb0rg 2 days ago
up vote 3 down vote

The comment to the last function getDiv is bad. I can't figure out what it does by reading the comment, only by reading the code itself. For example: 

/* getDiv prints the result of dividing a by b and then by c. */
share|improve this answer
    
can you suggest how to comment it, i can upvote you as your answer right now does not really help me. –  user2733436 2 days ago
    
Thanks for pointing out , up voted. I also was wondering for instance if i comment on top of the functions deceleration do i need to comment again on top of the actual functions? Which is preferred? –  user2733436 2 days ago
up vote 3 down vote

Here's a crazy idea. What if you code eliminated most of these comments and instead became self-documenting?

Is this dark magic, plain nonsense or actually viable for production code? Let's take a stab at it.

Let's simply change your function names and parameters and see if it makes a difference.

(UPDATED first function name due to error pointed out in comments.)

 void printSumOfThreeNumbers(int first, int second, int third);
 double divideFirstNumberBySecondAndThenThird(float first, float second, float third);
 void divideFirstNumberBySecondAndThenThirdAndPrintResult(float first, float second, float third);

Now I'm willing to bet you can delete EVERY COMMENT in your original code snippet and most sensible people will not whine about an apparent lack of comments; your code's intention is expressed rather well in the names alone.

A lot of people reading code would prefer the above instead of some fluffy flavor text that will very soon rot; meaning that the next guy who comes along may change your (private) functions, add/subtract parameters and not even bother to change your elaborate comments rendering them ineffective at best and inaccurate/misleading at worst.

By letting your code read better with meaningful function and variable names, you are doing your readers a favor.

Software is often a write-once, read-many-times activity.

share|improve this answer
3  
getSumOfThreeNumbers isn't self explanatory - it should be printSumOfThreeIntegers. The function doesn't return anything. divideFirstNumberBySecondAndThenThird should be firstFloatDividedBySecondAndThenThird, because it doesn't just divide, it returns the value. divideFirstNumberBySecondAndThenThirdAndPrintResult should be printFirstFloatDividedBySecondAndThenThird because we want to avoid overly long function names. –  gnasher729 2 days ago
    
Missed the print on the get() so my bad. Thanks for refining it further. –  shivsky 2 days ago
2  
Sorry, but long function names like these are awful and just increase noise in the source code. Contrary to popular belief, meaningful names do not have to be long and redundant, but should be concise while remaining reasonably descriptive. For instance printSum, or printSumOf is perfectly understandable. The function already takes three numbers, for crissake! –  Thomas yesterday
    
Noise level compared to an ocean of comments? I think conciseness is better served for public APIs and methods. These are simple methods but they might as well be the mile long God functions that fester in most source code... The question was asking for a critique on the attempt at over commenting so my answer was an attempt to be descriptive in the code to the maximum extent possible... Even if it is stretching things. –  shivsky yesterday

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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