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

SamuelRoettgermann · 2024-09-10T14:13:07Z

Checklist

General

This is a small issue that I tested locally.
Language: I followed the guidelines for inclusive, diversity-sensitive, and appreciative language.
I chose a title conforming to the naming conventions for pull requests.

Server

Important: I implemented the changes with a very good performance.
I strictly followed the server coding and design guidelines.

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
- Improved handling of complaints and filtering of sensitive information.
- Enhanced clarity in the logic for determining participant types and submission types.
Bug Fixes
- Improved error handling for unrecognized submission and exercise types.
Refactor
- Simplified code structure with early returns and switch-case statements for better readability and maintainability.

github-actions · 2024-09-10T14:16:15Z

⚠️ Unable to deploy to test servers ⚠️

The docker build needs to run through before deploying.

Strohgelaender · 2024-09-10T14:45:45Z

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

SamuelRoettgermann · 2024-09-10T14:54:50Z

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.

coderabbitai

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

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

github-actions · 2024-10-05T12:04:57Z

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.

dfuchss · 2024-10-16T22:27:15Z

src/main/java/de/tum/in/www1/artemis/domain/Complaint.java

+                this.student = null;
+                this.team = null;
+            }
+            default -> throw new Error("Unknown participant type");


Is it possible to seal the participant in order to remove the default case?

Ah sorry, I'm sick rn so I'm not too active on my computer hence the delay.

What exactly do you mean by "seal"? Do you mean the Java 17 sealed keyword and that I should make Participant a sealed class so that we can remove the default cases?

No problem at all. Yes, I mean "sealed". I think the pattern matching should detect that you have all cases. I'm not 100% sure whether the base is an interface, abstract class or normal class, but you have some possibilities with sealed here. See sample below :)

Oh yeah, that looks really nice, thx for the suggestion.
I'll look more into sealed. I've looked at it on Java 17 release, but haven't ever actually used it in a project since, so I just want to make sure that by adding this, I don't accidently "destroy" something else (be it compatibility, performance, etc.).

I'll push the changes tomorrow (if my research didn't reveal something bad).

The problem could be the package structure of Artemis here. Maybe that's something to discuss with @krusche from a programming perspective, it would be really good to use sealed here because it ensures that you'll find all places if another type of participant occurs. You are also welcome to contact me via Slack in this regard.

Yeah, the package structure is a problem.

I tried a few things, but we'd basically have to move de.tum.in.www1.artemis.domain.User.java and de.tum.in.www1.artemis.domain.Team.java into the de.tum.in.www1.artemis.domain.participation folder, otherwise it complains about Class is not allowed to extend sealed class from another package.

But yeah, in order to address that we'd definitely need to talk to @krusche to ensure we even can make such changes.

Errrr. I also just realized that since this PR had been created some time ago (but I couldn't work on it cause vacation, into retake preparation into sickness) the entire file structure changed.

Like, it's no longer de.tum.in.www1.artemis, but now it's de.tum.cit.aet.artemis.

So yeah... I guess I'll need to spend some time again to manually apply all the changes to the new files, cause I honestly have no idea how to automatically merge this, if it's even possible.

This also means that the locations I said earlier, i.e.

we'd basically have to move de.tum.in.www1.artemis.domain.User.java and de.tum.in.www1.artemis.domain.Team.java into the de.tum.in.www1.artemis.domain.participation folder

should've said this:

we'd basically have to move de.tum.cit.aet.artemis.core.domain.User.java and de.tum.cit.aet.artemis.exercise.domain.Team.java into the de.tum.cit.aet.artemis.exercise.domain.participation folder

dfuchss · 2024-10-16T22:27:56Z

src/main/java/de/tum/in/www1/artemis/repository/SubmissionRepository.java

@@ -450,25 +450,14 @@ SELECT COUNT(DISTINCT s)
     * @return a new submission for the given type connected to the given participation
     */
    default Submission initializeSubmission(Participation participation, Exercise exercise, SubmissionType submissionType) {


same here. you could think about sealing the submissions. this should remove the default case

Actually here we would need to seal the Exercise, not the Submission, but it wouldn't give us too much, because we would still need the null case anyways.

Yes, but not the default case.

coderabbitai

Actionable comments posted: 2

🧹 Outside diff range and nitpick comments (2)

src/main/java/de/tum/cit/aet/artemis/assessment/web/ComplaintResource.java (2)
340-357: Excellent refactoring using pattern matching and switch expression!

The changes improve readability and adhere to the KISS principle. The early return for null originalSubmission simplifies the logic flow. However, consider extracting the switch expression into a separate method for better modularity and reusability.

Consider extracting the switch expression into a separate method:
private Submission createSubmissionWithOnlyId(Submission originalSubmission) {
    return switch (originalSubmission) {
        case TextSubmission ignored -> new TextSubmission();
        case ModelingSubmission ignored -> new ModelingSubmission();
        case FileUploadSubmission ignored -> new FileUploadSubmission();
        case ProgrammingSubmission ignored -> new ProgrammingSubmission();
        default -> null;
    };
}
Then, you can simplify the main method:
Submission submissionWithOnlyId = createSubmissionWithOnlyId(originalSubmission);
if (submissionWithOnlyId == null) {
    return;
}
This change would improve modularity and make the code more reusable.

362-367: Good use of switch statement for type checking and object creation!

The refactoring improves readability and error handling. However, for consistency with the previous segment and to further improve readability, consider using a switch expression here as well.

Consider refactoring to use a switch expression:
Exercise exerciseWithOnlyTitle = switch (originalParticipation.getExercise()) {
    case TextExercise ignored -> new TextExercise();
    case ModelingExercise ignored -> new ModelingExercise();
    case FileUploadExercise ignored -> new FileUploadExercise();
    case ProgrammingExercise ignored -> new ProgrammingExercise();
    case null, default -> {
        log.error("Invalid exercise {} passed", originalParticipation.getExercise().getType());
        yield new Exercise();
    }
};
This change would make the code more consistent with the previous segment and potentially more readable.

📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between 3b48009 and e832686.

📒 Files selected for processing (2)

src/main/java/de/tum/cit/aet/artemis/assessment/domain/Complaint.java (1 hunks)
src/main/java/de/tum/cit/aet/artemis/assessment/web/ComplaintResource.java (1 hunks)

🧰 Additional context used

📓 Path-based instructions (2)

src/main/java/de/tum/cit/aet/artemis/assessment/domain/Complaint.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/cit/aet/artemis/assessment/web/ComplaintResource.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

📓 Learnings (1)

src/main/java/de/tum/cit/aet/artemis/assessment/web/ComplaintResource.java (2)

Learnt from: julian-christl
PR: ls1intum/Artemis#7829
File: src/main/java/de/tum/in/www1/artemis/web/rest/ComplaintResponseResource.java:0-0
Timestamp: 2024-10-08T15:35:42.972Z
Learning: The user has fixed the issue regarding the redundant wildcard import in `ComplaintResponseResource.java` by removing it in commit 7e392e0.

Learnt from: julian-christl
PR: ls1intum/Artemis#7829
File: src/main/java/de/tum/in/www1/artemis/web/rest/ComplaintResponseResource.java:0-0
Timestamp: 2024-01-08T18:32:55.982Z
Learning: The user has fixed the issue regarding the redundant wildcard import in `ComplaintResponseResource.java` by removing it in commit 7e392e0.

🔇 Additional comments (1)

src/main/java/de/tum/cit/aet/artemis/assessment/web/ComplaintResource.java (1)

369-369: Good addition of whitespace for improved readability.

The empty line after the switch statement helps to separate logical blocks of code, enhancing overall readability and structure.

coderabbitai · 2024-10-20T22:07:07Z

src/main/java/de/tum/cit/aet/artemis/assessment/domain/Complaint.java

+                this.student = null;
+                this.team = null;
+            }
+            default -> throw new Error("Unknown participant type");


⚠️ Potential issue

Throw an appropriate exception instead of 'Error' in the default case

Throwing an Error is generally reserved for serious system failures and is not recommended for handling unexpected input or states. Instead, use a more appropriate unchecked exception like IllegalArgumentException to indicate that the participant type is not recognized.

Apply this diff to fix the issue:

default -> throw new Error("Unknown participant type"); + default -> throw new IllegalArgumentException("Unknown participant type");

📝 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

default -> throw new Error("Unknown participant type");

default -> throw new IllegalArgumentException("Unknown participant type");

coderabbitai · 2024-10-20T22:07:07Z

src/main/java/de/tum/cit/aet/artemis/assessment/domain/Complaint.java

-            throw new Error("Unknown participant type");
+        switch (participant) {
+            case User user -> this.student = user;
+            case Team team1 -> this.team = team1;


🛠️ Refactor suggestion

Use consistent variable naming in switch-case patterns

In the switch statement, the variable team1 is used in case Team team1 -> this.team = team1;. For consistency and clarity, consider renaming team1 to team, aligning it with the field name this.team and improving readability.

Apply this diff to rename the variable:

switch (participant) { case User user -> this.student = user; - case Team team1 -> this.team = team1; + case Team team -> this.team = team; case null -> { this.student = null; this.team = null; } default -> throw new Error("Unknown participant type"); }

📝 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

case Team team1 -> this.team = team1;

case Team team -> this.team = team;

SimonEntholzer

Code lgtm. I assume I integrating the "sealed" classes didn't work because of our package structure?

github-actions · 2024-11-04T12:06:12Z

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.

use pattern matching instead of if-else chain

8640ab2

github-actions bot assigned SamuelRoettgermann Sep 10, 2024

github-actions bot added the server Pull requests that update Java code. (Added Automatically!) label Sep 10, 2024

SamuelRoettgermann added chore small performance code quality deploy:artemis-test1 and removed deploy:artemis-test1 labels Sep 10, 2024

github-actions bot added the deployment-error Added by deployment workflows if an error occured label Sep 10, 2024

SamuelRoettgermann removed the deployment-error Added by deployment workflows if an error occured label Sep 10, 2024

SamuelRoettgermann added 16 commits September 10, 2024 18:26

Complaint now uses pattern matching

4c01129

SubmissionRepository now uses pattern matching

bae5a6e

SubmissionVersionService now uses pattern matching

ce46af4

ExamService now uses pattern matching

16699ee

StudentExamService now uses pattern matching

93b4a24

DataExportQuizExerciseCreationService now uses pattern matching

3350eb4

QuizExerciseImportService now uses pattern matching

733d571

QuizService now uses pattern matching

04c639e

ComplaintResource now uses pattern matching

53b5194

LectureResource now uses pattern matching

4bd1e24

ExamUtilService now uses pattern matching

67ae585

StudentExamIntegrationTest now uses pattern matching

e4c93e6

AthenaRequestMockProvider now uses pattern matching

8b028f6

QuizComparisonTest now uses pattern matching

93cf232

ParticipationUtilService now uses pattern matching

13dbf4f

SubmissionExportIntegrationTest now uses pattern matching

b5997ff

github-actions bot added stale and removed ready for review labels Sep 19, 2024

extracted functionality, unable to extract into a single function

ff47d77

coderabbitai bot requested changes Sep 20, 2024

View reviewed changes

SamuelRoettgermann added ready for review and removed ready for review labels Sep 20, 2024

github-actions bot removed the stale label Sep 21, 2024

github-actions bot added the stale label Oct 5, 2024

dfuchss reviewed Oct 16, 2024

View reviewed changes

github-actions bot removed the stale label Oct 17, 2024

coderabbitai bot previously approved these changes Oct 20, 2024

View reviewed changes

fixed a typo

3b48009

SamuelRoettgermann dismissed coderabbitai[bot]’s stale review via 3b48009 October 20, 2024 19:16

coderabbitai bot previously approved these changes Oct 20, 2024

View reviewed changes

SamuelRoettgermann added 2 commits October 20, 2024 23:26

merged develop into this branch

91c323d

reapplied all the changes from before the merge

e832686

SamuelRoettgermann dismissed coderabbitai[bot]’s stale review via e832686 October 20, 2024 22:00

coderabbitai bot requested changes Oct 20, 2024

View reviewed changes

SimonEntholzer approved these changes Oct 28, 2024

View reviewed changes

github-actions bot added the stale label Nov 4, 2024

github-actions bot closed this Nov 18, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

SamuelRoettgermann commented Sep 10, 2024 •

edited by coderabbitai bot

Loading

github-actions bot commented Sep 10, 2024

Strohgelaender commented Sep 10, 2024

SamuelRoettgermann commented Sep 10, 2024

coderabbitai bot left a comment

github-actions bot commented Oct 5, 2024

dfuchss Oct 16, 2024

SamuelRoettgermann Oct 18, 2024 •

edited

Loading

dfuchss Oct 18, 2024 •

edited

Loading

dfuchss Oct 18, 2024

SamuelRoettgermann Oct 18, 2024

dfuchss Oct 20, 2024

SamuelRoettgermann Oct 20, 2024

SamuelRoettgermann Oct 20, 2024

SamuelRoettgermann Oct 20, 2024

dfuchss Oct 16, 2024

SamuelRoettgermann Oct 20, 2024

dfuchss Oct 20, 2024

coderabbitai bot left a comment

coderabbitai bot Oct 20, 2024

coderabbitai bot Oct 20, 2024

SimonEntholzer left a comment

github-actions bot commented Nov 4, 2024

	default -> throw new Error("Unknown participant type");
	default -> throw new IllegalArgumentException("Unknown participant type");

	case Team team1 -> this.team = team1;
	case Team team -> this.team = team;

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

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

Conversation

SamuelRoettgermann commented Sep 10, 2024 • edited by coderabbitai bot Loading

Checklist

General

Server

Motivation and Context

Description

Steps for Testing

Review Progress

Code Review

Summary by CodeRabbit

github-actions bot commented Sep 10, 2024

⚠️ Unable to deploy to test servers ⚠️

Strohgelaender commented Sep 10, 2024

SamuelRoettgermann commented Sep 10, 2024

coderabbitai bot left a comment

Choose a reason for hiding this comment

github-actions bot commented Oct 5, 2024

Choose a reason for hiding this comment

SamuelRoettgermann Oct 18, 2024 • edited Loading

Choose a reason for hiding this comment

dfuchss Oct 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coderabbitai bot left a comment

Choose a reason for hiding this comment

coderabbitai bot Oct 20, 2024

Choose a reason for hiding this comment

coderabbitai bot Oct 20, 2024

Choose a reason for hiding this comment

SimonEntholzer left a comment

Choose a reason for hiding this comment

github-actions bot commented Nov 4, 2024

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

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

SamuelRoettgermann commented Sep 10, 2024 •

edited by coderabbitai bot

Loading

SamuelRoettgermann Oct 18, 2024 •

edited

Loading

dfuchss Oct 18, 2024 •

edited

Loading