C++ determinant calculator - follow-up

Question

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.

Answer 1

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];

Answer 2

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.

Answer 3

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.

• L. F.: You should add a mechanism to initialize the Matrix. Right now the official way is to assign the elements one by one.
• Toby Speight: 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.

Answer 4

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)