36
\$\begingroup\$

I wrote a C++ program for 99 Bottles of Beer, but it's kind of messy. Is there any way I can make it short and cleaner and more easier to read?

/*
 * Write a program that prints out the entire lyrics to a full rendition of "99 bottles of beer"
 */

#include <iostream>

using namespace std;

int main (void)
{
    int bottle = 99;
    while (bottle > 0)
    {
        cout << bottle << " bottles of beer on the wall, "
             << bottle << " bottles of beer." << endl;

        bottle--;

        cout << "Take one down and pass it around, " << bottle << " "
             << "bottles of beer on the wall.\n" << endl;

        if (bottle == 1)
        {
            break;
        }

    }

    cout << "1 bottle of beer on the wall, 1 bottle of beer." << endl
         << "Take one down and pass it around, no more bottles of beer on the wall.\n" << endl
         << "No more bottles of beer on the wall, no more bottles of beer." << endl
         << "Go to the store and buy some more, 99 bottles of beer on the wall.";

    return 0;
}
\$\endgroup\$
6
  • \$\begingroup\$ Prefer "\n" to std::endl. \$\endgroup\$ Aug 14, 2014 at 11:24
  • 4
    \$\begingroup\$ Not a code, but a grammar issue: just before you leave the loop, you'll print "... and pass it around, 1 bottles of beer ..."; that line should probably be printed with the others out of the loop, so it can be in the singular \$\endgroup\$
    – blgt
    Aug 14, 2014 at 11:43
  • 1
    \$\begingroup\$ RE: "\n" vs. std::endl...the point being raised by @LokiAstari is that endl forces a flush of the stream. This can be useful for debugging information, so you get output right up to the crash, but it interferes with the natural performance profile of the buffering otherwise. (Just wanting to be clear it's not a matter of dictating aesthetic preference, because I think endl is more "literate-looking".) \$\endgroup\$ Aug 14, 2014 at 12:00
  • 5
    \$\begingroup\$ I do not know about the code, but the bottles of beer were very good ... \$\endgroup\$
    – BlueTrin
    Aug 14, 2014 at 13:02
  • 1
    \$\begingroup\$ You could use a for loop instead of a while, it would save having to manually decrement for (int i = 99; i > 1; i--){...} \$\endgroup\$ Aug 15, 2014 at 13:14

6 Answers 6

33
\$\begingroup\$

A few short comments:

Don't use namespace std:

It's bad for a few reasons, but in short: You can have big name conflicts if you are using namespace std;. For more information: Why is using namespaec std considered bad practice?

Break conditions:

well lookie here:

while (bottle > 0) 
{
    //print the lyrics
    if (bottle == 1) 
    {
         break;
    }
}

This could be much simpler:

while (bottle > 1)
{
     //print the lyrics
}

Naming:

bottle is a crap name for a counter. Make it bottleCount, leftBottles or about anything else, but don't name it bottle. Because it isn't a bottle...

\$\endgroup\$
5
  • \$\begingroup\$ I'm following a C++ book to learn it and the author was recommending using the namespace std. Thanks for pointing out my mistakes. \$\endgroup\$
    – Amession
    Aug 14, 2014 at 11:50
  • 19
    \$\begingroup\$ @Amession it is common when teaching beginners because makes it simpler, but it is a bit like avoiding the sex talk to your children because it is difficult. \$\endgroup\$
    – Davidmh
    Aug 14, 2014 at 12:11
  • 5
    \$\begingroup\$ @Amession books for students often suggest things that you should never do in the real world to make things slightly simpler. Unfortunately, bad habits learned in school, don't magically get wiped from your brain when you enter the real world. using namespace std; in C++ isn't quite as bad as using the mysql_pwn_my_server() family of database functions is in PHP; but anyone who reads the tech press regularly sees major sites that were breached via SQL Injection by someone who did it wrong. \$\endgroup\$ Aug 14, 2014 at 14:51
  • \$\begingroup\$ @DanNeely I agree (I read many student books about C and C++, and I'm thoroughly effed-up). Any good resources for learning proper C or C++, other than trying-and-failing? \$\endgroup\$ Aug 14, 2014 at 18:24
  • 6
    \$\begingroup\$ @CanadianLuke First thing to know is that proper resources for learning C are not proper resources for learning C++. \$\endgroup\$
    – Morwenn
    Aug 15, 2014 at 12:49
18
\$\begingroup\$

In addition to the comments in the other answers I'd recommend you to create a function to print the sentences on the screen.

Something like

while (bottles > 0)
{
    printLyrics(bottles);
    bottles--;
}

In this case you need to handle the special case when you have a single bottle left.

void printLyrics (int bottles)
{
    if (bottles == 1)
    {
        printLastLyrics ();
    }
    else
    {
        printOtherLyrics (bottles);
    }
}

I think this way the problem is better decomposed and the special case is shown at the right level of abstraction.

\$\endgroup\$
5
  • \$\begingroup\$ Nice idea, I'm adjusting my code now, thanks! \$\endgroup\$
    – Amession
    Aug 14, 2014 at 11:52
  • 3
    \$\begingroup\$ Random tidbit: I know in VB, they want the more common scenario in an if block to come first. Is C++ the same way, or is performance the same? \$\endgroup\$ Aug 14, 2014 at 18:25
  • 6
    \$\begingroup\$ @CanadianLuke It would be the same in pretty much all programming languages because it would save you from a jump/branch instruction underneath the hood. But in most situations, you won't need such a micro-optimization. If you did need such a micro-optimization, then you wouldn't want to use this answer's advice since it places an unnecessary branch instruction into a loop. \$\endgroup\$
    – jliv902
    Aug 14, 2014 at 20:37
  • 2
    \$\begingroup\$ @CanadianLuke As far as I know, it's not meant as an optimisation, but for readability. Placing the more common scenario first lets the reader read that first: letting the reader read the exceptional cases first can make it harder to understand them, because the reader won't have seen the big picture yet to understand why they are exceptional. That said, I don't think the rule applies when dealing with trivial single-line if bodies. I have never seen anyone, as an extreme example, place the exception-throwing statement for ArgumentNullException etc. in VB.NET at the end of a function. \$\endgroup\$
    – hvd
    Aug 16, 2014 at 22:15
  • \$\begingroup\$ You'd save the comparison and branching in this answer if you changed to while (bottles > 1) especially as as the code stands it would print '1 bottles...' then '1 bottle...' \$\endgroup\$ Oct 28, 2014 at 8:35
17
\$\begingroup\$

This seems like a place that template meta programming would be useful (after all, we don't want to waste precious run-time in a program that's likely to execute this often).

#include <iostream>

template<int N>
struct song {
    inline static void sing() {
        std::cout << N << " bottles of beer on the wall, "
            << N << " bottles of beer.\nTake one down, pass it around, "
            << N - 1 << " bottles of beer on the wall.\n\n";
        song<N - 1>::sing();
    }
};

template<>
struct song<1> {
    inline static void sing() {
        std::cout << "1 bottle of beer on the wall, 1 bottle of beer.\n"
            << "Take it down and pass it around, no more bottles of beer on the wall.\n\n"
            << "No more bottles of beer on the wall, no more bottles of beer.\n"
            << "Go to the store and buy some more, 99 bottles of beer on the wall.\n";
    }
};

template<>
struct song<2> {
    inline static void sing() {
        std::cout << "2 bottles of beer on the wall, "
            << "2 bottles of beer.\nTake one down, pass it around, "
            << "1 bottle of beer on the wall.\n\n";
        song<1>::sing();
    }
};

int main() {
    song<99>::sing();
    return 0;
}

Although the ordering of the templates (first the general template, then the specialization for 1, and finally the specialization for 2) may seem strange and haphazard, it's actually necessary. The general case must come before any specialization. The specialization for 2 uses the specialization for 1, so it must follow it. Otherwise, the specialization for 2 would attempt to use the general template for 1, and only after it did so would the template for 1 be found, rendering that invalid.

Unfortunately, at least as I've written the code, this doesn't reduce the code size. OTOH, I believe it at least gets the grammar correct.

\$\endgroup\$
9
  • \$\begingroup\$ Nice logic you used there. Saving this program for future reference. \$\endgroup\$
    – Amession
    Aug 15, 2014 at 7:54
  • 8
    \$\begingroup\$ I'd like to see your performance benchmarks… and breathalyzer results. \$\endgroup\$ Aug 15, 2014 at 7:56
  • 2
    \$\begingroup\$ OP is a self-confessed beginner - this will scare the heck out of them! \$\endgroup\$ Aug 15, 2014 at 13:10
  • 3
    \$\begingroup\$ @user3791372: I can't imagine how this could scare anybody. It's literally the simplest possible program to do the job--it has a cyclomatic complexity of 1 (not a single if statement or other flow control statement in it). \$\endgroup\$ Aug 15, 2014 at 13:31
  • 1
    \$\begingroup\$ @JerryCoffin: You would be surprised to hear how many beginners struggle with the concept of recursion. \$\endgroup\$ Aug 28, 2014 at 10:42
14
\$\begingroup\$

Not needed code :

You can actually remove this :

if (bottle == 1)
{
    break;
}

You are in a while loop so your loop will continue as long bottle is bigger then 0.
So I'm guessing you inserted that code for the last bottle.
You just have to change :

while (bottle > 0)

to

while (bottle > 1)

for the same result.

Naming :

While you have not many var's to call => I like that you already point to the bottle.
Still I'm thinking bottleCount could be a better var name.

While loop :

Well it doesn't make a big difference but for your own sake, use a for-loop.
You don't forget to substract one then.

for (int bottleCount=99; bottleCount > 1; bottleCount--)

Summary:

While I'm no c++ coder, it looks for the rest good.
If you really want to score high you could use recursion for the for-loop.

\$\endgroup\$
4
  • 1
    \$\begingroup\$ Sure, now I'm using for loop and it works. using if was pointless, I'm noob at C++ so thanks for the info and help. \$\endgroup\$
    – Amession
    Aug 14, 2014 at 11:51
  • \$\begingroup\$ just edited the for loop as I had a copy paste fault in it, mine appology for that :) \$\endgroup\$
    – chillworld
    Aug 14, 2014 at 11:57
  • \$\begingroup\$ Please use --bottleCount instead of bottleCount--. Should have named this language ++C. Makes my cry every time :-( \$\endgroup\$
    – Mmmh mmh
    Aug 15, 2014 at 7:30
  • \$\begingroup\$ oke small performance issue : stackoverflow.com/a/24904/2853637 \$\endgroup\$
    – chillworld
    Aug 15, 2014 at 8:39
8
\$\begingroup\$

Output errors

You have a pluralization error:

2 bottles of beer on the wall, 2 bottles of beer.
Take one down and pass it around, 1 bottles of beer on the wall.

Also, it is conventional to end the output of a program with a newline. Yours ends with just a period.

Problem structure and decomposition

You have a special case for the one-bottle verse. Perhaps that could be generalized a bit.

As illustrated above, the primary challenge is to get the pluralization and capitalization exactly right. One possible strategy would be to treat this as an internationalization problem.

However, it would be more interesting and instructive to approach it as a C++ programming exercise. Therefore, I think that an object that knows how to print itself with the right pluralization and capitalization would be a good idea.

Suggested solution

#include <iostream>

class BeerWall {
  public:
    BeerWall(unsigned n=99) : n(n), capitalizeNext(false) {}

    BeerWall &consume(int bottles=1) {
        n -= bottles;
        return *this;
    }

    BeerWall &buy(int bottles=1) {
        // Not to be confused with barfing it up.
        return consume(-bottles);
    }

    BeerWall &capitalize(bool cap=true) {
        capitalizeNext = cap;
        return *this;
    }

    unsigned remaining() const {
        return n;
    }

    friend std::ostream &operator<<(std::ostream &os, BeerWall& b) {
        switch (b.n) {
          case 0:
            os << (b.capitalizeNext ? "No more bottles" : "no more bottles");
            break;
          case 1:
            os << "1 bottle";
            break;
          default:
            os << b.n << " bottles";
        }
        b.capitalizeNext = false;
        return os;
    }

  private:
    unsigned n;
    bool capitalizeNext;
};

int main(void)
{
    BeerWall bw;
    do
    {
        std::cout << bw.capitalize() << " of beer on the wall, "
                  << bw << " of beer.\n";

        if (!bw.remaining()) break;

        std::cout << "Take one down and pass it around, "
                  << bw.consume() << " of beer on the wall.\n" << std::endl;
    } while (true);

    std::cout << "Go to the store and buy some more, "
              << bw.buy(99) << " of beer on the wall." << std::endl;
}
\$\endgroup\$
2
  • \$\begingroup\$ Why can't I find jeff atwoods comment about pluralization "bugs" right now?? \$\endgroup\$
    – Vogel612
    Aug 14, 2014 at 22:22
  • 7
    \$\begingroup\$ by the time you've drunk 98 bottles of beer you try getting your grammar right.. \$\endgroup\$
    – CashCow
    Aug 15, 2014 at 11:13
5
\$\begingroup\$

Chillworld alluded to this solution, I will expand a little bit.

your code:

int main (void)
{
    int bottle = 99;
    while (bottle > 0)
    {
        cout << bottle << " bottles of beer on the wall, "
             << bottle << " bottles of beer." << endl;

        bottle--;

        cout << "Take one down and pass it around, " << bottle << " "
             << "bottles of beer on the wall.\n" << endl;

        if (bottle == 1)
        {
            break;
        }
    }

    cout << "1 bottle of beer on the wall, 1 bottle of beer." << endl
         << "Take one down and pass it around, no more bottles of beer on the wall.\n" << endl
         << "No more bottles of beer on the wall, no more bottles of beer." << endl
         << "Go to the store and buy some more, 99 bottles of beer on the wall.";
    return 0;
} 

Everyone talked about the while statement not being set up efficiently and the if block not being needed.

if you set up the while statement to say while (bottle > 1) and remove the if block of code it would look like this.

int main (void)
{
    int bottle = 99;
    while (bottleCount > 1)
    {
        std::cout << bottleCount << " bottles of beer on the wall, "
             << bottleCount << " bottles of beer." << std::endl;

        bottleCount--;

        std::cout << "Take one down and pass it around, " << bottleCount << " "
             << "bottles of beer on the wall.\n" << std::endl;
    }

    std::cout << "1 bottle of beer on the wall, 1 bottle of beer." << std::endl
         << "Take one down and pass it around, no more bottles of beer on the wall.\n" << std::endl
         << "No more bottles of beer on the wall, no more bottles of beer." << std::endl
         << "Go to the store and buy some more, 99 bottles of beer on the wall.\n";
    return 0;
} 

unfortunately you still have the issue of the plural form of bottle when you get down to one bottle left, I would assume though after 98 bottles of beer it would take a miracle of science to even sing the last verse.

\$\endgroup\$
2
  • \$\begingroup\$ To fix the plural form of bottle the only solution is to break the loop using (bottleCount > 2) and print out outside of loop using std::cout function. \$\endgroup\$
    – Amession
    Aug 16, 2014 at 7:37
  • \$\begingroup\$ @Amession how did you come to that conclusion? \$\endgroup\$
    – Malachi
    Aug 29, 2016 at 13:03

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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