Skip to content

add models to communicate with lecture interface 2#3

Draft
IlijazM wants to merge 16 commits intomainfrom
feature/bugfinder-backend
Draft

add models to communicate with lecture interface 2#3
IlijazM wants to merge 16 commits intomainfrom
feature/bugfinder-backend

Conversation

@IlijazM
Copy link
Copy Markdown
Collaborator

@IlijazM IlijazM commented Sep 27, 2022

@IlijazM IlijazM requested a review from maexled September 27, 2022 13:33
@delvh
Copy link
Copy Markdown
Contributor

delvh commented Sep 27, 2022

I'm vetoing against this PR.
You can't just introduce a new naming scheme without commenting it!
Also, why do the VM classes look like DTO classes?

@delvh
Copy link
Copy Markdown
Contributor

delvh commented Sep 28, 2022

@IlijazM there has been widespread confusion about this naming scheme.
You have to rename them. Your VM syntax is too confusing!

@delvh
Copy link
Copy Markdown
Contributor

delvh commented Sep 28, 2022

ERROR: update or delete on table "word" violates foreign key constraint "fkdkqevhvo9ou92q6bvqrtbgt1h" on table "bug"
Detail: Key (id)=(279f5b5a-0e2b-425b-93ee-245b199485bd) is still referenced from table "bug".
at de.unistuttgart.bugfinder.ConfigurationControllerTest.deleteBasicData(ConfigurationControllerTest.java:80)

good that we test. Deletion currently fails.

Comment on lines +96 to +141
public ConfigurationDTO build(final ConfigurationVM configurationVM) {
Set<Code> codesToPersist = new HashSet<>(configurationVM.getCodes().size());
for (CodeVM codeVM : configurationVM.getCodes()) {
List<Word> wordsToPersistToCode = new ArrayList<>(codeVM.getWords().size());
Set<Bug> bugsToPersistToSolution = new HashSet<>();
bugsToPersistToSolution = new HashSet<>();
for (List<WordVM> row : codeVM.getWords()) {
// check if the row only contains blank strings
// the first row will never contain only blank strings since it comes from the lecture interface
if (row.stream().allMatch(wordVM -> wordVM.getCorrectValue().isBlank())) {
continue;
}
// if it is not the first row in the code we add a new line
if (codeVM.getWords().indexOf(row) != 0) {
wordsToPersistToCode.add(wordRepository.save(new Word(null, "\n")));
}
for (WordVM wordVM : row) {
// check if the word is blank
// the first word will never be blank since it comes from the lecture interface
if (wordVM.getCorrectValue().isBlank()) {
continue;
}
// if it is not the first word in the row we add a space
if (row.indexOf(wordVM) != 0) {
wordsToPersistToCode.add(wordRepository.save(new Word(null, " ")));
}
String displayValue = wordVM.getDisplayValue() != null ? wordVM.getDisplayValue() : wordVM.getCorrectValue();
Word word = new Word(null, displayValue);
word = wordRepository.save(word);
wordsToPersistToCode.add(word);
if (wordVM.getErrorType() != null) {
Bug bug = new Bug(word, wordVM.getErrorType(), wordVM.getCorrectValue());
bug = bugRepository.save(bug);
bugsToPersistToSolution.add(bug);
}
}
}
Code code = new Code(null, wordsToPersistToCode);
codesToPersist.add(code);
Solution solution = new Solution(null, bugsToPersistToSolution, code);
code = codeRepository.save(code);
solution = solutionRepository.save(solution);
}
Configuration configuration = new Configuration(null, codesToPersist);
return configurationMapper.toDTO(configurationRepository.save(configuration));
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

way too long method, also javadoc missing

Comment on lines +171 to +216
public ConfigurationVM getViewModel(UUID id) {
log.debug("get configuration view model {}", id);
final Configuration configuration = getConfiguration(id);
final ConfigurationVM configurationVM = new ConfigurationVM(new ArrayList<>());
for (Code code : configuration.getCodes()) {
final CodeVM codeVM = new CodeVM(new ArrayList<>());
final Solution solution = solutionRepository
.findByCodeId(code.getId())
.orElseThrow(() ->
new ResponseStatusException(
HttpStatus.NOT_FOUND,
String.format("Solution with code id %s not found.", code.getId())
)
);

List<WordVM> row = new ArrayList<>();
for (Word word : code.getWords()) {
if (word.getWord().equals("\n")) {
codeVM.getWords().add(row);
row = new ArrayList<>();
continue;
}
Optional<Bug> bug = solution
.getBugs()
.stream()
.filter(b -> b.getWord().getId().equals(word.getId()))
.findFirst();
if (word.getWord().equals(" ") && bug.isEmpty()) {
continue;
}
final WordVM wordVM = new WordVM();
if (bug.isPresent()) {
wordVM.setErrorType(bug.get().getErrorType());
wordVM.setCorrectValue(bug.get().getCorrectValue());
wordVM.setDisplayValue(word.getWord());
} else {
wordVM.setCorrectValue(word.getWord());
}
row.add(wordVM);
}
codeVM.getWords().add(row);
configurationVM.getCodes().add(codeVM);
}

return configurationVM;
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

too long method, split it up, add java doc

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Also, misused Optional use.

Copy link
Copy Markdown
Contributor

@delvh delvh Sep 28, 2022

Choose a reason for hiding this comment

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

And equals(" ") or equals("\n") WON'T scale well!
Those are error prone AF.
It's probably best to extract them into methods.

@IlijazM IlijazM changed the title add vm to communicate with lecture interface add models to communicate with lecture interface Sep 28, 2022
@madebyTimo madebyTimo requested review from delvh and maexled September 29, 2022 07:45
@IlijazM IlijazM changed the title add models to communicate with lecture interface add models to communicate with lecture interface 2 Sep 29, 2022
@IlijazM IlijazM marked this pull request as draft September 29, 2022 10:35
@sonarqubecloud
Copy link
Copy Markdown

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.

3 participants