Feedback on a programming practice problem in C

Question

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).

Answer 1

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.

Answer 2

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;
}

Answer 3

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.

Answer 4

• 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)
  

Answer 5

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.)

Answer 6

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; 

Answer 7

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.
{
     [...]
}

Answer 8

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.