Trying to understand what is bad about my code...

[awilford0]

So my professor for a beginners c++ class docked me 2 points for a quiz to write a program based on a given 2D array.

He gave us that our array had to be initialized with the following numbers:

{ {75000, 30200, 67800, 45000, 6000, 67500}, {4000, 75000, 64000, 32600, 47800, 39000} }

and said our output needed to look like this:

Total Domestic Sales: $291500
Total International Sales: $262400
Total Company Sales: $553900
January Sales: $79000
February Sales: $105200
March Sales: $131800
April Sales: $77600
May Sales: $53800
June Sales: $106500

He told us specifically we could NOT use nested for loops but we were
allowed to use multiple loops.

He graded me and took off two points for the below saying that it was "bad coding":

for (j = 0; j < ROW; j++)
{
janSales += sales[j][0];
febSales += sales[j][1];
marSales += sales[j][2];
aprSales += sales[j][3];
maySales += sales[j][4];
juneSales += sales[j][5];
}

could anyone explain to me why this might be? because he won't give me any
clearer specifics of why it is bad code. My program ran exactly as he asked,
he was very vague about how we were supposed to go about coding the program, so this was my solution that didn't have me adding up each individual index location.

Thanks in advance!

#include <iostream>
using namespace std;

const int COL = 6;
const int ROW = 2;

int sales[ROW][COL] = { {75000, 30200, 67800, 45000, 6000, 67500}, {4000, 75000, 64000, 32600, 47800, 39000} };

void displaySales(int array[][COL], int ROW, int COL);

int main()

{
	displaySales(sales, ROW, COL);
	
	return 0;
}

void displaySales(int array[][COL], int ROW, int COL)
{
	int i = 0;
	int j = 0;
	int domesticSum = 0;
	int internationalSum = 0;
	int janSales = 0;
	int febSales = 0;
	int marSales = 0;
	int aprSales = 0;
	int maySales = 0;
	int juneSales = 0;

	for (i = 0; i < COL; i++)
	{
		domesticSum += sales[0][i];
		internationalSum += sales[1][i];

	}

	for (j = 0; j < ROW; j++)
	{
		janSales += sales[j][0];
		febSales += sales[j][1];
		marSales += sales[j][2];
		aprSales += sales[j][3];
		maySales += sales[j][4];
		juneSales += sales[j][5];
	}

	cout << "Total Domestic Sales: " << "\t\t" << "$" << domesticSum << endl;
	cout << "Total International Sales: " << "\t" << "$" << internationalSum << endl;
	cout << "Total Company Sales: " << "\t\t" << "$" << domesticSum + internationalSum << endl;
	cout << "January Sales: " << "\t\t\t" << "$" << janSales << endl;
	cout << "February Sales: " << "\t\t" << "$" << febSales << endl;
	cout << "March Sales: " << "\t\t\t" << "$" << marSales << endl;
	cout << "April Sales: " << "\t\t\t" << "$" << aprSales << endl;
	cout << "May Sales: " << "\t\t\t" << "$" << maySales << endl;
	cout << "June Sales: " << "\t\t\t" << "$" << juneSales << endl;
}

[Duthomhas]

Have you studied functions yet?

Wait a bit and see if he doesn't review another way of looking at loops with the class. Sometimes patience is the answer. 🙂

[jonnin]

for (j = 0; j < ROW; j++)
{
janSales += sales[j][0];
febSales += sales[j][1];
marSales += sales[j][2];
aprSales += sales[j][3];
maySales += sales[j][4];
juneSales += sales[j][5];
}

could be written a lot better using what you know and are allowed.
consider:
double monthlysales[12]{0};
const int jan = 0;
const int feb = 1; ///all the months. a type called enum would do here, if you know it yet?

sec, messed the loop up.
for (i = 0, j = 0; j < ROW; )
{
monthlysales[i] += sales[j][i];
i = (i+1)%6; //getting funky with all the indexing because of the dumb requirement.
if(i == 0) j++;
}
then later
cout << monthlysales[jan] or whatever
and better, if you had a list of strings, do you know them?
string months[12]{"january ", ... fill it in .. notice the extra space, if you want it!
the print becomes a loop of:
cout << months[j] << "moar words " << monthlysales[j] << "whatever" << endl;


basically, it comes down to eliminating repeated code where you could have looped with only limited tools. Which is tricky to do because you can't nest loops or functions, so it got a little weird.

[zapshe]

I wouldn't worry to much about that. The only "bad" things about that is that its more code than needed (but if you couldn't use nested loops, then there aren't many other alternatives) and, more importantly, using hardcoded numbers (your 0-5 accessing elements in "sales") is considered bad practice.

Its bad practice because usually arrays aren't static like you've used them, and tend to change in size based on the input the program is given or even during run-time.

However, seeing how you have COL and ROWS hardcoded already as 6 and 2, I don't see any reason to deduct points for simply making logical use of the fact that the array is always the same size.


You should learn C++ on your own time as well, don't be reliant on professors. People tend to drop out early in this major because so many professors just can't teach. And they're honestly might not be expected to be good teachers by the university. Universities tend to make introductory classes difficult on purpose in order to "weed out" the weak.



EDIT: If you're questioning why the professor graded you a certain way and they're being vague about the reasoning, they might not fully agree with the grade themselves. Also, make sure that you're correct that the professor is grading you and not some nameless TA.

[salem c]

Docking 2 points, he was being generous.

> void displaySales(int array[][COL], int ROW, int COL)
1. You completely ignore the array parameter and go straight for the global variable sales.
Declare your array inside main, then see if it still works.

2. Your array size parameters have the same names as your global constants.
This is called shadowing, and it leads to strange bugs where you expect one thing and get another thing with the same name.

$ g++ -Wall -Wextra -Wshadow baz.cpp
baz.cpp: In function ‘void displaySales(int (*)[6], int, int)’:
baz.cpp:19:53: warning: declaration of ‘COL’ shadows a global declaration [-Wshadow]
 void displaySales(int array[][COL], int ROW, int COL)
                                                     ^
baz.cpp:4:11: note: shadowed declaration is here
 const int COL = 6;
           ^
baz.cpp:19:53: warning: declaration of ‘ROW’ shadows a global declaration [-Wshadow]
 void displaySales(int array[][COL], int ROW, int COL)
                                                     ^
baz.cpp:5:11: note: shadowed declaration is here
 const int ROW = 2;
           ^
baz.cpp: At global scope:
baz.cpp:19:34: warning: unused parameter ‘array’ [-Wunused-parameter]
 void displaySales(int array[][COL], int ROW, int COL)

[zapshe]

While what you said is true salem, he claims the points were taken off for the specific code inside that one loop.


Also, we don't know how much code was his and which was given to him.


However, for reference, here is a better way to write that program:

//

#include <iostream>
#include <string>

const int COL = 6;
const int ROW = 2;

void displaySales(int array[][COL], int ROW, int COL);

int main()
{
	//Not global anymore - its safer in here!
	int sales[ROW][COL] = { {75000, 30200, 67800, 45000, 6000, 67500}, {4000, 75000, 64000, 32600, 47800, 39000} };

	displaySales(sales, ROW, COL);

	return 0;
}

//Changed "array" to "arr" since array is a keyword - don't want to confuse ourselves
void displaySales(int arr[][COL], int ROW, int COL)
{
	int domesticSum = 0;
	int internationalSum = 0;

	const int SIZE = 6;
	int values[SIZE] = { 0 };

	std::string months[SIZE] = { "January", "February", "March", "April", "May", "June" };

	//Loop variables are local to the loop since they arent needed outside
	for (int i = 0; i < COL; i++)
	{
		domesticSum += arr[0][i];
		internationalSum += arr[1][i];
	}

	//I know - no nested loops - but just for reference
	for (int i = 0; i < ROW; i++)
	{
		for (int j = 0; j < COL; j++)
		{
			for (int x = 0; x < SIZE; x++)
			{
				values[x] += arr[i][j];
			}
		}
	}

	std::cout << "Total Domestic Sales: " << "\t\t" << "$" << domesticSum << std::endl;
	std::cout << "Total International Sales: " << "\t" << "$" << internationalSum << std::endl;
	std::cout << "Total Company Sales: " << "\t\t" << "$" << domesticSum + internationalSum << std::endl;

	//Turn all those output lines to one clean loop
	for (int i = 0; i < SIZE; i++)
	{
		std::cout << months[i] << " Sales: " << (i != 1 ? "\t\t\t":"\t\t") << "$" << values[i] << std::endl;
	}
}

But really, the program still works and it looks like something someone could have coded if they were in a hurry. Plenty of room to learn!

[seeplus]

Perhaps:

#include <iostream>

constexpr size_t COL {6};
constexpr size_t ROW {2};
enum LOCAT {DOM, INTL};

using Array = unsigned [ROW][COL];

constexpr Array sales {{75000, 30200, 67800, 45000, 6000, 67500}, {4000, 75000, 64000, 32600, 47800, 39000}};

void displaySales(const Array arr);

int main()
{
	displaySales(sales);
}

void displaySales(const Array arr)
{
	constexpr const char* const monNames[COL] {"Jan", "Feb", "Mar", "Apr", "May", "Jun"};
	unsigned monSales[COL] {};
	unsigned locSales[ROW] {};

	for (size_t j = 0; j < COL * ROW; ++j) {
		locSales[j / COL] += sales[j / COL][j % COL];
		monSales[j % COL] += sales[j / COL][j % COL];
	}

	std::cout << "Total Domestic Sales: " << "\t\t$" << locSales[DOM] << '\n';
	std::cout << "Total International Sales: " << "\t$" << locSales[INTL] << '\n';
	std::cout << "Total Company Sales: " << "\t\t$" << locSales[DOM] + locSales[INTL] << '\n';

	for (size_t i = 0; i < COL; ++i)
		std::cout << monNames[i] << " sales: \t\t\t$" << monSales[i] << '\n';
}

Total Domestic Sales:           $291500
Total International Sales:      $262400
Total Company Sales:            $553900
Jan sales:                      $79000
Feb sales:                      $105200
Mar sales:                      $131800
Apr sales:                      $77600
May sales:                      $53800
Jun sales:                      $106500

[Handy Andy]

Hello awilford0,

Here is another take on your program:

#include <iostream>
#include <iomanip>
#include <string>

using namespace std;  // <--- Best not to use.

constexpr int ROW{ 2 };  // <--- These can easily be defined in the function. The only place
                         // they are used. But OK if you leave them here.
constexpr int COL{ 6 };

void displaySales();

int main()
{

    displaySales();

    return 0;
}

void displaySales()
{
    constexpr int TWIDTH{ 32 }, NWIDTH{ 7 };
    constexpr int monthlySales[ROW][COL]
    {
        {75000, 30200, 67800, 45000,  6000, 67500},
        { 4000, 75000, 64000, 32600, 47800, 39000}
    };
    const std::string MonthNames[]
    {
        " January Sales:",
        " February Sales:",
        " March Sales:",
        " April Sales:",
        " May Sales:",
        " June Sales:"
    };

    //int i{};  // <--- Best defined in for loop
    int col{};
    int domesticSum{};
    int internationalSum{};
    int monthlySalesArr[COL]{};
    for (int i{}; i < COL; i++)
    {
        domesticSum += monthlySales[0][i];

        internationalSum += monthlySales[1][i];
    }

    while (col < COL)
    {
        int monthlySalesTotal{};

        for (int row{}; row < ROW; row++)  // <--- Best to define the type in the for loop, most of the time.
        {
            monthlySalesTotal += monthlySales[row][col];
        }

        monthlySalesArr[col] = monthlySalesTotal;

        col++;
    }

    cout <<
        "\n"
        " Total Domestic Sales: " << "\t\t" << "$ " << domesticSum << '\n' <<
        " Total International Sales: " << "\t" << "$ " << internationalSum << '\n' <<
        " Total Company Sales: " << "\t\t" << "$ " << domesticSum + internationalSum << "\n\n";

    for (int idx = 0; idx < COL; idx++)
    {
        cout << MonthNames[idx] << "\t\t\t" << "$" << monthlySalesArr[idx] << '\n';
        //cout <<
        //    std::left <<
        //    std::setw(TWIDTH) << MonthNames[idx] << "$" <<
        //    std::right << 
        //    std::setw(NWIDTH) << monthlySalesArr[idx] << '\n';
    }
}

In addition to the problem with your for loop This also addresses other issues.

Defining your array at global level is OK, but as a non constant variable it can be changes by any line of code that follows. It should be a constant variable. Next the only place it is used in in the function, so it is better defined in the function eliminating the need to pass it to the function.

In "main" you have displaySales(sales, ROW, COL);. All these are global variables and do not need passed to the function. The function already has access to them.

In void displaySales(int array[][COL], int ROW, int COL). This makes these local variables to the function. Not the global variables that you are think about.

In the function you said:

He told us specifically we could NOT use nested for loops but we were allowed to use multiple loops.

It can be argued that a "while" loop containing a "for" loop is not a nested for loop even though a "while" loop is another way of writing a "for" loop.

Some tips:
Notice the way I wrote the 2D array. Visually it makes it easier to understand the 2D array and also makes it easier to make changes. Before the figures 6000 and 4000 there is an extra space. Not to worry the compiler ignores these spaces, but it makes it easier to read. And that is the whole point.

For the "cout" statements you do not need a "cout" and an "endl" for each line. Depending on what is in the cout will determine what you will need. If you have just strings in double quotes every line of strings becomes 1 big string. Otherwise you can use the insertion operator (<<) to chain everything together. How you use (<<) and the style is your choice. Look at what I did as an example.

In the last for loop of the function there are 2 ways of writing that "cout" statement. Yours using the (\t) does not always space things out the way that you would want.

Your code output:

 Total Domestic Sales:          $ 291500
 Total International Sales:     $ 262400
 Total Company Sales:           $ 553900

 January Sales:                 $79000
 February Sales:                        $105200
 March Sales:                   $131800
 April Sales:                   $77600
 May Sales:                     $53800
 June Sales:                    $106500

Because "February" is longer than everything else it throws off the (\t)s.

Reversing the comments:

 Total Domestic Sales:          $ 291500
 Total International Sales:     $ 262400
 Total Company Sales:           $ 553900

 January Sales:                 $  79000
 February Sales:                $ 105200
 March Sales:                   $ 131800
 April Sales:                   $  77600
 May Sales:                     $  53800
 June Sales:                    $ 106500

Using the "setw()" gives you a block for the text and aligns the numbers to the right for a nicer looking output. Just an alternative. Also needs the header file "<iomanip>" for the "setw()".

Andy

[againtry]

#include <iostream>

const int COLS{6};

int main()
{
    int sales[][COLS]
    {
        {75000, 30200, 67800, 45000, 6000, 67500}, // DOMESTIC
        {4000, 75000, 64000, 32600, 47800, 39000}  // INTERNATIONAL
    };
    
    
    int total_domestic{0}, total_international{0};
    for(int i = 0; i < COLS; i++)
    {
        total_domestic += sales[0][i];
        total_international += sales[1][i];
    }
    std::cout
    << "     Domestic sales: $" << total_domestic << '\n'
    << "International sales: $" << total_international << '\n'
    << "    Total all sales: $" << total_domestic + total_international << '\n';
    
    
    std::string month[]{"January", "February", "March", "April", "May", "June"};
    int total{0};
    for(int i = 0; i < COLS; i++)
    {
        total = sales[0][i] + sales[1][i];
        std::cout
        << month[i] << " sales: $" << total << '\n';
    }
    
    return 0;
}

     Domestic sales: $291500
International sales: $262400
    Total all sales: $553900
January sales: $79000
February sales: $105200
March sales: $131800
April sales: $77600
May sales: $53800
June sales: $106500
Program ended with exit code: 0