Добрый день. В общем тут такое дело, устраивался недавно в одну контору, где в почете многопоточность. Экспертом себя никогда не считал, но и не полный нуб в этой теме. Дали тестовое задание сделать, сделал - отписались, мол вы нам не подходите, потому что "There were some critical errors in the task which in real multithreading environment wouldn't work correctly". Естественно без деталей:) И вот чето меня так цепануло по проф пригодности что места не могу найти. Если есть возможность, сделайте ревью кода и пните меня в эти критикал эррорс, ну и вообще жесточайшая критика приветствуется:). Задание в принципе не сложное - тривиальный сервис по обработке входящих данных, накоплению, и возвращению результата(ТЗ прилагается - там всего 2 листика). Напишите мне на мыло в профиле если есть желание(тема может быть интересна многим) - вышлю зипку с проектом, ну так чисто мозг размять. Проект мавеновский, из зависимостей только guice, кода немного - старался сделать хорошим). Если вы в одном городе со мной - щедро угостил бы пивом, но сомневаюсь что вы оттуда:)
В общем если есть желание - you are welcome.

Думал на форум не влезет - ан нет) Спасибо что откликнулись.

А, да еще пару уточнений - собирайте под java 1.6(изза @Override над методами интерфейсов) и я когда посылал задание уточнил - что в компоненте Storage -сознательно упростил некоторые вещи, вроде проверок на нулл, так как считаю что он внутренний по отношению к системе, и считаю его доверенным.

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

Да, вы правы, сказывается отсутствие опыта по работе с финансами, доменную модель сделал сразу, а потом благополучно забыл про таймстамп. Сконцентрировался на строчке в ТЗ - "Quotes are coming in a natural order, that is timestamp of a next quote is always bigger than timestamp of a previous one." И наивно полагал что локальное время сервера - это таймстамп квоты, лажанул короче. Ну вот походу и вторая ошибка, спасибо - хоть полезное почерпнул.

Blazkowicz Пока trendbar не закрыт (например поток, который его закрывает, решил немного поголодать) все квоты идут в старый trendbar.



schwaЗакрытие в одном потоке, а апдейт в другом.
TrendBar




Что будет в closeClose на момент закрытия неизвестно.

Да, вот это все по сути и есть та ошибка, которую я нашел. Сначала делал однопоточную версию, а потом решил добавить асинхронность. Суть идеи была такова - на каждый Symbol - завести по экзекьютору с пулом в один поток, и сабмитить CloseTask и HandleTask в этот самый эзекьютор - тогда бы они гарантированно выполнялись по очереди и я бы экономил на синхронизации, но closeTask забыл доработать, а учитывая косяк с таймстампом, указанный выше - да, соглашусь это действительно критикал эрроры и код не будет работать корректно. Ну хоть душа спокойна :).
Сказывается что у меня не было реального проекта с многопоточностью, точнее был но я там был один и варился в собственном соку, вот и хотел уйти в эту область чтобы прокачаться - ну что ж, пока не судьба.

BlazkowiczЗакрытие нет смысла делать скедулерами. Пришла - квота. Проверили последний TrendBar, попадает в него - ОК. Не попадает, наделали пустых TrendBar-ов чтобы закрыть предыдущий период, и потом сделали текущий в него записались и всё.
В этом случае будет только с history проблема. Новые трендбары не будут наполнятся, пока не придёт квота. Сдругой стороны а нафига нужны пустые трендбары в системе. 8)

Да! Я выбирал между этими двумя вариантами кстати, остановился на своем как более логичном чтоли - чтобы код был красивее, и не было проблемы с хистори. Даже хотел вопрос им написать - но не написал).

Всем спасибо) Получил ценный опыт, есть еще комментарии по улучшению, по качеству кода и тп? Ну если по многопоточности еще что-то найдете - тоже кул)

BlazkowiczНе совсем про многопоточность но ещё сильно напрягает обращение с датами в Period + PeriodType.
Period.<init> - new Date()
PeriodType.calculateEndPeriodDate() - Calendar.newInstance(), чтобы отсчитать от new Date(), который создали выше?

Да - согласен, неинуитивно? Или все-таки неправильно? Как бы вы поступили? Долго думал над этим моментом.


насчет авторИ вот это озадачило окнчательно.
new Date(currentCalendar.getTime().getTime())

Наследие того что в java дату - мьютабл, машинально написал похоже, хотя может и была причина - если честно не помню)

BlazkowiczВариант с паралельным CloseTask никак не может быть проще.
Как сейчас - нужно синхронизировать гонки между CloseTask и HandleTask.
Даже если отказаться от ScheduledExecutor-а и обрабатывать CloseTask в единственном потоке TaskBar, то всё равно не понятно как его синхронизировать с очередью HandleTask. Потому что CloseTask надо впихнуть в правильное место очереди.

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

schwaЕще. Тоже в закрытии.



У нас минутный интервал работает закрывается вместе с часовым и теряем мы просто trendBar. Ибо храним их в не в потокобезопастном LinkedList.

Я сейчас подумаю над этим - с ходу не понял, можете на пальцах пока я подумаю?

Blazkowiczзабыл никВсем спасибо) Получил ценный опыт, есть еще комментарии по улучшению, по качеству кода и тп? Ну если по многопоточности еще что-то найдете - тоже кул)
Очень напрягает обилие chained вызовов. Они ухудшают читаемость. А в случае NPE делают невозможным анализ лога.
storage.get(trendBar.getSymbol()).get(trendBar.getPeriodType()).add(trendBar)
5 вызовов методов. Угадай кто был null.
2 get метода - если там вдруг одинаковая коллекция и в ней произошло исключение, тоже нельзя сказать кто же из двух это был.

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