Can't figure out why only one line of matrix is being read from a file and why I can't pass a 2D array to a function

Question

I've been struggling on this for about an hour now so I'm turning to the almighty entity that is the internet for assistance.

I'm trying to write a program that will A) read a matrix from a txt file in the following format where the first number is the columns (4) and the second number is the rows(3) in the matrix. And each row of numbers corresponds to a row in the matrix.

4 3
1 2 3 4
0 1 2 7
4 1 9 2

and B) calculate the number of ones in the matrix. So the above example would return 3. My code is below.

#include <iostream>
#include <fstream>
#include <string>


using namespace std;

void count_ones(int matrix[][], int rows, int columns)
{

int count = 0;

for(int i = 0; i < rows; i++)
{
        for( int j = 0; j < columns; j++)
        {
                if( matrix[i][j] == 1)
                { count++;}
        }
}

cout << "There are " << count << " ones in this matrix.";
}

int main(int argc, char* argv[])
{

int rows, columns;
string file_name = argv[1];

ifstream reader("m1.txt");

reader >> columns;
reader >> rows;

int matrix[rows][columns];

for(int i = 0; i < rows; i++)
{
        for(int j = 0; j < columns; j++)
        {
          reader >>  matrix[i][j];
        }
}


cout << columns << " " << rows;
cout << endl;
for( int k = 0; k < rows; k++) {
      for( int l = 0; l < columns; l++)
         cout << matrix[k][l] << " ";
      cout << endl;

reader.close();

count_ones(matrix, rows,columns);
return 0;
}
}

Right now I have two issues. The code i'm using to print the matrix I'm reading from the "m1.txt" file is only printing the first two lines and I have absolutely no clue what could be causing this but I'm guessing it has something to do with my ifstream reader.

4 3
1 2 3 4

Secondly, I'm getting a bunch of errors I don't understand when I try to pass my matrix to my count_ones function. I'm not very good with C++ so I would appreciate all the help I can get.

Answer 1

In a comment, you asked

Does anyone have a better way to pass the matrix to the count_ones method?

1. Don't use

   int matrix[rows][columns];
   

   This is not standard C++. It is supported by some compilers as an extension.

2. Use

   std::vector<std::vector<int>> matrix;
   

   You can initialize it with the correct sizes for rows and columns using

   std::vector<std::vector<int>> matrix(rows, std::vector<int>(columns));
   

3. Change the declaration of count_ones to accept a std::vector<std::vector<in>>.

   int count_ones(std::vector<std::vector<in>> const& matrix);
   

   Update its implementation accordingly.

---

Suggestion for improvement

You can avoid the error of putting the closing } in the wrong place by using helper functions to write the matrix to cout.

std::ostream& operator<<(std::ostream& out, std::vector<int> const& row)
{
   for ( int item : row )
      out << item << " ";
   return out;
}

std::ostream& operator<<(std::ostream& out, std::vector<std::vector<int>> const& matrix)
{
   for ( auto const& row : matrix )
      out << row << std::endl;
   return out;
}

and then use

std::cout << matrix;

in main.

Answer 2

You have done a mistake in the last for loop.

for( int k = 0; k < rows; k++) {
      for( int l = 0; l < columns; l++)
         cout << matrix[k][l] << " ";
      cout << endl;

reader.close();

count_ones(matrix, rows,columns);
return 0;
}
}

It should be like this

for( int k = 0; k < rows; k++) {
      for( int l = 0; l < columns; l++)
         cout << matrix[k][l] << " ";
      cout << endl;
}

reader.close();

count_ones(matrix, rows,columns);
return 0;
}

Because of this the outer for loop in your code runs only once and prints only first row of matrix.

Edit: Some more things to correct. You can not use matix[][] as a function parameter, it will through the error multidimensional array must have bounds for all dimensions except the first

You can use double pointer for this work. Change the check ones function declaration to this

void count_ones(int **matrix, int rows, int columns)

replace

int matrix[rows][columns];

with

int **matrix = (int **)malloc(sizeof(int *)*columns);
for(int i=0; i < columns; i++)
    *(matrix + i) = (int *)malloc(sizeof(int)*rows);

and the code should work like a charm. And also remove this line, its redundant as file_name is not being used.

string file_name = argv[1];

Answer 3

So, I don't know what the errors are until you post them, but I have an idea why your output is cut off prematurely.

So, to recap, let's look at your code again (the relevant part":

cout << columns << " " << rows;
cout << endl;
for( int k = 0; k < rows; k++) {
    for( int l = 0; l < columns; l++) /* { */
        cout << matrix[k][l] << " ";
    /* } */
    cout << endl;

    reader.close();

    count_ones(matrix, rows,columns);
    return 0;
}

I've indented it so it's easier to read as well as added the comment braces, just so it's clearer what is getting executed by what.

And now, the output:

4 3
1 2 3 4

Okay, now let's break down what's happening.

cout << columns << " " << rows;
cout << endl;

This is creating the line:

4 3

So far so good, right?

Now, we enter the lop:

for( int k = 0; k < rows; k++) {
    for( int l = 0; l < columns; l++) /* { */
        cout << matrix[k][l] << " ";
    /* } */
    cout << endl;

and get this:

1 2 3 4

This must be the first line of the matrix.

More code executes:

reader.close();

count_ones(matrix, rows,columns);

which isn't relevant to your problem.

And now this:

    return 0;
}

Whoops! We've just left the function by calling return.

The loop will no longer execute because we've terminated it prematurely by returning, only outputting the first line of the matrix.

Solution: Just move the return statement outside the loop like so:

cout << columns << " " << rows;
cout << endl;
for( int k = 0; k < rows; k++) {
    for( int l = 0; l < columns; l++) /* { */
        cout << matrix[k][l] << " ";
    /* } */
    cout << endl;

    reader.close();

    count_ones(matrix, rows,columns);
}
return 0;

And that should fix the problem.

Finally, some friendly advice, take Sami Kuhmonen's advice and indent your code. It makes it easier to read and catch things like this.

EDIT: One more point, as R.k. Lohana mentioned, you probably want to pull these lines out of the loop too:

    reader.close();

    count_ones(matrix, rows,columns);

Like so:

for( int k = 0; k < rows; k++) {
    for( int l = 0; l < columns; l++) /* { */
        cout << matrix[k][l] << " ";
    /* } */
    cout << endl;

}

reader.close();

count_ones(matrix, rows,columns);

return 0;

Since you probably only want to do them once and not multiple times.