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.

This is a programming practice that our teacher gave us, and I would appreciate if someone can look over my program and tell me if I did a good job.

Basically, the context is I joined a company and am supposed to write a program in C that takes three inputs (ft of steel, number of ball bearings, and lbs of bronze), and returns how many bells and whistles can be made the next day.

Making a whistle requires 0.5 ft of steel and 1 ball bearing. Making a bell requires 1.10 lbs of bronze (.75 lbs of bronze and 1 clapper (.35 lbs of bronze)).

This is the program I have written. I apologize if it seems messy.

#include <stdio.h>
#include <stdbool.h>

void PrintLine();
int CalculateWhistles(int steel, int bearings);
int CalculateBells(int bronze);

int main() {
    bool runProgram = true;
    int runAgain = 0;

    while (runProgram == true) {
        int ftSteelIn = 0;
        int numBallBearingIn = 0;
        int ibsBronzeIn = 0;

        printf("\n\n"); // Formatting...
        PrintLine();

        printf("    How many feet of steel was received today:               "); // Steel received
        scanf("%d", &ftSteelIn);
        printf("    How many ball bearings were received today:              "); // Ball bearings received
        scanf("%d", &numBallBearingIn);
        printf("    How many pounds of bronze was received today:            "); // Bronze received
        scanf("%d", &ibsBronzeIn);

        PrintLine(); // Print line

        PrintLine(); // Print line

        // Print company logo and program title
        printf("                                Acme Corporation\n");
        printf("                                Product Report\n");

        PrintLine();// Print line

        // Print number of bells and whistles
        printf("    Number of Bells                                                      %d\n", CalculateBells(ibsBronzeIn));
        printf("    Number of Whistles                                                   %d\n", CalculateWhistles(ftSteelIn, numBallBearingIn));

        PrintLine(); // Print line
        PrintLine();

        printf("\n\n");
        PrintLine();

        // Ask the user if they would like to run the program again
        printf("    Would you like to run the program again? ('1' for yes, '2' for no): ");
        scanf("%d", &runAgain);
        PrintLine();

        // If user does not say 'yes', end the program and say goodbye
        if (runAgain != 1) {
            printf("\n    Exitting the program. Have a good day.\n\n");
            PrintLine();
            runProgram = false;
        }
    }

    return 0;
}

void PrintLine() {
    int i = 0;
    printf("    ");
    while (i <= 70) {
        printf("-");
        i++;
    }
    printf("\n");
}

int CalculateWhistles(int steel, int bearings) {
    steel = steel / 0.5;
    if (steel < bearings) {
        return steel;
    }
    else if (bearings < steel) {
        return bearings;
    }
}

int CalculateBells(int bronze) {
    int bells = bronze / 1.1;
    return bells;
}

It runs fine with no errors. There is a second part where I calculate the remainder, but I want some feedback before I go on to that part.

Any advice would be appreciated, how to make it more efficient or anything like that (I'm just learning C).

share|improve this question
1  
Forgot to add, the instructor wants the PrintLine() functions for the output. –  Qwurticus Feb 17 at 4:07
    
Since you mentioned you are a beginner, and you seem eager to learn, check out this article. It's full of good programming habits and if you get into them now, you will have a much easier time learning more about programming. –  Kevin Feb 17 at 12:17
    
Thanks, I definitely want to get better and will read it when I have the time! :) –  Qwurticus Feb 17 at 17:43
    
"PrintLine(); // Print line" This may be the best code comment I have ever seen :o) –  Paul Feb 18 at 14:47
add comment

8 Answers

up vote 27 down vote accepted

Roughly 50% of all programming is about dealing with errors. You say your code works, but it doesn't handle errors and therefore doesn't work.

Example 1:

How many feet of steel was received today:  four

Program doesn't say "Error, try again". Instead it displays all prompts without waiting for any user input and exits.

Example 2:

How many feet of steel was received today:  3.7

Program doesn't say "Error, try again". Instead it displays all prompts without waiting for any user input and exits.

Example 3:

How many feet of steel was received today:  3 4

Program doesn't say "Error, try again". Instead it assumes 3 feet of steel; displays "How many ball bearings" and doesn't wait for user input and assumes 4 ball bearings.

Example 4:

Would you like to run the program again? ('1' for yes, '2' for no): yes

Program doesn't say "Error, try again". Instead it exits.

Example 5:

How many feet of steel was received today:               -99
How many ball bearings were received today:              -99
How many pounds of bronze was received today:            -99

Program decides you can make -89 bells and -198 whistles.

Example 6:

How many feet of steel was received today:               999999999999999999
How many ball bearings were received today:              999999999999999999
How many pounds of bronze was received today:            999999999999999999

Program decides you can make -1351471477 bells and -2147483648 whistles.

Example 7:

How many feet of steel was received today:               200
How many ball bearings were received today:              100

Program decides you can make 0 whistles. Note: unlike the previous examples, this is not a "failed to check for invalid input" problem.

share|improve this answer
    
Thanks for the advice, I'll change up the program. Some of these I will add if I have time, but I don't think they want too complex a program (this is literally a beginners course). I'm a bit of an overachiever ;), so I'll add to it but I don't want the grader getting frustrated at having to check my code for all the extra 'junk'. –  Qwurticus Feb 17 at 4:03
7  
@Brendan The other 50% is documentation! –  Jason C Feb 17 at 6:20
1  
I'd upvote you if you changed that overly exaggerated 50% up there. If you plan your application properly, adding error messages and warnings should not take more more than 10% of total development time. –  Kevin Feb 17 at 11:52
1  
@Kevin to try to give any general number of percentage on debugging in a project is impossible. It depends on a number of factors: skill, methodology, code base, programming language and personality to mention some. –  daramarak Feb 17 at 12:03
2  
@daramarak It's about adding error handling, not about debugging. –  Kevin Feb 17 at 12:04
show 5 more comments

I'm not sure how far you are in your class, but here are a few notes I didn't see in other answers:

  • It is possible your CalculateWhistles() method could fail the if condition tests and reach the end of the non-void method. Your methods should ALWAYS have a return statement for every case, even if you have to return nothing. For example, what will your code do if steel is equal to bearings?

    int CalculateWhistles(int steel, int bearings) {
        steel = steel / 0.5;
        if (steel < bearings) {
            return steel;
        }
        else if (bearings < steel) {
            return bearings;
        }
    }
    

    Because this method fails to return a value if the test conditions fail, it wouldn't compile on my system. I have a more strict compiler than most, so that tells me that you aren't compiling with warnings enabled, which you should have turned on. I rewrote the method so that it would compile on my system properly.

    int calculateWhistles(int steel, int bearings)
    {
        steel /= 0.5;
        if (steel <= bearings) return steel;
        else return bearings;
    }
    
  • Use a for loop in your PrintLine() method.

    void PrintLine() {
        int i = 0;
        printf("    ");
        while (i <= 70) {
            printf("-");
            i++;
        }
        printf("\n");
    }
    

    However, when examining the for loop, all it does is print out some space and a bunch of dashes. printf() statements can be expensive on a system, so to maximize efficiency, you should use as few of them as possible.

    void printLine()
    {
         printf("    ------------------------------------------\n");
    }
    

    But here is where we can really boost the efficiency, since your teacher is requiring the use of this method: use the inline keyword on the function. The point of making a function inline is to hint to the compiler that it is worth making some form of extra effort to call the function faster than it would otherwise - generally by substituting the code of the function into its caller. As well as eliminating the need for a call and return sequence, it might allow the compiler to perform certain optimizations between the bodies of both functions.

    This does not mean that you should inline everything.

    Use inline:

    • instead of #define
    • with very small functions are good candidates for inline: faster code and smaller executables (more chances to stay in the code cache)
    • when the function is small and called very often

    Don't use inline:

    • with large functions: leads to larger executables, which significantly impairs performance regardless of the faster execution that results from the calling overhead
    • when the function is seldom used

    For your functions it is alright to use them though.

    inline void printLine()
    {
         puts("------------------------------------------");
    }
    
  • You don't need the bells variable in the method CalculateBells().

    int CalculateBells(int bronze) {
        int bells = bronze / 1.1;
        return bells;
    }
    

    You can just return the math you do to the parameter alone, surrounded by parenthesis.

    int calculateBells(int bronze)
    {
        return (bronze / 1.1);
    }
    
  • Some of your comments I don't find necessary.

       printLine(); // Print line
    
  • You can initialize all of your variables of one type on one line. This will help later with organization of larger programs.

    int ftSteelIn = 0;
    int numBallBearingIn = 0;
    int ibsBronzeIn = 0;
    

    This will also cut down on your lines of code a bit.

    int ftSteelIn = 0, numBallBearingIn = 0, ibsBronzeIn = 0;
    
  • If you put all of your methods before main(), you don't have to prototype them beforehand. However, since we are using __inline__, we will have to use prototypes.e

  • I would have the user input a char of either "y" or "n" to indicated if they want to run the program again, perhaps in a do-while loop.

  • You could use the function puts() instead of printf() in some places.

  • Personally I feel like your "report" has too much space in it.

       printf("                                Acme Corporation\n");
    

    But that is to your discretion.


Final code (with my changed implemented):

#include <stdio.h>
#include <stdbool.h>
#include <ctype.h>

void printLine();
int calculateWhistles(int steel, int bearings);
int calculateBells(int bronze);

inline void printLine()
{
    puts("----------------------------------------------------");
}

inline int calculateWhistles(int steel, int bearings)
{
    steel /= 0.5;
    if (steel <= bearings) return steel;
    else return bearings;
}

inline int calculateBells(int bronze)
{
    return (bronze / 1.1);
}

int main()
{
    bool isRunning = true;
    char runAgain = 'n'; // default to no

    do
    {
        int ftSteelIn = 0, numBallBearingIn = 0, ibsBronzeIn = 0;

        printf("How many feet of steel was received today? ");
        scanf("%d", &ftSteelIn);
        printf("How many ball bearings were received today? ");
        scanf("%d", &numBallBearingIn);
        printf("How many pounds of bronze was received today? ");
        scanf("%d", &ibsBronzeIn);
        for(; getchar() != '\n'; getchar())
        {
        } // eat superfluous input, including the newline

        // Print company logo and program title
        puts("\nAcme Corporation: Product Report");
        printLine();

        // Print number of bells and whistles
        printf("Number of Bells: %d\n", calculateBells(ibsBronzeIn));
        printf("Number of Whistles: %d\n", calculateWhistles(ftSteelIn, numBallBearingIn));

        // Ask the user if they would like to run the program again
        printf("Would you like to run the program again? (y/N) "); // capital 'N' to suggest it is default
        scanf("%c", &runAgain);

        if (tolower(runAgain) != 'y') isRunning = false;
    } while (isRunning);

    puts("Exitting the program. Have a good day.");
    return 0;
}
share|improve this answer
    
Thanks! I didn't think the if/else statements through too much. The printLine() statements are redundant, I'll remove them (I should have wrote the program and then added formatting). Is there a real difference between using a for loop instead of a while loop for the printLine()? We haven't learned too much about C yet (I'm pretty good with Python and C++ and usually use for loops). I used prototypes because our instructor wants us to use them to show we know "how functions work" Rolls eyes. –  Qwurticus Feb 17 at 5:43
    
@Qwurticus Ahh, "prototypes" was the word I was looking for! :) In the case where I used the for loop, there isn't much difference between that and a while loop besides looking cleaner. It should perform in the exact same way (though with the for loop I am seeing fewer instructions when I look at the Assembly output, so there may be a negligible efficiency increase). –  syb0rg Feb 17 at 5:56
1  
Ah, okay. I like efficiency (and I agree that for loops look cleaner ;) ). –  Qwurticus Feb 17 at 6:05
4  
The compiler should alert you to the first point (reaching the end of a non-void function) if you turn on warnings. For that reason, always compile with warnings turned on. –  200_success Feb 17 at 6:45
1  
@syb0rg I upvoted, but I think you should consider adding a warning about inlining functions. inline does make sense for this example, but you have to be careful with noobies since they may decide to inline everything, regardless of the size of the project. –  David Schwartz Feb 18 at 20:27
show 3 more comments

The other answers here are very good, especially the accepted one by Brendan. I would like to add one additional comment. You wrote:

Any advice would be appreciated, how to make it more efficient ...

I want to touch on your comment about making it "more efficient" with just a simple comment: Don't. Write your code, get it working. Code cleanly and do what makes sense to keep your design straightforward. Document your code, think about others who might view or work with it, and think about yourself in the future coming back to it and not remembering what you were thinking when you wrote it.

Once your program is written, then ask yourself: Is it not meeting the hard performance requirements I have set? Is the UI too slow? Is some algorithm here or there taking too long and actually affecting usage of my program in a noticeable and negative way? Am I running out of memory somewhere? If so, then first concentrate on improving any algorithms or logic on a higher level; perhaps, for example, you are sorting a large amount of data and it is definitively too slow or resource intensive -- first consider a different sorting algorithm. After you are satisfied with that, then if necessary you can proceed to add further micro-optimizations but only after you have clearly identified where the actual bottlenecks are (e.g. profiling, or measuring function times, not just blind guessing).

Do not worry about wasting a few CPU cycles here or there if it leads to clean, maintainable, clear code, especially during initial development where you may be changing things unexpectedly. You want to avoid premature optimizations that both distract you from your real goal and lock you in to a certain implementation that cannot be easily changed if and when it is necessary.

It is very common for new programmers to start wanting to make unnecessary micro-optimizations right off the bat; especially in areas that don't really matter (e.g. writing a program that say, generates image files, but trying to optimize the code that checks if an output filename string is valid.) Don't go down that path! Design -> Implement -> Test -> Profile -> Optimize -> Test, and only do the last 3 if your performance requirements aren't met.

I know this is may be general advice and a bit premature, but if you keep this in mind (as well as all the information in the other great answers here) you will be setting yourself up for a smooth and productive experience.

share|improve this answer
1  
Thanks! I definitely get impatient and will focus more on actually getting the code working. I've heard horror stories about programs that work just fine but are impossible to read, and what good is that to someone who wants to improve it? –  Qwurticus Feb 17 at 6:28
    
@Qwurticus It's great for a contractor being paid by the hour, actually. There's nothing contractors love more than big, messy programs that rack up the billable hours. Plus, they don't have to do any clean up because once the contract is up, they're off to the next gig. –  corsiKa Feb 18 at 18:55
    
@corsiKa I hope you are being facetious, because that is terrible advice, especially for a new programmer. If a contractor wants a "next gig", that's not the best work ethic. It works for me, though, when I eventually get hired to clean up your mess. I make far more money from repeat clients (and the references they bring) than from maintenance -- plus good work leads to longer term lead/consulting jobs (and happier end-users, there's some karma point opportunities there -- next time you get frustrated with a piece of software, perhaps the developers behind it had that same bad attitude). –  Jason C Feb 18 at 23:18
add comment
  • Although it is common to have functions start with a lowercase letter, it depends on what is preferred in your programming environment. You can choose either case, preferably the one mentioned, unless your instructor has you use a certain one.

  • I'd consider removing all these PrintLine() calls, as well as the function itself, if they're not that necessary. It just clutters the code, especially with most of the work being done in main(). Since the instructor requires this, you'll just have to keep them. But if you're keeping or want to keep personal copies of your assigned programs, you may make this change.

  • You don't need the else if statement in CalculateBells(). As that statement is only the opposite of the first one, a plain else will work. You may still keep it that way in case you may need to make additions at some point.

  • This:

    steel = steel / 0.5;
    

    can be shortened to this:

    steel /= 0.5;
    

    This works for all mathematical operators and similar situations.

  • For conditional statements like this, you don't need to include the true:

    while (runProgram == true)
    

    This does the same thing:

    while (runProgram)
    

    If you were to use something with false, you would have this:

    while (!someCondition)
    
share|improve this answer
    
Unfortunately, the $printLine()$ functions are necessary, the teacher wants the output to have those long lines. :/ I'll change the if/else to an else statement, that was stupid on my part, but the function is supposed to return an integer and it gives back the number I want, but I'll change it to a float or double and see how that works. Thanks for the advice! –  Qwurticus Feb 17 at 3:58
    
@Qwurticus: That's fine in this case. Just wanted to make sure this wasn't something you wanted to do. :-) Just make sure to not edit the original code, otherwise answers will be invalidated. You may still add a note about the printing functions so that others are aware. –  Jamal Feb 17 at 4:00
3  
@Jamal Just to pick nits, there isn't really a "C naming convention", at least, not in the sense that it's some "official" C way. More importantly, a programmer should use a style convention consistent with the other programmers he is working with, or if he is alone, at least consistently throughout all of his code so he doesn't confuse himself later. There's a zillion popular naming conventions for programs written in C, the real problems occur when different ones collide. –  Jason C Feb 17 at 6:19
5  
Actually, CalculateBells() and CalculateWhistles() should return ints. What kind of factory would produce half a whistle? And what kind of sound would half a whistle make? –  200_success Feb 17 at 6:40
1  
@200_success: half as good sound :-) –  Juha Untinen Feb 17 at 11:41
show 3 more comments

You have some 'magic numbers' embedded in the code which are a really bad idea as they make it difficult to maintain or change. You should separate out the fixed ratios and give them meaningful names. Using #define for a numerical constant isn't too bad in C as it doesn't have a const modifier. So add #define CLAPPER_BRONZE 0.35 etc.

A final subtle point is that you've stated a whistle needs an X and a Y and you've added X's and Y's weights (0.75 + 0.35) and implemented a function to divide by 1.1.

It would actually be much better to capture the knowledge you have about the domain more directly and transparently in the code. If you're told making a widget needs an X and a Y then code that. Then worry about coding what an X needs. This may seem like overkill but it will be much easier to change the code to adapt to changing circumstances. (E.g. we switch to making X's out of something else.)

share|improve this answer
    
I couldn't recall if my instructor went over #define so I didn't want to throw that in there yet (This is a freshman course in C), but I'll keep it in mind for the next programming practice. Could you possibly explain your final point about making the "domain more transparent"? If it takes 1.1 lbs of bronze to create a bell, is not simple just to divide by that number to get the optimum number of bells that can be produced? :( –  Qwurticus Feb 17 at 17:37
    
That seems like massive overkill when X is "0.75lb of bronze" and "Y is 0.35lb of bronze", unless all you're suggesting is to replace bronze / 1.1 with bronze / (BELL_BRONZE + CLAPPER_BRONZE). How else would you sensibly code the requirement that making a bell requires 0.75lb of bronze, and then another 0.35lb of bronze? –  David Richerby Feb 17 at 18:05
    
Of course it's overkill for this little example. Just trying to stimulate the braincells for a real case. –  TheMathemagician Feb 18 at 12:21
add comment

Minor thing: Don't write

steel = steel / 0.5;

The result of dividing steel by 0.5 isn't an amount of steel. It's an upper limit for the number of whistles you can make. So write

int whistles = steel / 0.5;

Now the big thing: You don't have "steel". You have "feet of steel". What if the spec changes and the amount of steel is entered in meters? Or in pound, and you have to calculate the length? Better to write

steelInFeet

so if the spec changes it is much more obvious where in your code you have to make changes. For example:

double steelInFeet = steelInMeters / 0.3048;
int whistles = steelInFeet / 0.5; 
share|improve this answer
    
Okay, I'll change the name and keep that in mind for later programs. :) They don't expect us to think about those things just yet, so while I doubt I'll be hurt by not changing it, it can't hurt to do so either. –  Qwurticus Feb 17 at 17:40
    
Also, x / 0.5 is the same as x * 2, just harder to read and more prone to rounding/truncation errors. (No error in this case, but there could be.) I always choose multiplication over division if given the option. –  Darrel Hoffman Feb 18 at 15:09
    
As a matter of taste, I typically omit the In. So I personally would use steelFeet or steelMeters. I do this a lot with my training application. You get trainingMinutes and defaultCourseDurationHours. Although 100% I'd take steelInFeet over steel any day! –  corsiKa Feb 18 at 18:57
add comment

People have said plenty about the code so I'll just make one point.

Your commenting is poor. Commenting is difficult for beginners because it's hard for you to tell which parts of your code are obvious and need no more explanation, and which parts would benefit from having comments added. As you gain more experience, you'll get better at this.

Most of your comments are completely redundant because they don't add any explanation. For example, you have several instances of

PrintLine(); // Print line

Use comments to document the PrintLine function, not the places where it's called. It's pretty obvious that it prints a line, so you don't need to keep saying it.

In a similar vein, your comments on the lines that read user input don't help the reader understand the code: it's obvious that those lines read in the amount of steel, brass and ball bearings.

On the other hand, you don't document the two Calculate functions at all. That's the only part of the program that really needs explaining, since it's the only part that uses facts specific to the problem that you're trying to solve (e.g., what materials you need to make a bell or a whistle.) You could say something like

int CalculateBells (int bronze)
// Calculate the number of bells that can be made from a given amount
// of bronze.  Each bell requires 1.1lb.
{
     [...]
}
share|improve this answer
    
Thanks for the advice, I did realize how stupid my comments were and have since gone back through and cleaned it up. –  Qwurticus Feb 17 at 17:31
    
Welcome to CR. Great first answer. +1 –  rolfl Feb 17 at 17:34
add comment

Do you really want to make these calculations with integers? Shouldn't you be using floating point variables?

I'm impressed on how every other answer focused on esthetics but didn't comment on the most critical problem of your code: it doesn't work even with correct input.

edit: Actually, other answers touched the issue, it was I that failed to notice that this is not exactly a bug, since using ints do achieve the desirable answer.

share|improve this answer
3  
I would have commented if it didn't work. Assigning to an int automatically rounds a floating point value towards zero to the next integer. If you use 1.1 pounds of bronze for a bell then "int bells = bronze / 1.1;" will indeed give the correct result since you can't build 3 1/2 bells. –  gnasher729 Feb 17 at 12:46
4  
It does work, since rounding the results down to the nearest int is the reasonable behaviour. Furthermore, @Brendan's accepted answer points out that the program fails to accept floating-point inputs. –  200_success Feb 17 at 12:54
    
I stand corrected. I was reading this on my phone and didn't realized @Brendan's answer already touched on the floating point issue. The integer truncation is also really desirable, my bad. –  Spidey Feb 17 at 14:18
    
@200_success: Yeah, that's also what confused me with my answer. Sufficient documentation would've helped, but the OP was not expected to excel in that. –  Jamal Feb 17 at 17:40
    
It is my bad, in the future I will post the original document/question to make it clearer. There is a second "extra-credit" part to this problem that calculates the remainder and adds it to next day's materials. That would use floats/doubles. –  Qwurticus Feb 17 at 17:45
add comment

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.