×Закрыть

Нужна критика Java пет-проекта

Всем привет!
Написал вот такой пет проект как начинающий Java разработчик: github.com/...abok/PerformanceAppraisal

Проект позволяет ставитьзадачи сотрудникам, вносить данные о прогрессе исполнения, рассчитывает производительность сотрудника / отдела / компании по данной задаче и высчитывает бонусы исполнителю за выполнение каждой задачи.

Я его делал как пет проект поэтому понимаю что:
1. Функционала явно недостаточно (буду дальше его расширять потихоньку);
2. Интерфейс в стиле «вырвиглаз» — мне так удобнее тестировать и он не является основной задачей;
3. Часть вычислений можно было решить на стороне БД, но я пока не силен с БД поэтому все вычисления делает сама программа.

Хотелось бы получить ответы (и критику конечно же) по следующим вопросам:
1. Достаточен ли уровень проекта чтобы добавлять его в резюме или необходимо писать что-то более масштабное?
2. Есть ли в проекте «косяки» за которые джуну стоило бы надавать по рукам (кроме пунктов, указанных в описании)? Если да — то какие?
3. Стоит ли продолжать работу над этим проектом или лучше написать еще парочку, но на другие темы / использование других технологий?

Заранее спасибо за любые ответы и за конструктивную критику — в первую очередь!

UPD: Много комментариев по техническим «косякам» — и за это большое всем спасибо. Но никто не ответил на вопросы 1 и 3. Это значит что проект мал и убог для того, чтобы его хотя бы в резюме показать? И джуна с таким пет проектом вы бы даже не рассматривали?

LinkedIn
Допустимые теги: blockquote, a, pre, code, ul, ol, li, b, i, del.
Ctrl + Enter
Допустимые теги: blockquote, a, pre, code, ul, ol, li, b, i, del.
Ctrl + Enter

Прогнати стандартні(можна трошки підтюнити штуки типу Demeter Rule) набори PMD, CheckStyle, FindBugs для початку.

Поскольку maven не поддерживается в исходниках то нужна еще инструкция как и на чём это собрать. Помните правило 3х секунд в вебе? Думаю что потенциальный code rewiewer тоже потеряет интерес к проекту и быстро переключиться на другой где можно сделать mvn jetty:run к примеру.

Плохой подход к написанию кода в целом: куча однострочных методов (причем без особых перспектив роста, не видно чтобы по задумке там потом стало больше), и много логики в методах на две страницы, которые делают несколько совсем разных логических операций в одной пачке кода.
Оба подхода плохи для продолжения разработки — в первом случае нужно просмотреть три десятка файлов чтобы найти ту строчку, которую хочешь поменть. Во втором — нужно долго искать нужное место, которое хочешь исправить.
Ну и — раз уж сам проект такой простенький, то обработка ошибок в духе «у вас тут что-то не так, разберитесь сами и исправьте» — непростительна, и так ничего по сути не сделано, так еще и пожалел лишние 20 минут на то, чтобы прописать нормальные сообщения.

В целом если ситуация «вот, неделю назад начал джаву учить, тестовый проектик пишу» — то нормально, первая сотня страниц кода у любого выглядит ужасно. Но если подавать это как «неплохо умею программировать, вот мой пет-проект» — то нет, совсем нет.

На github залили это вы молодец.
А теперь минусы:
— без мавена/грэдла нет ни малейшего желания сие чудо запускать. Комитить папку lib ? Так нельзя делать.
— Добавьте readme что это такое и чтобы можно было запустить 1-й командой
— где тесты?
— база данных есть/нет? где это все хранится.
— Именование: не DaoEmployee, a EmployeeDao. Когда у вас будет скажем 50 dao будет искать гораздо легче.
— <td bgcolor="#ff4500">Task</td> создайте CSS-ку

Вместо самодельного Dao лучше использовать SpringData.
JSP-ки можно сделать с Bootstrap-ом для красоты.

Очень много дорабатывать еще, показывать очень рано.

Да, до уровня джуна в Епам не явно не дотягиваю — это знаю :)

Я б сказав, що до рівня трейні будь-де ще як до неба пішки.

Перше, що кинулося в очі — imgur.com/IwZ6b6A

Честно говоря уж больно сильно бросается в глаза солянка классов *Command. Что Вы вообще ими хотели сказать, если это сервисы, то не лучшая идея использовать HttpServlet* параметры.
Как Вы это планируете тестировать. Про паттерн Builder слышали?
Может все таки нормальное MVC сделаете?
То, что есть сейчас в никакие ворота не лезет. Продолжать в таком виде не вижу смысла, еще больше запутаетесь и больше приучитесь писать «говнокод». Мне кажется лучше будет почитать mykong-a и сравнить с тем, что у вас есть сейчас.
И опять-же тесты, без них лучше никому не показывать)

return «/error.jsp»;
Это в ту же степь. Надо выбрасывать ексепшен, но ни возвращать страницу с файлом Оо
В командах вместо интерфейса
employee.setPassword(request.getParameter("password«));
Почему не хешируется пароль ?
Постоянные проверки не пустая ли длина, тоже огорчают
Что нельзя написать функцию в которую передавать сразу scope параметров, вместо проверки по одному
При логине кастомера, ты закрываешь connection с базой но не открываешь его.

Ответы на вопросы
1. Это похоже на то, что пишут на курсовых студенты. Никакого проку от этого нет. Когда друг устраивался на работу он показывал pull requests на игровой сервер. Я веду к тому, что обязательно свой проект иметь не обязательно. Все равно что-то стоящее создавать очень долго и накладно, особенно если делать это ради 1 секунды HR или технических спецов. Оптимизируй свою жизнь — это и сможешь показать
2.

Есть ли в проекте «косяки» за которые джуну стоило бы надавать по рукам (кроме пунктов, указанных в описании)? Если да — то какие?
Да. Нарушение DRY принципов, например
3.
Стоит ли продолжать работу над этим проектом или лучше написать еще парочку, но на другие темы / использование других технологий?
В чем интерес этого проекта? Ты как джун каждый день ставишь задачи сотрудникам и не можешь обойтись без этого????
Часть вычислений можно было решить на стороне БД, но я пока не силен с БД поэтому все вычисления делает сама программа.
Чувак откровенно говоря, всю программу можно было написать процедурами SQL :)

Интерес проекта — писать код. Предметная область выбрана не потому что я хочу помочь какому-то абстрактному эйчару, а потому что сам до того, как занялся изучением Джавы, более 10 лет занимался именно эйчаром (предметная область знакома и не надо на этапе придумывания задачи для пет проекта изобретать велосипед).
Подозреваю что можно было и процедурами SQL написать. Но с БД я пока дружу слабо. Поэтому и пишу то, что знаю лучше (хоть и с проблемами — но при написании процедур — уверен — проблем было бы больше, по крайней мере на этом этапе изучения).
Надеюсь, ответил на вопросы и спасибо за критику. Будем менять :)

boolean isTrue;

Интересная переменная. Судя из названия это должна быть константа

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

Взагалі-то починати треба було з мат.частини. А те що ти зробив — простенький бюрократичний імітатор, який може використовувати лише самий дебільний босс. Він навіть для рабовласника не згодиться.

Як учбовий проект це ще можна терпіти, але дуже сподіваюся що там ніколи не з′явиться справжніх людей. Мені їх шкода.

Это действительно учебный проект. Но ради интереса, а что такого «по дебильному написанного» в логике проекта? Вы считаете что нельзя оценивать результативность человека по соотношению план/факт выполнения им задач? Или вы считаете что в этом проекте я описал единственную и неповторимую систему оценки мне известную? Как-то так написали, что аж хочется подискутировать.

Я хочу сказать, что результативность человека оценивать так МОЖНО. Но электронные системы управления являются катализатором скорости, и к сожалению, ингибитором ответственности. Иными словами, ты написал убийцу процесса.

Сам ты пользоваться естественно можешь, потому что ты знаешь какая логика в системе есть, а какую должен думать ты сам. Другим людям давать простые системы в руки — всё равно что обезьяне калаш. Особенно если поощрение и наказание выполняются одной кнопкой.

PS. Если бы всё было так просто, руководитель был бы самой низкооплачиваемой профессией.

Вот так согласен почти полностью :)
Как я писал уже несколько раз (ну или подразумевал в словах «пет проект») — это НЕ есть рабочая система, призванная оценивать реальную работу реальных сотрудников где бы то ни было. По крайней мере она очень далека от такого состояния.
Про убийцу процесса: такие системы призваны оценивать задачи ориентированные на результат, «процессные» должности, как правило, оцениваются ставкой и субъективным мнением руководителя такой должности — все верно.
Но, похоже мы пришли к общему пониманию предметной области.
Можно пару слов по вопросам, озвученным в топике (особенно 1й и 3й)? Если есть пара минут конечно.

Давай ту же проблему кратко: ты катализируешь те причинно-следственные связи, которые и так отлично работали. Это имеет смысл делать только для массового контроля, где у 1 начальника допустим 100 линейных подчинённых. Например так контролируется приход-уход с работы — ты ж не можешь наполовину прийти.

И когда катализируешь достаточное количество показателей, это автоматически ингибирует контроль зависимостей этих самых показателей от других. Допустим, есть 10 показателей качества, и 1 количества. Контролируя количество, ты ударишь по качеству. В результате ты добавишь ЕЩЁ ОДИН показатель качества, но его вес будет не более 15% от веса количества. И всё, качество ты убил. А вместе с ним убил и количество — высокое качество давало высокую повторяемость и простоту обучения. Теперь представь, что показателей качества не 10, а 200, притом большинство из них никогда подчинённому не озвучиваются — их можно достать только у конечного потребителя.

И есть совсем уж сценарий катастрофы: когда подчинённые ЗНАЮТ о качестве, а руководитель — нет. Такое бывает сплошь и рядом. Раз ключевые показатели контролирует система, значит руководителем можно поставить некомпетентного человека со связями. И тогда подчинённые могут намеренно отомстить ему за самодурство — угробят показатели качества очень низко, и в тех местах которые сложно контролируются. Когда это заметят, будет уже поздно, продукт уже поступит к массовому потребителю!

PS. Я всё это видел вживую, в некоторых процессах участвовал. Поверь, зрелище не для слабонервных.

Знаю, тоже видел, слышал, участвовал, даже пытался исправить. :)
Подозреваю, что это можно частично интрепретировать как ответ на третий вопрос: «Чувак, не лезь туда уже со своими потугами автоматизации — там и без тебя тошно. Сходи лучше напиши какой-нибудь простенький текстовый процессор или XML’ ки поразбирай. Это тебе быстрее поможет найти работу.». Верно?

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

Да, и если обсуждать именно предметную область то наверное стоит заметить, что такая система оценки результативности подходит только для определенных категорий должностей и тем более для определенных категорий задач (только для тех задач, которые могут быть оценены количественно). Как пример: результативность швеи на сдельной форме оплаты труда. Ей необходимо за за месяц пошить 100 рубашек (план), она пошила 120 (факт). Результативность 120% — она заработала больше денег. Если же она пошила меньше запланированного кол-ва — получила меньше денег.
И конечно же есть другие подходы к оценке результативности, особенно там, где результат не может быть оценен количественно. Их в данном проекте действительно нет. И еще в нем нет (по крайней мере пока нет) кучи удобных и нужных фич, как пример: сигнал о перегруженности сотрудника задачами, сигнал о том, что у сотрудника нет (или мало) задач, подсчет общего бонуса сотрудника по всем задачам, подсчет бонусов отдела / компании в целом и т.д. Я собираюсь их дописывать потихоньку, со временем. Ведь основная задача этого приложения — показать что я могу писать код и могу оформлять свои мысли из предметной области в логику работы приложения.
Я действительно считаю, что такого уровня приложение просто не купит даже самый

дебільний босс
или
рабовласник
. Для них надо еще допилить модули с приковыванием цепью к рабочему месту, конвоированием строем на обед и обратно, ну и мотивационый модуль “дом плохо — работа круче, приходи к нам у мягенькие плеточки у надсмотрщиков”.

Система оценки результативности полезна всем. Но проблема систем именно в том, что они реализуют ПРОСТЫЕ зависимости (потому что программисту их делать проще, да и тех.задание по ним элементарное). А вот те части которые сложные — внезапно оказываются за рамками системы.

Смотри что получается в результате: людей контролируют именно по простым показателям, потому что это очень просто. И не контролируют более сложные. Раньше это делалось хоть иногда. Сейчас же может кто-то и вспомнит, что они есть, но... их нет в системе. А значит проблема из разряда периодической перерастает в хроническую, и неизменно убивает бизнес-процесс.

Дальше больше. Сам бизнес-процесс тоже контролируется показателями, и тоже простыми. А раз так, никто НЕ БУДЕТ решать тех вопросов, которых нет в системе. Просто УСИЛЯТ мотивацию: будут давать больше бонусы, и сильнее наказывать. И процесс немедленно даст отклик. Те кто получал много бонусов — будет получать их ещё больше, потому что теперь их показатели ещё выше. Те кто получал меньше — будут приносить... упс, сюрпрайз, меньше, притом меньше существенно. И либо будут уволены, либо уйдут сами.

А первыми уйдут лучшие. Именно те, кто решал сложные вопросы, пока система их игнорировала. И когда они уйдут, внезапно окажется что результат работы системы никому не нужен. Да, показатели красивые, но результат — не покупают!

Все верно. Действительно, более подходящая для адекватной оценки результативности, система будет гораздо более сложной и будет содержать гораздо более сложные как показатели, так и методы их интерпретации (библиотеку компетенций (неколичественных показателей), их оценку, выделять области для дальнешйго обучения сотрудника и его дальнейшего развития как специалиста, мотивационную карту сотрудника и соответствие задач его мотивации и многое-многое другое, которое плотно завязано на другие области управления человеческими ресурсами и в которых оценка является всего лишь одним из инструментов). Но это уже обсуждение вопроса в области видения процесса оценки персонала и соответствия каждой конкретной системы оценки целям бизнеса. Ведь если цель «срубить бабла пока рубится в отрасли, где нужно много делать и немного думать» — хватит и простой «варварской» оценки, а вот, например, в технологически сложном бизнесе, где мозги сотрудников реально являются активом компании, даже сложная система оценки с учетом максимального количества переменных, будет всего лишь вспомогательным инструментом и одним из нескольких аргументов для принятия управленческих решений.
Но что-то это уже сильно в сторону от моей просьбы оценить пет проект :)

Ну я оценил. Как учебный проект — хорошо. Как pet-project — убей пока он маленький. Иными словами, не разбираясь в мат.части — не создавай pet-project.

Люди — крайне взрывоопасная субстанция. Просто не рекомендую даже браться ни за что, что связано с мотивацией людей, пока тебе не стукнет тридцатник, а лучше все пятьдесят.

Единственная область где ты можешь это делать — игры. Вот там мотивировать можешь пробовать. Но там другое предостережение: ЕСЛИ у тебя начнёт получаться, УЙДИ из индустрии. Потому что игры, которые завязаны на правильной мотивации — это фактически медленные убийцы времени, то есть прожигатели жизни. На один доллар заработанный придётся потраченного времени игроков на десятки тысяч. Это почему ушёл я. В реальный мир. И сейчас у меня в руках проект, который мотивирует реальных людей совершать реальные действия.

В моем понимании пет проект = учебный проект.
Тридцатник уже стукнул, хотя до пятидесяти еще далеко (и это радует).
Спасибо за комменты. Очень интересно было пообщаться.

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

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

То бишь пет-проекты это «вторые мышки», которым достаётся бесплатный сыр. И чем быстрее развивается рынок, чем стремительнее гонка за инновациями — тем больше бесплатного сыра достаётся вторичным участникам рынка. Благодаря своей экономичности, пет-проекты не умирают. Они могут быть приостановлены на любое время — ведь у них нет инвесторов, а значит и нет обязательств. Они могут существовать и с обязательствами перед клиентами, и даже монетезироваться в плюс как MVP. Да, больших прибылей нет, но ключевая особенность именно петов — выживаемость.

Для примера: Фейсбук рождён как пет-проект. И он не растранжирил свою долю преждевременно. И не умер, как если бы рванул тратить деньги инвестора. Он пошёл в качество, и вышел в рынок на проверенных технологиях.

Ещё пример: МакДональдс — это пет-проект. Он вошёл на занятый рынок, дико конкурентный, но за счёт выявления качественной ниши ступень за ступенью покорил планету. А те кто стремился к быстрому росту — в основном так же быстро выдохлись.

Дальше, Apple — это пет-проект. Никто не создавал его учебным. Равно как никто не планировал захватить глобальный рынок. Просто была ниша, и её не спеша протестили. Apple и по сей день отстаёт в технологиях, но умеет хапнуть тренд именно на проверенных ценностях.

Смотри в чём фишка: мало кто хочет покупать у стартапов. И чем более наглый стартап, тем сильнее от него отворачиваются платежеспособные клиенты. А вот у пет-проекта покупать приятно. Потому что SEOшник пета не имеет много клиентов, однако ценит мнение тех, что имеет.

PS. DOU.UA — тоже пет-проект.

Спасибо за такой развернутый ответ. Было интересно почитать.

Загально
1. Назви пекеджів мають бути в нижньому регістрі
2. for ( int i = 0; i < collection.size(); i++ ) -> for ( T value : collection )
3. Якщо вже дійсно закривати ресурси, то у finally-блоках.
4. import static може мати позитивний вплив на читабельність.

Сервлети
1. Команди потрібно розділити і виокремити рівні контролера і сервісів, перший з яких мав би відповідати за взаємодію із зовнішнім світом (не обо’вязково сервлети+HTTP) і просто в лоб перетворювати запити і відповіді на/з сервера, а інший — власне, будувати логіку, і звертатися до DAO-шару у разі чого.
2. CommandFactory якась не дуже-то й фекторі. Більше щось типу як «command-локатор».
3. Команди можна було б розділити на щонайменше read- і на write-операції, що могло би мати позитивний наслідок для MainController, який незалежно від HTTP-запиту делегує виконання на той самий код.
4. Перевірка вхідних параметрів із запитів — там жесть. Щонайменше стрічки в константи повиносити + можливо, написати вручну маппери, які би запит перетворювали у «істівне» з точки зору сервісів.
5. Перевірка вхідних параметрів, можливо, нехай би краще викидала IllegalArgumentException, а вже би контроллер сам знав, як про це сповістити користувачу.

Можливо, варто дивитися в бік Spring MVC.

DAO, JDBC
1. DaoFoo -> FooDao; DaoBar -> BarDao.
2. Всі DAO-класи могли би мати спільного предка, окрім як java.lang.Object, для пристойності.
3. Жорстка зав’язка на синглтон DaoFactory загалом. Я би їх взагалі уникав і у клієнтський код просто би передавав посилання на фекторі, а чи вона синлтон чи ні — питання другорядне (з точки зору клієнтського коду, звісно).
4. Connection, ***Statement, ResultSet не закриваються.
5. Можна було б мати connection pool.
6. resultSet.getString("foo«) можна було б виносити в змінну і лише тоді порівнювати n-ну кількість разів
7. Перебір одного Iterable і переливання елементів з нього за умовою в інший Collection, по суті, є базовим алгоритмом filter, який реалізовано як в Stream API, так і в Google Guava (там навіть ще простіше + немає вимоги щодо Java 8)
8. Часто сутності (entity) для сервісів виду «прочитав з бази — віддав назовні» можна робити іммутабельними, передаючи аргументи через єдиний конструктор. (Хоча тут є спірні моменти з кількістю параметрів у таких конструкторів і з шаблоном Builder)
9. Витягувати запити з ресурсів, я думаю, все ж не дуже варто. («//», до речі, не є коментарем у properties).
10. DAO-рівень також мав би мати мінімум логіки (перераховування бонусів мало би бути десь на сервісному рівні). + Туди ж і локальні змінні, які ведуть себе як константи
11. Запити, такі як ADD_BONUSES_TO_DB, можна було б за допомогою квері-білдерів можна було б перетворювати в batch-INSERT-запити, якими одним махом можна відіслати все в базу.
12. Спірне по суті, але я би старався уникати методів, які повертають null (можливо, throw?). Або принаймі явно оголошувати такі методи як @Nullable (JSR-305)
13. Якщо потрібно перевірити наявність якогось об’єкта за ключами, не варто передавати в такий метод весь об’єкт. Наприклад, isExist(Employee): 1) не зрозуміло, що робиться із таким об’єктом (він мутабельний); 2) не зрозуміло, які його поля впливають на поведінку isExist(); 3) для виклику того метода потрібно взагалі спочатку інстанціювати Employee. Можливо, exists(login, password) краще? До речі, read(Employee) також дивний, оскільки приймає Employee, за потрібних умов його змінює, і його ж повертає. До речі, користувача варто було б однозначно ідентифікувати лише за логіном.
14. getDs() -> getDataSource()
15. ADD_BONUSES_TO_DB -> ADD_BONUSES (й так зрозуміло, що мається на увазі БД); TUNE_EMPLOYEE -> GET_EMPLOYEE?
16. catch (NamingException e) { /*logger.error( e); */ } -> загорнути в RuntimeException і викинути

Можливо, варто дивитися в бік Spring JDBC Template.

Фундаментальненько. Спасибо. Пока прочитал — аж мозг напряг. Вечерком обработаю ту часть, которую хотя бы понял.
P.S. Проект писал на том, что знаю. Про Спринг и его модули пока только слышал и его (вкупе с Хибернейтом) мне еще предстоит изучить.

PerformanceAppraisal/src/DAO/DaoFactory.java

...
public DaoEmployee getDaoEmployee() throws SQLException {
DaoEmployee daoEmployee = new DaoEmployee(ds);
...

public DaoStructure getDaoStructure() throws SQLException {
DaoStructure daoStructure = new DaoStructure(ds);
...

public DaoTask getDaoTask() throws SQLException {
DaoTask daoTask = new DaoTask(ds);
...

public DaoTaskArchive getDaoTaskArchive() throws SQLException {
DaoTaskArchive daoTaskArchive = new DaoTaskArchive(ds);
...

На каждый запрос к getXxx создаете новый инстанс сервиса, сделайте их синглтонами

PerformanceAppraisal/src/DAO/DaoStructure.java

public class DaoStructure {
private DataSource ds;
private Connection con;
...
con = DaoFactory.getInstance().getDs().getConnection();
...
зачем, если есть ds.getConnection()

Есть предложение поучаствовать в пет проекте, который будет релизиться как мобильное приложение. Не хватает именно java бекенда.

Предварительно готов. Напишите, пожалуйста, в bolviccon@gmail.com для дальнейшей связи.

PerformanceAppraisal/src/DAO/DaoEmployee.java

List<employee> employeesList = new LinkedList<>();
...
for (int index = 0; index < list.size(); index++) {
...
Здесь должен быть итератор

А есть весомые отличия в производительности между for, foreach и итератором при одном проходе коллекции?

Спасибо, ознакомился. Принял к сведению.

Може мати суттєвий ефект, зокрема linkedList.get(i) не є O(1). Ітератори самі по собі вже можуть бути оптимізовані для оптимальної ітерації. + for(each) все ж позитивно впливає на читабельність.

Т.е оптимальнымвариантомтаки будет for each? Заменим.

Подписаться на комментарии