Add files via upload#1
Conversation
First commit
| @Component | ||
| public class UserDAO { | ||
| final private String fileName = "jsonData.txt"; | ||
|
|
There was a problem hiding this comment.
jsonData.txt - Для json файлов принято расширение .json
| final String stringPosts = restTemplate.getForObject("https://api.privatbank.ua/p24api/pubinfo?json&exchange&coursid=5", String.class); | ||
|
|
||
| return stringPosts; | ||
| } |
There was a problem hiding this comment.
getForObject это блокирующая функция, изза нее будут медленно работать запросы при нагрузках. Тут есть два варианта: либо сделать ее асинхронной, либо сделать отдельный тред, в котором раз в минуту (или раз во сколько он обновляется) спрашивать API приватбанка и сохранять результат. Первый способ лучше работает у API, которые часто обновляются, второй у API, которые обновляются в обозначенные промежутки времени.
| boolean tf = false; | ||
| if (str.contains("{\"email\":\"" + user.getEmail() + "\",\"password\":\"" + user.getPassword() + "\"}")) { | ||
| tf = true; | ||
| } |
There was a problem hiding this comment.
Лучше поменять на
boolean tf = str.contains(твое условие)
Название переменной tf скорее неудачное - без контекста не понятно, что она отображает
Опять же повторюсь, есть готовые библиотеки для работы с json, так что так проверять лучше не стоит.
| fileWriter.write("[" + user.toString() + "]"); | ||
| } else fileWriter.write("\b" + "," + user.toString() + "]"); | ||
| } | ||
| fileWriter.close(); |
There was a problem hiding this comment.
-
Записывать все данные в один файл нежелательно. При логине любого пользователя или проверке его существования как в функции
check, придется итерироваться через весь файл, что дает сложность поиска O(n). -
Для работы с JSON файлами существуют готовые решения. Вручную это лучше не делать, так как это магнит для ошибок.
|
|
||
| @Controller | ||
| @RequestMapping("/user") | ||
| public class FirstController { |
There was a problem hiding this comment.
Имя не отображает содержимое класса. "UserController", например, в данном случае смотрится гораздо лучше.
| @Autowired | ||
| private UserDAO userDAO; | ||
|
|
||
| @Autowired | ||
| private RateDAO rateDAO; |
There was a problem hiding this comment.
Внедрение зависимостей через поле - возможный вариант, но лучше отдать предпочтение внедрению через конструктор. Это, в частности, сильно упростит тестирование класса (например, внедрение моков)
| @GetMapping("/BTC") | ||
| public String BTCPage(Model model) { | ||
| double result = rateDAO.getRate(); | ||
| model.addAttribute("result", result); | ||
| return "BTC"; | ||
| } |
There was a problem hiding this comment.
Даный эндпойнт сильно отличается от предыдущих по контексту. Заявка на нарушение SRP) Думаю, стоит вынести его в отдельный контроллер (напр. RateController, BitcoinController, ...)
| private String ccy; | ||
| private String base_ccy; | ||
| private double buy; | ||
| private double sale; |
There was a problem hiding this comment.
Возможно тут подвела автогенерация кода (в частности, автогенерация геттеров\сеттеров), тем не мение, очень не привычно видеть переменные класса внизу, после объявления методов, а не внчале описания класса, что является классическим вариантом.
| public class User { | ||
| public void setPassword(String password) { | ||
| this.password = password; | ||
| } |
There was a problem hiding this comment.
Честно говоря, данный класс выглядит достаточно хаотично - слишком неупорядочены объявления членов класса. Классическая схема объявления членов класса: статические поля, переменные класса, конструкторы класса (начиная с базового), публичные методы, приватные методы, переопределённые методы класса Object (equals, hashCode, toString).
| private void parse() { | ||
| String stringPosts = this.getFromAPI(); | ||
| int alreadyPassed = 0; | ||
|
|
||
| for (int i = 0; i < numberOfRatesInAPI; i++) { | ||
| int start = stringPosts.indexOf("{", alreadyPassed); | ||
| int end = stringPosts.indexOf("}", alreadyPassed); | ||
| alreadyPassed = end + 1; | ||
| String sub = stringPosts.substring(start, end + 1); | ||
| Gson g = new Gson(); | ||
| Rate r = g.fromJson(sub, Rate.class); | ||
| rates[i] = r; | ||
| } | ||
| } | ||
|
|
||
|
|
||
| public double getRate() { | ||
| this.parse(); |
There was a problem hiding this comment.
Снова отклонение от классического порядка объявление методов. Прослеживается интуитивный перенос особенностей С\С++ в Java). Т.е. сначала объявляется метод 1, потому что он вызывается в методе 2 (значит в момент нахождения в методе 2, компилятор уже должен располагать информацией о методе 1), при этом модификаторы доступа не берутся во внимание.
Тем не мение, такое ограничение на код Java не распространяется, и порядок объявления методов обычно обратный: публичный метод, а затем один за другим цепочка из приватных методов. Т.е. в данном случае более природным было бы следующее: getRate -> parse() -> getFromAPI()
|
|
||
|
|
||
| private String getFromAPI() { | ||
| final RestTemplate restTemplate = new RestTemplate(); |
There was a problem hiding this comment.
Думаю, тут также можно было бы задействовать механизмы DI.
| public class UserDAO { | ||
| final private String fileName = "jsonData.txt"; | ||
|
|
||
| public void save(User user) throws Exception { |
There was a problem hiding this comment.
Пробрасывание базового исключения - bad practice. Как минимум - в данной ситуации можно пробросить более понятное исключение (в частности, IOException), что даст больше информации "наверху", где мы соответственно его попытаемся обработать.
Однако более предпочтительным вариантом, я считаю, должна быть обработка исключения прямо в этом методе.
П.С. Такая же позиция и по отношению к другим местам в коде с аналогичной ситуацией.
| @Component | ||
| public class RateDAO { | ||
|
|
||
| final private int numberOfRatesInAPI = 4; |
There was a problem hiding this comment.
Подобные константы принято объявлять статическими, придерживаясь соответствующего именования, а именно UpperCasе, т.е:
private final static int NUMBER_OF_RATES_IN_API = 4;
3 participants
First commit