Skip to content
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

Development: Replace if-else chains with pattern matching #9303

Open
wants to merge 24 commits into
base: develop
Choose a base branch
from

Conversation

SamuelRoettgermann
Copy link

@SamuelRoettgermann SamuelRoettgermann commented Sep 10, 2024

Checklist

General

Server

Motivation and Context

If-else chains aren't pleasant to read, nor are they particularly efficient, and since Java 21 we have pattern matching, so let's use that.
Switch-statements may be optimized to run in O(1), whereas if-else chains will typically lead to O(n) runtime complexity, however this isn't guaranteed as it depends on the respective Java implementation.
Either way, the benefit in terms of performance will probably be negligible, however the readability improvement alone justifies this change.

Description

Changed if-else-instanceof-chains to utilize pattern matching (introduced by Java 21) instead. No logic was changed.

Steps for Testing

Since this doesn't affect any logic, there are no observable changes.
Only ways to test are code review and running the server tests, which do all pass for me locally, except for 8 unrelated tests.

Review Progress

Code Review

  • Code Review 1
  • Code Review 2

Summary by CodeRabbit

  • New Features

    • Enhanced control flow in various services with modern switch expressions for handling different types of submissions and exercises.
  • Bug Fixes

    • Improved error handling for unsupported types in submission and exercise processing.
  • Refactor

    • Replaced multiple if-else statements with switch expressions across several classes, significantly improving readability and maintainability.
  • Tests

    • Updated test methods to reflect changes in control flow, ensuring clarity and consistency in handling different exercise types.

Copy link

⚠️ Unable to deploy to test servers ⚠️

The docker build needs to run through before deploying.

@github-actions github-actions bot added the deployment-error Added by deployment workflows if an error occured label Sep 10, 2024
@SamuelRoettgermann SamuelRoettgermann removed the deployment-error Added by deployment workflows if an error occured label Sep 10, 2024
@Strohgelaender
Copy link
Contributor

Nice. Is this the only place where the new style can be used? If not, can you convert other places as well?

@SamuelRoettgermann
Copy link
Author

Nice. Is this the only place where the new style can be used? If not, can you convert other places as well?

Good idea, I actually didn't check. I'll try to find other suitable places.

…e.java


Good advice to eliminate duplicate code
coderabbitai[bot]
coderabbitai bot previously approved these changes Sep 10, 2024
coderabbitai[bot]
coderabbitai bot previously approved these changes Sep 10, 2024
@SamuelRoettgermann
Copy link
Author

I think the solution to these long cases would be moving the case-content into its own method.

Good idea. I'll look into the really convoluted classes tomorrow and see where I can apply this technique, but for today I'm too exhausted.

If you see a class that seems too convoluted in your eyes (like src/main/java/de/tum/in/www1/artemis/service/exam/StudentExamService.java) please just mention it. I will also look over all classes, but multiple opinions can't hurt.

BBesrour
BBesrour previously approved these changes Sep 10, 2024
Copy link
Member

@BBesrour BBesrour left a comment

Choose a reason for hiding this comment

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

Nice addition! code lgtm except for a minor nitpick.

I agree with @Strohgelaender that we should move the case-content to its own method for long cases.

…java


improved variable naming

Co-authored-by: Mohamed Bilel Besrour <[email protected]>
@SamuelRoettgermann
Copy link
Author

Okay, I will now look at optimizing readability for long cases

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

Review details

Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE

Commits

Files that changed from the base of the PR and between 886f8b5 and b496140.

Files selected for processing (1)
  • src/main/java/de/tum/in/www1/artemis/service/exam/ExamService.java (1 hunks)
Additional context used
Path-based instructions (1)
src/main/java/de/tum/in/www1/artemis/service/exam/ExamService.java (1)

Pattern src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports

Additional comments not posted (3)
src/main/java/de/tum/in/www1/artemis/service/exam/ExamService.java (3)

896-922: Excellent refactoring to improve readability!

Replacing the if-else-instanceof chain with a switch expression makes the code more concise and easier to understand, while preserving the original behavior for each exercise type.

The IllegalArgumentException for unsupported exercise types is also handled well in the default case.


904-906: LGTM!

The TextExercise case correctly uses the TextSubmission type and checks if the submitted text is not null and not blank.


918-920: Verify the logic for QuizExercise submissions.

Unconditionally returning true for QuizExercise submissions without checking the actual submission content seems unexpected.

The comment mentions this is due to performance concerns and is handled differently. Could you please provide more context on how quiz submissions are validated for non-emptiness elsewhere in the codebase? This would help verify if this logic is indeed correct and appropriate.

coderabbitai[bot]
coderabbitai bot previously approved these changes Sep 11, 2024
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

Review details

Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE

Commits

Files that changed from the base of the PR and between b496140 and 4a139b7.

Files selected for processing (4)
  • src/main/java/de/tum/in/www1/artemis/service/exam/ExamService.java (1 hunks)
  • src/main/java/de/tum/in/www1/artemis/service/exam/StudentExamService.java (1 hunks)
  • src/main/java/de/tum/in/www1/artemis/service/export/DataExportQuizExerciseCreationService.java (2 hunks)
  • src/test/java/de/tum/in/www1/artemis/exam/StudentExamIntegrationTest.java (4 hunks)
Additional context used
Path-based instructions (4)
src/main/java/de/tum/in/www1/artemis/service/export/DataExportQuizExerciseCreationService.java (1)

Pattern src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports

src/main/java/de/tum/in/www1/artemis/service/exam/StudentExamService.java (1)

Pattern src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports

src/main/java/de/tum/in/www1/artemis/service/exam/ExamService.java (1)

Pattern src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports

src/test/java/de/tum/in/www1/artemis/exam/StudentExamIntegrationTest.java (1)

Pattern src/test/java/**/*.java: test_naming: descriptive; test_size: small_specific; fixed_data: true; junit5_features: true; assert_use: assertThat; assert_specificity: true; archunit_use: enforce_package_rules; db_query_count_tests: track_performance; util_service_factory_pattern: true; avoid_db_access: true; mock_strategy: static_mocks; context_restart_minimize: true

Additional comments not posted (8)
src/main/java/de/tum/in/www1/artemis/service/export/DataExportQuizExerciseCreationService.java (2)

103-106: LGTM!

The code changes correctly handle the case when a question is not answered and the submittedAnswer is null by skipping to the next question.


Line range hint 108-124: LGTM!

The switch expression improves the readability and maintainability of the code. Each case handles a specific type of submittedAnswer appropriately.

The error handling for the DragAndDropSubmittedAnswer case is correct, and the empty default case is acceptable since there are no other expected types of submittedAnswer.

src/main/java/de/tum/in/www1/artemis/service/exam/StudentExamService.java (2)

319-344: LGTM!

The saveSubmissionQuizExercise helper function looks good. It handles the quiz specific logic appropriately, ensuring data consistency and avoiding redundant saves.


296-316: LGTM! The switch improves readability.

Replacing the if-else chain with a switch expression improves the readability of the code. Each case handles the corresponding exercise type appropriately.

Using the saveSubmissionQuizExercise helper function for the QuizExercise case keeps the switch concise and readable.

src/main/java/de/tum/in/www1/artemis/service/exam/ExamService.java (1)

896-922: Refactored hasNonEmptySubmission using switch expression. LGTM!

The switch expression improves code readability compared to if-else chains. The logic for determining non-empty submissions for each exercise type looks correct.

src/test/java/de/tum/in/www1/artemis/exam/StudentExamIntegrationTest.java (3)

1375-1428: Refactoring to switch statement improves readability.

The refactoring of the if-else chain to a switch statement enhances code readability and maintainability. The cases cover the necessary exercise types (ModelingExercise, TextExercise, QuizExercise) and perform appropriate actions and assertions for each type.


1478-1509: Switch statement improves readability for submission assertions.

The switch statement used for handling different exercise types (ModelingExercise, TextExercise, QuizExercise, ProgrammingExercise) and performing assertions on the submissions improves code readability. The cases cover the necessary exercise types and perform appropriate assertions for each type.


1593-1617: Switch statement improves readability for versioned submission assertions.

The switch statement used for handling different submission types (TextSubmission, ModelingSubmission, FileUploadSubmission, QuizSubmission) and performing assertions on the versioned submission improves code readability. The assertions for each type are appropriate.

The special handling for QuizSubmission, using a non-strict deep JSON comparison after removing IDs, is a good approach to account for potential discrepancies in the order of submitted answers and the presence of IDs.

Comment on lines 256 to 317

// check that the submission belongs to the already saved participation
if (!existingParticipationInDatabase.getSubmissions().contains(submissionFromClient)) {
throw new AccessForbiddenException("User " + currentUser.getLogin() + " cannot submit a different submission " + submissionFromClient + " for participation "
+ existingParticipationInDatabase.getId());
}
// check that no result has been injected
if (submissionFromClient.getLatestResult() != null) {
throw new AccessForbiddenException("User " + currentUser.getLogin() + " cannot inject a result " + submissionFromClient.getLatestResult() + " for submission "
+ submissionFromClient + " and participation " + existingParticipationInDatabase.getId());
}
submissionFromClient.setParticipation(studentParticipationFromClient);
submissionFromClient.submissionDate(ZonedDateTime.now());
submissionFromClient.submitted(true);
if (exercise instanceof QuizExercise) {
// recreate pointers back to submission in each submitted answer
for (SubmittedAnswer submittedAnswer : ((QuizSubmission) submissionFromClient).getSubmittedAnswers()) {
submittedAnswer.setSubmission(((QuizSubmission) submissionFromClient));
if (submittedAnswer instanceof DragAndDropSubmittedAnswer) {
((DragAndDropSubmittedAnswer) submittedAnswer).getMappings()
.forEach(dragAndDropMapping -> dragAndDropMapping.setSubmittedAnswer(((DragAndDropSubmittedAnswer) submittedAnswer)));
}
else if (submittedAnswer instanceof ShortAnswerSubmittedAnswer) {
((ShortAnswerSubmittedAnswer) submittedAnswer).getSubmittedTexts()
.forEach(submittedText -> submittedText.setSubmittedAnswer(((ShortAnswerSubmittedAnswer) submittedAnswer)));
}
}
// check that exactly one submission exists
if (studentParticipationFromClient.getSubmissions() == null || studentParticipationFromClient.getSubmissions().size() != 1) {
return;
}

// load quiz submissions for existing participation to be able to compare them in saveSubmission
// 5. DB Call: read
submittedAnswerRepository.loadQuizSubmissionsSubmittedAnswers(List.of(existingParticipationInDatabase));
// check that the current user owns the participation
if (!studentParticipationFromClient.isOwnedBy(currentUser) || !existingParticipationInDatabase.isOwnedBy(currentUser)) {
throw new AccessForbiddenException("User " + currentUser.getLogin() + " is not allowed to access the participation " + existingParticipationInDatabase.getId());
}
studentParticipationFromClient.setExercise(exercise);

QuizSubmission existingSubmissionInDatabase = (QuizSubmission) existingParticipationInDatabase.findLatestSubmission().orElse(null);
QuizSubmission quizSubmissionFromClient = (QuizSubmission) submissionFromClient;
Submission submissionFromClient = studentParticipationFromClient.getSubmissions().iterator().next();

if (!isContentEqualTo(existingSubmissionInDatabase, quizSubmissionFromClient)) {
quizSubmissionRepository.save(quizSubmissionFromClient);
saveSubmissionVersion(currentUser, submissionFromClient);
}
}
else if (exercise instanceof TextExercise) {
TextSubmission existingSubmissionInDatabase = (TextSubmission) existingParticipationInDatabase.findLatestSubmission().orElse(null);
TextSubmission textSubmissionFromClient = (TextSubmission) submissionFromClient;
if (!isContentEqualTo(existingSubmissionInDatabase, textSubmissionFromClient)) {
textSubmissionRepository.save(textSubmissionFromClient);
saveSubmissionVersion(currentUser, submissionFromClient);
}
// check that the submission belongs to the already saved participation
if (!existingParticipationInDatabase.getSubmissions().contains(submissionFromClient)) {
throw new AccessForbiddenException("User " + currentUser.getLogin() + " cannot submit a different submission " + submissionFromClient + " for participation "
+ existingParticipationInDatabase.getId());
}
// check that no result has been injected
if (submissionFromClient.getLatestResult() != null) {
throw new AccessForbiddenException("User " + currentUser.getLogin() + " cannot inject a result " + submissionFromClient.getLatestResult() + " for submission "
+ submissionFromClient + " and participation " + existingParticipationInDatabase.getId());
}

submissionFromClient.setParticipation(studentParticipationFromClient);
submissionFromClient.submissionDate(ZonedDateTime.now());
submissionFromClient.submitted(true);
switch (exercise) {
case QuizExercise ignored -> saveSubmissionQuizExercise(submissionFromClient, existingParticipationInDatabase, currentUser);
case TextExercise ignored -> {
TextSubmission existingSubmissionInDatabase = (TextSubmission) existingParticipationInDatabase.findLatestSubmission().orElse(null);
TextSubmission textSubmissionFromClient = (TextSubmission) submissionFromClient;
if (!isContentEqualTo(existingSubmissionInDatabase, textSubmissionFromClient)) {
textSubmissionRepository.save(textSubmissionFromClient);
saveSubmissionVersion(currentUser, submissionFromClient);
}
else if (exercise instanceof ModelingExercise) {
ModelingSubmission existingSubmissionInDatabase = (ModelingSubmission) existingParticipationInDatabase.findLatestSubmission().orElse(null);
ModelingSubmission modelingSubmissionFromClient = (ModelingSubmission) submissionFromClient;
if (!isContentEqualTo(existingSubmissionInDatabase, modelingSubmissionFromClient)) {
modelingSubmissionRepository.save(modelingSubmissionFromClient);
saveSubmissionVersion(currentUser, submissionFromClient);
}
}
case ModelingExercise ignored -> {
ModelingSubmission existingSubmissionInDatabase = (ModelingSubmission) existingParticipationInDatabase.findLatestSubmission().orElse(null);
ModelingSubmission modelingSubmissionFromClient = (ModelingSubmission) submissionFromClient;
if (!isContentEqualTo(existingSubmissionInDatabase, modelingSubmissionFromClient)) {
modelingSubmissionRepository.save(modelingSubmissionFromClient);
saveSubmissionVersion(currentUser, submissionFromClient);
}
}
default -> {
}
}
}
Copy link

Choose a reason for hiding this comment

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

LGTM! Consider extracting common validations.

The changes to the saveSubmission function look good. It has a clear responsibility and the control flow is easy to follow.

Consider extracting the common validation logic (checking for single participation and submission, verifying user access) into a separate function. This will make the main function more readable by abstracting away the details.

@SamuelRoettgermann
Copy link
Author

What do you think of the bot suggestion?

I personally think it's a good idea.

Copy link

There hasn't been any activity on this pull request recently. Therefore, this pull request has been automatically marked as stale and will be closed if no further activity occurs within seven days. Thank you for your contributions.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 5

Review details

Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE

Commits

Files that changed from the base of the PR and between 4a139b7 and ff47d77.

Files selected for processing (1)
  • src/main/java/de/tum/in/www1/artemis/service/exam/StudentExamService.java (1 hunks)
Additional context used
Path-based instructions (1)
src/main/java/de/tum/in/www1/artemis/service/exam/StudentExamService.java (1)

Pattern src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports

Comment on lines +258 to +261
// check that exactly one participation exists
if (exercise.getStudentParticipations() == null || exercise.getStudentParticipations().size() != 1) {
return;
}
Copy link

Choose a reason for hiding this comment

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

Consider adding logging for validation failures

In the validation checks where the method returns early due to the conditions not being met (lines 259-260 and 270-271), consider adding logging statements. This will help in monitoring and debugging by providing insights into why certain submissions or participations were not processed.

Also applies to: 270-272

Comment on lines +308 to +331
private void saveSubmissionQuizExercise(Submission submissionFromClient, StudentParticipation existingParticipationInDatabase, User currentUser) {
// recreate pointers back to submission in each submitted answer
for (SubmittedAnswer submittedAnswer : ((QuizSubmission) submissionFromClient).getSubmittedAnswers()) {
submittedAnswer.setSubmission(((QuizSubmission) submissionFromClient));
if (submittedAnswer instanceof DragAndDropSubmittedAnswer dragAndDropSubmittedAnswer) {
dragAndDropSubmittedAnswer.getMappings().forEach(dragAndDropMapping -> dragAndDropMapping.setSubmittedAnswer(((DragAndDropSubmittedAnswer) submittedAnswer)));
}
else if (submittedAnswer instanceof ShortAnswerSubmittedAnswer shortAnswerSubmittedAnswer) {
shortAnswerSubmittedAnswer.getSubmittedTexts().forEach(submittedText -> submittedText.setSubmittedAnswer(((ShortAnswerSubmittedAnswer) submittedAnswer)));
}
}

// load quiz submissions for existing participation to be able to compare them in saveSubmission
// 5. DB Call: read
submittedAnswerRepository.loadQuizSubmissionsSubmittedAnswers(List.of(existingParticipationInDatabase));

QuizSubmission existingSubmissionInDatabase = (QuizSubmission) existingParticipationInDatabase.findLatestSubmission().orElse(null);
QuizSubmission quizSubmissionFromClient = (QuizSubmission) submissionFromClient;

if (!isContentEqualTo(existingSubmissionInDatabase, quizSubmissionFromClient)) {
quizSubmissionRepository.save(quizSubmissionFromClient);
saveSubmissionVersion(currentUser, submissionFromClient);
}
}
Copy link

Choose a reason for hiding this comment

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

Reduce code duplication in 'saveSubmission' helper methods

The methods saveSubmissionQuizExercise, saveSubmissionTextExercise, and saveSubmissionModelingExercise share similar logic for checking content equality, saving submissions, and saving submission versions. Consider refactoring to abstract the common functionality into a generic method or utilizing polymorphism to reduce code duplication and enhance maintainability.

Also applies to: 336-343, 348-355

/**
* Helper function for {@link #saveSubmission(User, List, Exercise)}
*/
private void saveSubmissionTestExercise(Submission submissionFromClient, StudentParticipation existingParticipationInDatabase, User currentUser) {
Copy link

Choose a reason for hiding this comment

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

Correct method name from 'saveSubmissionTestExercise' to 'saveSubmissionTextExercise'

The method saveSubmissionTestExercise handles submissions for TextExercise. To maintain consistency and avoid confusion, consider renaming the method to saveSubmissionTextExercise. Update the method name and its invocation in the switch statement accordingly.

Apply the following diff to rename the method:

- private void saveSubmissionTestExercise(Submission submissionFromClient, StudentParticipation existingParticipationInDatabase, User currentUser) {
+ private void saveSubmissionTextExercise(Submission submissionFromClient, StudentParticipation existingParticipationInDatabase, User currentUser) {

...

- case TextExercise ignored -> saveSubmissionTestExercise(submissionFromClient, existingParticipationInDatabase, currentUser);
+ case TextExercise ignored -> saveSubmissionTextExercise(submissionFromClient, existingParticipationInDatabase, currentUser);

Also applies to: 298-298

Comment on lines +266 to +267
StudentParticipation existingParticipationInDatabase = existingRelevantParticipations.stream().filter(p -> p.getId().equals(studentParticipationFromClient.getId()))
.findFirst().orElseThrow();
Copy link

Choose a reason for hiding this comment

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

Provide a specific exception when participation is not found

Using orElseThrow() without specifying an exception results in a NoSuchElementException if the participation is not found. To improve error handling and make debugging easier, consider throwing a custom exception with a detailed message.

Apply the following diff to throw a custom exception:

- .findFirst().orElseThrow();
+ .findFirst().orElseThrow(() -> new EntityNotFoundException("Participation not found for ID: " + studentParticipationFromClient.getId()));
Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
StudentParticipation existingParticipationInDatabase = existingRelevantParticipations.stream().filter(p -> p.getId().equals(studentParticipationFromClient.getId()))
.findFirst().orElseThrow();
StudentParticipation existingParticipationInDatabase = existingRelevantParticipations.stream().filter(p -> p.getId().equals(studentParticipationFromClient.getId()))
.findFirst().orElseThrow(() -> new EntityNotFoundException("Participation not found for ID: " + studentParticipationFromClient.getId()));

Comment on lines +296 to +300
switch (exercise) {
case QuizExercise ignored -> saveSubmissionQuizExercise(submissionFromClient, existingParticipationInDatabase, currentUser);
case TextExercise ignored -> saveSubmissionTestExercise(submissionFromClient, existingParticipationInDatabase, currentUser);
case ModelingExercise ignored -> saveSubmissionModelingExercise(submissionFromClient, existingParticipationInDatabase, currentUser);
default -> {
Copy link

Choose a reason for hiding this comment

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

Handle unexpected exercise types in the 'default' case

Currently, the default case in the switch statement is empty. To ensure that unhandled exercise types are properly accounted for, consider adding a logging statement or throwing an exception in the default case. This will help identify unexpected exercise types at runtime.

Apply the following diff to handle the default case:

default -> {
+   throw new IllegalArgumentException("Unsupported exercise type: " + exercise.getClass());
}
Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
switch (exercise) {
case QuizExercise ignored -> saveSubmissionQuizExercise(submissionFromClient, existingParticipationInDatabase, currentUser);
case TextExercise ignored -> saveSubmissionTestExercise(submissionFromClient, existingParticipationInDatabase, currentUser);
case ModelingExercise ignored -> saveSubmissionModelingExercise(submissionFromClient, existingParticipationInDatabase, currentUser);
default -> {
switch (exercise) {
case QuizExercise ignored -> saveSubmissionQuizExercise(submissionFromClient, existingParticipationInDatabase, currentUser);
case TextExercise ignored -> saveSubmissionTestExercise(submissionFromClient, existingParticipationInDatabase, currentUser);
case ModelingExercise ignored -> saveSubmissionModelingExercise(submissionFromClient, existingParticipationInDatabase, currentUser);
default -> {
throw new IllegalArgumentException("Unsupported exercise type: " + exercise.getClass());
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chore code quality performance server Pull requests that update Java code. (Added Automatically!) tests
Projects
Status: Ready For Review
Development

Successfully merging this pull request may close these issues.

4 participants