Насколько грамотен этот код?

146
19 декабря 2016, 19:42
class accountBase {
protected:  
    struct account;
    account * pacc;   //начало массивa аккаунтов
public:
    int  accN;        //кол-во аккаунтов
    accountBase(){
        accN=0;
        pacc=0;
    };
    ~accountBase(){
        delete pacc;
    };
    struct account {
        char* name;
        char* login;
        char* password;
        char* category; 
    };
    void addacc(account& acc){
        accN++;
        pacc=new account[accN];
        account*pacc2=pacc+(accN-1);
        *pacc2=acc;    
    };
    account* getacc(int accNumber){
        if (accNumber>accN) return 0;
        return pacc+(accNumber-1);
    };
};

UPD:

class accountBase {
protected:  
    struct account;
    account * pacc;   //начало массивa аккаунтов
public:
    int  accN;        //кол-во аккаунтов
    accountBase(){
        accN=0;
        pacc=0;
    };
    ~accountBase(){
        delete pacc;
    };
    struct account {
        char* name;
        char* login;
        char* password;
        char* category; 
    };
    void addacc(account& acc){
        accN++;
        pacc=new account[accN];
        account*pacc2=pacc+(accN-1);
        *pacc2=acc;    
    };
    account* getacc(int accNumber){
        if (accNumber>accN) return 0;
        return &(pacc[accNumber-1]);        
    };
    void delacc(int accNumber){
        if (accNumber>accN) return; 
        accN--;
        *(pacc+(accNumber-1))=*(pacc+accN);     
        pacc = new account[accNumber];
    };
};
Answer 1

Этот код ужасен.

  1. В C++ необходимо отказаться от ручного управления памятью, и использовать стандартные контейнеры.
  2. Вы оставили открытой переменную int accN на запись для пользователей класса. Это грубое нарушение инкапсуляции. Спрячьте переменную, оставьте открытым только getter к ней.
  3. Функция getacc должна сигнализировать об неверном индексе не возвращением нуля, а, как это и принято в C++, выбросом исключения.
  4. Функция addacc теряет данные. Добавленные ранее account'ы будут потеряны с утечкой памяти.
  5. Функция delacc молча глотает неправильный индекс и снова теряет данные. Строка pacc = new account[accNumber] приводит к утечке памяти.
  6. Название класса в lowerCamelCase, так на C++ никто не пишет: либо PascalCase, либо snake_case. Методы либо в lowerCamelCase, либо snake_case. Сокращённые имена не нужны, байты за последние годы подешевели, так что addAccount или просто add — вполне подходящее имя.
Answer 2

Приведу и я свои комментарии. Сразу с кодом.

// здесь можно и class написать, но это ничего не поменяет
// ну разве что потом нужно добавить спецификатор public + get/set методы
// но зачем усложнять?
struct Account {
        std::string name;
        std::string login;
        std::string password;
        std::string category; 
};
// list в назании более точно отражает сущность
class AccountList {
private:  
    // имя можно было бы дать и m_account_list
    // но зачем, дублировать слово account
    // тем более в названии внутреннего поля
    std::vector<Account> m_list;
public:
    // конструктор-деструктор теперь не нужны
    // в названии этого метода слово account не нужно. Это понятно из названия класса.
    // конструкция с const + & намекает компилятору на возможность оптимизаций.
    void add(const Account& account){
        m_list.push_back(account);   
    };
    // из оригинального кода я понял, что индексы (number) изменяются в диапазоне от 1 до размера
    // а не от 0 до size-1. Странное решение, но мало чего, может так по условию задачи нужно
    // было бы хорошо ещё константный метод с таким же именем добавить,
    // который не будет копировать данные
    // NB! range for number is [1..size]
    Account get(int number){
        // at умеет сам проверять диапазон и генерировать исключение.
        // [] - нет.
        // можно добавить свои проверки, но они будут захламлять код
        return m_list.at(number-1);
    };
    // может быть будет правильно назвать этот метод remove.
    // в этой функции наверное стоило бы проверить попадание в диапазон, но erase и сам это прекрасно умеет.
    void del(int number){
        m_list.erase(m_list.begin() + number -1, m_list.begin() + number);
    };
    // и размер, как же без него.
    const size_t size() {
        return m_list.size();
    }
};
Answer 3
  1. Есть проблема с утечкой памяти в addacc() при добавлении более чем одной записи предыдущее значение аллоцированного массива pacc пропадает, то есть так и останется висеть аллоцированным
  2. В той же функции не копируются значения предыдущего массива
  3. getacc() какой-то адъ

Код на троечку с минусом. Троечка только за хитрое объявление конструктора с деструктором, за остальное я бы дал двоечку.

Answer 4

я чего то так с ходу и не понял что это и для чего

что с pacc делаете? чем обычный список\массив и т.д. не угодил? accN почему то public, если он часть интерфейса, выходит, пользователь класса может менять его как хочет, это наверняка отразится на логике работы...

Answer 5

Я полностью согласен с ответов @VladD. Вот только 6 пункт - это уже не требования к коду на C++, а требования к "дизайну" любого кода, как к таковому. В прочем, вы можете называть функции, классы, структуры и т.д как вам угодно, главное, чтобы вам было понятно в дальнейшем значение того или иного логически отделенного блока кода по его имени.

И правда, ваш код испещерен утечками памяти, и, отчасти, это не ваша проблема, а проблема самого C++, где отстрелить себе ногу легче всего. Кроме того, структуру account целесообразно вынести за пределы класса( не смотря на то, что она более нигде использоваться не будет ), так как вложенность часто путает, а быстродействия тут особо не прибавится. Также ничего не известно о переменной pacc2. Информация о ней отсутствует как в классе, так и за его пределами. Что по поводу методов addacc и delacc. В них вы сначала увеличиваете или уменьшаете счетчик, а только затем добавляете или удаляете экземпляр структуры account. При этом у вас нет 100%-ой гарантии, что добавление или удаление прошло успешно( вы не перехватываете исключений нигде в коде ), следовательно, это чревато целой цепочкой ошибок и, опять же, утечек...

Судя по назначению вашего кода, вы пишите код, работа которого будет заключаться во взаимодействии с некоторой базой пользователей, которая, вероятно, получается из сети. C++ - не самый лучший выбор для решения подобного рода задачи. Вам нужно что-то более высокоуровневое. Высокоуровневый язык избавил бы вас от постоянной "борьбы" с утечками памяти и бесконечным подсчетом смещений байт. Он бы напривил вас в верное "русло", а именно, нацелил бы на решение проблемы, как таковой.

READ ALSO
Произойдёт ли утечка памяти?

Произойдёт ли утечка памяти?

В итоге p1 и p2 будут указывать на один и тот же участок памятиПроизойдёт ли утечка памяти, занятой сначала p1

202
Как правильно формировать criteria для связи HAS_MANY?

Как правильно формировать criteria для связи HAS_MANY?

Есть модель POSTИ есть модель category_r В последней 3 столбца: id, Post_id, category_id

158
Неправильный расчёт даты возврата книг

Неправильный расчёт даты возврата книг

Я написал код для подсчёта количества дней, через которые должны вернуть книги, взятые в библиотеке, но, к сожалению, код не правильно считает

159
Вопрос по substr_count($_SERVER[&#39;REQUEST_URI&#39;]

Вопрос по substr_count($_SERVER['REQUEST_URI']

Как показать информацию на нескольких страницах с помощью substr_count($_SERVER['REQUEST_URI']?

229