A simple array library program

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
#include
#include
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 money)
{
return 1;
}
}

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

cout > verification;

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

if (checkDuplicate(songNumber) == 1)
{
for (int i = 0; i > 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 > userPurchaseQuery;

purchase(userPurchaseQuery);
goto programEnd;

transactionLibrary:

system("CLS");
cout > viewTransactionHistory;

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

StackExchange Code Review Q#38602, answer score: 5