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.

For my junior high, IT final project, I decided to make a C++ program which functions as a simple library manager. The functions of the manager are to check your own library, view the store's library and buy from it, and check your transaction history.

I made separate arrays for each of the three above, and use it as a main database storage system. I added some extra functions such as to check if the data already has been purchased and if the person has sufficient funds to buy the data. The code is can be read at : http://pastebin.com/UQW0sAaQ

/* dycesM */

#include <iostream>
#include <string>
#include <string.h>
using namespace std;

int money = 5000; // Global Variable

struct database
{
    string songName;
    int songPrice;
    int songNumber;
    int songID;
}userStorage[30], storeStorage[30];

struct transaction
{
    string songName;
    int songPrice;
}transactionStorage[30];

void userStorageDisplay()
{
    for (int i = 0; i <= 30; i++)
    {
        if (userStorage[i].songID != NULL)
        {
            cout << "\n" << i << ". " << userStorage[i].songName << "\n" << endl;
        }
    }
}

void storeStorageDisplay()
{
    for (int i = 0; i <= 30; i++)
    {
        if (storeStorage[i].songID != NULL)
        {
            cout << "\n" << i << ". " << storeStorage[i].songName << endl;
            cout << "\t" << "Price: $" << storeStorage[i].songPrice << "\n" << endl;
        }
    }
}

void transactionStorageDisplay()
{
    for (int i = 0; i < 30; i++)
    {
        if (transactionStorage[i].songPrice != NULL)
        {
            cout << "\n" << transactionStorage[i].songName << " $:" << transactionStorage[i].songPrice << "\n" << endl;
        }
    }
}

int checkDuplicate(int songStorageValue)
{
    for (int j = 0; j <= 30; j++)
    {
        if (userStorage[j].songID == storeStorage[songStorageValue].songID)
        {
            return 0;
        }
    }
    return 1;
}

int checkBankDetails(int songNumber)
{
    if (storeStorage[songNumber].songPrice > money)
    {
        return 1;
    }
}

void purchase(int songNumber)
{
    int verification;
    int &moneyBalance = money;

    cout << "\nAre you sure you wish to purchase: " << storeStorage[songNumber].songName << " for a price of: " << storeStorage[songNumber].songPrice << " ?" << endl;
    cout << "Press 1 to confirm or 2 to exit." << endl;
    cin >> verification;

    if (verification == 1)
    {
        if (checkBankDetails(songNumber) != 1)
        {

            if (checkDuplicate(songNumber) == 1)
            {
                for (int i = 0; i <= 30; i++)
                {
                    if (userStorage[i].songID == NULL)
                    {
                        userStorage[i].songName = storeStorage[songNumber].songName;
                        userStorage[i].songNumber = i;
                        userStorage[i].songPrice = storeStorage[songNumber].songPrice;
                        userStorage[i].songID = storeStorage[songNumber].songID;

                        for (int a = 0; a <= 30; a++)
                        {
                            if (transactionStorage[a].songPrice == NULL)
                            {
                                 transactionStorage[a].songName = storeStorage[songNumber].songName;
                                 transactionStorage[a].songPrice = storeStorage[songNumber].songPrice;

                                money = money - transactionStorage[a].songPrice;

                                cout << "\nRemaining Value: $" << money << "\n";

                                a = 30; // To end the loop. 
                            }
                        }

                        cout << "\n" << storeStorage[songNumber].songName << " has been added to your library at position number: " << i << "\n" << endl;
                        i = 30; // To end the loop.
                    }
                }
            }
            else if (checkDuplicate(songNumber) == 0)
            {
                cout << "\nYou already own this song. Purchase Cancelled." << endl;
            }
        }
        else if (checkBankDetails(songNumber) == 1)
        {
            cout << "\nInsufficient funds. Purchase cancelled." << endl;
        }
    }
    else if (verification != 1)
    {
        cout << "Transaction Aborted.";
    }
    }

void main()
{

    char rerun;

    //Store Data added for demonstration purposes. 

    storeStorage[0].songName = "Daughter - Youth";
    storeStorage[0].songPrice = 25;
    storeStorage[0].songID = 2000;
    storeStorage[1].songName = "Archive - Bullets";
    storeStorage[1].songPrice = 25;
    storeStorage[1].songID = 2001;
    storeStorage[2].songName = "Swedish House Mafia - Don't you worry child";
    storeStorage[2].songPrice = 25;
    storeStorage[2].songID = 2002;
    storeStorage[3].songName = "Roykossop - Running to the sea";
    storeStorage[3].songPrice = 25;
    storeStorage[3].songID = 2003;
    storeStorage[4].songName = "French Teen Idol - Shouting can have different meanings";
    storeStorage[4].songPrice = 25;
    storeStorage[4].songID = 2004;



    do
    {
        int userOperationChoice;
        int userPurchaseQuery;

        cout << "Hello and Welcome to the Dyces Song Library. \n\nPlease select one of the operations below. \n\n 1 - View your own library \n\n 2 - View the Store Library \n\n 3 - View your transactions. \n\n 4 - Check Bank Details \n\n Selection:";
        cin >> userOperationChoice;

        switch (userOperationChoice)
        {
        case(1) :
        {
                    goto userLibrary;
                    break;
        }
        case(2) :
        {
                    goto storeLibrary;
                    break;
        }
        case(3) :
        {
                    goto transactionLibrary;
                    break;
        }
        case(4) :
        {
                    goto bankDetails;
                    break;
        }
        }


    userLibrary:

        system("CLS");
        cout << "\nThe current songs in your library are: \n\n";
        userStorageDisplay();

        goto programEnd;


    storeLibrary:

        system("CLS");
        cout << "\nThe Store's library is: \n";
        storeStorageDisplay();

        cout << "\nPlease enter the song you wish to purchase. \n";
        cin >> userPurchaseQuery;

        purchase(userPurchaseQuery);
        goto programEnd;


    transactionLibrary:

        system("CLS");
        cout << "Your transactions are: \n";
        transactionStorageDisplay();
        goto programEnd;


    bankDetails:

        char viewTransactionHistory;

        system("CLS");
        cout << "You have: $" << money << " left in your bank account." << endl;

        cout << "\nWould you like to check your transaction history? Y/N" << endl;
        cin >> viewTransactionHistory;

        if (viewTransactionHistory == 'y' || viewTransactionHistory == 'Y')
        {
            goto transactionLibrary;
        }
        goto programEnd;


    programEnd:

        cout << "\nWould you like to go back to the main menu? Y/N" << endl ;
        cin >> rerun;
        system("CLS");

    } while (rerun == 'y' || rerun == 'Y');
}

Any suggestions / reviews are greatly welcome!

share|improve this question
    
I'll take a better look at this later, but I can say this for now: global variables and goto are bad. Any future answers should address that. –  Jamal Jan 5 at 3:18
    
@Jamal Yes, I've heard about them too. Personally, I think I didn't use goto to a great extent, just to make the code a bit more readable I guess. I'm trying to find a workaround for the global variable –  Mayur Mohan Jan 5 at 3:22
2  
You really need to revise your taste in music ;-) –  rolfl Jan 5 at 4:23
3  
There's quite a few things that compiler warnings can catch on this. It's typically a good idea to crank warnings up as high as possible. For g++, I tend to use the flags -Wall -Wextra -Weffc++ -Wstrict-aliasing -pedantic as a minimum. I'm not very familiar with the flags on any other compilers unfortunately :) –  Corbin Jan 5 at 5:21
1  
@Corbin: I'm almost sure they (gender neutral pronoun) use Visual C++, as there is CLS, and void main (that none of Linux compilers accept). Enable at least /W4 in project settings (or even /Wall, but this will be really strict - but that's good in my opinion). Fix all warnings, this way you will learn to code properly. –  xfix Jan 5 at 8:59
show 4 more comments

4 Answers 4

Jamal and syb0rg have covered things well, but I have a few things to add.


Globals are rather harmful to programs:

  • They essentially murder any kind of independence any code that uses them could have had
  • They encourage mysterious-side-effect based programming
  • They can create annoying-to-manage namespace pollution

Imagine down the road if you wanted to change your program to handle 2 users' libraries (or better yet, n users' libraries). How do you do that? Well, the obvious first step is to have 2 sets of arrays instead of a single set of arrays. Simple enough. But wait... Your functions all operate on a global variable. The rather nasty solution then is to just do something like:

struct simple { int x; }

simple global_simple;

void double_x() {
    global_simple.x *= 2;
}

void some_code() {
    simple some_simple = {3};
    simple some_other_simple = {5};

    global_simple = some_simple;
    double_x();
    some_simple = global_simple;

    global_simple = some_other_simple;
    double_x();
    some_other_simple = global_simple;
}

This has a few very bad problems though:

  • It directly couples double_x to something. That defeats the purpose of a function.
    • Functions are meant to be little black boxes of code that take one or more things in, operate on them, and synthesize some kind of result
  • It's extremely error prone since it relies on a human remembering to always do the proper assignments
  • It requires a performance hit for no reason due to the constant assignments

Consider this instead:

void double_x(simple& s) {
    s.x *= 2;
}

void some_code() {
    simple some_simple = {3};
    simple some_other_simple = {5};

    double_x(some_simple);
    double_x(some_other_simple);
}

It's clearer, it's less error prone, and it has higher performance. Yes, for the singular version, you do have to type an annoying extra some_simple rather than just double_x(), but the benefits far outweigh that one tiny drawback.

Note: As Loki Astari pointed out, this only applies to mutable globals. Immutable globals (sometimes called constants) are usually quite useful and a different beast altogether (with their own, smaller, set of concerns). The reason globals are bad is that they can be changed from anywhere. Taking away the ability to change them negates that concern.


When using non-standard C or C++, I like to abstract it away into a wrapper function. For example, your system("CLS") is platform specific in that the cls command is specific to the Windows command line (it's clear on sh based systems).

I would do something like this (untested):

void clear_console() {
    #ifdef _WIN32
    std::system("cls");
    #elif linux
    std::system("clear");
    #else
    #error Unknown system
    #endif
}

This means that your program can now work on more than one system. More importantly though, it centralizes your dependency. Let's say you want to add support for yet another operating system. You could either Control + F to find all system("cls") and hope you didn't miss any corner cases, or you could go to your one function for it and add it.

Also, this avoids code duplication since some OS specific code can be a lot more than 1 function call.

For what it's worth by the way, this is a very (very, very, very) simplified version of what any kind of cross platform library does. Abstract away the platform specificity enough, and your non-utility code will never have to worry about whether it's running on Windows, linux, Mac, or a calculator.

Then again, it's always a valid decision to decide "you know, we don't really care about any system/environment other than X." Just make sure that you really, really mean it :).


There's no point to have a switch that all it does is execute a jump. Just put your content inside of the switch directly. It has the exact same effect. There are a few legitimate uses of goto, but this is not one of them. (And really the usefulness of goto in C++ rather than C is questionable.)


Your switch doesn't cover every case. What if the user enters 5? You should likely have a default fall through case that spits out some kind of error.


case (x) sticks out a bit as odd. Just do case x. It's not a big deal though if (x) is your preference. It's just non-common.


Instead of handling each cases' code in main, I would extract it into functions. Each function should have one and only one responsibility, and main is no exception. main should typically be responsible for handling program flow, not containing program flow.


I would be tempted to use std::vector instead of an array for this. That way you don't have the awkwardness of using songID as a sentinel value.


Existing reviews have touched on this, but I shall reiterate.

if (songID == NULL) is very wrong. Never compare a non-pointer to NULL. NULL is not 0. Ok, it may be technically defined as 0. Semantically though, it is much more. It is a pointer that does not point to anything. 0 is not a pointer, so their comparison makes no sense. Also, if you're using C++11, nullptr should be preferred over NULL.


Logic and input should be completely separated. Asking a user about a purchase to be made and excuting a purchase are two separate concerns. Imagine if this were being used for a real record store. Now imagine that they want a feature where customers can sign up to buy a new album as soon as it's released (basically a pre-order system). Ok, great. You've already got the order processing done. Easy! But wait... Your ordering process expects a lot of user input. How do you schedule that? Suddenly someone has to be there to actually enter the orders since your program prompts them.

What if the input of required information were handled separately from the ordering process? Suddenly your ordering process no longer depends on the input of the information and the information. It only depends on the information. This allows you to get the information from wherever you want and pass it on to the actual processing. It's no longer required that the actual processing also handle the input.

It's always possible to have your code do the input grabbing and then processing if those two are handled separately. It's not possible to do only one of two if the two are tied together though. This is why it's crucial to only do one thing. This is the basic crux of the single responsibility principle (though the SRP was formulated and is typically applied in a object oriented context).

share|improve this answer
1  
+1 phenomenal review. I especially liked the review of the design, not just the syntax. I'm glad someone tackled the global issue, too. ;-) –  Jamal Jan 5 at 6:52
2  
Lets distinguish between global mutable and global immutable state. global const variables are fine. The trouble is the term variable it implies that an object can change state but this is not always the case. –  Loki Astari Jan 6 at 21:54
    
@LokiAstari Very good point. I got caught up in the global witch hunt and forgot that not all globals are mutable :). I'm too used to the "constants" terminology. I will update in a second. –  Corbin Jan 6 at 23:33
add comment

This is my first time reviewing C++ code! I'm may be a bit harsh since this is for a final grade.


There is a place in your code where one of your methods could reach then end of a non-void function, and doesn't return anything. This was actually an error for me, and the code would not compile until I fixed it (maybe this is due to my strict compiler).

int checkBankDetails(int songNumber)
{
    if (storeStorage[songNumber].songPrice > money)
    {
        return 1;
    }
    return money;  // This needs to be here in case that if condition fails
}

You should never declare main() so that it doesn't return a value. This is not a "proper declaration". The return value for main() should indicate how the program exited. Normal exit is generally represented by a 0 return value from main(). Abnormal termination is usually signalled by a non-zero return but there is no standard for how non-zero codes are interpreted. Also, void main() is explicitly prohibited by the C++ standard and shouldn't be used.

int main()
{
    ...
    return 0;
}

I rarely would use gotos. There is almost always a better option, and extreme use of them can make your code spaghetti code. In your program I see no purpose to keeping them around.


Don't use using namespace std.


There is a consistent magic-number 30 lurking around your code. Define it somewhere and use the constant in the magic-number's place.

static int const ARRAYLENGTH = 30

To end a loop, you should break from it. You shouldn't be setting a variable like the way you are now to get out of it (there are certain conditions where this is not true, such as game loops, but that does not apply here).

i = 30; // Not the best way to break from a loop here.
break; // Use this

I'm getting a lot of warnings about:

Comparison between NULL and non-pointer ('int' and NULL)

Let's get rid of those by comparing them to 0. NULL is actually defined as 0 in C++, so this is the same comparison minus the warning.


Going off of the last point, you have some comparisons that could be simplified.

if (storeStorage[i].songID != 0)
if (storeStorage[i].songID) // Does the same thing

If you think about it, all if conditional tests will work if the value is not 0, so testing if something is not equal to 0 is redundant and unneeded.


You have an unused variable moneyBalance


You include the library <string.h> unnecessarily.


Final code:

#include <iostream>
#include <string>

static int const ARRAYLENGTH = 30

int money = 5000; // Global Variable

struct database
{
    std::string songName;
    int songPrice;
    int songNumber;
    int songID;
} userStorage[ARRAYLENGTH], storeStorage[ARRAYLENGTH];

struct transaction
{
    std::string songName;
    int songPrice;
}transactionStorage[ARRAYLENGTH];

void userStorageDisplay()
{
    for (int i = 0; i <= ARRAYLENGTH; i++)
    {
        if (userStorage[i].songID)
        {
            std::cout << "\n" << i << ". " << userStorage[i].songName << "\n" << std::endl;
        }
    }
}

void storeStorageDisplay()
{
    for (int i = 0; i <= ARRAYLENGTH; i++)
    {
        if (storeStorage[i].songID)
        {
            std::cout << "\n" << i << ". " << storeStorage[i].songName << std::endl;
            std::cout << "\t" << "Price: $" << storeStorage[i].songPrice << "\n" << std::endl;
        }
    }
}

void transactionStorageDisplay()
{
    for (int i = 0; i < ARRAYLENGTH; i++)
    {
        if (transactionStorage[i].songPrice)
        {
            std::cout << "\n" << transactionStorage[i].songName << " $:" << transactionStorage[i].songPrice << "\n" << std::endl;
        }
    }
}

int checkDuplicate(int songStorageValue)
{
    for (int j = 0; j <= ARRAYLENGTH; j++)
    {
        if (userStorage[j].songID == storeStorage[songStorageValue].songID)
        {
            return 0;
        }
    }
    return 1;
}

int checkBankDetails(int songNumber)
{
    if (storeStorage[songNumber].songPrice > money)
    {
        return 1;
    }
    return money;
}

void purchase(int songNumber)
{
    int verification;

    std::cout << "\nAre you sure you wish to purchase: " << storeStorage[songNumber].songName << " for a price of: " << storeStorage[songNumber].songPrice << " ?" << std::endl;
    std::cout << "Press 1 to confirm or 2 to exit." << std::endl;
    std::cin >> verification;

    if (verification == 1)
    {
        if (checkBankDetails(songNumber) != 1)
        {

            if (checkDuplicate(songNumber) == 1)
            {
                for (int i = 0; i <= ARRAYLENGTH; i++)
                {
                    if (userStorage[i].songID == 0)
                    {
                        userStorage[i].songName = storeStorage[songNumber].songName;
                        userStorage[i].songNumber = i;
                        userStorage[i].songPrice = storeStorage[songNumber].songPrice;
                        userStorage[i].songID = storeStorage[songNumber].songID;

                        for (int a = 0; a <= ARRAYLENGTH; a++)
                        {
                            if (transactionStorage[a].songPrice == 0)
                            {
                                transactionStorage[a].songName = storeStorage[songNumber].songName;
                                transactionStorage[a].songPrice = storeStorage[songNumber].songPrice;

                                money = money - transactionStorage[a].songPrice;

                                std::cout << "\nRemaining Value: $" << money << "\n";

                                break;
                            }
                        }

                        std::cout << "\n" << storeStorage[songNumber].songName << " has been added to your library at position number: " << i << "\n" << std::endl;
                        break;
                    }
                }
            }
            else if (checkDuplicate(songNumber) == 0)
            {
                std::cout << "\nYou already own this song. Purchase Cancelled." << std::endl;
            }
        }
        else if (checkBankDetails(songNumber) == 1)
        {
            std::cout << "\nInsufficient funds. Purchase cancelled." << std::endl;
        }
    }
    else if (verification != 1)
    {
        std::cout << "Transaction Aborted.";
    }
}

int main()
{

    char rerun;

    //Store Data added for demonstration purposes.

    storeStorage[0].songName = "Daughter - Youth";
    storeStorage[0].songPrice = 25;
    storeStorage[0].songID = 2000;
    storeStorage[1].songName = "Archive - Bullets";
    storeStorage[1].songPrice = 25;
    storeStorage[1].songID = 2001;
    storeStorage[2].songName = "Swedish House Mafia - Don't you worry child";
    storeStorage[2].songPrice = 25;
    storeStorage[2].songID = 2002;
    storeStorage[3].songName = "Roykossop - Running to the sea";
    storeStorage[3].songPrice = 25;
    storeStorage[3].songID = 2003;
    storeStorage[4].songName = "French Teen Idol - Shouting can have different meanings";
    storeStorage[4].songPrice = 25;
    storeStorage[4].songID = 2004;



    do
    {
        int userOperationChoice;
        int userPurchaseQuery;

        std::cout << "Hello and Welcome to the Dyces Song Library. \n\nPlease select one of the operations below. \n\n 1 - View your own library \n\n 2 - View the Store Library \n\n 3 - View your transactions. \n\n 4 - Check Bank Details \n\n Selection:";
        std::cin >> userOperationChoice;

        switch (userOperationChoice)
        {
            case(1) :
            {
                system("CLS");
                std::cout << "\nThe current songs in your library are: \n\n";
                userStorageDisplay();
                break;
            }
            case(2) :
            {
                system("CLS");
                std::cout << "\nThe Store's library is: \n";
                storeStorageDisplay();

                std::cout << "\nPlease enter the song you wish to purchase. \n";
                std::cin >> userPurchaseQuery;

                purchase(userPurchaseQuery);
                break;
            }
            case(3) :
            {
                system("CLS");
                std::cout << "Your transactions are: \n";
                transactionStorageDisplay();
                break;
            }
            case(4) :
            {
                char viewTransactionHistory;

                system("CLS");
                std::cout << "You have: $" << money << " left in your bank account." << std::endl;

                std::cout << "\nWould you like to check your transaction history? Y/N" << std::endl;
                std::cin >> viewTransactionHistory;

                if (viewTransactionHistory == 'y' || viewTransactionHistory == 'Y')
                {
                    std::cout << "Your transactions are: \n";
                    transactionStorageDisplay();
                }
                break;
            }
        }


        std::cout << "\nWould you like to go back to the main menu? Y/N" << std::endl;
        std::cin >> rerun;
        system("CLS");

    } while (rerun == 'y' || rerun == 'Y');
    return 0;
}
share|improve this answer
3  
NULL is actually often defined as (void*)0. Prefer using nullptr when comparing to a pointer in C++11. –  Yuushi Jan 5 at 4:12
add comment
  • Prefer to initialize a struct member by calling its constructor directly:

    struct S
    {
        int number;
        std::string string;
    } structs[2];
    
    structs[0] = { 1, "hello" };
    structs[1] = { 2, "world" };
    

    Note that the S is capitalized for a reason. This is because it's a custom type, which should be capitalized as per naming convention. Instances, on the other hand, should start with a lowercase.

  • Prefer this type of pattern when using for loops in C++:

    for (int i = 0; i < 5; i++) { }
    

    This loop will still iterate five times (i = 0 to i = 4).

  • If your check functions just return 0 or 1, have them return a bool instead:

    bool areSameNumbers()
    {
        int a = 5;
        int b = 10;
    
        if (a == b)
            return true;
        else
            return false;
    }
    

    I should also note that this particular method was shown just to illustrate the true and false keywords. I'd recommend the below form, which is much shorter and also preferred:

    bool areSameNumbers()
    {
        int a = 5;
        int b = 10;
    
        return a == b;
    }
    
  • Consider using while loops instead of do-while as the former is more readable. That wouldn't work too well with the loop contents you have now, but it's still worth keeping in mind.

share|improve this answer
    
Keep in mind that do-while loops will 1 or more times, where as while loops will run 0 or more times. –  syb0rg Jan 5 at 4:32
1  
@syb0rg: I know. There are workarounds to that, although I haven't mentioned anything specific. I've mostly went the "example route" here. –  Jamal Jan 5 at 4:33
add comment

Let's analyze this line by line, because there are lots of issues. Generally, problems involve globals (pass class instances instead when they are needed), and goto (no, I'm not against goto - I sometimes use it (to make code more readable), but in this case, you simply tried to replace function calls with goto).

/* dycesM */

#include <iostream>
#include <string>
#include <string.h>

Generally, in C++, it's preferred to use <cstring> instead of <string.h> from C.

using namespace std;

Using using namespace std is generally considered harmful.

int money = 5000; // Global Variable

Globals are generally a bad idea, and try to avoid them if possible. Instead, make class or struct called user_t (or something).

struct database
{
    string songName;
    int songPrice;
    int songNumber;
    int songID;
}userStorage[30], storeStorage[30];

That one is a tricky code. For me, declaring struct, at variables looks tricky. Instead, make a statement just declaring a struct, and move userStorage to user_t class, and storeStorage to store_t class. This class also needs a better name, as database could be anything. I would rename this to song_data_t, or something like that. Also, using C arrays is usually bad idea, as they cannot expand. Instead use std::set class from <set> (as it allows easy storage of values (without stupid limits), and checking if they exist in O(1) time). Also, songID gives me feeling of metadata that simply isn't needed.

struct transaction
{
    string songName;
    int songPrice;
}transactionStorage[30];

In my opinion, this should also use song_t type. It would use additional 4 bytes for transaction, but it doesn't matter much. Move this to store_t class.

void userStorageDisplay()

This method is duplicated multiple times. Instead, make a method inherited from storage_t for every storage class.

{
    for (int i = 0; i <= 30; i++)

Don't hardcode 30. Instead, store data properly, and use for each loop.

    {
        if (userStorage[i].songID != NULL)

songID is not a NULL pointer, so it's completely wrong to use it. Many C++ compilers compile NULL to 0 (because void pointers are useless in C++), but using NULL here doesn't mean it's valid semantically. Many C compilers would refuse this code, but this is C++...

        {
            cout << "\n" << i << ". " << userStorage[i].songName << "\n" << endl;

Oh, wow. If I read this correctly, you are outputing new line three times. You could use << "\n\n" instead. Well, it's just strange in my opinion. Also, in my opinion, outputing "\n" at beginning of the line feels strange.

        }
    }
}

void storeStorageDisplay()

Duplicate code is not a good idea. Make a class.

{
    for (int i = 0; i <= 30; i++)

Again, change this to for each loop.

    {
        if (storeStorage[i].songID != NULL)
        {
            cout << "\n" << i << ". " << storeStorage[i].songName << endl;
            cout << "\t" << "Price: $" << storeStorage[i].songPrice << "\n" << endl;
        }
    }
}

void transactionStorageDisplay()
{
    for (int i = 0; i < 30; i++)
    {
        if (transactionStorage[i].songPrice != NULL)
        {
            cout << "\n" << transactionStorage[i].songName << " $:" << transactionStorage[i].songPrice << "\n" << endl;
        }
    }
}

int checkDuplicate(int songStorageValue)

Move this to user_storage_t class. Also, functions returning booleans should return bool.

{
    for (int j = 0; j <= 30; j++)

With std::set, you can use find method.

    {
        if (userStorage[j].songID == storeStorage[songStorageValue].songID)
        {
            return 0;
        }
    }
    return 1;
}

int checkBankDetails(int songNumber)

Make this user_t method that returns bool, and takes song_t as argument.

{
    if (storeStorage[songNumber].songPrice > money)
    {
        return 1;
    }
}

void purchase(int songNumber)

Make this shop_t method, and give it song_t, and user_t instance. Rename it to ask_for_purchase, or something.

{
    int verification;
    int &moneyBalance = money;

    cout << "\nAre you sure you wish to purchase: " << storeStorage[songNumber].songName << " for a price of: " <<

storeStorage[songNumber].songPrice << " ?" << endl; cout << "Press 1 to confirm or 2 to exit." << endl; cin >> verification;

    if (verification == 1)
    {
        if (checkBankDetails(songNumber) != 1)

Move this to verify_purchase method.

        {

            if (checkDuplicate(songNumber) == 1)
            {
                for (int i = 0; i <= 30; i++)
                {
                    if (userStorage[i].songID == NULL)
                    {
                        userStorage[i].songName = storeStorage[songNumber].songName;
                        userStorage[i].songNumber = i;
                        userStorage[i].songPrice = storeStorage[songNumber].songPrice;
                        userStorage[i].songID = storeStorage[songNumber].songID;

Assign struct directly, instead of reassigning directly. structs are valid types to assign, you don't have to move every property arround. But I would move this code to insert method anyway.

                        for (int a = 0; a <= 30; a++)
                        {
                            if (transactionStorage[a].songPrice == NULL)
                            {
                                 transactionStorage[a].songName = storeStorage[songNumber].songName;
                                 transactionStorage[a].songPrice = storeStorage[songNumber].songPrice;

Again, assign a struct, and use insert method for storage.

                                money = money - transactionStorage[a].songPrice;

                                cout << "\nRemaining Value: $" << money << "\n";

                                a = 30; // To end the loop. 
                            }
                        }

                        cout << "\n" << storeStorage[songNumber].songName << " has been added to your library at position number: " << i << "\n" << endl;

Again, a long statement, but I'm sure that if you would properly move stuff to methods, it would get shorter.

                        i = 30; // To end the loop.
                    }
                }
            }
            else if (checkDuplicate(songNumber) == 0)
            {
                cout << "\nYou already own this song. Purchase Cancelled." << endl;
            }
        }
        else if (checkBankDetails(songNumber) == 1)
        {
            cout << "\nInsufficient funds. Purchase cancelled." << endl;
        }
    }
    else if (verification != 1)
    {
        cout << "Transaction Aborted.";
    }
    }

void main()

This should be int main. I'm surprised that your compiler accepts such nonsense, as both g++ and clang++ would fail compilation here. C++ specification also requires to stop the compilation. In C++, unlike C, main() is a special method.

{

    char rerun;

    //Store Data added for demonstration purposes. 

    storeStorage[0].songName = "Daughter - Youth";
    storeStorage[0].songPrice = 25;
    storeStorage[0].songID = 2000;
    storeStorage[1].songName = "Archive - Bullets";
    storeStorage[1].songPrice = 25;
    storeStorage[1].songID = 2001;
    storeStorage[2].songName = "Swedish House Mafia - Don't you worry child";
    storeStorage[2].songPrice = 25;
    storeStorage[2].songID = 2002;
    storeStorage[3].songName = "Roykossop - Running to the sea";
    storeStorage[3].songPrice = 25;
    storeStorage[3].songID = 2003;
    storeStorage[4].songName = "French Teen Idol - Shouting can have different meanings";
    storeStorage[4].songPrice = 25;
    storeStorage[4].songID = 2004;

Use song_t constructor here, to remove duplication.

    do
    {
        int userOperationChoice;
        int userPurchaseQuery;

        cout << "Hello and Welcome to the Dyces Song Library. \n\nPlease select one of the operations below. \n\n 1 - View your own library \n\n 2 - View the Store Library \n\n 3 - View your transactions. \n\n 4 - Check Bank Details \n\n Selection:";

This is a long message. So long that Stack Exchange formatter moved it to multiple lines (incorrectly) after pressing quote button. Use string concatenation here, and split it between several lines.

        cin >> userOperationChoice;

        switch (userOperationChoice)
        {
        case(1) :

case statement doesn't require braces or parenthesis.

        {
                    goto userLibrary;

This design pattern is called "I'm too cool for a function". Just use a function. I'm not against goto (I'm one of those rare programmers who think that goto is sometimes useful), but it's not a case here. This can be easily replaced by a function.

                    break;
        }
        case(2) :
        {
                    goto storeLibrary;
                    break;
        }
        case(3) :
        {
                    goto transactionLibrary;
                    break;
        }
        case(4) :
        {
                    goto bankDetails;
                    break;
        }

There is no error handling. Make a default statement, or the flow will go to userLibrary automatically.

        }


    userLibrary:

        system("CLS");

This is not portable. Then again, considering this is Windows, well. I guess it's fine.

        cout << "\nThe current songs in your library are: \n\n";
        userStorageDisplay();

        goto programEnd;


    storeLibrary:

        system("CLS");
        cout << "\nThe Store's library is: \n";
        storeStorageDisplay();

        cout << "\nPlease enter the song you wish to purchase. \n";
        cin >> userPurchaseQuery;

        purchase(userPurchaseQuery);
        goto programEnd;


    transactionLibrary:

        system("CLS");
        cout << "Your transactions are: \n";
        transactionStorageDisplay();
        goto programEnd;


    bankDetails:

        char viewTransactionHistory;

        system("CLS");
        cout << "You have: $" << money << " left in your bank account." << endl;

        cout << "\nWould you like to check your transaction history? Y/N" << endl;
        cin >> viewTransactionHistory;

        if (viewTransactionHistory == 'y' || viewTransactionHistory == 'Y')
        {
            goto transactionLibrary;
        }
        goto programEnd;


    programEnd:

        cout << "\nWould you like to go back to the main menu? Y/N" << endl ;
        cin >> rerun;
        system("CLS");

    } while (rerun == 'y' || rerun == 'Y');
}
share|improve this answer
1  
Only <string.h> is unneeded; <string> should stay. Also, _t is reserved. –  Jamal Jan 5 at 17:18
    
@Jamal _t (as a name beginning with an underscore) is only reserved as a name in the global namespace. Names containing __ (double underscore) or beginning with an underscore followed by an upper-case letter are reserved to the implementation for all purposes. Names like storage_t that contain _t are not reserved (unless they run afoul of one of the other rules listed, or one I haven't mentioned -- see [reserved.names] which is 17.6.4.3 in the C++11 standard). –  ruds Jan 7 at 3:29
    
@ruds: Hm. I once used _t is one of my programs, and was told not to do so. I would refrain from using it in my own programs anyway. –  Jamal Jan 7 at 3:33
    
This _t usage appears to be controversial. I decided to ask for this at codereview.stackexchange.com/questions/38733/…. –  xfix Jan 7 at 6:36
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.