It doesn't show the matrix to multiply and don't multiply these.(C++, DYNAMIC MATRIX)

Question

I want to multiply 2 matrix and multiply escalar and matrix. Nevertheless, before this I want to show the matrix to multiply and then multiply, but this doesn't work fine. I don't know what is the problem. This is the code. Thanks in advance. Are here the different constructors, destructors, operators and functions.

Matriu::Matriu(){
    m_fila = 0;
    m_columna = 0;
    m_valor = 0;
    matriu = new float*[m_fila];
    for(int i = 0; i < m_fila; i++){
       matriu[i] = new float[m_columna];
    }
}

Matriu::Matriu(int nFiles, int nColumnes){      
    m_fila = nFiles;
    m_columna = nColumnes;
    matriu = new float*[m_fila];
    for(int i = 0; i < m_fila; i++){
        matriu[i] = new float[m_columna];
    }       
}

Matriu::Matriu(const Matriu& m){
    m_fila = m.m_fila;
    m_columna = m.m_fila;
    matriu = new float*[m_fila];
    for(int i = 0; i < m_fila; i++){
        matriu[i] = new float[m_columna];
    }
    for(int i = 0; i < m_fila;i++){
        for(int j = 0; j < m_columna; j++){
            matriu[i][j] = m.matriu[i][j];
        }
    }
}

Matriu::~Matriu(){
    for(int i = 0; i < m_fila; i++){
        delete[] matriu[i];
    }
    delete[] matriu;
}

void Matriu::setValor(float valor){
    for(int i = 0; i < m_fila; i++){
        for(int j = 0; j < m_columna; j++){
            matriu[i][j] = valor;
        }
    }
}

Matriu& Matriu::operator=(const Matriu& m){
    for(int i = 0; i < m_columna; i++){
        delete[] matriu[i];
    }
    delete[] matriu;
    m_fila = m.m_fila;
    m_columna = m.m_columna;
    matriu = new float*[m_fila];
    for(int i = 0; i < m_fila; i++){
        matriu[i] = new float[m_columna];
    }
    for(int i = 0; i < m_fila; i++){
        for(int j = 0; j < m_columna;j++){
            matriu[i][j] = m.matriu[i][j];
        }
    }
    return *this;
}

void Matriu::init(int nFiles, int nColumnes){
    if(!esBuida()){
        for(int i = 0; i < m_columna; i++){
            delete[] matriu[i];
        }
    }
    delete[] matriu;

    m_fila = nFiles;
    m_columna = nColumnes;
    matriu = new float*[m_fila];
    for(int i = 0; i < m_fila; i++){
        matriu[i] = new float[m_columna];
    }
}

Matriu Matriu::operator*(const Matriu& m){
    if(m_columna != m.m_fila){
        for(int i = 0; i < m_fila; i++){
            for(int j = 0; j < m_columna; j++){
                matriu[i][j] = 0;
            }
        }
    } 
    Matriu aux(m);
    for (int i=0; i< m.m_fila; i++) 
        for (int j=0; j< m.m_columna; j++)
        {
            aux.matriu[i][j] = 0;

            for(int x = 0; x < m.m_fila; x++){
                aux.matriu[i][j] = aux.matriu[i][j] + matriu[i][x]*matriu[x][j];
            }           
        }
    return aux;     
}

Matriu Matriu::operator*(float s){      
    for(int i = 0; i < m_fila; i++){
        for(int j = 0; j < m_columna; j++){
            matriu[i][j] = s * matriu[i][j];
        }
    }
    return *this;
}

bool Matriu::esBuida() const{
    if(matriu == NULL){
        return true;
    }else{
        return false;
    }
}

float& Matriu::operator()(int fila, int columna){       
    assert(fila >= 0 && fila < m_fila);
    assert(columna >= 0 && columna < m_columna);
            return matriu[fila][columna];       
}

float& Matriu::operator()(int fila,int columna)const{
    if(fila >= 0 && fila < m_fila){
        if(columna >= 0 && columna < m_columna){
            return matriu[fila][columna];
        }
    }       
}

And this is my class.

class Matriu
{
public:
    Matriu();
    Matriu(int nFiles, int nColumnes);
    Matriu(const Matriu& m);
    ~Matriu();

    Matriu& operator=(const Matriu& m);
    void init(int nFiles, int nColumnes);
    void setValor(float valor);
    void setNFiles(int fila){m_fila = fila;}
    void setNColumnes(int columna){m_columna = columna;}
    Matriu operator*(const Matriu& m);
    Matriu operator*(float s);
    float getValor(){return m_valor;}
    bool esBuida() const;
    int getNFiles() const{return m_fila;}
    int getNColumnes() const{return m_columna;}
    float& operator()(int fila, int columna);
    float& operator()(int fila,int columna)const;
private:
    float** matriu;


    int m_fila;
    int m_columna;
    int m_valor;
};

For show the matrix before multiply I have 2 functions in main (this are correct, I know that this doesn't the problem). Thanks in advance.

Please try to create a [**Minimal**, Complete, and Verifiable Example](http://stackoverflow.com/help/mcve) to show us. Also please [read about how to ask good questions](http://stackoverflow.com/help/how-to-ask), as well as [this question checklist](https://codeblog.jonskeet.uk/2012/11/24/stack-overflow-question-checklist/). — Some programmer dude, Sep 28 '18 at 11:27
this is merely the class definition, ie this code will not print or multiply anything. Where is your `main`? — 463035818_is_not_an_ai, Sep 28 '18 at 11:29
You have a lot of duplicate code, both in your constructor and assignment operator. Removing the duplicate code and providing a `main` not only *may* solve your issue, but possibly you will find the error yourself. — PaulMcKenzie, Sep 28 '18 at 11:33
Also, please specify what you mean with "this doesn't work fine". Does it crash? Is nothing happening? Are the results wrong? — Max Langhof, Sep 28 '18 at 11:33
why does you `operator*` as the first step set all matrix elements to 0? `matriu[i][j] = 0;` ? My maths classes are long time ago, but I dont think this is how you multiply matrices... — 463035818_is_not_an_ai, Sep 28 '18 at 11:34
'I know this isn't the problem' With respect you probably don't. It's **very** common for newbies to post only correct code, and not post the code that really has the problem. Please post your main function. — john, Sep 28 '18 at 11:34
There are already many SO questions and answers about matrix. In general, earlier or later these two facts are mentioned: 1. Don't use `new` - use `std::vector`. 2. Don't nest vectors into vectors (or arrays into arrays) for a MATRIX. Just make one vector (or array) with size width * height. You may add an overloaded `operator[]` if you really like `m[i][j]` or just give it a get() and set() method for 2 dim access. — Scheff's Cat, Sep 28 '18 at 11:34
Why is this horrible way of allocating a 2d array even taught anymore, where there are nested calls to `new[]` in a `for` loop? Even if `vector` can't be used, this is an awful way to allocate memory for a matrix. — PaulMcKenzie, Sep 28 '18 at 11:36
@user463035818 That seems to be the error handling for mismatched matrix sizes. Terrible idea (because it will still go out of bounds later, it also zeros out the current matrix for no reason) but shouldn't affect the code given correct dimensions. — Max Langhof, Sep 28 '18 at 11:36
dont get frustrated by downvotes or comments asking for clarifications. Imho your problem is that you wrote too much code and now you dont know where/what to fix. Do you have a code that just multiplies two matrices and does nothing else? If not I would strongly suggest you to start with that and then add more in small steps. At least thats how i would tackle the problem, start small and add things in tiny steps only once you verified by testing that your already existing code is correct — 463035818_is_not_an_ai, Sep 28 '18 at 11:41
Should follow up: [Better ways of allocating for a 2d array](https://stackoverflow.com/questions/21943621/how-to-create-a-contiguous-2d-array-in-c) — PaulMcKenzie, Sep 28 '18 at 11:44

score 1 · Accepted Answer · answered Sep 28 '18 at 11:43

There are several problems in this code.

Matriu::operator*(float s) changes the matrix itself, instead of returning a changed copy. A * 5 should not modify A (but it currently does).
When you detect a matrix size mismatch in Matriu::operator*(const Matriu& m), you set the current matrix to zero. That is not proper error handling - you can still access arrays out of bounds. Throw an exception or return an empty matrix. Don't just obliterate the current matrix.
The returned matrix Matriu aux(m); in your matrix multiplication has the wrong size. You use the size of the right hand side matrix, which is incorrect.

Then there's also the host of other problems regarding design that the comments have mentioned.

score 0 · Answer 2 · answered Sep 28 '18 at 11:39

There's an error here

Matriu::Matriu(const Matriu& m){
    m_fila = m.m_fila;
    m_columna = m.m_fila;

should be

Matriu::Matriu(const Matriu& m){
    m_fila = m.m_fila;
    m_columna = m.m_columna;

That would certainly stop your matrix multiplication working

It doesn't show the matrix to multiply and don't multiply these.(C++, DYNAMIC MATRIX)

2 Answers2