Некоторые комментарии:
1: использовать методы-члены, а не функции
2: не использовать using namespace std.
3: если в массиве есть m членов, его более традиционно использовать<в циклах for. </p>
for (int i=0; i <= mat.m-1; i++)
// More traditional to use:
for (int i=0; i < mat.m; ++i)
4: использовать более длинные имена переменных, чем i.Попробуйте найти в коде все экземпляры переменной «i».Вы получите много ложных срабатываний.Что-то вроде loopM будет проще для чтения.
5: Используйте оператор []
*(*(mat.a + i) + j) = BLA;
// Cab be written as:
mat.a[i][j] = BLA;
6: В нормальных ситуациях new никогда не вернет NULL.Так что не пишите код, который проверяет это.Если это не удастся, будет выдано исключение.Это позволяет вам удалить код обнаружения ошибок из обычного потока кода и обрабатывать ошибки в исключениях.
mat.a = new int*[mat.m];
// This is a waste of time. Here mat.a will NEVER be NULL
if (mat.a == NULL)
{
return 0;
}
7: все в порядке, чтобы удалить объекты NULL.Так что не проверяйте NULL перед удалением.
if (mat.a != NULL)
{
delete [] mat.a;
}
// If it is NULL nothing bad will happen.
delete [] mat.a
8: Вы забыли удалить все элементы.
Примечание: я был бы более уверен, что это правильно, если вы использовали конструктор / деструктор каккомпилятор гарантирует, что деструктор не будет вызван, если конструктор выдает исключение.С другой стороны, использование вами функций не гарантирует этого, и вы можете поймать исключение и все же выполнить FreeMatrix () для недопустимого объекта матрицы.Но так как в вашем текущем коде нет обработки исключений, я чувствую себя в безопасности.
for(int loop = 0;loop < m;++loop)
{
delete [] mat.a[loop];
}
delete [] mat.a;
9: ваш оператор +, кажется, использует некоторую случайную переменную temp?
Предпочитаете объявлять temp внутри функции и возвращатькопия.Но для этого вам понадобится правильный конструктор копирования и оператор присваивания (см. Ниже).
matrix operator +(const matrix &mat1, const matrix &mat2)
{
matrix temp; // add mat1 and mat2 into temp
}
return temp;
10: в вашей функции + =.Вы суммируете значения во временную переменную, а затем снова присваиваете mat1.Нет причин не делать этого на месте.
11: объявлять только одну переменную в строке.
matrix mat1, mat2, mat3;
// All coding standards say use:
matrix mat1;
matrix mat2;
matrix mat3;
Причина в том, что она обнаруживает пару проблем, связанных с указателями.На мой взгляд, это также облегчает чтение кода (что также является плюсом).Все компании примут участие в нем, поэтому просто привыкните к нему.
12: Ваше распределение матриц никогда не будет неудачным (как указывалось ранее).Это бросит исключение.Таким образом, слежка никогда не напечатает «Недостаточно памяти».Также я хотел бы отметить, что вы возвращаете целое число, где bool был бы лучшим типом для обозначения сбоя (если бы вы могли указать сбой).
if (!AllocMatrix(mat1))
{
//
// This will never be executed. EVER.
//
cout << "Out of memory!" << endl;
FreeMatrix(mat1);
return 1;
}
13: Здесь у нас есть потенциальная проблема.Ваш код выполняет operator +, как вы ожидаете.Но он также выполняет matrix::operator=.Вы не определили это, но компилятор автоматически сгенерировал код для этого метода.И это не делает то, что вы думаете.К счастью, вы не освобождаете mat3, поэтому он не взрывается.
mat3 = mat1 + mat2;
Любой код, в котором класс (или структура) содержит указатели, которыми владеют (вы размещаете указатели и удаляете их), необходимособлюдайте правило 3 (плюс есть правильный конструктор).
struct matrix
{
int** a;
int m;
int n;
};
Причина в том, что если вы не отключите их специально, компилятор сгенерирует для вас 4 метода.Если вы начнете выделять / удалять память вручную, два из этих методов (конструкция копирования и оператор присваивания) будут делать то, что затрудняет правильное освобождение.В результате вы должны либо отключить их, либо явным образом определить все 4 метода, сгенерированных компилятором.
В вашем случае компилятор генерирует следующие 4 метода:
matrix::matrix() { /* Do nothing or value initialize */ }
matrix::~matrxc(){ /* Do nothing */ }
matrix::matrix(matrix const& rhs)
: a(rhs.a)
, m(rhs.m)
, n(rhs.n)
{}
matrix& matrix::operator+(matrix const& rhs)
{
a = rhs.a;
m = rhs.m;
n = rhs.n;
return *this;
}
Теперь рассмотрим следующий кодсделаю:
matrix m1;
m1.m = 5;
m1.n = 5;
AllocMatrix(m1));
matrix m2;
m2.m = 5;
m2.n = 5;
AllocMatrix(m2));
m2 = m1; /* Simplified version of m3 = m1 + m2; */
// The problem is that the a member is being copied from m1 to m2.
// But the copy is a shallow copy. So now both m1 and m2 point at the same
// piece of memory.
FreeMatrix(m2); // Frees m2.a
FreeMatrix(m1); // Whops. This Frees m1.a but it points at the same memory as m2.a
// So now you have a double delete.
14: Разделение интересов.
Это важная техника для изучения.Это означает, что класс должен сделать одну вещь.Либо он должен содержать логику для выполнения некоторой бизнес-логики, либо он должен содержать управление памятью (или другие элементы управления).Это не должно делать обоих.
Это означает, что ваш матричный объект должен сосредоточиться на логике выполнения матричных операций и делегировать управление памятью другому типу объекта, который специально разработан для управления памятью.Я оставлю это так, как Charles Bailey подробно описывает это в своем ответе.