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.

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;
}
share|improve this question
    
Prefer "\n" to std::endl. –  Loki Astari Aug 14 at 11:24
4  
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 –  blgt Aug 14 at 11:43
    
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".) –  HostileFork Aug 14 at 12:00
4  
I do not know about the code, but the bottles of beer were very good ... –  BlueTrin Aug 14 at 13:02
1  
You could use a for loop instead of a while, it would save having to manually decrement for (int i = 99; i > 1; i--){...} –  user3791372 Aug 15 at 13:14

6 Answers 6

up vote 23 down vote accepted

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

share|improve this answer
    
I'm following a C++ book to learn it and the author was recommending using the namespace std. Thanks for pointing out my mistakes. –  Amession Aug 14 at 11:50
15  
@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. –  Davidmh Aug 14 at 12:11
4  
@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. –  Dan Neely Aug 14 at 14:51
    
@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? –  Canadian Luke Aug 14 at 18:24
5  
@CanadianLuke First thing to know is that proper resources for learning C are not proper resources for learning C++. –  Morwenn Aug 15 at 12:49

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.

share|improve this answer
    
Nice idea, I'm adjusting my code now, thanks! –  Amession Aug 14 at 11:52
2  
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? –  Canadian Luke Aug 14 at 18:25
5  
@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. –  jliv902 Aug 14 at 20:37
1  
@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. –  hvd Aug 16 at 22:15

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.

share|improve this answer
1  
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. –  Amession Aug 14 at 11:51
    
just edited the for loop as I had a copy paste fault in it, mine appology for that :) –  chillworld Aug 14 at 11:57
    
Please use --bottleCount instead of bottleCount--. Should have named this language ++C. Makes my cry every time :-( –  Aurélien Ooms Aug 15 at 7:30
    
oke small performance issue : stackoverflow.com/a/24904/2853637 –  chillworld Aug 15 at 8:39

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.

share|improve this answer
    
Nice logic you used there. Saving this program for future reference. –  Amession Aug 15 at 7:54
4  
I'd like to see your performance benchmarks… and breathalyzer results. –  200_success Aug 15 at 7:56
    
My clang++ compiler ignores inline. It generates runtime recursive function calls. There is just one copy of the generic 99-to-3 verse in the binary code. –  200_success Aug 15 at 8:59
    
OP is a self-confessed beginner - this will scare the heck out of them! –  user3791372 Aug 15 at 13:10
2  
@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). –  Jerry Coffin Aug 15 at 13:31

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;
}
share|improve this answer
    
Why can't I find jeff atwoods comment about pluralization "bugs" right now?? –  Vogel612 Aug 14 at 22:22
6  
by the time you've drunk 98 bottles of beer you try getting your grammar right.. –  CashCow Aug 15 at 11:13

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.

share|improve this answer
    
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. –  Amession Aug 16 at 7:37

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.