WW #11

[Sokolov Viacheslav]

В выбранной реализации в манипуляторах не нужен friend. Достаточно просто декларации свободной функции.

Лучше сделать конструкторы explicit (нужен ли комментарий, почему?)

Вообще говоря, после манипуляции стоило бы проверять, что она удалась (внутри манипулятора). Сейчас проверяется лишь, что на момент начала работы есть смысл пытаться что-то делать.

В main не хватает проверок успешности выполнения чтения из std::cin

Можно было бы принимать не указатель, а ссылку, тогда и проверка на not null не нужна

127 EmployeeArray::~EmployeeArray() {
128     for (int i = 0; i < (int)_employees.size(); i++) {

по какой причине переменная здесь типа int, а не size_t?

метод copy_this_to не очень удачный, потому что:

• предполагает наличие объекта, куда нужно осуществить копирование
• может быть невыполним, поскольку у передаваемого объекта может быть другой тип

вместо этого лучше сделать метод clone:
{{{Employee
{
virtual Employee* clone() const = 0;
} }}}

при этом возикает одна проблема, которая вместе с решением описана здесь: ​https://stackoverflow.com/questions/15575272/how-to-use-clone-in-c-with-multiple-inheritance-of-abstract-classes
(но так делать сейчас не предлагается, потому что становится сложнее)

метод read_employee выбивается по названию. Может быть, read_textually?

Предполагается ли наличие неименованных сотрудников? Я бы думал, что нет; может быть, сделать так, чтобы в конечных классах логику имени не нужно было бы дублировать?

Инициализацию объекта стоит делать в списке инициализации и инициализировать явно все его поля.

[Sokolov Viacheslav]

16 inline std::ostream &operator<<(std::ostream &out, write_le_int32 given_value) {

Зачем inline, почему не реализовать в .cpp, как обычно?

Классы, от которых не предполагается наследоваться, стоит помечать как final

Лучше избегать совпадения имен аргументов и полей класса, чтобы случайно не ошибиться; конструкции вида x(x) вызывают вопросы (нет ли здесь ошибки). Удобно с помощью code style гарантировать различное именование (например, добавляя уникальный префикс к полям классов)

 56     out << "Developer\nName: " << _name << "\n"
 57         << "Base Salary: " << _base_salary << "\n"
 58         << "Has bonus: " << (_has_bonus ? "+" : "-") << "\n";

здесь посередине может произойти fail, а потом опять все хорошо (место на диске закончилось и тут жен освободилось). В идеале нужно с этим что-то сделать. Например, обернуть в манипулятор, который проверит. По-нормальному же просить stream бросить исключение, как только что-то пошло не так.

Непонятно, почему в приведенном ниже методе ответственность за копирование лежит на нем (почему не передать во владение указатель?)
131 void EmployeeArray::add(const Employee *empl) {
132 _employees.push_back(empl->clone());
133 }

[Карнаухов Кирилл]

Надеюсь, я нормально использовал try/catch и не набагал в этом.

[Sokolov Viacheslav]

.exceptions(std::istream::failbit)
стоит также badbit перехватывать

Исключение можно принимать по const-ссылке; в случае, если что-то пошло не так, код возврата из main не должен быть 0

62 in.read(&char_value, 1);

можно же просто in.read(char_value)?
(и с write та же история)

Рекомендую посмотреть на ​​https://en.cppreference.com/w/cpp/io/basic_istream/getline

Если хочется что-то свое кинуть, то лучше кидать std::exception. Например, std::logic_error.

177 SalesManager? empl;
178 in >> empl;
179 empl_array.add(empl.clone());

проще:

SalesManager?* empl = new SalesManager?();
in >> *empl;
empl_array.add(empl);

[Sokolov Viacheslav]

На самом деле достаточно перехватывать const std::exception& (все стандартные исключениия отнаследованы от этого класса)

Не обратил внимание раньше, но read_and_add дублирует код с std::ifstream &operator>>(std::ifstream &in, EmployeeArray? &arr) , а также явно использует константы 1 и 2, что плохо, потому что при добавлении новых Employee или изменении значений 1,2 на что-то другое придется менять много где. Да и вообще в main оказалась какая-то неуправляющая логика, что тоже плохо.