Тестовое задание от компании Exness

@Pechkin80 · Регистрация: 14.03.2014

Author24 — интернет-сервис помощи студентам

У меня нет никакого вопроса, но я так и не понял в какую ветку надо написать (все изучил внимательно), поэтому пишу сюда.
Я просто хочу поделиться своим опытом трудоустройства на тот случай если это кому нибудь поможет и узнать мнение общественности.
Я получил тестовое задание от компании Exness и решил сделать его рефакторинг, а потом выполнил его и получил рецензию.
Всё во вложении.
Аннотация:
TA.2017.refactoring-optimization (2) (1).pdf - тестовое задание
MultiQueueProcessor.zip - код для рефакторинга
Описание_задачи.docx - моя версия рефакторинга самого задания
exness_testwork.zip - мой результат выполнения

Рецензия от lyubov.belogrudova@exness.com>: вот:
Юрий, добрый день! В пятницу не успела Вам написать.
Вас благодарим за внимание к нашей компании, за уделенное на тестовое задание время. Но, к сожалению, по результатам тестового задания делать оффер мы не готовы.
Пишу feedback от коллег:

Есть несколько основных проблемы присланного кода:
1. Читаемость и понимаемость кода стремится к нулю
2. Нет краткости, элегантности, какой-то красоты в присланном решении
3. Технически код выглядит очень старомодным:
a. Встречаются new/delete, unique_ptr/shared_ptr не найден
b. Обилие малооправданных "велосипедов"
c. С++11 очень мало по проекту

Вам успехов. Еси решение изменится, дам Вам знать.

@Pechkin80 · 09.08.2017, 12:48 **[ТС]**

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

Croessmah · 09.08.2017, 13:02

Сообщение от Pechkin80

Так что нахрена козе баян ?

Действительно. Нафига читать то что пишут.

Сообщение от Croessmah

Хотя если send_exit_event убьет процесс (не поток, а весь процесс),
то может и не надо заботится об освобождении ресурса,
хотя всё равно всё зависит от устройства.

@Pechkin80 · 09.08.2017, 13:11 **[ТС]**

Croessmah, send_exit_event сама по себе не убивает процесс, но запускает цепочку событий, окончанием которой является завершение процесса. Ваш упрёк не по адресу, так как я точно также могу сказать, а нафига я тут привёл программный код ? Её реализаия там приведена.

Croessmah · 09.08.2017, 13:23

Сообщение от Pechkin80

send_exit_event сама по себе не убивает процесс, но запускает цепочку событий, окончанием которой является завершение процесса.

Но мне, например, неизвестно что такое TLN и как оно устроено.
Оно может создать злополучный файл, который потом не убьет,
и получим какое-нибудь "проге не удается получить доступ к файлу блокировки".
И ладно если прога упала (линуксоидам знакома ситуация),
но если она не позаботилась об этом файле из-за того,
что кто-то забыл написать delete - то это ужас.

Сообщение от Pechkin80

Её реализаия там приведена.

Я буду лазить по всему коду в поисках того, как это реализовано?
Ради чего, спрашивается?
Тем более сегодня оно приводит к завершению процесса,
а завтра, решили что процесс должен устоять и продолжить работу,
возможно, запустив новый поток или еще как-то.
Процесс выжил, ресурсы утекли.
Я же написал - потенциальная утечка.
В задании сказано:

Сообщение от Pechkin80

Провести рефакторинг существующего кода так, чтобы класс было легко развивать
и поддерживать

А по факту получаем, что в одном месте поправили,
но при этом в другом утекать ресурсы начали.
Поддерживать и расширять такой код затруднительно.

В общем, смысла продолжать не вижу.

@Pechkin80 · 09.08.2017, 19:38 **[ТС]**

Croessmah, Но за желание потратить время на изучение моего программного кода большое спасибо. Я это очень ценю.

Добавлено через 11 минут

Тем более сегодня оно приводит к завершению процесса,
а завтра, решили что процесс должен устоять и продолжить работу,

Почему такую ситуацию я исключаю, я написал выше.
Croessmah, TLN не нужно развивать. Там много разных требований к реализации. Я выбирал программные решения которые учитывают всю совокупность требований.

Добавлено через 6 часов 1 минуту
А кто нибудь напишет тут:
1) почему умные указатели для многопоточной очереди это плохо
2)почему дублирование кода это не всегда плохо
3)почему "велосипеды" очень даже обоснованы в случае многопоточной очереди
А то совсем тоскливо както.

Croessmah · 09.08.2017, 19:42

Сообщение от Pechkin80

почему умные указатели это плохо

Ими нужно уметь пользоваться.
В остальном всё нормально с ними,
если они применяются там, где нужны.

Сообщение от Pechkin80

почему "велосипеды" очень даже обоснованы

Когда общее решение работает хуже,
а вам нужна узкоспециализированная приблуда
под конкретный случай, то можно и навелосипедить.

Кликните здесь для просмотра всего текста

@avgoor · 09.08.2017, 21:16

Croessmah, ТС писал:

Сообщение от Pechkin80

У меня нет никакого вопроса, но я так и не понял в какую ветку надо написать

Вероятнее всего он расчитывал на ответы в стиле: "ты молодец, а они придираются". Когда не прокатило... Бесполезно объяснять человеку, что "так он слона не продаст" если общение с ним в стиле "он д'Артаньян а мы ..."

@Pechkin80 · 10.08.2017, 12:34 **[ТС]**

Croessmah, Ну вот в том месте где вы показали применение умных указателей и тем способом для данной конкретной задачи, вы считаете они уместны ?

Добавлено через 1 минуту
avgoor, Вы меня разоблачили)))

Добавлено через 14 часов 2 минуты
Единственный пункт, где мне возможно стоит посмотреть как это делается у других это читаемость и понимаемость кода. Что надо сделать с кодом ? Только не говорите про комментарии. Их там не хватает потому что времени не было больше на данное тестовое задание.

@Pechkin80 · 27.10.2017, 22:41 **[ТС]**

ЧуднО будоражить старые темы, но всё же вопрос timeoverhead-a не давал мне покоя всё это время и я наконец, причесав код, и сделав рефакторинг всей архитектуры, хотел бы выложить новую версию, а заодно и статиситику. Собственно дело в том, что работодатель запросил максимальную производительность, а когда увидел код начал плеваться, мол где умные указатели.
остальные претензии про велосипеды тоже не в кассу(ИМХО).
новая_версия
статистика

@Pechkin80 · 27.10.2017, 23:03 **[ТС]**

Забыл...
чтобы работало надо буст пропатчить.
Файлы во вложении.

@Kastaneda · 28.10.2017, 14:51

Сообщение от Pechkin80

читаемость и понимаемость кода. Что надо сделать с кодом ? Только не говорите про комментарии.

Комментарии не улучшают читабельность кода, в общем случае они только захламляют код. А как ты считаешь, твой код легко читать и понимать?

Добавлено через 11 минут

Сообщение от Pechkin80

чтобы работало надо буст пропатчить.
Файлы во вложении.

а где патч? во вложении файлы с кодом

@Pechkin80 · 28.10.2017, 15:04 **[ТС]**

Kastaneda, Смотhя что. Тестбенч не оформлял. Остальной код написан аккуратно с комментариями. Не самую удачную архитектуру выбрал с самого начала и это наверно затрудняет читаемость кода. Чтото ещё кроме архитектуры и стиля кодирования может существенно повлиять на читаемость кода ?
Патч в последнем сообщении.

Комментарии не улучшают читабельность кода, в общем случае они только захламляют код.

Спорное утверждение. Но тема не об этом.

@Kastaneda · 28.10.2017, 15:07

Сообщение от Pechkin80

Патч в последнем сообщении.

Там не патч, там файлы, которые (похоже) нужно заменить. Интересно посмотреть на патч.

@Pechkin80 · 28.10.2017, 15:34 **[ТС]**

Kastaneda,
Наверно не тот случай когда важна терминология. Патч, в переводе на русский - заплатка.
Заменить файлы проще, но если надо просто посмотреть, то см. вложении. сделал 2 .diff файла

@Pechkin80 · 28.10.2017, 15:40 **[ТС]**

Только надо пути подправить. Утилита создала патч с абсолютными путями.

@Kastaneda · 28.10.2017, 19:42

Сообщение от Pechkin80

Наверно не тот случай когда важна терминология.

да вроде патч он и в Африке патч - создаешь утилитой patch (или похожей, или умной VCS) патч, потом этой же утилитой патчишь файлы.

А зачем понадобилось буст патчить? Решение с патчем явно попахивает.

@Pechkin80 · 28.10.2017, 19:46 **[ТС]**

Kastaneda, А вы попробуйте, без патча. Может и не нужен он)))

Croessmah · 28.10.2017, 20:08

Pechkin80, а что переделал?
Вот если бы был патч, сразу были бы видны изменения.

@Pechkin80 · 28.10.2017, 20:20 **[ТС]**

Croessmah, патччи бесполезны когда уже иная архитектура. Там почти всё по другому.

Добавлено через 7 минут
Croessmah, Я уже думую код вполне себе зрелый, хотя критика интересна, но интересно уже и куда эту самую многопоточную очередь впихнуть чтобы вышел программный продукт. Наверно в каких то серверах нужна, но вот в каких и где большой вопрос. времени и сил потрачено много так что есть интерес найти ей применение.

Croessmah · 28.10.2017, 20:33

C++

template <typename TValue, TTraitID _ID>
TMultiPipeProcessor<TValue,_ID>::
TMultiPipeProcessor(TThreadArgs & args) : m_args(&args)
{
    const char * funcname = __PRETTY_FUNCTION__;
    try
    {
        m_mngthread = nullptr;
        m_prodlist = new TProducerRecord[TLockNodeCodes::lock_last];
        initProdList(m_prodlist);
        m_expextedRemoveNode.store(nullptr);
        m_mngthread = new ManagedThread(&TMultiPipeProcessor::Process,this,boost::ref(args));
    }
    catch (...)
    {
        this->~TMultiPipeProcessor();
        errspace::show_errmsg(funcname);
    }
}

1) Возможна утечка
2) Вызов деструктора в конструкторе?
3) А деструктор еще и виртуальный.

Ух! Всё полетит классно в случае исключения.

Не, не буду портить Boost.

@Pechkin80 21 / 19 / 7 Регистрация: 14.03.2014 Сообщений: 249
	09.08.2017, 19:38 [ТС]	25
	Croessmah, Но за желание потратить время на изучение моего программного кода большое спасибо. Я это очень ценю. Добавлено через 11 минут Тем более сегодня оно приводит к завершению процесса, а завтра, решили что процесс должен устоять и продолжить работу, Почему такую ситуацию я исключаю, я написал выше. Croessmah, TLN не нужно развивать. Там много разных требований к реализации. Я выбирал программные решения которые учитывают всю совокупность требований. Добавлено через 6 часов 1 минуту А кто нибудь напишет тут: 1) почему умные указатели для многопоточной очереди это плохо 2)почему дублирование кода это не всегда плохо 3)почему "велосипеды" очень даже обоснованы в случае многопоточной очереди А то совсем тоскливо както. 0

Croessmah Неэпический 17870 / 10635 / 2054 Регистрация: 27.09.2012 Сообщений: 26,736 Записей в блоге: 1
	09.08.2017, 19:42	26
	Сообщение от Pechkin80 почему умные указатели это плохо Ими нужно уметь пользоваться. В остальном всё нормально с ними, если они применяются там, где нужны. Сообщение от Pechkin80 почему "велосипеды" очень даже обоснованы Когда общее решение работает хуже, а вам нужна узкоспециализированная приблуда под конкретный случай, то можно и навелосипедить. Кликните здесь для просмотра всего текста 0

@Kastaneda 5231 / 3204 / 362 Регистрация: 12.12.2009 Сообщений: 8,113 Записей в блоге: 2
	28.10.2017, 14:51	31
	Сообщение от Pechkin80 читаемость и понимаемость кода. Что надо сделать с кодом ? Только не говорите про комментарии. Комментарии не улучшают читабельность кода, в общем случае они только захламляют код. А как ты считаешь, твой код легко читать и понимать? Добавлено через 11 минут Сообщение от Pechkin80 чтобы работало надо буст пропатчить. Файлы во вложении. а где патч? во вложении файлы с кодом 0

@Pechkin80 21 / 19 / 7 Регистрация: 14.03.2014 Сообщений: 249
	28.10.2017, 15:04 [ТС]	32
	Kastaneda, Смотhя что. Тестбенч не оформлял. Остальной код написан аккуратно с комментариями. Не самую удачную архитектуру выбрал с самого начала и это наверно затрудняет читаемость кода. Чтото ещё кроме архитектуры и стиля кодирования может существенно повлиять на читаемость кода ? Патч в последнем сообщении. Комментарии не улучшают читабельность кода, в общем случае они только захламляют код. Спорное утверждение. Но тема не об этом. 0

@Pechkin80 21 / 19 / 7 Регистрация: 14.03.2014 Сообщений: 249
	28.10.2017, 15:40 [ТС]	35
	Только надо пути подправить. Утилита создала патч с абсолютными путями. 0

@Pechkin80 21 / 19 / 7 Регистрация: 14.03.2014 Сообщений: 249
	09.08.2017, 12:48 [ТС]	21
	Croessmah, Там выводиться сообщение об ошибки, потом идёт посылка события о завершении потока, которое будет передано в главный поток. После того как главный поток получит событие, весь процесс будет убит. Освобождать память при её нехватки и продолжать работу плохая идея, так как ситуация с нехваткой памяти в общем случае - это признак неудачной программной архитектуры, и тут главное вовремя исправить ситуацию. Так что нахрена козе баян ? 0

Croessmah Неэпический 17870 / 10635 / 2054 Регистрация: 27.09.2012 Сообщений: 26,736 Записей в блоге: 1
	09.08.2017, 13:02	22
	Сообщение от Pechkin80 Так что нахрена козе баян ? Действительно. Нафига читать то что пишут. Сообщение от Croessmah Хотя если send_exit_event убьет процесс (не поток, а весь процесс), то может и не надо заботится об освобождении ресурса, хотя всё равно всё зависит от устройства. 0

@Pechkin80 21 / 19 / 7 Регистрация: 14.03.2014 Сообщений: 249
	09.08.2017, 13:11 [ТС]	23
	Croessmah, send_exit_event сама по себе не убивает процесс, но запускает цепочку событий, окончанием которой является завершение процесса. Ваш упрёк не по адресу, так как я точно также могу сказать, а нафига я тут привёл программный код ? Её реализаия там приведена. 0

Croessmah Неэпический 17870 / 10635 / 2054 Регистрация: 27.09.2012 Сообщений: 26,736 Записей в блоге: 1
	09.08.2017, 13:23	24
	Сообщение от Pechkin80 send_exit_event сама по себе не убивает процесс, но запускает цепочку событий, окончанием которой является завершение процесса. Но мне, например, неизвестно что такое TLN и как оно устроено. Оно может создать злополучный файл, который потом не убьет, и получим какое-нибудь "проге не удается получить доступ к файлу блокировки". И ладно если прога упала (линуксоидам знакома ситуация), но если она не позаботилась об этом файле из-за того, что кто-то забыл написать delete - то это ужас. Сообщение от Pechkin80 Её реализаия там приведена. Я буду лазить по всему коду в поисках того, как это реализовано? Ради чего, спрашивается? Тем более сегодня оно приводит к завершению процесса, а завтра, решили что процесс должен устоять и продолжить работу, возможно, запустив новый поток или еще как-то. Процесс выжил, ресурсы утекли. Я же написал - потенциальная утечка. В задании сказано: Сообщение от Pechkin80 Провести рефакторинг существующего кода так, чтобы класс было легко развивать и поддерживать А по факту получаем, что в одном месте поправили, но при этом в другом утекать ресурсы начали. Поддерживать и расширять такой код затруднительно. В общем, смысла продолжать не вижу. 0

@avgoor 1550 / 875 / 179 Регистрация: 05.12.2015 Сообщений: 2,555
	09.08.2017, 21:16	27
	Croessmah, ТС писал: Сообщение от Pechkin80 У меня нет никакого вопроса, но я так и не понял в какую ветку надо написать Вероятнее всего он расчитывал на ответы в стиле: "ты молодец, а они придираются". Когда не прокатило... Бесполезно объяснять человеку, что "так он слона не продаст" если общение с ним в стиле "он д'Артаньян а мы ..." 0

@Pechkin80 21 / 19 / 7 Регистрация: 14.03.2014 Сообщений: 249
	10.08.2017, 12:34 [ТС]	28
	Croessmah, Ну вот в том месте где вы показали применение умных указателей и тем способом для данной конкретной задачи, вы считаете они уместны ? Добавлено через 1 минуту avgoor, Вы меня разоблачили))) Добавлено через 14 часов 2 минуты Единственный пункт, где мне возможно стоит посмотреть как это делается у других это читаемость и понимаемость кода. Что надо сделать с кодом ? Только не говорите про комментарии. Их там не хватает потому что времени не было больше на данное тестовое задание. 0

@Kastaneda 5231 / 3204 / 362 Регистрация: 12.12.2009 Сообщений: 8,113 Записей в блоге: 2
	28.10.2017, 15:07	33
	Сообщение от Pechkin80 Патч в последнем сообщении. Там не патч, там файлы, которые (похоже) нужно заменить. Интересно посмотреть на патч. 0

@Kastaneda 5231 / 3204 / 362 Регистрация: 12.12.2009 Сообщений: 8,113 Записей в блоге: 2
	28.10.2017, 19:42	36
	Сообщение от Pechkin80 Наверно не тот случай когда важна терминология. да вроде патч он и в Африке патч - создаешь утилитой patch (или похожей, или умной VCS) патч, потом этой же утилитой патчишь файлы. А зачем понадобилось буст патчить? Решение с патчем явно попахивает. 0

@Pechkin80 21 / 19 / 7 Регистрация: 14.03.2014 Сообщений: 249
	28.10.2017, 19:46 [ТС]	37
	Kastaneda, А вы попробуйте, без патча. Может и не нужен он))) 0

Croessmah Неэпический 17870 / 10635 / 2054 Регистрация: 27.09.2012 Сообщений: 26,736 Записей в блоге: 1
	28.10.2017, 20:08	38
	Pechkin80, а что переделал? Вот если бы был патч, сразу были бы видны изменения. 0

@Pechkin80 21 / 19 / 7 Регистрация: 14.03.2014 Сообщений: 249
	28.10.2017, 20:20 [ТС]	39
	Croessmah, патччи бесполезны когда уже иная архитектура. Там почти всё по другому. Добавлено через 7 минут Croessmah, Я уже думую код вполне себе зрелый, хотя критика интересна, но интересно уже и куда эту самую многопоточную очередь впихнуть чтобы вышел программный продукт. Наверно в каких то серверах нужна, но вот в каких и где большой вопрос. времени и сил потрачено много так что есть интерес найти ей применение. 0