WW#11

[Sokolov Viacheslav]

╰─>$ make
mkdir -p obj
g++ -O2 -g -Wall -Werror -std=c++11 -Iinclude -fsanitize=address -c -MMD -o obj/bin_manip.o src/bin_manip.cpp
src/bin_manip.cpp: In member function ‘virtual void write_le_int32::apply(std::ostream&) const’:
src/bin_manip.cpp:32:5: error: ‘assert’ was not declared in this scope
   32 |     assert(_v >= 0);
      |     ^~~~~~
src/bin_manip.cpp:2:1: note: ‘assert’ is defined in header ‘<cassert>’; did you forget to ‘#include <cassert>’?
    1 | #include "bin_manip.h"
  +++ |+#include <cassert>
    2 |
src/bin_manip.cpp: In member function ‘virtual void write_c_str::apply(std::ostream&) const’:
src/bin_manip.cpp:45:17: error: ‘strlen’ was not declared in this scope
   45 |     s.write(_v, strlen(_v) + 1);
      |                 ^~~~~~
src/bin_manip.cpp:2:1: note: ‘strlen’ is defined in header ‘<cstring>’; did you forget to ‘#include <cstring>’?
    1 | #include "bin_manip.h"
  +++ |+#include <cstring>
    2 |
src/bin_manip.cpp: In member function ‘virtual void read_c_str::apply(std::istream&) const’:
src/bin_manip.cpp:77:14: error: comparison of integer expressions of different signedness: ‘int’ and ‘const size_t’ {aka ‘const long unsigned int’} [-Werror=sign-compare]
   77 |         if(i == _cap) {
      |            ~~^~~~~~~
cc1plus: all warnings being treated as errors
Makefile:17: recipe for target 'obj/bin_manip.o' failed
make: *** [obj/bin_manip.o] Error 1

[Sokolov Viacheslav]

╰─>$ make
g++ -O2 -g -Wall -Werror -std=c++11 -Iinclude -fsanitize=address -c -MMD -o obj/bin_manip.o src/bin_manip.cpp
src/bin_manip.cpp: In member function ‘virtual void read_c_str::apply(std::istream&) const’:
src/bin_manip.cpp:78:14: error: comparison of integer expressions of different signedness: ‘int’ and ‘const size_t’ {aka ‘const long unsigned int’} [-Werror=sign-compare]
   78 |         if(i == _cap) {
      |            ~~^~~~~~~
cc1plus: all warnings being treated as errors
Makefile:17: recipe for target 'obj/bin_manip.o' failed
make: *** [obj/bin_manip.o] Error 1

[Sokolov Viacheslav]

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

отсутствует undef для макросов в main

Для сообщений об ошибках есть специальный поток std::cerr

void add(const Employee *e)
такой интерфейс порождает неудобство, требуется копирование. Проще было бы исправить сам интерфейс (принимать объект во владение). В неучебных целях здесь стоило бы использовать std::unique_ptr.

Не все манипуляции проверяются на успешность.

std::cout << a << b;
Происходит 2 операции вывода. Первая операция может не получиться, а вторая - получиться. Флаги нужно проверять после каждой операции вывода (что конечно же затруднительно).
Если для записи это не так важно, то вот со чтением все хуже:
in >> _name >> _base_salary >> _has_bonus;
после ввода
aba caba true
флаги будут гласить, что ввод успешно произошел.
По этой причине гораздо проще иметь дело с исключениями.
Если их в арсенале нет, можно использовать манипуляторы, которые возьмут проверку на себя.

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

static Employee* get_by_type(int type);
эта функция форсирует знание в базовом классе Employee всех своих наследников. Обычно такого стараются избегать, потому что это мешает изолировать классы друг от друга.

Кроме того, константы 1 и 2 многократно повторяются в коде, при добавлении нового Employee или модификации уже имеющихся придется много всего менять.

[Igor Engel]

fixed

fixed

fixed

В условиях лабы есть "Экземпляр класса EmployeeArray? владеет всеми своими сотрудниками.", я интепретирую это так, что неважно что произойдёт с объектом на под указателем из add, сотрудник всё-ещё должен остаться в массиве, следовательно требуется копирование.

Переделал на эксепшены

Использовал

Функция создания сотрудника по типу нужна в любом случае, и если не городить систему с "регистрацией" типов сотрудников, то она может быть только захордкоженной. Так-что единственный выбор который я тут вижу - делать её статической функцией класса, или свободной функций. И мне кажется, что она всё-таки должна быть в классе, хоть это и повышает связность. При добавлении сотрудников править её придётся в любом случае.

Вынес в константы

[Sokolov Viacheslav]

Владеет - значит отвечает за время жизни объекта. Вопрос в семантике метода add. Исходно в задании на этот метод нет никаких ограничений, но можно добавить смысл "при вызове этого метода объект передается во владение". Так, например, сделано в unique_ptr: ​https://en.cppreference.com/w/cpp/memory/unique_ptr/reset . При этом вызывающая сторона должна гарантировать корректность (например, нельзя передать во владение указатель со стэка). С такой семантикой метод clone становится ненужен.

Проблема фундаментальная. Ключевое слово "фабрика".
Вот какие-то тексты на эту тему, хорошего текста к сожалению не нашел быстро.
​http://cpp-reference.ru/patterns/creational-patterns/factory-method/
​https://habr.com/ru/post/129202/
Ключевая мысль - сделать класс, который отвечает за mapping index <--> type . Без шаблонов не реализовать "нормально". В минимальном виде это "захардкоженное" отображение. Базовый класс при этом про наследников не знает.

[Igor Engel]

Ок.

Допустим, вынес в свободную функцию, формально сам класс теперь не знает о наследниках.
Создание класса фабрики ради одного stateless метода выглядит странно, а создание иерархии фабрик в данном случае не даёт никакого эффекат, так-как надо как-то создавать фабрику правильного типа.

[Sokolov Viacheslav]

230 s << (i+1) << ". " << emp->job_name() << " " << "\n"
здесь пробел перед переводом строки не нужен

забыт #undef WITH_EXCEPTIONS