Lesson 142 (Servlet API. Знакомство с сервлетами)#1
Lesson 142 (Servlet API. Знакомство с сервлетами)#1Binary-Cat-01 wants to merge 3 commits intoKFalcon2022:for-prfrom
Conversation
| /** | ||
| * Преобразует объект бизнес-логики {@code Calculation} в dto-объект {@code CalculationDto}, | ||
| * для отправки пользователю. | ||
| */ |
There was a problem hiding this comment.
избыточная документация - она как лишние гандоны. Надежно, но портит ощущения
| calculationDto.setData(calculation.getData() | ||
| .stream() | ||
| .map(CalculationData::toString) | ||
| .collect(Collectors.toList())); |
There was a problem hiding this comment.
Тут же джава повыше 11? Тогда зачем изменяемая коллекция?
| var calculationDto = new CalculationDto(); | ||
|
|
||
| calculationDto.setType(calculation.getType() | ||
| .getValue()); |
There was a problem hiding this comment.
спорный момент. Если клиентом выступает фронтовая апликуха - она уж как-нибудь сама строковый литерал для енама смапит в то, что ей нужно. Если же у тебя сервер раздает фактическое отображение (html или что ты собрался в браузере рисовать) - зачем тебе вообще DTO? Конвертируй сразу доменный объект в страничку:)
На самом деле, расклад домен->дто->html тоже имеет право на жизнь, но не в этой же задачке на 4 строки кода. Тут просто не на чем пордемонстрировать разумность такого подхода
| .getValue()); | ||
|
|
||
| calculationDto.setData(calculation.getData() | ||
| .stream() |
There was a problem hiding this comment.
не пихай цепочки стримов в параметр. Всегда выглядит как колхоз и читается дерьмово
|
|
||
| calculationDto.setData(calculation.getData() | ||
| .stream() | ||
| .map(CalculationData::toString) |
There was a problem hiding this comment.
Строго говоря, toString() не особо предназначен для таких ситуаций. Так можно делать в совсем учебных задачках для начинающих, но здесь уже моветон. toString() ок для каких-нить логов, мб в каких-то редких случаях - как часть exception message. Но точно не в основной логике. Хотя бы потому что в логике может потребоваться несколько различных вариантов отображений в виде строки, в зависимости от контекста использования
Нужен строковый эквивалент объекта - выдели отдельный метод или класс, который будет превращать твою модель в строку
| .collect(Collectors.toList())); | ||
|
|
||
| calculationDto.setResult(calculation.getResult() | ||
| .toString()); |
There was a problem hiding this comment.
Зачем ты вообще отображаешь объект как строку - тоже открытый вопрос. Того же рода, что и конвертация енама в хрен пойми что:)
|
|
||
| @Override | ||
| public Calculation convert(CalculationRequest source) { | ||
| return calculationService.calculated(source.getType(), source.getData()); |
There was a problem hiding this comment.
Конвертер должен заниматься конвертацией - подготовкой моделей одного архитектурного слоя для использования в другом. Но не вызывать бинзес-логику
There was a problem hiding this comment.
В данном случае я вообще не вижу смысла в конвертере, проще в сервис два аргумента передать прямо в сервлете (или откуда ты этот конвертер вызываешь)
| package com.walking.servletpractice.exception; | ||
|
|
||
| public class ParsingException extends RuntimeException { | ||
| public ParsingException() { |
| @@ -0,0 +1,31 @@ | |||
| package com.walking.servletpractice.model; | |||
There was a problem hiding this comment.
тут можно потиху заводить мой любимый пакет - domain. Те же модельки, но для доменного слоя. А model оставить для dto и прочего вспомогательного говна
| */ | ||
| public class Calculation { | ||
| private final CalculationType type; | ||
| private final List<CalculationData> data; |
There was a problem hiding this comment.
дело хозяйское, но не инициализированный список - это предвестник неожиданного NPE:)
| import java.util.List; | ||
|
|
||
| /** | ||
| * Объект бизнес-логики отражающий конкретное вычисление. Инкапсулирует данные |
There was a problem hiding this comment.
Это что угодно, но не объект Объект бизнес-логики:)
Доменная моделька - да
| public static CalculationType getByValue(String value) { | ||
| for (CalculationType type : CalculationType.values()) { | ||
| if (type.getValue() | ||
| .equals(value)) { |
There was a problem hiding this comment.
такие переносы очень плохо читаются. Хочешь переносить - выдели в переменную. Но как по мне здесь и в одну строку норм - NPE негде упасть
| * Преобразует данные вычислений в строку вида: "type=value&data=value1,valueN&result=value" | ||
| */ | ||
| @Override | ||
| public String toString() { |
There was a problem hiding this comment.
все о том же. Не под эту задачу данный метод
|
|
||
| private void addResult(StringJoiner stringJoiner) { | ||
| stringJoiner.add("result" + NAME_VALUE_DELIMITER + result); | ||
| } |
There was a problem hiding this comment.
Я бы какой-нить отдельный класс-сериализатор выделил. Императивная сериализация внутри модели почти всегда стреляет в ногу
| * разделенных запятой(,). <p> | ||
| * Пример допустимой строки: type=multiplication&data=-1,0,1,-2.2,0.0,2.2 | ||
| */ | ||
| public class CalculationRequestParsingService { |
There was a problem hiding this comment.
По логике класса - это скорее какой-то десериализатор или нечто похожее
|
|
||
| List<CalculationData> calculationDataList = new ArrayList<>(); | ||
|
|
||
| for (String value : values) { |
There was a problem hiding this comment.
прям просится переписывание на стрим
| var calculationData = new CalculationData(doubleValue); | ||
|
|
||
| calculationDataList.add(calculationData); | ||
| } catch (NumberFormatException e) { |
There was a problem hiding this comment.
try-блок лучше минимизировать. Ты здесь сделал его на три строки вместо необходимой одной
| * {@code CalculationData} и {@code CalculationResult} | ||
| * @throws IllegalArgumentException если в списке data, менее 2-х значений | ||
| */ | ||
| public Calculation calculated(CalculationType type, List<CalculationData> data) { |
There was a problem hiding this comment.
calculated - не выглядит как глагол:)
|
|
||
| private CalculationResult calculate(CalculationType type, List<CalculationData> data) { | ||
| return switch (type) { | ||
| case ADDITION -> add(data); |
There was a problem hiding this comment.
просится паттерн вроде стратегии или команды. В таком наивном виде выглядит несурьезно:)
| return new CalculationResult(result); | ||
| } | ||
|
|
||
| private CalculationResult subtract(List<CalculationData> calculationData) { |
There was a problem hiding this comment.
в чем разница с List? Кроме самого факта наличия лишней обертки
| .reduce((a, b) -> a - b) | ||
| .orElseThrow(() -> getIllegalException(calculationData.size())); | ||
|
|
||
| return new CalculationResult(result); |
There was a problem hiding this comment.
new CalculationResult(result) - логичнее впихнуть в map() перед orElseThrow()
| return new CalculationResult(result); | ||
| } | ||
|
|
||
| private IllegalArgumentException getIllegalException(int actualSize) { |
There was a problem hiding this comment.
валидировать в домене - обычно дурная затея. Ты уже впустил кривые данные в святая святых
|
|
||
| Calculation calculation = calculationRequestConverter.convert(calculationRequest); | ||
|
|
||
| calculationStorageService.save(calculation); |
There was a problem hiding this comment.
Как будто этот сервис разумнее из CalculationService вызывать, а не здесь
| * Хранит историю вычислений и обеспечивает к ней доступ. | ||
| */ | ||
| public class CalculationStorageService { | ||
| private final List<Calculation> storage = new ArrayList<>(); |
There was a problem hiding this comment.
Сервлетное приложение по умолчанию многопоточное. Любая инмемори хранилка должна учитывать конкурентность доступа
| resp.getWriter() | ||
| .print(calculationDto.toString()); | ||
| } catch (RuntimeException e) { | ||
| resp.sendError(HttpServletResponse.SC_BAD_REQUEST); |
There was a problem hiding this comment.
то есть любой косяк в обработке - это клиентская ошибка? А ты неплох:)
400 обычно означает, что клиент отправил косячный запрос. Тип строку вместо числа или другие траблы с валидацией. У тебя же фактически что угодно может ляснуться
|
|
||
| if (historyValue != null) { | ||
| switch (historyValue) { | ||
| case "all" -> sendAllHistory(resp); |
There was a problem hiding this comment.
а к чему параметр? Что мешает на любой гет-запрос отдавать историю? Другой функциональности вроде не предусмотрено
|
|
||
| default -> resp.sendError(HttpServletResponse.SC_BAD_REQUEST); | ||
| } | ||
| } else { |
There was a problem hiding this comment.
я бы инвертировал и явный return сделал. else (не else if) - почти всегда лишний
В этот раз точно хотел реализовать простое решение)) Но уже после того как сделал, подумал, что наверное опять переусложнил. В целом ориентировался на код из репозитория 'test-http-client'.