Is this good code or is there a better way?

Here's the programming request:
A country club, which currently charges $2500 per year for membership, has announced it will increase its membership fee by 4% each year for the next six years. Write a program that uses a loop to display the projected rates for the next six years.
And this is the code I wrote:
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
#include <iostream>
#include <iomanip>
#include <string>
#include <fstream>

using namespace std;

const double ANNUAL_CHARGE = 2500.0;
const int PERCENT_INC = 4;

int main()
{
	double amount_inc, increase = 0;
	cout << "The annual charge of $" << ANNUAL_CHARGE << " will be increased by 4% each year"
		 << " for the next six years\n";

	for(int count = 0; count < 6; count++)
	{
		if(count < 1)
			cout << "Year" << count + 1 << ":\t\t$" << ANNUAL_CHARGE << endl;
		else
		{
			increase += (ANNUAL_CHARGE / 100) * PERCENT_INC;
			amount_inc = ANNUAL_CHARGE + increase;
			cout << "Year " << count + 1 << ":\t\t$" << amount_inc << endl;
		}
	}
	
	return 0;
}


Is this good coding, or is there a better way to do this program?
closed account (48T7M4Gy)
Another way:

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
#include <iostream>
#include <iomanip>

const int PERCENT_INC = 4;

int main()
{
    double annual_charge = 2500;
    
    std::cout << "The annual charge of $" << annual_charge << " will be increased by 4% each year for the next six years:\n";
    
    for(int year = 0; year < 6; year++)
    {
            annual_charge *= (100.0 + PERCENT_INC) / 100;
            std::cout
            << "Year " << year + 1 << ':'
            << std::setw(7) << std::fixed << std::setprecision(2) << '$' << annual_charge
            << '\n';     
    }
    
    return 0;
}
Last edited on
Basically the same as @kemorts solution but the way I would do it and with basic explanation about why you should change some things:

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
#include <iostream>
// #include <iomanip> Not used
// #include <string>  Not used
// #include <fstream> Not used

// using namespace std; Why grab everything from the std namespace? That is A LOT of stuff. Instead,
//                      you could grab only what you need/use (see below), which is std::cout and std::endl
using std::cout;
using std::endl;

// const double ANNUAL_CHARGE = 2500.0; // Why is this const? You could remove a few variables if you made it non-const
const double PERCENT_INC = 1.04; // Use 1.04 to represent 4%  increase as opposed to (ANNUAL_CHARGE / 100) * PERCENT_INC

int main() {
	double charge = 2500.0; 

	// NOTE: instead of "using std::cout/endl" you could resolve the scope inline like so:
	// 	std::cout << "The annual charge of $" << ANNUAL_CHARGE << " will be increased by 4% each year"
	//            << " for the next six years\n";
	cout << "The annual charge of $" << charge << " will be increased by 4% each year"
		 << " for the next six years\n";

	// If you put update the value AFTER you output, there is no need for the if/else (which is computationally expensive)
	for (int i = 0; i < 6; i++) { // 'i' is traditionally used as a loop-counter (then j, k... for nested loops)
			cout << "Year " << i + 1 << ":\t\t$" << charge << endl;

			charge *= PERCENT_INC;
	} // END for(i)

return 0;
} // END main() 
Last edited on
This program is very, very small, so there's little room to critique design, which looks fine.
But it could be refactored a little bit:

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
// Only <iostream> is required.
#include <iostream>

// `constexpr` is stronger than `const`, that's preferred.  We might as well mark
// these `static`, too.
//
// UPPER_CASE is customarily reserved for preprocessor macros.  
// Certain conventions specify UPPER_CASE for constants in general.
static constexpr double annual_charge = 2500.0;
static constexpr int    percent_inc   = 4;

// using namespace std; is removed.  This is probably the most important
// critique I can give you.  Don't use the form
/*   using namespace x; */
// Because it can introduce [i]silent changes[i] to your program's behavior

int main()
{
  // Lines are wrapped at 80 characters, as is convention.  This is essentially
  // a courtesy to programmers who edit their code in the terminal.  There are
  // more people who do this than you might expect.
  std::cout << "The annual charge of $" << annual_charge
            << " will be increased by 4% each year"
            << " for the next six years\n";

  // Declaring variables at the beginning of a block is borrowed from C89, where
  // this practice is required.  Even so, you should limit the scope of symbols
  // as much as possible.
  //
  // I wish I could remember specifics, but IIRC at least one well-known design
  // book on my shelf suggests the "span" of a variable (the distance between
  // its' first and last use) should be minimized.
  double increase = 0.0;

  // Magic number 6 should be replaced with a symbolic constant:
  static constexpr int num_years = 6;
  for(int count = 0; count < num_years; count++)
    {
      if(count < 1)
        std::cout << "Year" << count + 1 << ":\t\t$" << annual_charge << "\n";
      else
        {
          increase += (annual_charge / 100.0) * percent_inc;
          // This variable is constant and limited within this scope.  Declare
          // it here.
          double const amount_inc = annual_charge + increase;
          std::cout << "Year " << count + 1 << ":\t\t$" << amount_inc << "\n";
        }
    }
}


http://coliru.stacked-crooked.com/a/e961c7026d10900c
Last edited on
Wow, thank you kemort, and especially edge6768 and mbozzi for taking the time to write out all that stuff. I'm really thankful to have people give me pointers on things they've figured out that perhaps they wish someone would have told them when they started. Thanks a lot guys, I'll use all of this information!
Some more things:

Try to avoid global variables - they will become a nuisance one day. This is part of keeping the scope of variables as small as possible. Remove the static as this has the effect of making them global anyway.

Learn to write your code in terms of functions: at the moment you could have 2. One to print the initial text, another to print out the results of the calculations. Even though these functions are only called once each, doing so aids in understanding and is a form abstraction. There is a kind of unwritten rule that functions should never be more 40 Lines Of Code (LOC) - some go for even less, one can always split a function into parts by having more functions. Sometimes it's worth it to have a function even if it is only a few lines, it's not only to avoid writing code more than once. Remember to put function declarations before main, with the function definitions after main.

Also very helpful are meaningful names for variables and functions. If done well, the code by itself should tell a story of what is happening - without having too many comments. Use comments to say why code is doing something, as well as pre and post-conditions and expected valid ranges of values, or anything else that might actually be useful. Here is a comical version of what I mean by good names and story telling:

http://www.cplusplus.com/forum/lounge/176684/#msg872881

Another thing is to think about the types for your variables. For example num_years should never be negative, so make it unsigned. However be careful mixing signed and unsigned calculations, as there are different ways of representing signed ints - this can cause problems. So the thing to do there is make count unsigned as well, so the for loop uses the same type throughout.

As everyone has mentioned, it's best in the end to just put std:: before each std thing. I used to do using std::cout; but that gets tiresome as well - if one is using various STL containers and algorithms, it becomes a pain to put 20 of them at the beginning of a file every time.

Finally, while we are on namespaces: It's a good idea to put all of your own code into it's own namespaces ; yet another way to restrict scope. It's possible to have more than one namespace and they can be nested. Perhaps it's a bit trivial here (but still not unreasonable) - it is more important when you get to writing your own classes. For example you could have this in a header file - I like to use *.hpp for header file extensions - it means header file with cpp code inside:

CountryClub.hpp

1
2
3
4
namespace CountryClub {
   double increase = 0.0;
   constexpr unsigned int num_years = 6;
} //end of CountryClub namespace 


before main:
#include "CountryClub.hpp"

And then in main:

namespace CC = CountryClub; // an alias to make things easier

Then refer to the variables like this:

for(unsigned int count = 0; count < CC::num_years; ++count) {

As I say this is way over the top for now, just a good idea to mention things early on :+)

Good Luck !!
Remove the static as this has the effect of making them global anyway.

I don't understand what you mean. As far as I'm aware, "global" doesn't mean anything except "declared in the global namespace".

I disagree about removing the static specifiers.

The symbols on lines 9 and 10 are internally linked because of static. This is more restrictive than the default. WRT the symbol on line 36, marking the object static may possibly save a tiny bit of stack space, and I can't think of any argument against it.
Last edited on
closed account (48T7M4Gy)
those arguments are crap

I would be very surprised if OP is grappling with these potentially world shattering considerations at the moment. I suspect there are higher priorities.

We'll possibly never know but the 'argument' closer to home is whether the increase in fees is compounded or not. On that note @The Snowman, if still present, needs to take care in what is submitted for assessment, even if it is just to show some proficiency at formatting the results tidily. The Snowman's call as always.

Continue ...
@mbozzi

Ah, I had it half right:

http://en.cppreference.com/w/cpp/language/storage_duration

cppreference wrote:
static storage duration. The storage for the object is allocated when the program begins and deallocated when the program ends. Only one instance of the object exists. All objects declared at namespace scope (including global namespace) have this storage duration, plus those declared with static or extern.


But as you say, static implies internal linkage which means: The name can only be referred to in the current translation unit. So that is indeed more restrictive than global.

++ThingsLearnt;

In my previous post, I should have asked a question instead of making a statement about something I only half knew*

But for the OP, would it better to advise not to put variables before main?

*I knew that I thought that I had it right, and I thought that I knew it was right :+)
But for the OP, would it better to advise not to put variables before main?

In general, yes.
IMO the most significant argument against "global" variables has to do with eliminating global state (which should be avoided if possible), but an immutable constant isn't stateful.

It isn't uncommon to provide global constants for instance:
namespace foo { constexpr double pi = 3.141...; }
And eliminating them outright doesn't seem like a good idea: are there alternatives besides redefining pi more than once?

@Kemort
world shattering considerations

Lol
I edited my post before I saw your comment, else I would have left my original irrelevant comment in. :)
closed account (48T7M4Gy)
LOL too. Cheers :)
it will increase its membership fee by 4% each year for the next six years.... Write a program that uses a loop to display the projected rates for the next six years.

1. That means the increase compounds. Otherwise they would have said the fee will increase $100 each year.

2. You're supposed to print out the fee for the next 6 years, not the fee for the current year and the next 5.

You can fix this in kemort's solution (for example) by moving line 19 to just before line 14.
The annual charge of $2500 will be increased by 4% each year for the next six years:
Year 1:      $2600.00
Year 2:      $2704.00
Year 3:      $2812.16
Year 4:      $2924.65
Year 5:      $3041.63
Year 6:      $3163.30
closed account (48T7M4Gy)
done
Topic archived. No new replies allowed.