How Did I do?

[DirtyDan]

I am new to C++, start my class in two weeks. I do have some self taught experience in VB.NET.

To help prepare for my C++ class I have been reading and going thru tutorials. I have also been playing around with some beginner questions on these boards. Someone posted their homework question a week or so ago and I decided to try it out.

The instructions for the homework was posted as a link to a picture but I summed up the instructions as the beginning comment in the code.

How did I do? Is there something that could have been done better?

Thanks

/*
In main() function create 3x3 array (m)
populate array (m) with random numbers from 0....9

pass array (m) to modify() function
modify array (m) based on this:
if odd then *(-1)
if even then *(2)

Send modified (m) to row_sum() function
put summation of each row in modified (m) in new
one dimension array (n) then print (n)
*/

#include <iostream>
#include <string>
#include <cstdlib>
#include <ctime>
using namespace std;

void modify(int m[3][3]);
void row_sum(int m[3][3]);

int main()
{
    int m[3][3];
    srand(time(0));
    for (int iRow = 0; iRow < 3; iRow++)
    {
        for (int iCol = 0; iCol < 3; iCol++)
        {
            m[iRow][iCol] = rand()%10;
        }
    }

    modify(m);
    row_sum(m);
    
}

void modify(int m[3][3])
{
    for (int iRow = 0; iRow < 3; iRow++)
    {
        for (int iCol = 0; iCol < 3; iCol++)
        {
            if (m[iRow][iCol] % 2 == 0) //if is even
            {
                m[iRow][iCol] *= 2;
            } else { //if is odd
                m[iRow][iCol] *= (-1);
            }
        }
    }
}

void row_sum(int m[3][3])
{
    int n[3];
    int rowsum=0;
    for (int iRow = 0; iRow < 3; iRow++)
    {
        for (int iCol = 0; iCol < 3; iCol++)
        {
            rowsum += m[iRow][iCol];
            
            if (iCol == 2) //at the end of the row
            {
                n[iRow] = rowsum;
                rowsum = 0; //reset rowsum
            }
        }
    }
    
    for (int iRow = 0; iRow < 3; iRow++)
    {
        cout << n[iRow] << "\n";
    }
}

[AbstractionAnon]

Looks good. I don't see any obvious problems.

I like your use of iRow and iCol. Much clearer that the usual use of just i and j.

I would tend to initialize rowsum to 0 at line 62 rather than do a post use reset at line 70.
Purely a style issue. Pre-use initialization needs to only be done in one place. Post use reset requires both that the variable is initialized to 0 and is properly reset at the end of the loop. One thing to get right rather than two.

[Peter87]

In the row_sum function you don't need to check for the end of the row (iCol == 2). Instead you could place those lines after the inner loop.

If you define and initialize the rowsum variable at the beginning of the outer loop you could get rid of the "reset rowsum" line.

Instead of assigning the rowsum to the array (n[iRow] = rowsum) you could print it directly right there. Then you don't need the n array, and the loop at the end, anymore.

[DirtyDan]

Thanks for the feedback, I updated the code accordingly...

Peter, the assignment called for populating array n with the rowsums from m and then printing n which is why I did it that way.

/*
In main() function create 3x3 array (m)
populate array (m) with random numbers from 0....9

pass array (m) to modify() function
modify array (m) based on this:
if odd then *(-1)
if even then *(2)

Send modified (m) to row_sum() function
put summation of each row in modified (m) in new
one dimension array (n) then print (n)
*/

#include <iostream>
#include <string>
#include <cstdlib>
#include <ctime>
using namespace std;

void modify(int m[3][3]);
void row_sum(int m[3][3]);

int main()
{
    int m[3][3];
    srand(time(0));
    for (int iRow = 0; iRow < 3; iRow++)
    {
        for (int iCol = 0; iCol < 3; iCol++)
        {
            m[iRow][iCol] = rand()%10;
        }
    }

    modify(m);
    row_sum(m);
    
}

void modify(int m[3][3])
{
    for (int iRow = 0; iRow < 3; iRow++)
    {
        for (int iCol = 0; iCol < 3; iCol++)
        {
            if (m[iRow][iCol] % 2 == 0) //if is even
            {
                m[iRow][iCol] *= 2;
            } else { //if is odd
                m[iRow][iCol] *= (-1);
            }
        }
    }
}

void row_sum(int m[3][3])
{
    int n[3];
    for (int iRow = 0; iRow < 3; iRow++)
    {
        int rowsum=0;
        for (int iCol = 0; iCol < 3; iCol++)
        {
            rowsum += m[iRow][iCol];
        }
        n[iRow] = rowsum;
    }
    
    for (int iRow = 0; iRow < 3; iRow++)
    {
        cout << n[iRow] << "\n";
    }
}

[JLBorges]

#include <iostream>
// #include <string>
#include <cstdlib>
#include <ctime>

// using namespace std; // consider avoiding this at namespace scope
// see: http://www.cplusplus.com/forum/general/72248/#msg385442

const int SIZE = 3 ; // avoid magic numbers
// Avoid spelling literal constants like 42 or 3.14159 in code. They are not
// self-explanatory and complicate maintenance by adding a hard-to-detect form
// of duplication. Use symbolic names and expressions instead
// Names add information and introduce a single point of maintenance; raw numbers
// duplicated throughout a program are anonymous and a maintenance hassle.
// Alexandrescu and Sutter in 'C++ Coding Standards: 101 Rules, Guidelines, and Best Practices'

void modify(int m[SIZE][SIZE]);
// void row_sum(int m[SIZE][SIZE]);
void row_sum( const int m[SIZE][SIZE] ); // const added
// see: https://isocpp.org/wiki/faq/const-correctness

int main()
{
    int m[SIZE][SIZE];
    // srand(time(0));
    std::srand( std::time(nullptr) ) ; // favour nullptr over literal zero
                                       // http://www.stroustrup.com/C++11FAQ.html#nullptr

    // consider using range-based loops
    // see: http://www.stroustrup.com/C++11FAQ.html#for
    // for each row in m, for each int v in that row
    // for( auto& row : m ) for( int& v : row ) v = std::rand()%10;

    for (int iRow = 0; iRow < SIZE; iRow++)
    {
        for (int iCol = 0; iCol < SIZE; iCol++)
        {
            // m[iRow][iCol] = rand()%10;
            m[iRow][iCol] = std::rand()%10;
        }
    }

    modify(m);
    row_sum(m);

}

void modify(int m[SIZE][SIZE])
{
    for (int iRow = 0; iRow < SIZE; iRow++)
    {
        for (int iCol = 0; iCol < SIZE; iCol++)
        {
            if (m[iRow][iCol] % 2 == 0) //if is even
            {
                m[iRow][iCol] *= 2;
            } else { //if is odd // *** this is fine, as long as you use this brace-style
                                 // for every if-else construct, without exception.
                                 // otherwise, use a consistent brace style throughput the program
                m[iRow][iCol] *= (-1);
            }
        }
    }
}

// void row_sum(int m[SIZE][SIZE])
void row_sum( const int m[SIZE][SIZE] ) // const added
{
    // ...
}

[DirtyDan]

thanks for the feedback JLBorges

[Computergeek01]

OP wrote:

Someone posted their homework question a week or so ago and I decided to try it out.

You have a bright future ahead of you.

[DirtyDan]

JLBorges
I am trying to understand range based loops...

If i use
for( auto& row : m ) for( int& v : row ) v = rand()%10;
in main()

why cant i use
for( auto row : m ) for( int v : row ) cout << v << "\n"; ERROR ON THIS LINE
in modify() to print out each item?

I get the following error

In function 'void modify(int (*)[3])':
41:21: error: 'begin' was not declared in this scope
41:21: note: suggested alternatives:
In file included from /usr/include/c++/4.9/bits/basic_string.h:42:0,
from /usr/include/c++/4.9/string:52,
from /usr/include/c++/4.9/bits/locale_classes.h:40,
from /usr/include/c++/4.9/bits/ios_base.h:41,
from /usr/include/c++/4.9/ios:42,
from /usr/include/c++/4.9/ostream:38,
from /usr/include/c++/4.9/iostream:39,
from 15:
/usr/include/c++/4.9/initializer_list:89:5: note: 'std::begin'
begin(initializer_list<_Tp> __ils) noexcept
^
/usr/include/c++/4.9/initializer_list:89:5: note: 'std::begin'
41:21: error: 'end' was not declared in this scope
41:21: note: suggested alternatives:
In file included from /usr/include/c++/4.9/bits/basic_string.h:42:0,
from /usr/include/c++/4.9/string:52,
from /usr/include/c++/4.9/bits/locale_classes.h:40,
from /usr/include/c++/4.9/bits/ios_base.h:41,
from /usr/include/c++/4.9/ios:42,
from /usr/include/c++/4.9/ostream:38,
from /usr/include/c++/4.9/iostream:39,
from 15:
/usr/include/c++/4.9/initializer_list:99:5: note: 'std::end'
end(initializer_list<_Tp> __ils) noexcept
^
/usr/include/c++/4.9/initializer_list:99:5: note: 'std::end'
41:38: error: unable to deduce 'auto&&' from 'row'

[JLBorges]

Before I try to explain: Have you been introduced to pointers? What about references?

[DirtyDan]

no, I don't start class for another two weeks...

I have just been playing around with tutorials and examples ahead of the first day of class.

[JLBorges]

Ok. Leave it at this for now:

Arrays are not copyable objects. This would be an error:

int a[3] = {0} ;
int b[3] = a ; // b is a copy of a 

In main(), int m[SIZE][SIZE] is an array, and we can use the range-based loop on the array.

In void modify( int m[SIZE][SIZE] ) even though m looks like an array, it isn't an array.
m is a pointer, hence the diagnostic: In function 'void modify(int (*)[3])';
and we can't use a range-based loop on a pointer.

For now, the rule is:
We can use a range-based loop on the array in a function where the array is defined (in main).
But we can't use it in other functions where it is passed like void modify( int m[SIZE][SIZE] )

[DirtyDan]

that makes sense, thanks