4
\$\begingroup\$

After following some suggestions you can find here, I'd like to show you the result:

#include <iostream> 
#include <string>
#include <vector>

double getDeterminant(std::vector<std::vector<double>> vect, 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;

    if(dimension < 0) {
        std::cout << "ERROR: Dimension cannot be < 0." << std::endl;
        return -1;
    }

    //Now, the user has to enter the matrix line by line, seperated by commas
    std::vector<std::vector<double>> vect(dimension, std::vector<double> (dimension));
    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 if(count < dimension) {
                if(number.find_first_not_of("0123456789.-") != std::string::npos) {
                    std::cout << "ERROR: Not only numbers entered." << std::endl;
                    return -1;
                }
                vect[i - 1][count] = std::stod(number);
                number = "";
                count++;
            }
            else {
                std::cout << "ERROR: Too many numbers entered." << std::endl;
                return -1;
            }
        }
    }

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

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

double getDeterminant(std::vector<std::vector<double>> vect, int dimension) {

    if(dimension == 0) {
        return 0;
    }

    if(dimension == 1) {
        return vect[0][0];
    }

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

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

        //Submatrix
        std::vector<std::vector<double>> subVect(dimension - 1, std::vector<double> (dimension - 1));
        for(int m = 1; m < dimension; m++) {
            int z = 0;
            for(int n = 0; n < dimension; n++) {
                if(n != i) {
                    subVect[m-1][z] = vect[m][n];
                    z++;
                }
            }
        }

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

    return result;
}

Do you have any more suggestions to improve the code?


You can find the follow-up question here.

\$\endgroup\$
10
\$\begingroup\$
double getDeterminant(std::vector<std::vector<double>> vect, int dimension);

This will create a copy of the std::vector. For small std::vector this is not a problem, but it's good to make it a habit to pass complex data structures as const&, so the copy will not be created:

double getDeterminant(const std::vector<std::vector<double>>& vect, int dimension);

To make your code more readable, you can use an alias for the long vector name:

using Matrix = std::vector<std::vector<double>>;
double getDeterminant(const Matrix& vect, int dimension);

Lastly it is not necessary to pass the dimension, as it is accessible from the vector class:

double getDeterminant(const Matrix& vect);  
// instead dimension = vect.size();

std::endl will flush the output buffer. Only use it, if you want the buffer to be flushed. Writing to the screen takes a fair amount of time (compared to other instructions). Instead just use \n, it is cross-platform compatible

std::cin >> dimension;
std::cout << '\n';

This terminates the program. A better way of handling the erroneous input would be to allow the user to repeat the line

if(number.find_first_not_of("0123456789.-") != std::string::npos) {
     std::cout << "ERROR: Not only numbers entered." << std::endl;
     return -1;
}

Furthermore, this still allows for invalid numbers, I could input something like .-.-.-. for example.

This

number = number + str[k];

Can be replaced by the shorter version

number += str[k];
\$\endgroup\$
14
\$\begingroup\$
if(dimension == 0) {
    return 0;
}

Mathematically, that is not correct. The determinant of an empty (i.e. zero-dimensional) matrix is one, see for example What is the determinant of []? on Mathematics Stack Exchange.


With respect to efficiency: Your program computes the determinant recursively using the Laplace formula, which requires \$ O(n!) \$ arithmetic operations, and the creation of many temporary “submatrices”.

A better method (at least for larger matrices) is Gaussian elimination which requires \$ O(n^3) \$ arithmetic operations, and can operate on a single copy of the original matrix.

\$\endgroup\$
8
\$\begingroup\$

A vector of vectors can be an inefficient representation, because the storage may be scattered across many pages of memory. We can keep the data close together by using a single vector, and linearising the rows within it, perhaps like this:

#include <cstddef>
#include <vector>

class Matrix
{
    std::size_t width;
    std::size_t height;
    std::vector<double> content;

public:
    Matrix(std::size_t width, std::size_t height)
        : width{width},
          height{height},
          content(width * height)
    {}

    double& operator()(std::size_t row, std::size_t col) {
        return content[row * width + col];
    }

    double const& operator()(std::size_t row, std::size_t col) const {
        return content[row * width + col];
    }
};

Repeating from the original review:

  • always check the result of stream input operations
  • avoid comparing signed and unsigned types; or better, avoid the comparison by using range-based for.
\$\endgroup\$
2
  • \$\begingroup\$ You should add a mechanism to initialize the Matrix. Right now the official way is to assign the elements one by one. \$\endgroup\$
    – L. F.
    Feb 11 '20 at 9:30
  • \$\begingroup\$ Left as an excercise; it's not hard to add a std::initializer_list<double> to the arguments. That's a distraction from the main point, which is to linearise the array of elements. \$\endgroup\$ Feb 11 '20 at 11:08
3
\$\begingroup\$

Though there is already an accepted answer, I would add that usually, you would want to only return values between 0 and 127 in main -- just replace return -1; with return 1;, following the convention that non-zero exit codes represent errors or exceptions. (https://stackoverflow.com/a/8083027/9870592)

\$\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.