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

6 ответов 6

12

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

  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 — вполне подходящее имя.
4

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

// здесь можно и 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();
    }
};
6
  • @KoVadim мне интересно знать почему вы так часто используете полное указания пространства имен (std::string), когда можно указать using std::string; всего один раз?
    – perfect
    28 июн 2013 в 8:29
  • 1
    @KoVadim, замечательно. Такой вот "урезаный" vector. Только может стоит добавить в комментариях, что деструктор по умолчанию (также как и del, который точно надо переименовать в remove) удалит только вектор ссылок, а не login, password и т.д. в каждом из аккаунтов?
    – avp
    28 июн 2013 в 8:30
  • to @perfect - такой у меня стиль. Я считаю, что так правильно. to @avp - деструктор по умолчанию вызовет деструктор вектора, который вызовет деструкторы для всех своих элементов. Вот если бы я написал std::vector<Account*> m_list; -тогда нужно было бы городить деструктор.
    – KoVadim
    28 июн 2013 в 8:52
  • @KoVadim, т.е. пользователь должен понимать, что в векторе будут копии struct Account с копиями всех стрингов? Или память, занимаемая login, password ... не будет дуплицироваться при вызове add()? IMHO половина программеров (и я в их числе) сходу путаются - когда, что и куда будет копироваться в С++ классах.
    – avp
    28 июн 2013 в 9:18
  • и пусть будут. В противном случае придется либо применять smart pointer, либо городить свои собственные счетчики ссылок. >> Или память, занимаемая login, password ... не будет дуплицироваться при вызове add()? скорее всего нет - std::string обычно реализован с подсчетом ссылок. Подсчет ссылок значительно быстрее копирования памяти. Но если путаетесь в том, что и как будет копироваться - значит либо Вам рано в С++, либо просто не переживайте по этому поводу:) >> т.е. пользователь должен понимать, что в векторе будут копии struct Account а вот пользователя это не особо должно тревожить.
    – KoVadim
    28 июн 2013 в 9:27
3
  1. Есть проблема с утечкой памяти в addacc() при добавлении более чем одной записи предыдущее значение аллоцированного массива pacc пропадает, то есть так и останется висеть аллоцированным
  2. В той же функции не копируются значения предыдущего массива
  3. getacc() какой-то адъ

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

6
  • @Barmaley ♦, буду благодарен, если напишите свою версию addacc и getacc
    – ололо
    27 июн 2013 в 18:40
  • 2: @Barmaley ♦, а какой ф-ией можно скопировать? memcpy подойдёт? И что хитрого в объявлении конструктора с деструктором?
    – ололо
    27 июн 2013 в 18:58
  • 1) Писать не буду - уж извините. 2) Хитрый только потому, что в конструкторе обнулен указатель, а в деструкторе приведен delete на (иногда) нулевой указатель - красиво.
    – Barmaley
    27 июн 2013 в 19:18
  • 1
    @Barmaley а вы уверены, что это сделано осознанно, а не по незнанию? Боюсь, что второй вариант куда как вероятнее
    – DreamChild
    27 июн 2013 в 19:28
  • @ололо, извините за прямоту, но пока это полный отстой. Раз. Нет комментариев: --а) для чего предназначен этот код; --б) как его предполагается использовать; --в) основные идеи реализации. Два. Абсолютно не продумано, что делать с памятью, на которую указывают поля struct account. Три. Возможно для хранения account-ов правильней использовать стандартный контейнер. -- Но, если использовать его (контейнер), то совсем непонятно, зачем вообще нужен Ваш класс? Для инкапсуляции ради инкапсуляции и возможности наследования (которое неизвестно кому, когда и зачем может потребоваться)?
    – avp
    27 июн 2013 в 19:53
3

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

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

2
  • @nik604, а как сделать так, чтобы извне его можно было только читать?
    – ололо
    27 июн 2013 в 19:11
  • 2
    int getAccN() { return accN; } accN сделай protected\private
    – nik604
    27 июн 2013 в 19:15
3

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

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

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

4
  • > и, отчасти, это не ваша проблема, а проблема самого о да, учитывая, что код написан вообще то на С... > C++ - не самый лучший выбор для решения подобного рода задачи. Вам нужно что-то более высокоуровневое. ага, java/python/C$. да, конечно.
    – KoVadim
    28 июн 2013 в 9:52
  • @KoVadim, в смысле, этот код на СИ ? О_o в сях есть классы?)
    – AseN
    28 июн 2013 в 9:54
  • имелся ввиду код автора. То, что там есть class, ещё не делает этот код кодом на с++. Это, как пишут некоторые, с+.
    – KoVadim
    28 июн 2013 в 10:19
  • А, если это такой жаргон, то я, конечно же, согласен. От C++ здесь осталось лишь "слово" class)
    – AseN
    28 июн 2013 в 10:24
0
class accountBase {
protected:  
    struct account;
    account * pacc;
public:
    int  accN; //Количество элементов массива должно иметь тип size_t, остальные не гарантируют достаточной разрядности. Исключение – массивы, имеющие явные ограничения на количество элементов.
    accountBase(){
        accN=0;
        pacc=0; //Указатель никуда должен иметь значение nullptr, стандарт не гарантирует кодирование этого значения именно нолём.
    };
    ~accountBase(){
        delete pacc;
    };
    struct account {
        char* name;
        char* login;
        char* password;
        char* category; 
    };
    void addacc(account& acc){
        accN++; // Если количество элементов уже равно максимально представимому значению, то accN обнулится из-за переполнения. А при использовании знакового типа (int – это на самом деле signed int) – отрицательным. Кроме того, нет гарантии, что память будет выделена в нужном количестве, а Вы УЖЕ записали новое количество элементов.
        pacc=new account[accN]; // Если масссив уже существует, то в этот момент произойдёт утечка, надо или реклокатить, или присваивать адрес выделенной памяти локальному буферу, потом скопировать данные, потом освободить память по pacc и, наконец, скопировать буфер в pacc.
        account*pacc2=pacc+(accN-1); // А в чём суперхитрость присваивать что-то произведению, да ещё и умножать на тип? И где объявлена pacc2?
        *pacc2=acc; // Где объявлена pacc2?
    };
    account* getacc(int accNumber){
        if (accNumber>accN) return 0; //Указатель никуда должен иметь значение nullptr, стандарт не гарантирует кодирование этого значения именно нолём. Кроме того, надо ещё защититься от accNumber<1.
        return pacc+(accNumber-1); // По-хорошему это надо под else для облегчения чтения. Или уж положитесь на оптимизацию, заведите локальную переменную Result типа указатель на account, инициируйте её nullptr, замените условие так, чтоб оно было равно true при допустимом значении accNumber, а не при недопустимом, а при срабатывании ифа присвойте переменной Result значение адреса запрошенного элемента. И где сказано, что массив начинается с первого элемента? Внутренности реализации пряно прятать от пользователя, соответственно, автор вызывающего кода pacc+(accNumber-1) не прочитает и будет думать, что массив начинается с нулевого элемента. Вы имеете право обычай начинать массив с нулевого элемента послать нафиг, а свой массив начать с любого идекса, но об этом надо написать в документирующем комментарии. Кроме того, ему надо возвращаемое значение куда-то положить. Положить можно или в переменную типа указатель на account, или в параметр другой функции типа указатель на account, или в операнд типа указатель на account, а account наружу не торчит, то есть положить будет некуда.
    };
};
5
  • 3
    «pacc=0; //Указатель никуда должен иметь значение nullptr, стандарт не гарантирует кодирование этого значения именно нолём.» Хотя использование ключевого слова nullptr более ясно выражает намерения программиста, тем не менее стандарт языка гарантирует, что присвоение pacc=0; поместит в указатель pacc нулевое указательное значение независимо от того, какой последовательностью битов кодируется нулевое указательное значение.
    – wololo
    3 янв в 8:27
  • account*pacc2=pacc+(accN-1); - это не умножение, а объявление локальной переменной типа указателя.
    – AlexGlebe
    3 янв в 10:54
  • .. @wololo и только число 0 в стандарте называется null pointer, который преобразуется в std::nullptr_t тип.
    – AlexGlebe
    3 янв в 10:59
  • А как же те системы, у которых нулевой адрес вполне валиден? 3 янв в 11:26
  • И кто ж Вас учил объявлять слитно? 3 янв в 11:26

Ваш ответ

Нажимая на кнопку «Отправить ответ», вы соглашаетесь с нашими условиями использования и подтверждаете, что прочитали и поняли наши политику конфиденциальности и нормы поведения.

Всё ещё ищете ответ? Посмотрите другие вопросы с метками или задайте свой вопрос.