1
\$\begingroup\$

I've wrote a C++ program to calculate the determinant of a matrix:

#include <iostream> 
#include <string>

double getDeterminant(double arr[], int dimension);

int main() { 


    //First, the user has to enter the dimension of the matrix
    int dimension;
    std::cout << "Please enter dimension of Matrix: ";
    std::cin >> dimension;
    std::cout << std::endl;
    double matrix[dimension][dimension];

    //Now, the user has to enter the matrix line by line, seperated by commas
    std::string str;
    for(int i = 1; i <= dimension; i++) {
        std::cout << "Enter line " << i << " only seperated by commas: ";
        std::cin >> str;
        std::cout << std::endl;
        str = str + ',';
        std::string number;
        int count = 0;
        for(int k = 0; k < str.length(); k++) {
            if(str[k] != ',') {
                number = number + str[k];
            }
            else {
                matrix[i - 1][count] = std::stod(number);
                number = "";
                count++;
            }
        }
    }

    //Conversion to a onedimensional matrix to be able to give it over as a parameter
    double array[dimension * dimension];
    int k = 0;
    for(int i = 0; i < dimension; i++) {
        for(int j = 0; j < dimension; j++) {
            array[k] = matrix[i][j];
            k++;
        }
    }

    //Output
    for(int i = 0; i < dimension; i++) {
        for(int j = 0; j < dimension; j++) {
            std::cout << matrix[i][j] << " ";
        }
        std::cout << std::endl;
    }

    std::cout << "Determinant of the matrix is : " << getDeterminant(array, dimension) << std::endl; 
    return 0;
} 

double getDeterminant(double array[], int dimension) {

    //Formula for 2x2-matrix
    if(dimension == 2) {
        return array[0] * array[3] - array[1] * array[2];
    }

    //Conversion back to 2D-array
    double matrix[dimension][dimension];
    int k = 0;
    for(int i = 0; i < dimension; i++) {
        for(int j = 0; j < dimension; j++) {
            matrix[i][j] = array[k];
            k++;
        }
    }

    double result = 0;
    int sign = 1;
    for(int i = 0; i < dimension; i++) {

        //Submatrix
        double subMatrix[dimension - 1][dimension -1];
        for(int m = 1; m < dimension; m++) {
            int z = 0;
            for(int n = 0; n < dimension; n++) {
                if(n != i) {
                    subMatrix[m-1][z] = matrix[m][n];
                    z++;
                }
            }
        }

        //Conversion of the submatrix to 1D-array
        double array2[(dimension - 1) * (dimension - 1)];
        int k = 0;
        for(int x = 0; x < dimension - 1; x++) {
            for(int y = 0; y < dimension - 1; y++) {
                array2[k] = subMatrix[x][y];
                k++;
            }
        }

        //recursive call
        result = result + sign * matrix[0][i] * getDeterminant(array2, dimension -1);
        sign = -sign;
    }

    return result;
}

I would appreciate any suggestions on improving the code!


You can find the follow-up question here.

\$\endgroup\$
2
\$\begingroup\$
double matrix[dimension][dimension];
double array[dimension * dimension];
double matrix[dimension][dimension];
    double subMatrix[dimension - 1][dimension -1];
    double array2[(dimension - 1) * (dimension - 1)];

None of those are legal C++, because dimension isn't a constant-expression.


We have a signed/unsigned comparison here:

    for(int k = 0; k < str.length(); k++) {

We can easily eliminate the compiler warning by using a more appropriate type:

    for (std::size_t k = 0;  k < str.length();  ++k) {

But a better fix, given we only use k to index str, is to use a range-based loop:

    for (auto const c: str) {

When using operator>> on a stream, we must always check that it succeeded, before we depend on the result:

std::size_t dimension;
std::cin >> dimension;
if (!std::cin) {
    std::cerr << "Input failed\n";
    return EXIT_FAILURE;   // needs <cstdlib>
}
if (dimension == 0) {
    std::cout << "1\n";    // empty matrix determinant
}
| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Should I use vectors instead? \$\endgroup\$ – Philipp Wilhelm Feb 10 at 10:15
  • \$\begingroup\$ That would be a good choice, as that handles memory management etc. Do note that vector-of-vector doesn't have the good locality properties of array-of-array, so consider using or making a matrix class that's more efficient. You could use OpenCV cv::Mat, or look at the Matrix class I recently reviewed. \$\endgroup\$ – Toby Speight Feb 10 at 10:26
  • \$\begingroup\$ I would use const auto& in the range-for loop, not that the type of a string will ever change. \$\endgroup\$ – S.S. Anne Feb 10 at 12:20
  • \$\begingroup\$ “empty matrix has zero determinant” – Actually not: codereview.stackexchange.com/a/236988/35991 :) \$\endgroup\$ – Martin R Feb 10 at 12:53
  • 1
    \$\begingroup\$ @S.S.Anne, that might make sense (I'm guessing that most compilers are smart enough to copy characters rather than blindly using references as coded). \$\endgroup\$ – Toby Speight Feb 10 at 13:23

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.