Skip to content

Develop/bjs2 77785 #3074

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 12 commits into
base: dev
Choose a base branch
from
Open

Develop/bjs2 77785 #3074

wants to merge 12 commits into from

Conversation

flower25-hub
Copy link

No description provided.

Copy link

@Hitman-nn Hitman-nn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Юлия, реально советую еще раз почитать и пересмотреть, что есть по лекциям, и хотя бы схематично понять как это работает, далее еще больше закапаешься и будет совсем не понятно. что откуда берется.
Так же по заданию: читай и делай прям по предложениям, сейчас все описано очень подробно что и как. У тебя пока по моему субъективному мнению общего понимании картины нет, ты валишь все в кучу.

EducationController.addEducation();
}
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Данный класс не надо трогать и что-то менять. Для чего были сделаны изменения? Думаю стоит вернуть все назад к первоначальной версии.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Проверяла. что задачка работает

Comment on lines 15 to 16
public static void setId() {
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Это тут лишнее, метод без какой-либо логики


public void setUser(User user) {

}
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


public class DataValidationException extends RuntimeException {
public DataValidationException(String massage) {
super(String.valueOf(message));

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Зачем так? просто передать message, оно же и есть String

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

да, всё правильно) Я так сначала и указывала просто super(message), но у меня выдавало ошибку и в рекомендациях подсвечивало именно так написать.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ну если его написать с тем же названием, то будет все ок)

import school.faang.user_service.dto.EducationDto;
import school.faang.user_service.entity.Education;

@Mapper(componentModel = "spring")

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

стоит наверное обезопасить и добваить:
unmappedTargetPolicy = ReportingPolicy.IGNORE

@Service
@RequiredArgsConstructor
@Data
public class EducationService {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"Получить данные об образовании по id"
Вообще нет в классе


@Controller
@RequiredArgsConstructor
@Data

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Для чего тут?

@Data
public class EducationController {

private static final EducationService educationService = null;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Зачем ты инициализируешь как null? Почему статик? Не сработает.
просто
private final EducationService educationService;

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Согласна, это лишнее

public class EducationController {

private static final EducationService educationService = null;
private static long EducationDto;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Это тут зачем? Удалить стоит.

Comment on lines 18 to 29
public static void addEducation() {
EducationDto education;
education = null;
validateEducation(education);
educationService.addEducation(EducationDto, education);
}

private static void validateEducation(EducationDto education) {
if (education.getId() != 0) {
throw new DataValidationException("Education id is blank");
}
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Все в кучу. Все заявленные методы не реализованы.
Надо сделать примерно так:

    public EducationDto addEducation(long userId, EducationDto dto) {
        return educationService.addEducation(userId, dto);
    }

остальное по аналогии)

Comment on lines +20 to +21
@RequiredArgsConstructor
@Data

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

данные аннотации тут не требуются, давай уберем)

import school.faang.user_service.controller.education.EducationController;
import school.faang.user_service.dto.EducationDto;

import static java.lang.Long.parseLong;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

точно лишний импорт, посмотри, может еще есть те, которые не используются, их нужно убрать.
это можно сделать сочетанием ctrl + alt + o

public EducationDto addEducation(long userId, EducationDto dto) {
return educationService.addEducation(userId, dto);
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

лишний отступ


public EducationDto getById(long educationId) {
return educationService.getById(educationId);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

лишний отступ

@@ -21,6 +22,7 @@
@Table(name = "education")
public class Education {

public Object get;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

что это за поле?
кажется нужно убрать

}

User user = userRepository.findById(userId)
.orElseThrow(() -> new DataValidationException("User with id=" + userId + " not found"));

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. тут лучше подойдет EntityNotFoundException, т.к мы не можем найти сущность в бд.
  2. давай вместо конкатенации использовать String.format("User with id=%d not found", userId)
    для строковых значений используют %s

}

Optional<Education> existingEducation = educationRepository.findById(educationDto.getId());
if (existingEducation == null) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Optional existingEducation - не может быть null.
можно проверить через isEmpty()


Optional<Education> existingEducation = educationRepository.findById(educationDto.getId());
if (existingEducation == null) {
throw new DataValidationException("Образование не найдено");

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

тоже EntityNotFoundException и давай сообщение на английском напишем и добавим в сообщение id, по которому искали

throw new DataValidationException("Образование не найдено");
}

if (!(existingEducation.get().getId() == userId)) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

getId() - это точно userId а не уникальный id сущности Education ???

Comment on lines +62 to +66
Optional<Education> education = educationRepository.findById(educationId);
if (education.isEmpty()) {
throw new DataValidationException("Образование не найдено");
}
return educationMapper.toEducationDto(education.orElse(null));

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

можно написать более правильно и элегантно
return educationRepository.findById(educationId)
.map(educationMapper::toEducationDto)
.orElseThrow(() -> new DataValidationException("Образование не найдено"));

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants