1
\$\begingroup\$

Here is what I've written for a program to print upper half triangle of a matrix. Now I need a way to do it without using goto:

    #include <iostream>
using namespace std;

int main()
{
    int a[10][10], r, c, i, j;
    cout << "Enter the number of rows and columns";
    cin >> r >> c ;
    cout << "Enter the matrix :";
    for(i=0;i<r;++i)
    {
                    for(j=0;j<c;++j)
                    {
                                    cin >> a[i][j];
                    }
    }
    cout << "The matrix is : \n";
    for(i=0;i<r;++i)
    {
                    for(j=0;j<c;++j)
                    {
                                    if(j<i)
                                    goto loop;
                                    cout << a[i][j] << "\t";
                                    loop:
                                         if(j<i)
                                         cout<< " \t";
                                    if(j==(c-1))
                                    cout<<"\n"; 
                    }
    }
    cin >> r;
}                           

Any help is appreciated.

\$\endgroup\$
3
\$\begingroup\$

The only goto I see is the following:

if(j<i)
    goto loop;
cout << a[i][j] << "\t";
loop:
    if(j<i)
         cout<< " \t";
if(j==(c-1))
    cout<<"\n"; 

You can easily change that to a simple if-else:

if(j<i) {
    cout<< " \t";
} else {
    cout << a[i][j] << "\t";
}
if(j==(c-1)) {
    cout<<"\n"; 
}
\$\endgroup\$
  • \$\begingroup\$ but that does not give me the matrix in triangular form. I just prints the elements horizontally \$\endgroup\$ – H G Sur Jan 4 '16 at 15:40
  • 2
    \$\begingroup\$ well I assumed you could add the last 2 lines yourself, try now \$\endgroup\$ – ratchet freak Jan 4 '16 at 15:41
4
\$\begingroup\$

When looking at your code I just want to say: Please reconsider your indentation!. This code should be rather easy to read, but you've complicated it quite a bit by not using braces around the blocks of the if statements, and leaving them at the same indentation, and why the excessive amount of indentation with the for loops?

So here are some style suggestions:

  • Keep indentation consistent at 4 spaces – Don't change indentation between different blocks, but keep it the same all the time
  • Always indent after if, for, while – Don't skip it like you've done in your inner loop
  • Add space after , and ;, and between operators – Having the blocks like for(i=0;i<r;++i) makes your code hard to read. Use spaces like in for (i=0; i<r; ++i) or if (j == (c-1)))
  • Use descriptive variable names – Whilst i and j is common as loop variables, please use more descriptive names like row, column, or matrix for the other variables to describe what they represent
  • Use braces around the blocks – Your code is prime example of the need to use braces around the blocks of if, for and while. By always adding braces around your code blocks, you are sure that the correct code is executed (and your 'faulty' indentation) won't give you any surprises!

And here are some code suggestions:

  • Use the if statement properly as suggested by ratchet freak – Either use it directly, or depending on which triangle you want to output a loop where you avoid the if completely. i.e. for (j=0; j<i && j < c; ++j), possibly with an indentation before that if needed and not zero based. If you don't understand this, do go with the suggestion by ratchet freak
  • Bug/Feature: If you enter r or c larger than 10, it fails miserably – You should add error handling if the input is outside of your intended range, or you should make your code more dynamic allowing for larger ranges.
\$\endgroup\$
  • \$\begingroup\$ This is a great review. Thank you for your time ! I'll take into consideration your advice. \$\endgroup\$ – H G Sur Jan 7 '16 at 11:24
  • \$\begingroup\$ The IDE I used actually had automatic indentation but when I pasted it here, indentations are gone. \$\endgroup\$ – H G Sur Jan 7 '16 at 11:30
1
\$\begingroup\$

Here is a refactored version (according to my other answer) with dynamic allocation of the two dimensional matrix, and also using a lot more functions to separate the concern of the main function, and finally some simple error handling related to not being able to allocate (and subsequently failing to deallocate).

using namespace std;
#include <cstddef>
#include <iostream>
#include <sstream>

int **allocateDynamicArray(int rows, int columns) {
    int **matrix;

    // Allocate the row dimension ...
    matrix = new (nothrow) int* [rows];

    if (matrix == NULL) {
        cerr << "Failed allocating memory for row count of: " << rows << std::endl;
        return NULL;

    } else {
        // ... before allocating each of the columns
        for (int i=0; i<rows; ++i) {
            matrix[i] = new (nothrow) int [columns];
            if (matrix[i] == NULL) {
                cerr << "Failed allocating memory for row: " 
                     << i << " columns: " << columns << std::endl;

                // Free already allocated memory
                for (int j=0; j<i; ++j) {
                    delete [] matrix[j];
                }

                delete [] matrix;
                return NULL;
            }
        }
    }
    return matrix;
}


void freeDynamicArray(int **matrix, int rows) {
    // Free columns first ...
    for (int i=0; i<rows; ++i) {
        delete [] matrix[i];
    }

    // ... and then the row dimension
    delete [] matrix;
}


void fillArray(int **matrix, int rows, int columns, bool autoFill=true) {

    cout << "Enter values into the matrix:";
    for (int i=0; i<rows; ++i) {
        for (int j=0; j<columns; ++j) {
            if (autoFill) {
                matrix[i][j] = (i+1) * 100 + j;
            } else {
                cout << "Row: " << i << ", column: " << j;
                cin >> matrix[i][j];
            }
        }
    }
    cout << std::endl;
}


void printUpperRight(int** matrix, int rows, int columns) {
    for (int i=0; i<rows; ++i) {
        for (int j=0; j<columns; ++j) {
            if (j<i) {
                cout << "\t";
            } else {
                cout << matrix[i][j] << "\t";
            }
        }
        cout << std::endl;
    }
}   

int main() {
    int **matrix, rows, columns, i, j;

    cout << "Enter the number of rows and columns";
    cin >> rows >> columns;

    matrix = allocateDynamicArray(rows, columns);

    if (matrix == NULL) {
        cerr << "Couldn't allocate memory for matrix" << std::endl;
        return -1;
    }

    fillArray(matrix, rows, columns);
    printUpperRight(matrix, rows, columns);

    freeDynamicArray(matrix, rows);

    return 0;
}

The above version should work identical to the OP version, only that it allows for dynamic sizes, and by default it autofills the matrix, as I'm lazy don't want to enter all the values.

An alternative way when printing would be to use ternary similar to: (j<i) ? "" : matrix[i][j] but this leads to a type mismatch in C++. This can be addressed using C++11 and std::to_string(matrix[i][j]), but this fails in some minGW versions (and repl.it), according to this thread and answer. This answer also suggest the following slightly hackish solution:

// Hackish macro (placed at top of code) to convert to str 
#define SSTR( x ) dynamic_cast< std::ostringstream & >( \
        ( std::ostringstream() << std::dec << x ) ).str()


            // Replace the if ... else in print_array with this:    
            cout <<  ((j<i) ? "" : SSTR(matrix[i][j])) << "\t";
            // or possibly this:
            cout << ((j<i) ? "" : std::to_string(matrix[i][j])) << "\t";

Note that this solution is longer than the OP code due to the safer handling of dynamic allocation of the 2D array and freeing of this memory, and the use of functions which in turn are recommended to keep the code clean and readable.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.