Iris: Improve code quality
#9494
Conversation
github-actions
bot
added
tests
server
WalkthroughThe changes in this pull request involve significant modifications to several classes within the Iris service, particularly focusing on the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 30
🧹 Outside diff range comments (2)
src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/data/PyrisUserDTO.java (1)
Line range hint
1-13: Approved: Good use of Java record and static factory method.The changes to
PyrisUserDTOare well-implemented and align with the coding guidelines. Here are the positive aspects:
- Use of Java record for the DTO, which aligns with the guideline for DTOs.
- Introduction of a static factory method
of, which improves readability and provides a clear way to create DTOs from User objects.- Removal of the direct constructor, which prevents accidental creation of DTOs without using the factory method.
- Use of
@JsonIncludeannotation to minimize DTO data by excluding empty fields.- Adherence to CamelCase naming convention.
- Compliance with the single responsibility principle.
For consistency with Java conventions, consider renaming the
ofmethod tofrom. This is a common naming pattern for conversion methods in Java, e.g.,List.of()vsArrays.asList().- public static PyrisUserDTO of(User user) { + public static PyrisUserDTO from(User user) { return new PyrisUserDTO(user.getId(), user.getFirstName(), user.getLastName()); }src/main/java/de/tum/cit/aet/artemis/iris/service/session/IrisTextExerciseChatSessionService.java (1)
Line range hint
86-108: LGTM: Improved pipeline execution and DTO usage.The changes in the
requestAndHandleResponsemethod enhance code quality:
- Introduction of
exerciseDTOimproves readability and reusability.- Simplified pipeline execution maintains stateless service design.
- Proper use of DTOs avoids entity exposure in the service layer.
For consistency, consider extracting the lambda expressions in the pipeline execution to named methods, especially if they grow in complexity.
Example refactoring for improved readability:
private TextExerciseChatJob createJob(String token, Long courseId, Long exerciseId, Long sessionId) { return new TextExerciseChatJob(token, courseId, exerciseId, sessionId); } private PyrisTextExerciseChatPipelineExecutionDTO createPipelineExecutionDTO( SomeDTO dto, PyrisTextExerciseDTO exerciseDTO, List<PyrisMessageDTO> conversation, String latestSubmissionText) { return new PyrisTextExerciseChatPipelineExecutionDTO( dto, exerciseDTO, conversation, latestSubmissionText); } // Usage in pipeline execution pyrisPipelineService.executePipeline( "text-exercise-chat", "default", token -> createJob(token, course.getId(), exercise.getId(), session.getId()), dto -> createPipelineExecutionDTO(dto, exerciseDTO, conversation, latestSubmissionText), stages -> irisChatWebsocketService.sendMessage(session, null, stages) );
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
📒 Files selected for processing (28)
- src/main/java/de/tum/cit/aet/artemis/iris/service/IrisCompetencyGenerationService.java (2 hunks)
- src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/PyrisJobService.java (2 hunks)
- src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/PyrisPipelineService.java (4 hunks)
- src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/PyrisProgrammingExerciseDTOService.java (4 hunks)
- src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/chat/PyrisChatPipelineExecutionBaseDataDTO.java (0 hunks)
- src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/chat/PyrisChatStatusUpdateDTO.java (1 hunks)
- src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/chat/course/PyrisCourseChatPipelineExecutionDTO.java (1 hunks)
- src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/chat/exercise/PyrisExerciseChatPipelineExecutionDTO.java (1 hunks)
- src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/chat/textexercise/PyrisTextExerciseChatPipelineExecutionDTO.java (1 hunks)
- src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/chat/textexercise/PyrisTextExerciseChatStatusUpdateDTO.java (1 hunks)
- src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/competency/PyrisCompetencyExtractionInputDTO.java (1 hunks)
- src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/competency/PyrisCompetencyExtractionPipelineExecutionDTO.java (2 hunks)
- src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/data/PyrisCompetencyDTO.java (1 hunks)
- src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/data/PyrisCourseDTO.java (1 hunks)
- src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/data/PyrisExerciseWithStudentSubmissionsDTO.java (2 hunks)
- src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/data/PyrisExtendedCourseDTO.java (2 hunks)
- src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/data/PyrisLectureDTO.java (1 hunks)
- src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/data/PyrisLectureUnitDTO.java (1 hunks)
- src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/data/PyrisMessageContentBaseDTO.java (1 hunks)
- src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/data/PyrisProgrammingExerciseDTO.java (1 hunks)
- src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/data/PyrisResultDTO.java (1 hunks)
- src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/data/PyrisSubmissionDTO.java (1 hunks)
- src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/data/PyrisTextExerciseDTO.java (1 hunks)
- src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/data/PyrisUserDTO.java (1 hunks)
- src/main/java/de/tum/cit/aet/artemis/iris/service/session/IrisCourseChatSessionService.java (6 hunks)
- src/main/java/de/tum/cit/aet/artemis/iris/service/session/IrisExerciseChatSessionService.java (4 hunks)
- src/main/java/de/tum/cit/aet/artemis/iris/service/session/IrisTextExerciseChatSessionService.java (3 hunks)
- src/test/java/de/tum/cit/aet/artemis/iris/PyrisLectureIngestionTest.java (2 hunks)
💤 Files with no reviewable changes (1)
- src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/chat/PyrisChatPipelineExecutionBaseDataDTO.java
🧰 Additional context used
📓 Path-based instructions (27)
src/main/java/de/tum/cit/aet/artemis/iris/service/IrisCompetencyGenerationService.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_importssrc/main/java/de/tum/cit/aet/artemis/iris/service/pyris/PyrisJobService.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_importssrc/main/java/de/tum/cit/aet/artemis/iris/service/pyris/PyrisPipelineService.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_importssrc/main/java/de/tum/cit/aet/artemis/iris/service/pyris/PyrisProgrammingExerciseDTOService.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_importssrc/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/chat/PyrisChatStatusUpdateDTO.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_importssrc/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/chat/course/PyrisCourseChatPipelineExecutionDTO.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_importssrc/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/chat/exercise/PyrisExerciseChatPipelineExecutionDTO.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_importssrc/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/chat/textexercise/PyrisTextExerciseChatPipelineExecutionDTO.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_importssrc/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/chat/textexercise/PyrisTextExerciseChatStatusUpdateDTO.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_importssrc/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/competency/PyrisCompetencyExtractionInputDTO.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_importssrc/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/competency/PyrisCompetencyExtractionPipelineExecutionDTO.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_importssrc/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/data/PyrisCompetencyDTO.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_importssrc/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/data/PyrisCourseDTO.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_importssrc/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/data/PyrisExerciseWithStudentSubmissionsDTO.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_importssrc/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/data/PyrisExtendedCourseDTO.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_importssrc/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/data/PyrisLectureDTO.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_importssrc/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/data/PyrisLectureUnitDTO.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_importssrc/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/data/PyrisMessageContentBaseDTO.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_importssrc/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/data/PyrisProgrammingExerciseDTO.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_importssrc/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/data/PyrisResultDTO.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_importssrc/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/data/PyrisSubmissionDTO.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_importssrc/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/data/PyrisTextExerciseDTO.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_importssrc/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/data/PyrisUserDTO.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_importssrc/main/java/de/tum/cit/aet/artemis/iris/service/session/IrisCourseChatSessionService.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_importssrc/main/java/de/tum/cit/aet/artemis/iris/service/session/IrisExerciseChatSessionService.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_importssrc/main/java/de/tum/cit/aet/artemis/iris/service/session/IrisTextExerciseChatSessionService.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_importssrc/test/java/de/tum/cit/aet/artemis/iris/PyrisLectureIngestionTest.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 (44)
src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/data/PyrisSubmissionDTO.java (1)
Line range hint
1-20: DTO structure aligns with coding guidelinesThe
PyrisSubmissionDTOrecord adheres to the project's coding guidelines:
- It uses CamelCase naming convention.
- It's implemented as a Java record, promoting immutability and reducing boilerplate.
- It contains minimal data fields, focusing on essential information for a Pyris submission.
- The
@JsonIncludeannotation helps in minimizing the DTO size when serialized.The DTO demonstrates a single responsibility and uses appropriate data types (e.g.,
Instantfor date), which aligns with best practices.src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/competency/PyrisCompetencyExtractionInputDTO.java (1)
5-11: Javadoc and annotation usage look goodThe Javadoc comment provides a clear and concise description of the DTO and its parameters, which is excellent for maintaining code readability and understanding.
The use of
@JsonInclude(JsonInclude.Include.NON_EMPTY)is a good practice as it helps to minimize the size of the JSON payload by excluding empty values. This can be particularly useful in REST communications to reduce unnecessary data transfer.Good job on these aspects!
src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/data/PyrisMessageContentBaseDTO.java (1)
8-14: Approve: Improved formatting control for@JsonSubTypesannotationThe addition of formatter comments (
// @formatter:offand// @formatter:on) around the@JsonSubTypesannotation is a good practice. These comments ensure that the annotation remains readable and consistently formatted across different IDE configurations. This change improves code maintainability without altering the functionality.The
PyrisMessageContentBaseDTOinterface adheres to the coding guidelines by:
- Following the CamelCase naming convention
- Adhering to the single responsibility principle
- Using minimal DTOs (marker interface)
- Utilizing Java annotations for proper JSON handling
src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/chat/PyrisChatStatusUpdateDTO.java (2)
Line range hint
1-7: LGTM: Package declaration and imports are appropriate.The package declaration follows the correct naming convention, and the imports are specific and necessary for the DTO. Good job avoiding wildcard imports.
16-18: LGTM: Record declaration adheres to DTO guidelines.The use of a Java record for this DTO is appropriate and aligns with our coding guidelines. It follows the single responsibility principle, uses minimal data, and the @JsonInclude annotation helps to keep the DTO concise. The parameters (result, stages, and suggestions) seem to capture the necessary data for a chat status update effectively.
src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/chat/textexercise/PyrisTextExerciseChatStatusUpdateDTO.java (5)
Line range hint
1-7: LGTM: Package declaration and imports are correct and follow guidelines.The package declaration is appropriate, and the imports are necessary and specific. Good job avoiding star imports, which aligns with our coding guidelines.
9-15: LGTM: Well-documented DTO with clear Javadoc comments.The Javadoc comments provide excellent context for the DTO's purpose and explain why it's used instead of
PyrisChatStatusUpdateDTO. The parameter descriptions are clear and concise, which will be helpful for developers using this class.
17-18: LGTM: Well-structured record declaration following DTO guidelines.The record
PyrisTextExerciseChatStatusUpdateDTOis well-named following CamelCase convention. It appropriately uses the Java record feature for DTOs, adhering to our guidelines for minimal data, single responsibility, and avoiding entities. The parametersresultandstagesare suitable for the DTO's purpose, and the use ofList<PyrisStageDTO>demonstrates good code reuse.
16-16: LGTM: Appropriate use of @JsonInclude annotation.The
@JsonInclude(JsonInclude.Include.NON_EMPTY)annotation is well-placed and aligns with our guideline for minimal DTOs. This will help reduce unnecessary data in the JSON output by excluding null or empty values during serialization.
Line range hint
1-19: Excellent implementation of PyrisTextExerciseChatStatusUpdateDTO.This file demonstrates a well-structured, properly documented, and efficiently implemented DTO using Java records. It adheres to our coding guidelines, including proper naming conventions, minimal data representation, and appropriate use of annotations. The code is clean, focused, and serves its purpose effectively within the Iris subsystem.
src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/data/PyrisCompetencyDTO.java (2)
Line range hint
24-27: Theofmethod adheres to good practices.The
ofmethod effectively creates a DTO from aCompetencyentity, adhering to the principles of code reuse and separation of concerns. It follows the DTO guidelines by not exposing entities directly.
Line range hint
1-28: Overall, the changes improve code quality and adhere to guidelines.The modifications to
PyrisCompetencyDTO.javaenhance readability while maintaining adherence to coding guidelines. The file follows Java and DTO-specific best practices, including:
- Use of Java records for DTOs
- CamelCase naming convention
- Single responsibility principle
- Minimal data in DTOs
- Code reuse in the
ofmethodsrc/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/chat/textexercise/PyrisTextExerciseChatPipelineExecutionDTO.java (1)
11-18: LGTM: Well-documented DTO with clear Javadoc.The added Javadoc comments provide a clear description of the DTO's purpose and its parameters. This improvement in documentation enhances code readability and maintainability.
src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/competency/PyrisCompetencyExtractionPipelineExecutionDTO.java (2)
Line range hint
9-17: LGTM: Documentation improvementsThe addition of the period at the end of the first sentence in the class documentation improves consistency. The documentation clearly describes the purpose of the DTO and provides concise explanations for each parameter, adhering to the single responsibility principle.
18-26: LGTM: Improved formatting and adherence to guidelinesThe reformatting of the record declaration significantly improves readability. The use of
@formatter:offand@formatter:oncomments ensures that the desired formatting is preserved. This record adheres to the DTO guidelines by:
- Using a Java record
- Containing no entities
- Including only minimal, necessary data
- Following the single responsibility principle
The
@JsonIncludeannotation is appropriately used to minimize DTO size, which is good for performance.src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/data/PyrisTextExerciseDTO.java (1)
25-25: Excellent use of a static factory method!The change from
new PyrisCourseDTO(...)toPyrisCourseDTO.of(...)is a good improvement. It aligns well with several best practices:
- It enhances the single responsibility principle by delegating the creation logic to the
PyrisCourseDTOclass.- It promotes code reuse and flexibility, as the factory method can handle different creation scenarios if needed in the future.
- It simplifies the code in this method, adhering to the KISS principle.
- It potentially allows for better encapsulation of the
PyrisCourseDTOcreation process.This change is approved and aligns well with our coding guidelines.
src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/chat/course/PyrisCourseChatPipelineExecutionDTO.java (1)
Line range hint
1-11: LGTM: Imports are appropriate and follow guidelines.The imports are well-organized and avoid using wildcard imports, which aligns with the coding guideline to avoid star imports.
src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/chat/exercise/PyrisExerciseChatPipelineExecutionDTO.java (2)
Line range hint
1-11: LGTM: Imports are appropriate and follow guidelines.The imports are specific and avoid using star imports, which aligns with the coding guideline to avoid star imports in Java files.
Line range hint
1-38: Overall: Good quality DTO implementation with minor suggestions for improvement.The
PyrisExerciseChatPipelineExecutionDTOis well-implemented and follows most of the coding guidelines. Here's a summary of the review:
- Imports are appropriate and avoid star imports.
- Javadoc is comprehensive but contains TODOs that could be addressed.
- The record declaration follows DTO guidelines and uses appropriate annotations.
Consider addressing the TODO items and the minor formatting suggestion to further improve the code quality.
src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/data/PyrisExtendedCourseDTO.java (2)
19-35: Improved record declaration formatting.The changes to the
PyrisExtendedCourseDTOrecord declaration enhance readability by placing each parameter on a separate line. This formatting improvement aligns with best practices for long parameter lists and makes the code easier to maintain.The record adheres to the DTO guidelines by using Java records, avoiding entities, and including only the necessary data. It also follows the single responsibility principle and uses appropriate types, including primitives where suitable.
Line range hint
1-68: Overall improvement in code readability and maintainability.The changes in this file focus on enhancing code formatting, particularly in the
PyrisExtendedCourseDTOrecord declaration and theofmethod. These improvements significantly boost readability and maintainability without altering the functionality.Key points:
- The code adheres to the provided coding guidelines, including naming conventions and DTO best practices.
- The changes promote the single responsibility principle and code reuse.
- The use of Java records and appropriate types aligns with modern Java practices.
These formatting improvements will make it easier for developers to understand and maintain the code, contributing to the overall goal of enhancing code quality in the Iris subsystem.
src/main/java/de/tum/cit/aet/artemis/iris/service/IrisCompetencyGenerationService.java (1)
Line range hint
1-71: Overall, the changes improve code quality and simplify the class.The modifications to
IrisCompetencyGenerationServicehave successfully removed thePyrisJobServicedependency and simplified the code. The class now better adheres to the single responsibility principle and KISS (Keep It Simple, Stupid) principle.The changes are generally in line with the provided coding guidelines, including:
- Proper naming conventions (CamelCase)
- Constructor injection for dependency inversion
- Stateless service design
- Minimal DTOs
To further improve the code:
- Consider adding the
@Autowiredannotation to the constructor for clarity.- Refactor the
executeCompetencyExtractionPipelinemethod to improve readability and adhere more closely to the single responsibility principle.These suggestions will enhance the overall quality and maintainability of the code.
src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/PyrisProgrammingExerciseDTOService.java (5)
37-37: LGTM: Class renaming and logger update.The class renaming from
PyrisDTOServicetoPyrisProgrammingExerciseDTOServiceimproves clarity by being more specific about its purpose. The logger initialization has been correctly updated to reflect the new class name. These changes adhere to the CamelCase naming convention as specified in the coding guidelines.Also applies to: 39-39
Line range hint
47-51: LGTM: Constructor update.The constructor signature has been correctly updated to reflect the new class name. It continues to use constructor injection as specified in the coding guidelines. The parameters and their order remain unchanged, which maintains backwards compatibility.
Line range hint
1-184: Summary of review for PyrisProgrammingExerciseDTOService.javaOverall, the changes in this file represent a significant improvement in code clarity and consistency. The renaming of the class and methods provides a more accurate representation of their purposes. The reformatting of complex return statements enhances readability.
Key points:
- The class and method renamings adhere to the CamelCase convention and improve specificity.
- The changes align well with the single responsibility and small methods principles.
- Constructor injection is correctly maintained.
However, there are two points that require attention:
- The potential method overloading issue with the two
convertmethods needs verification to ensure it doesn't cause ambiguity in method calls.- The removal of the
toPyrisMessageDTOListmethod requires clarification and verification to ensure it doesn't negatively impact the codebase.Please address these points to further improve the code quality and maintainability.
Line range hint
1-184: Clarification needed: Removal oftoPyrisMessageDTOListmethod.The
toPyrisMessageDTOListmethod has been removed from this service. This change suggests a shift in the service's responsibilities or a refactoring of how messages are handled.Could you please clarify the reasoning behind removing this method? It's important to ensure that this removal doesn't negatively impact other parts of the codebase that might have been using it.
To check for potential issues, you can run the following command to search for any remaining references to the removed method:
#!/bin/bash # Search for calls to the removed toPyrisMessageDTOList method rg "toPyrisMessageDTOList" --type javaIf there are any results, please update those references accordingly or consider if the method needs to be reintroduced or moved to a different service.
94-113: LGTM with a concern: Method renaming and formatting improvement.The renaming of
toPyrisSubmissionDTOtoconvertis consistent with the previous method and aligns with the single responsibility principle. The reformatting of the return statement significantly improves readability without altering the functionality.However, there's a potential issue with method overloading. Both
convertmethods have different parameter types but the same name. While this is valid Java, it might lead to confusion. Please verify that this doesn't cause any ambiguity in method calls throughout the codebase.To check for potential issues, you can run the following command:
If the results show any ambiguous calls, consider renaming one of the methods or adding type-specific prefixes to differentiate them (e.g.,
convertExerciseandconvertSubmission).✅ Verification successful
Verified: Method overloading does not introduce ambiguity.
The presence of two
convertmethods with different parameter types does not result in any ambiguous calls withinPyrisProgrammingExerciseDTOService.java. The changes are safe and do not impact the functionality.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for calls to convert method in PyrisProgrammingExerciseDTOService rg "PyrisProgrammingExerciseDTOService.*convert\(" --type javaLength of output: 132
Script:
#!/bin/bash # Search for calls to convert methods in PyrisProgrammingExerciseDTOService.java rg 'convert\(' --type java src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/PyrisProgrammingExerciseDTOService.javaLength of output: 277
src/main/java/de/tum/cit/aet/artemis/iris/service/session/IrisTextExerciseChatSessionService.java (2)
58-58: LGTM: Constructor dependency reduction improves design.The removal of
PyrisJobServicefrom the constructor simplifies the class initialization and reduces dependencies. This change aligns well with the single responsibility principle and maintains the use of constructor injection for dependency management.
Line range hint
1-170: Overall assessment: Code changes improve quality and maintainability.The modifications to
IrisTextExerciseChatSessionServicealign well with the PR objectives of improving code quality and consistency. The changes maintain good practices such as:
- Adhering to the single responsibility principle
- Using constructor injection for dependency management
- Proper use of DTOs to avoid entity exposure
- Maintaining stateless service design
The code demonstrates a commitment to clean, maintainable, and efficient implementation of the Iris text exercise chat functionality.
src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/PyrisPipelineService.java (4)
19-19: Import statement is appropriate.The addition of the
PyrisJobimport is necessary for the updated functionality.
38-41: Constructor simplification improves code quality.By reducing the constructor parameters to only
PyrisConnectorServiceandPyrisJobService, the class adheres more closely to the single responsibility principle and reduces dependency coupling.
55-57: Parameter descriptions are clear and accurate.The updated
@paramdocumentation accurately reflects the changes to the method signature and provides clear guidance on the purpose of each parameter.
59-60: Method signature aligns with best practices.The updated
executePipelinemethod signature accurately represents the necessary parameters for pipeline execution and maintains code clarity.src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/PyrisJobService.java (1)
116-116: JavaDoc Reference Updated CorrectlyThe JavaDoc comment correctly updates the method reference to
registerJobusing@link, ensuring consistency in the documentation.src/main/java/de/tum/cit/aet/artemis/iris/service/session/IrisExerciseChatSessionService.java (1)
156-157: Handle potential nulllatestSubmissionDTOandexerciseDTOThe
latestSubmissionDTOmay benullif no submission exists. Verify thatPyrisExerciseChatPipelineExecutionDTOand the pipeline can handlenullvalues without throwing exceptions.Confirm that the constructors and methods used can handle
nullinputs properly. If not, consider adding checks or default values.src/main/java/de/tum/cit/aet/artemis/iris/service/session/IrisCourseChatSessionService.java (9)
75-79: Adhere to the "constructor injection" guideline.The changes introduce new dependencies that are injected via the constructor. This aligns with the coding guideline to use constructor injection for dependency injection.
83-84: Adhere to the "constructor injection" guideline.The changes update the constructor to inject the new dependencies. This aligns with the coding guideline to use constructor injection for dependency injection.
94-96: Adhere to the "constructor injection" guideline.The changes assign the injected dependencies to the corresponding fields. This completes the implementation of constructor injection for the new dependencies.
179-203: Verify the usage of the new methodloadCourseWithParticipationOfStudent.The changes introduce a new private method
loadCourseWithParticipationOfStudentto load a course with the participation of a student. Ensure that this method is used correctly and that the necessary data is available.Run the following script to verify the usage of the new method:
#!/bin/bash # Description: Verify the usage of the new method `loadCourseWithParticipationOfStudent`. # Test: Search for the usage of `loadCourseWithParticipationOfStudent`. Expect: Usage in the `requestAndHandleResponse` method. rg --type java -A 5 $'loadCourseWithParticipationOfStudent'
7-13:⚠️ Potential issueAdhere to the "avoid star imports" guideline.
The changes introduce new imports using the wildcard
*syntax. This goes against the coding guideline to avoid star imports.Refactor the imports to explicitly list the required classes or interfaces:
-import java.util.HashMap; -import java.util.HashSet; -import java.util.List; -import java.util.Map; +import java.util.HashMap; +import java.util.HashSet; +import java.util.List; +import java.util.Map; import java.util.Objects; -import java.util.Optional; -import java.util.Set; +import java.util.Optional; +import java.util.Set;Likely invalid or redundant comment.
44-47:⚠️ Potential issueAdhere to the "avoid star imports" guideline.
The changes introduce new imports using the wildcard
*syntax. This goes against the coding guideline to avoid star imports.Refactor the imports to explicitly list the required classes:
-import de.tum.cit.aet.artemis.iris.service.pyris.dto.chat.PyrisChatStatusUpdateDTO; -import de.tum.cit.aet.artemis.iris.service.pyris.dto.chat.course.PyrisCourseChatPipelineExecutionDTO; -import de.tum.cit.aet.artemis.iris.service.pyris.dto.data.PyrisExtendedCourseDTO; -import de.tum.cit.aet.artemis.iris.service.pyris.dto.data.PyrisMessageDTO; -import de.tum.cit.aet.artemis.iris.service.pyris.dto.data.PyrisUserDTO; +import de.tum.cit.aet.artemis.iris.service.pyris.dto.chat.PyrisChatStatusUpdateDTO; +import de.tum.cit.aet.artemis.iris.service.pyris.dto.chat.course.PyrisCourseChatPipelineExecutionDTO; +import de.tum.cit.aet.artemis.iris.service.pyris.dto.data.PyrisExtendedCourseDTO; +import de.tum.cit.aet.artemis.iris.service.pyris.dto.data.PyrisMessageDTO; +import de.tum.cit.aet.artemis.iris.service.pyris.dto.data.PyrisUserDTO;Likely invalid or redundant comment.
27-32:⚠️ Potential issueAdhere to the "avoid star imports" guideline.
The changes introduce new imports using the wildcard
*syntax. This goes against the coding guideline to avoid star imports.Refactor the imports to explicitly list the required classes or interfaces:
-import de.tum.cit.aet.artemis.core.repository.CourseRepository; +import de.tum.cit.aet.artemis.core.repository.CourseRepository; import de.tum.cit.aet.artemis.core.security.Role; import de.tum.cit.aet.artemis.core.service.AuthorizationCheckService; -import de.tum.cit.aet.artemis.exercise.domain.participation.StudentParticipation; -import de.tum.cit.aet.artemis.exercise.repository.StudentParticipationRepository; -import de.tum.cit.aet.artemis.exercise.service.LearningMetricsService; +import de.tum.cit.aet.artemis.exercise.domain.participation.StudentParticipation; +import de.tum.cit.aet.artemis.exercise.repository.StudentParticipationRepository; +import de.tum.cit.aet.artemis.exercise.service.LearningMetricsService;Likely invalid or redundant comment.
23-23:⚠️ Potential issueAdhere to the "avoid star imports" guideline.
The changes introduce a new import using the wildcard
*syntax. This goes against the coding guideline to avoid star imports.Refactor the import to explicitly list the required class:
-import de.tum.cit.aet.artemis.atlas.domain.competency.CompetencyJol; +import de.tum.cit.aet.artemis.atlas.domain.competency.CompetencyJol;Likely invalid or redundant comment.
148-177: Verify the usage of the new DTOs and services.The changes introduce several new DTOs and services to construct a more complex pipeline execution. Ensure that these DTOs and services are used correctly and that the necessary data is available.
Run the following script to verify the usage of the new DTOs and services:
✅ Verification successful
Verified: The new DTOs and services are correctly used in the
requestAndHandleResponsemethod.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of the new DTOs and services in the `requestAndHandleResponse` method. # Test: Search for the usage of `PyrisExtendedCourseDTO`. Expect: Usage in the `requestAndHandleResponse` method. rg --type java -A 5 $'PyrisExtendedCourseDTO' # Test: Search for the usage of `learningMetricsService`. Expect: Usage in the `requestAndHandleResponse` method. rg --type java -A 5 $'learningMetricsService' # Test: Search for the usage of `CompetencyJolDTO`. Expect: Usage in the `requestAndHandleResponse` method. rg --type java -A 5 $'CompetencyJolDTO' # Test: Search for the usage of `PyrisMessageDTO`. Expect: Usage in the `requestAndHandleResponse` method. rg --type java -A 5 $'PyrisMessageDTO' # Test: Search for the usage of `PyrisUserDTO`. Expect: Usage in the `requestAndHandleResponse` method. rg --type java -A 5 $'PyrisUserDTO'Length of output: 2401
Script:
#!/bin/bash # Description: Verify the usage of the new DTOs and services in the `requestAndHandleResponse` method. # Test: Search for the usage of `PyrisExtendedCourseDTO`. Expect: Usage in the `requestAndHandleResponse` method. rg --type java -A 5 'PyrisExtendedCourseDTO' src/main/java/de/tum/cit/aet/artemis/iris/service/session/IrisCourseChatSessionService.java # Test: Search for the usage of `learningMetricsService`. Expect: Usage in the `requestAndHandleResponse` method. rg --type java -A 5 'learningMetricsService' src/main/java/de/tum/cit/aet/artemis/iris/service/session/IrisCourseChatSessionService.java # Test: Search for the usage of `CompetencyJolDTO`. Expect: Usage in the `requestAndHandleResponse` method. rg --type java -A 5 'CompetencyJolDTO' src/main/java/de/tum/cit/aet/artemis/iris/service/session/IrisCourseChatSessionService.java # Test: Search for the usage of `PyrisMessageDTO`. Expect: Usage in the `requestAndHandleResponse` method. rg --type java -A 5 'PyrisMessageDTO' src/main/java/de/tum/cit/aet/artemis/iris/service/session/IrisCourseChatSessionService.java # Test: Search for the usage of `PyrisUserDTO`. Expect: Usage in the `requestAndHandleResponse` method. rg --type java -A 5 'PyrisUserDTO' src/main/java/de/tum/cit/aet/artemis/iris/service/session/IrisCourseChatSessionService.javaLength of output: 4927
src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/data/PyrisResultDTO.java
Outdated
Show resolved
Hide resolved
src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/data/PyrisLectureUnitDTO.java
Outdated
Show resolved
Hide resolved
src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/data/PyrisCourseDTO.java
Outdated
Show resolved
Hide resolved
src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/data/PyrisLectureDTO.java
Outdated
Show resolved
Hide resolved
src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/data/PyrisSubmissionDTO.java
Outdated
Show resolved
Hide resolved
src/main/java/de/tum/cit/aet/artemis/iris/service/session/IrisExerciseChatSessionService.java
Outdated
Show resolved
Hide resolved
src/main/java/de/tum/cit/aet/artemis/iris/service/session/IrisExerciseChatSessionService.java
Show resolved
Hide resolved
src/main/java/de/tum/cit/aet/artemis/iris/service/session/IrisExerciseChatSessionService.java
Outdated
Show resolved
Hide resolved
src/main/java/de/tum/cit/aet/artemis/iris/service/session/IrisExerciseChatSessionService.java
Outdated
Show resolved
Hide resolved
src/main/java/de/tum/cit/aet/artemis/iris/service/session/IrisCourseChatSessionService.java
Show resolved
Hide resolved
WalkthroughThe pull request introduces significant modifications across multiple classes in the Iris service, primarily focusing on the removal of the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
There was a problem hiding this comment.
Actionable comments posted: 40
🧹 Outside diff range comments (8)
src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/data/PyrisUserDTO.java (1)
Line range hint
1-13: Good use of record for DTO, but consider adding documentation and addressing potential breaking changes.The use of a record for
PyrisUserDTOaligns well with the DTO guidelines and simplifies the class. The@JsonIncludeannotation is a good practice for creating minimal DTOs.Consider the following improvements:
- Add JavaDoc comments to the class and its fields for better documentation:
/** * Data Transfer Object representing a user in the Pyris system. */ @JsonInclude(JsonInclude.Include.NON_EMPTY) public record PyrisUserDTO( /** The unique identifier of the user. */ long id, /** The first name of the user. */ String firstName, /** The last name of the user. */ String lastName ) { // ... rest of the class }
- If the removal of the previous constructor (
PyrisUserDTO(User user)) might break existing code, consider adding a deprecated version to maintain backwards compatibility:/** * @deprecated Use {@link #of(User)} instead. */ @Deprecated(since = "version", forRemoval = true) public PyrisUserDTO(User user) { this(user.getId(), user.getFirstName(), user.getLastName()); }This will allow for a smoother transition to the new static factory method while giving developers time to update their code.
src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/data/PyrisCompetencyDTO.java (1)
Line range hint
24-27: Consider making the static import more explicit.The
ofmethod effectively creates a DTO from aCompetencyobject, promoting code reuse. However, the static import forTimeUtil.toInstantmight be less obvious to other developers.Consider replacing the static import with an explicit import and method call for improved clarity:
- import static de.tum.cit.aet.artemis.core.util.TimeUtil.toInstant; + import de.tum.cit.aet.artemis.core.util.TimeUtil; // In the of method: - toInstant(competency.getSoftDueDate()), + TimeUtil.toInstant(competency.getSoftDueDate()),This change would make the code more self-documenting and easier to understand at a glance.
src/main/java/de/tum/cit/aet/artemis/iris/service/IrisCompetencyGenerationService.java (1)
Line range hint
47-51: Simplified job creation processThe modification to use a lambda expression for creating the
CompetencyExtractionJobis a good improvement. It simplifies the code and removes the dependency onPyrisJobServicefor token generation.To further improve readability, consider extracting the lambda expressions into separate methods:
pyrisPipelineService.executePipeline( "competency-extraction", "default", this::createCompetencyExtractionJob, this::createPipelineExecutionDTO, stages -> this.sendWebsocketUpdate(user.getLogin(), course.getId(), stages) ); private CompetencyExtractionJob createCompetencyExtractionJob(String jobId) { return new CompetencyExtractionJob(jobId, course.getId(), user.getLogin()); } private PyrisCompetencyExtractionPipelineExecutionDTO createPipelineExecutionDTO(Object executionDto) { return new PyrisCompetencyExtractionPipelineExecutionDTO(executionDto, courseDescription, currentCompetencies, CompetencyTaxonomy.values(), 5); } private void sendWebsocketUpdate(String userLogin, Long courseId, Object stages) { websocketService.send(userLogin, websocketTopic(courseId), new PyrisCompetencyStatusUpdateDTO(stages, null)); }This refactoring would improve the method's readability and maintainability while adhering to the single responsibility principle.
src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/PyrisJobService.java (1)
Line range hint
120-123: Handle possible nullAuthorizationheader to preventNullPointerExceptionIn
getAndAuthenticateJobFromHeaderElseThrow, if theAuthorizationheader is missing,authHeaderwill benull, leading to aNullPointerExceptionwhen callingauthHeader.startsWith("Bearer "). Check fornullbefore invokingstartsWith.Apply this diff to fix the issue:
var authHeader = request.getHeader("Authorization"); +if (authHeader == null || !authHeader.startsWith("Bearer ")) { -if (!authHeader.startsWith("Bearer ")) { throw new AccessForbiddenException("No valid token provided"); }src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/PyrisProgrammingExerciseDTOService.java (2)
Line range hint
60-84: Handle exceptions when fetching the test repositoryIn the method
convert(ProgrammingExercise exercise), if fetching the test repository fails due to aGitAPIException, the exception is caught, and an error is logged. However, the method then proceeds with an emptytestsRepositoryContents. This could lead to incomplete data in thePyrisProgrammingExerciseDTO.Consider informing the caller about the failure or including a flag in the DTO to indicate that the test repository contents could not be retrieved.
Line range hint
178-182: Avoid null returns in streamsIn the
getRepositoryContentsmethod, there is a possibility of returningnullwithin a stream, which can lead to aNullPointerException. Consider returning an empty map or handlingnullcases appropriately.Apply this diff:
if (lastCommitObjectId == null) { - return null; + return Map.of(); }src/main/java/de/tum/cit/aet/artemis/iris/service/session/IrisCourseChatSessionService.java (1)
Line range hint
83-96: Constructor has too many parametersThe constructor now has a large number of parameters (13 in total), which may indicate that the class is handling too many responsibilities. This can make the code harder to maintain and test.
Consider refactoring to reduce the number of constructor parameters:
- Apply the Single Responsibility Principle: Split the class into smaller classes, each handling a specific aspect of the functionality.
- Use Parameter Objects: Group related parameters into a single object. For example, you could create a configuration object or service holder.
- Leverage Dependency Injection Framework Features: Some frameworks allow injecting a whole set of beans, which can reduce the number of parameters.
This will improve maintainability and readability.
src/main/java/de/tum/cit/aet/artemis/iris/service/session/IrisTextExerciseChatSessionService.java (1)
Line range hint
86-108: Ensure null safety forlatestSubmissionText
latestSubmissionTextmay benullif there is no latest submission available. Ensure thatPyrisTextExerciseChatPipelineExecutionDTOand any downstream processes can handlenullvalues appropriately to prevent potentialNullPointerException.Consider updating the code to handle
nullvalues or providing a default value if necessary.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
📒 Files selected for processing (28)
- src/main/java/de/tum/cit/aet/artemis/iris/service/IrisCompetencyGenerationService.java (2 hunks)
- src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/PyrisJobService.java (2 hunks)
- src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/PyrisPipelineService.java (4 hunks)
- src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/PyrisProgrammingExerciseDTOService.java (4 hunks)
- src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/chat/PyrisChatPipelineExecutionBaseDataDTO.java (0 hunks)
- src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/chat/PyrisChatStatusUpdateDTO.java (1 hunks)
- src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/chat/course/PyrisCourseChatPipelineExecutionDTO.java (1 hunks)
- src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/chat/exercise/PyrisExerciseChatPipelineExecutionDTO.java (1 hunks)
- src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/chat/textexercise/PyrisTextExerciseChatPipelineExecutionDTO.java (1 hunks)
- src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/chat/textexercise/PyrisTextExerciseChatStatusUpdateDTO.java (1 hunks)
- src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/competency/PyrisCompetencyExtractionInputDTO.java (1 hunks)
- src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/competency/PyrisCompetencyExtractionPipelineExecutionDTO.java (2 hunks)
- src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/data/PyrisCompetencyDTO.java (1 hunks)
- src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/data/PyrisCourseDTO.java (1 hunks)
- src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/data/PyrisExerciseWithStudentSubmissionsDTO.java (2 hunks)
- src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/data/PyrisExtendedCourseDTO.java (2 hunks)
- src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/data/PyrisLectureDTO.java (1 hunks)
- src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/data/PyrisLectureUnitDTO.java (1 hunks)
- src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/data/PyrisMessageContentBaseDTO.java (1 hunks)
- src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/data/PyrisProgrammingExerciseDTO.java (1 hunks)
- src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/data/PyrisResultDTO.java (1 hunks)
- src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/data/PyrisSubmissionDTO.java (1 hunks)
- src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/data/PyrisTextExerciseDTO.java (1 hunks)
- src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/data/PyrisUserDTO.java (1 hunks)
- src/main/java/de/tum/cit/aet/artemis/iris/service/session/IrisCourseChatSessionService.java (6 hunks)
- src/main/java/de/tum/cit/aet/artemis/iris/service/session/IrisExerciseChatSessionService.java (4 hunks)
- src/main/java/de/tum/cit/aet/artemis/iris/service/session/IrisTextExerciseChatSessionService.java (3 hunks)
- src/test/java/de/tum/cit/aet/artemis/iris/PyrisLectureIngestionTest.java (2 hunks)
💤 Files with no reviewable changes (1)
- src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/chat/PyrisChatPipelineExecutionBaseDataDTO.java
🧰 Additional context used
📓 Path-based instructions (27)
src/main/java/de/tum/cit/aet/artemis/iris/service/IrisCompetencyGenerationService.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_importssrc/main/java/de/tum/cit/aet/artemis/iris/service/pyris/PyrisJobService.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_importssrc/main/java/de/tum/cit/aet/artemis/iris/service/pyris/PyrisPipelineService.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_importssrc/main/java/de/tum/cit/aet/artemis/iris/service/pyris/PyrisProgrammingExerciseDTOService.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_importssrc/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/chat/PyrisChatStatusUpdateDTO.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_importssrc/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/chat/course/PyrisCourseChatPipelineExecutionDTO.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_importssrc/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/chat/exercise/PyrisExerciseChatPipelineExecutionDTO.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_importssrc/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/chat/textexercise/PyrisTextExerciseChatPipelineExecutionDTO.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_importssrc/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/chat/textexercise/PyrisTextExerciseChatStatusUpdateDTO.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_importssrc/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/competency/PyrisCompetencyExtractionInputDTO.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_importssrc/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/competency/PyrisCompetencyExtractionPipelineExecutionDTO.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_importssrc/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/data/PyrisCompetencyDTO.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_importssrc/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/data/PyrisCourseDTO.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_importssrc/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/data/PyrisExerciseWithStudentSubmissionsDTO.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_importssrc/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/data/PyrisExtendedCourseDTO.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_importssrc/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/data/PyrisLectureDTO.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_importssrc/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/data/PyrisLectureUnitDTO.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_importssrc/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/data/PyrisMessageContentBaseDTO.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_importssrc/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/data/PyrisProgrammingExerciseDTO.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_importssrc/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/data/PyrisResultDTO.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_importssrc/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/data/PyrisSubmissionDTO.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_importssrc/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/data/PyrisTextExerciseDTO.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_importssrc/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/data/PyrisUserDTO.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_importssrc/main/java/de/tum/cit/aet/artemis/iris/service/session/IrisCourseChatSessionService.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_importssrc/main/java/de/tum/cit/aet/artemis/iris/service/session/IrisExerciseChatSessionService.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_importssrc/main/java/de/tum/cit/aet/artemis/iris/service/session/IrisTextExerciseChatSessionService.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_importssrc/test/java/de/tum/cit/aet/artemis/iris/PyrisLectureIngestionTest.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 (35)
src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/data/PyrisUserDTO.java (1)
Line range hint
1-13: Overall, good improvements to PyrisUserDTO with minor suggestions for enhancement.The changes to
PyrisUserDTOare generally positive:
- The use of a record class aligns with DTO guidelines and simplifies the code.
- The introduction of the static factory method
ofimproves the instantiation process.- The
@JsonIncludeannotation helps in creating minimal DTOs.To further enhance the code:
- Consider adding JavaDoc comments for better documentation.
- Implement null checking in the
ofmethod for robustness.- If necessary for backwards compatibility, consider temporarily keeping a deprecated version of the old constructor.
These changes contribute to improved code quality and maintainability, aligning well with the PR objectives.
src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/data/PyrisCourseDTO.java (1)
Line range hint
1-13: Excellent improvements to the PyrisCourseDTO class!The changes made to this class align well with our coding guidelines and best practices:
- Using a record for the DTO adheres to the
java_recordsguideline for DTOs.- The new static factory method
of(Course course)improves readability and provides a clear way to create the DTO from a Course object.- The
@JsonIncludeannotation helps with data economy by excluding empty fields from JSON serialization.- The class follows the single responsibility principle by only representing course data.
- The naming convention (CamelCase) is correctly followed for the class name.
- The use of primitive
longfor the id in the static factory method adheres to the "prefer primitives" guideline.These changes enhance the overall quality and maintainability of the code. Great job!
src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/competency/PyrisCompetencyExtractionInputDTO.java (5)
5-13: LGTM! The DTO structure adheres to coding guidelines.The
PyrisCompetencyExtractionInputDTOrecord is well-structured and follows the provided coding guidelines:
- It uses CamelCase naming convention.
- It adheres to the single responsibility principle.
- It's implemented as a Java record, which is appropriate for DTOs.
- It doesn't contain entities and seems to have minimal data.
- It uses the least access modifier (package-private).
5-10: Javadoc comment is clear and informative.The Javadoc comment provides a concise description of the DTO's purpose and explains each parameter. This adheres to best practices for documentation.
11-11: Appropriate use of JSON annotation.The
@JsonInclude(JsonInclude.Include.NON_EMPTY)annotation is correctly used to exclude empty properties from JSON serialization, which aligns with the data economy principle mentioned in the PR objectives.
12-13: Record declaration is concise and appropriate.The record declaration is well-structured and includes the necessary fields (
courseDescriptionandcurrentCompetencies) as mentioned in the Javadoc comment. The use of a record for this DTO is in line with the coding guidelines for DTOs.
Line range hint
1-13: Overall, excellent implementation of the PyrisCompetencyExtractionInputDTO.This DTO is well-designed, properly documented, and adheres to the coding guidelines. It effectively serves its purpose of encapsulating data for Pyris competency extraction jobs while maintaining good practices such as data economy and clear documentation.
src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/data/PyrisMessageContentBaseDTO.java (1)
8-14: LGTM: Formatting changes improve readability.The addition of formatter comments around the
@JsonSubTypesannotation is a good practice. It ensures consistent formatting and prevents automatic formatters from altering the annotation's layout, which can be crucial for readability in complex type hierarchies.src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/chat/PyrisChatStatusUpdateDTO.java (1)
Line range hint
1-7: LGTM: Package declaration and imports are correct and appropriate.The package declaration follows the expected structure, and the imports are necessary for the DTO. Good practice is followed by using specific imports rather than wildcard imports.
src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/chat/textexercise/PyrisTextExerciseChatStatusUpdateDTO.java (2)
Line range hint
1-7: LGTM: Package declaration and imports are correct.The package declaration follows the CamelCase naming convention, and imports are specific without using wildcards, adhering to the coding guidelines.
16-18: LGTM: Record declaration and annotation are well-designed.The record
PyrisTextExerciseChatStatusUpdateDTOfollows best practices:
- Uses Java record for DTO, as per guidelines.
- Follows CamelCase naming convention.
- Has a single responsibility of representing a chat status update.
- Uses
@JsonInclude(JsonInclude.Include.NON_EMPTY)for minimal DTO serialization.- Properly encapsulates stages using
List<PyrisStageDTO>, promoting code reuse.src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/data/PyrisCompetencyDTO.java (2)
13-22: Improved formatting enhances readability.The record declaration has been formatted across multiple lines, which improves readability. The use of formatter comments (
@formatter:offand@formatter:on) is a good practice to control IDE formatting behavior.
Line range hint
1-28: Overall, the changes improve code quality and adhere to guidelines.The modifications to
PyrisCompetencyDTO.javahave enhanced readability and maintain adherence to coding guidelines. The record structure is well-designed, following the principles of minimal DTOs and single responsibility. The static factory method promotes code reuse.Minor suggestions for improvement include:
- Adding a brief Javadoc comment to describe the DTO's purpose.
- Making the static import for
TimeUtil.toInstantmore explicit.These changes will further improve the code's clarity and maintainability.
src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/chat/textexercise/PyrisTextExerciseChatPipelineExecutionDTO.java (1)
11-18: LGTM: Well-documented DTOThe added Javadoc comment provides clear and concise documentation for the DTO and its parameters. This addition improves code readability and maintainability.
src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/competency/PyrisCompetencyExtractionPipelineExecutionDTO.java (2)
9-9: LGTM: Improved Javadoc formattingThe addition of a period at the end of the class-level Javadoc comment enhances the consistency and completeness of the documentation. This minor change contributes to overall code quality improvement.
18-26: Excellent: Improved record declaration formattingThe reformatting of the
PyrisCompetencyExtractionPipelineExecutionDTOrecord declaration significantly enhances code readability. Key improvements include:
- Each parameter is now on a separate line, making it easier to read and maintain.
- The addition of formatter comments (
// @formatter:offand// @formatter:on) ensures consistent formatting across different IDEs or tools.These changes adhere to our coding guidelines, particularly:
- Using CamelCase naming convention
- Following the "simple_code" principle
- Utilizing Java records for DTOs
- Adhering to "minimal_dtos" and "min_data" principles
Great job on improving the code quality!
src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/data/PyrisTextExerciseDTO.java (1)
25-25: Excellent refactoring to use a factory methodThe change from using a constructor
new PyrisCourseDTO(...)to a static factory methodPyrisCourseDTO.of(...)is a good improvement. This refactoring:
- Encapsulates the creation logic of
PyrisCourseDTOwithin its own class, adhering to the single responsibility principle.- Allows for potential future optimizations or variations in the creation of
PyrisCourseDTOwithout changing this class.- Improves code readability by clearly indicating that we're creating a DTO from another object.
- Promotes code reuse and maintainability.
This change aligns well with our coding guidelines, particularly in terms of using minimal DTOs, promoting code reuse, and keeping the code simple and readable.
src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/chat/course/PyrisCourseChatPipelineExecutionDTO.java (2)
Line range hint
1-11: LGTM: Package declaration and imports are well-organized.The package declaration is correct, and the imports are appropriate for the DTO. Good job avoiding wildcard imports and keeping the imports organized.
27-38: LGTM: Well-structured DTO with appropriate fields.The
PyrisCourseChatPipelineExecutionDTOrecord is well-defined with descriptive field names. The use of@JsonIncludeannotation is good for minimizing DTO size. The multi-line formatting improves readability.src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/chat/exercise/PyrisExerciseChatPipelineExecutionDTO.java (1)
Line range hint
1-11: LGTM: Package declaration and imports are well-organized.The package declaration is correct, and all imports are specific and necessary for the DTO. Good job avoiding wildcard imports, which aligns with the coding guidelines.
src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/data/PyrisExtendedCourseDTO.java (2)
18-36: Improved readability with better formattingThe record definition has been reformatted with each parameter on a new line, which significantly improves readability. The use of formatter comments is also a good practice for maintaining consistent formatting.
These changes adhere to the coding guidelines, particularly the use of Java records for DTOs and maintaining a single responsibility. Good job on improving the code structure!
50-68: Enhanced readability in theofmethodThe
ofmethod has been reformatted with each parameter on a new line, which greatly improves readability. The use of formatter comments is consistent with good coding practices and helps maintain formatting consistency.These changes align well with the KISS principle by making the code simpler and easier to read. Great job on improving the method's structure!
src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/data/PyrisExerciseWithStudentSubmissionsDTO.java (2)
21-35: Improved record declaration formatting.The changes to the
PyrisExerciseWithStudentSubmissionsDTOrecord declaration enhance readability by placing each field on a separate line. This aligns with best practices for code formatting and makes the structure of the DTO clearer. The record adheres to the coding guidelines for DTOs, including the use of Java records, appropriate data types, and following the single responsibility principle.
Line range hint
1-69: Overall assessment: Improved code quality and readability.The changes in this file successfully achieve the PR's objective of improving code quality. The formatting enhancements in both the record declaration and the
ofmethod significantly improve readability without altering functionality. The code adheres to the provided coding guidelines, including the use of Java records for DTOs, appropriate naming conventions, and following the single responsibility principle.These improvements will contribute to easier maintenance and better usability for developers working with the Iris subsystem, aligning well with the overall goals of the pull request.
src/main/java/de/tum/cit/aet/artemis/iris/service/IrisCompetencyGenerationService.java (2)
29-32: Improved constructor dependency injectionThe removal of the
PyrisJobServicedependency from the constructor is a positive change. It aligns with the single responsibility principle and simplifies the class. The use of constructor injection for the remaining dependencies is a good practice.
Line range hint
1-71: Overall improvement in code quality and adherence to best practicesThe changes made to
IrisCompetencyGenerationServicehave significantly improved the class:
- Removal of the
PyrisJobServicedependency aligns with the single responsibility principle.- Simplified job creation process in the
executeCompetencyExtractionPipelinemethod.- Improved constructor dependency injection.
These modifications have resulted in more maintainable and less coupled code, adhering to the provided coding guidelines. The class now better follows SOLID principles, particularly the single responsibility and dependency inversion principles.
To further enhance the code, consider implementing the suggested refactoring in the
executeCompetencyExtractionPipelinemethod to improve readability and maintainability.src/test/java/de/tum/cit/aet/artemis/iris/PyrisLectureIngestionTest.java (1)
29-29: Import statement added correctly.The import for
CourseChatJobis appropriately placed and aligns with the changes made in thetestRunIdIngestionJobmethod.src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/PyrisPipelineService.java (4)
19-19: Approved: Necessary import addedThe import statement for
PyrisJobis appropriately added and is necessary for the updated code.
38-40: Approved: Constructor updated correctlyThe constructor parameters are updated to include only
PyrisConnectorServiceandPyrisJobService, which simplifies the class dependencies and adheres to constructor injection best practices.
59-60: Approved: Method signature updated appropriatelyThe
executePipelinemethod signature is updated to reflect the new parameters, aligning with the refactoring changes. The use ofFunction<String, PyrisJob>forjobFunctionis appropriate.
69-70: Approved: Job registration handled correctlyThe job token is correctly generated using
pyrisJobService.registerJob(jobFunction), and exception handling is properly managed within the try-catch block.src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/PyrisProgrammingExerciseDTOService.java (1)
109-109: Prefer primitive types over wrapper classesIn the constructor call for
PyrisSubmissionDTO, the methodsubmission.isBuildFailed()returns aBooleanobject. If possible, prefer using primitive types to avoid unnecessary object creation and potentialNullPointerException.[practices:prefer_primitives]
Apply this diff:
-submission.isBuildFailed(), +Boolean.TRUE.equals(submission.isBuildFailed()),src/main/java/de/tum/cit/aet/artemis/iris/service/session/IrisCourseChatSessionService.java (3)
75-80: Dependencies are properly injectedThe new dependencies
CourseRepository,StudentParticipationRepository, andLearningMetricsServiceare correctly injected via constructor injection, which adheres to best practices for dependency management in Java.
161-175: Pipeline execution logic is well-implementedThe
pyrisPipelineService.executePipelinemethod is used effectively with clear lambda expressions for job creation and execution DTO construction. The code is concise and adheres to best practices.
179-201: MethodloadCourseWithParticipationOfStudentfollows single responsibilityThe method accurately encapsulates the logic for loading a course along with the student's participations and mapping them to the exercises. It maintains readability and adheres to the Single Responsibility Principle.
src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/data/PyrisResultDTO.java
Outdated
Show resolved
Hide resolved
src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/data/PyrisLectureUnitDTO.java
Outdated
Show resolved
Hide resolved
| public static PyrisUserDTO of(User user) { | ||
| return new PyrisUserDTO(user.getId(), user.getFirstName(), user.getLastName()); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
LGTM! Consider adding null check for robustness.
The static factory method of is a good addition, providing a clear and explicit way to create PyrisUserDTO instances from User objects. It aligns well with the single responsibility principle and improves code readability.
Consider adding a null check for the User parameter to prevent potential NullPointerExceptions:
public static PyrisUserDTO of(User user) {
+ if (user == null) {
+ throw new IllegalArgumentException("User cannot be null");
+ }
return new PyrisUserDTO(user.getId(), user.getFirstName(), user.getLastName());
}📝 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.
| public static PyrisUserDTO of(User user) { | |
| return new PyrisUserDTO(user.getId(), user.getFirstName(), user.getLastName()); | |
| } | |
| public static PyrisUserDTO of(User user) { | |
| if (user == null) { | |
| throw new IllegalArgumentException("User cannot be null"); | |
| } | |
| return new PyrisUserDTO(user.getId(), user.getFirstName(), user.getLastName()); | |
| } |
src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/data/PyrisCourseDTO.java
Outdated
Show resolved
Hide resolved
src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/data/PyrisLectureDTO.java
Outdated
Show resolved
Hide resolved
| private void requestAndHandleResponse(IrisCourseChatSession _session, String variant, CompetencyJol competencyJol) { | ||
| var session = (IrisCourseChatSession) irisSessionRepository.findByIdWithMessagesAndContents(_session.getId()); |
There was a problem hiding this comment.
Avoid parameter naming that starts with an underscore
The parameter _session uses an underscore prefix, which is unconventional in Java and may lead to confusion with private fields or conventions in other languages.
Consider renaming _session to sessionInput or another descriptive name to avoid confusion:
-private void requestAndHandleResponse(IrisCourseChatSession _session, String variant, CompetencyJol competencyJol) {
- var session = (IrisCourseChatSession) irisSessionRepository.findByIdWithMessagesAndContents(_session.getId());
+private void requestAndHandleResponse(IrisCourseChatSession sessionInput, String variant, CompetencyJol competencyJol) {
+ var session = (IrisCourseChatSession) irisSessionRepository.findByIdWithMessagesAndContents(sessionInput.getId());This improves code readability and adheres to standard Java naming conventions.
📝 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.
| private void requestAndHandleResponse(IrisCourseChatSession _session, String variant, CompetencyJol competencyJol) { | |
| var session = (IrisCourseChatSession) irisSessionRepository.findByIdWithMessagesAndContents(_session.getId()); | |
| private void requestAndHandleResponse(IrisCourseChatSession sessionInput, String variant, CompetencyJol competencyJol) { | |
| var session = (IrisCourseChatSession) irisSessionRepository.findByIdWithMessagesAndContents(sessionInput.getId()); |
| course.getExercises().forEach(exercise -> { | ||
| Set<StudentParticipation> exerciseParticipations = participationMap.getOrDefault(exercise.getId(), Set.of()); | ||
| exercise.setStudentParticipations(exerciseParticipations); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Simplify participations mapping using streams
The loop that maps participations to exercises can be simplified using Java Streams for better readability.
Consider refactoring as follows:
-course.getExercises().forEach(exercise -> {
- Set<StudentParticipation> exerciseParticipations = participationMap.getOrDefault(exercise.getId(), Set.of());
- exercise.setStudentParticipations(exerciseParticipations);
-});
+course.getExercises().forEach(exercise ->
+ exercise.setStudentParticipations(participationMap.getOrDefault(exercise.getId(), Set.of()))
+);This reduces verbosity and enhances clarity.
📝 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.
| course.getExercises().forEach(exercise -> { | |
| Set<StudentParticipation> exerciseParticipations = participationMap.getOrDefault(exercise.getId(), Set.of()); | |
| exercise.setStudentParticipations(exerciseParticipations); | |
| }); | |
| course.getExercises().forEach(exercise -> | |
| exercise.setStudentParticipations(participationMap.getOrDefault(exercise.getId(), Set.of())) | |
| ); |
| @@ -88,6 +83,7 @@ public void requestAndHandleResponse(IrisTextExerciseChatSession irisSession) { | |||
| throw new ConflictException("Iris is not enabled for this exercise", "Iris", "irisDisabled"); | |||
| } | |||
| var course = exercise.getCourseViaExerciseGroupOrCourseMember(); | |||
| var exerciseDTO = PyrisTextExerciseDTO.of(exercise); | |||
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Address the TODO: Implement client form data handling
The TODO comment indicates that the current implementation fetches the latest submission text instead of using client-provided form data via IrisMessageResource. Implementing this feature would improve the accuracy and relevance of the data processed.
Would you like assistance in implementing the handling of client form data? I can help by suggesting code changes or opening a GitHub issue to track this task.
| token -> new TextExerciseChatJob(token, course.getId(), exercise.getId(), session.getId()), | ||
| dto -> new PyrisTextExerciseChatPipelineExecutionDTO( | ||
| dto, | ||
| exerciseDTO, | ||
| conversation, | ||
| latestSubmissionText | ||
| ), |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance readability by extracting lambda expressions
The nested lambda expressions within the executePipeline call can impact readability and maintainability. Consider extracting these lambda expressions into separate methods or using method references for clarity.
Apply this refactoring to improve readability:
// @formatter:off
pyrisPipelineService.executePipeline(
"text-exercise-chat",
"default",
- token -> new TextExerciseChatJob(token, course.getId(), exercise.getId(), session.getId()),
- dto -> new PyrisTextExerciseChatPipelineExecutionDTO(
- dto,
- exerciseDTO,
- conversation,
- latestSubmissionText
- ),
+ this::createTextExerciseChatJob,
+ this::createPipelineExecutionDTO,
stages -> irisChatWebsocketService.sendMessage(session, null, stages)
);
// @formatter:on
+
+ private TextExerciseChatJob createTextExerciseChatJob(String token) {
+ return new TextExerciseChatJob(token, course.getId(), exercise.getId(), session.getId());
+ }
+
+ private PyrisTextExerciseChatPipelineExecutionDTO createPipelineExecutionDTO(SomeDtoType dto) {
+ return new PyrisTextExerciseChatPipelineExecutionDTO(
+ dto,
+ exerciseDTO,
+ conversation,
+ latestSubmissionText
+ );
+ }This refactoring separates the lambda expressions into named methods, enhancing code clarity and adherence to the KISS (Keep It Simple, Stupid) principle.
Committable suggestion was skipped due to low confidence.
| private final IrisChatWebsocketService irisChatWebsocketService; | ||
|
|
||
| private final AuthorizationCheckService authCheckService; | ||
|
|
||
| public IrisTextExerciseChatSessionService(IrisSettingsService irisSettingsService, IrisSessionRepository irisSessionRepository, IrisRateLimitService rateLimitService, | ||
| IrisMessageService irisMessageService, TextExerciseRepository textExerciseRepository, StudentParticipationRepository studentParticipationRepository, | ||
| PyrisPipelineService pyrisPipelineService, PyrisJobService pyrisJobService, IrisChatWebsocketService irisChatWebsocketService, | ||
| AuthorizationCheckService authCheckService) { | ||
| PyrisPipelineService pyrisPipelineService, IrisChatWebsocketService irisChatWebsocketService, AuthorizationCheckService authCheckService) { |
There was a problem hiding this comment.
💡 Codebase verification
Remaining References to PyrisJobService Found
Several instances of PyrisJobService still exist in the codebase:
src/test/java/de/tum/cit/aet/artemis/iris/PyrisLectureIngestionTest.javasrc/main/java/de/tum/cit/aet/artemis/iris/service/pyris/PyrisStatusUpdateService.javasrc/main/java/de/tum/cit/aet/artemis/iris/service/pyris/PyrisPipelineService.javasrc/main/java/de/tum/cit/aet/artemis/iris/service/pyris/PyrisWebhookService.javasrc/main/java/de/tum/cit/aet/artemis/iris/web/open/PublicPyrisStatusUpdateResource.java
Please review these files to ensure that all unnecessary references to PyrisJobService are removed.
🔗 Analysis chain
Confirm removal of unused PyrisJobService dependency
The PyrisJobService parameter has been removed from the constructor, which is appropriate since it's no longer needed. Please ensure that all associated fields, methods, and imports related to PyrisJobService are also removed to maintain code cleanliness.
Run the following script to check for any remaining references to PyrisJobService in the codebase:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that all references to PyrisJobService have been removed.
# Test: Search for occurrences of 'PyrisJobService' in Java files. Expect: No matches.
rg --type java 'PyrisJobService'
Length of output: 2864
WalkthroughThe pull request implements significant changes across several classes in the Iris service. Key modifications include the removal of the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
There was a problem hiding this comment.
Actionable comments posted: 31
🧹 Outside diff range comments (4)
src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/data/PyrisLectureDTO.java (1)
Line range hint
1-18: DTO adheres to coding guidelines with room for minor improvement.The
PyrisLectureDTOrecord adheres well to the DTO coding guidelines:
- Uses Java record as recommended.
- Contains only data fields without entities.
- Represents a single concept (lecture) with minimal necessary data.
- Follows single responsibility principle.
The use of
@JsonInclude(JsonInclude.Include.NON_EMPTY)is a good practice for optimizing JSON output.Consider adding a brief Javadoc comment describing the purpose of this DTO to further improve documentation:
/** * Data Transfer Object representing a lecture in the Pyris system. */ @JsonInclude(JsonInclude.Include.NON_EMPTY) public record PyrisLectureDTO( // ... existing fields ) {}src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/data/PyrisCompetencyDTO.java (1)
Line range hint
1-28: DTO implementation adheres to guidelines with room for minor improvement.The
PyrisCompetencyDTOrecord complies with our coding guidelines for DTOs:
- Uses Java records as recommended.
- Contains minimal necessary data.
- Includes a static factory method for convenient object creation.
- Uses
@JsonIncludeto minimize DTO size.Consider adding a
@Nonnullannotation to theofmethod parameter to enforce null-safety:- public static PyrisCompetencyDTO of(Competency competency) { + public static PyrisCompetencyDTO of(@Nonnull Competency competency) {src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/PyrisProgrammingExerciseDTOService.java (2)
Line range hint
60-84: Use more descriptive method names to enhance code readabilityThe methods
convert(ProgrammingExercise exercise)andconvert(ProgrammingSubmission submission)may cause confusion due to their generic names and overloading. To adhere to best practices and improve code clarity, consider renaming them to be more specific:
convertProgrammingExercise(ProgrammingExercise exercise)convertProgrammingSubmission(ProgrammingSubmission submission)This change enhances maintainability and aligns with the Single Responsibility Principle by making the method purposes immediately clear.
Apply this diff to update the method names:
-public PyrisProgrammingExerciseDTO convert(ProgrammingExercise exercise) { +public PyrisProgrammingExerciseDTO convertProgrammingExercise(ProgrammingExercise exercise) { ... -public PyrisSubmissionDTO convert(ProgrammingSubmission submission) { +public PyrisSubmissionDTO convertProgrammingSubmission(ProgrammingSubmission submission) {Also applies to: 94-113
Line range hint
128-138: Handle optional values safely to prevent potential exceptionsIn the
getLatestResultmethod, when accessingfeedback.getLongFeedback().orElseThrow(), there's a risk of throwing aNoSuchElementExceptionif theOptionalis empty, despitefeedback.getHasLongFeedbackText()returningtrue. This could occur if there's a discrepancy betweengetHasLongFeedbackText()and the presence ofLongFeedback.To enhance safety, consider using
orElse(null)and handle the possiblenullvalue:var text = feedback.getDetailText(); if (feedback.getHasLongFeedbackText()) { - text = feedback.getLongFeedback().orElseThrow().getText(); + var longFeedback = feedback.getLongFeedback().orElse(null); + if (longFeedback != null) { + text = longFeedback.getText(); + } else { + log.warn("Expected long feedback text, but none was found for feedback id: {}", feedback.getId()); + } }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
📒 Files selected for processing (28)
- src/main/java/de/tum/cit/aet/artemis/iris/service/IrisCompetencyGenerationService.java (2 hunks)
- src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/PyrisJobService.java (2 hunks)
- src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/PyrisPipelineService.java (4 hunks)
- src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/PyrisProgrammingExerciseDTOService.java (4 hunks)
- src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/chat/PyrisChatPipelineExecutionBaseDataDTO.java (0 hunks)
- src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/chat/PyrisChatStatusUpdateDTO.java (1 hunks)
- src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/chat/course/PyrisCourseChatPipelineExecutionDTO.java (1 hunks)
- src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/chat/exercise/PyrisExerciseChatPipelineExecutionDTO.java (1 hunks)
- src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/chat/textexercise/PyrisTextExerciseChatPipelineExecutionDTO.java (1 hunks)
- src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/chat/textexercise/PyrisTextExerciseChatStatusUpdateDTO.java (1 hunks)
- src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/competency/PyrisCompetencyExtractionInputDTO.java (1 hunks)
- src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/competency/PyrisCompetencyExtractionPipelineExecutionDTO.java (2 hunks)
- src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/data/PyrisCompetencyDTO.java (1 hunks)
- src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/data/PyrisCourseDTO.java (1 hunks)
- src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/data/PyrisExerciseWithStudentSubmissionsDTO.java (2 hunks)
- src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/data/PyrisExtendedCourseDTO.java (2 hunks)
- src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/data/PyrisLectureDTO.java (1 hunks)
- src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/data/PyrisLectureUnitDTO.java (1 hunks)
- src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/data/PyrisMessageContentBaseDTO.java (1 hunks)
- src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/data/PyrisProgrammingExerciseDTO.java (1 hunks)
- src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/data/PyrisResultDTO.java (1 hunks)
- src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/data/PyrisSubmissionDTO.java (1 hunks)
- src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/data/PyrisTextExerciseDTO.java (1 hunks)
- src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/data/PyrisUserDTO.java (1 hunks)
- src/main/java/de/tum/cit/aet/artemis/iris/service/session/IrisCourseChatSessionService.java (6 hunks)
- src/main/java/de/tum/cit/aet/artemis/iris/service/session/IrisExerciseChatSessionService.java (4 hunks)
- src/main/java/de/tum/cit/aet/artemis/iris/service/session/IrisTextExerciseChatSessionService.java (3 hunks)
- src/test/java/de/tum/cit/aet/artemis/iris/PyrisLectureIngestionTest.java (2 hunks)
💤 Files with no reviewable changes (1)
- src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/chat/PyrisChatPipelineExecutionBaseDataDTO.java
🧰 Additional context used
📓 Path-based instructions (27)
src/main/java/de/tum/cit/aet/artemis/iris/service/IrisCompetencyGenerationService.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_importssrc/main/java/de/tum/cit/aet/artemis/iris/service/pyris/PyrisJobService.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_importssrc/main/java/de/tum/cit/aet/artemis/iris/service/pyris/PyrisPipelineService.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_importssrc/main/java/de/tum/cit/aet/artemis/iris/service/pyris/PyrisProgrammingExerciseDTOService.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_importssrc/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/chat/PyrisChatStatusUpdateDTO.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_importssrc/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/chat/course/PyrisCourseChatPipelineExecutionDTO.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_importssrc/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/chat/exercise/PyrisExerciseChatPipelineExecutionDTO.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_importssrc/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/chat/textexercise/PyrisTextExerciseChatPipelineExecutionDTO.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_importssrc/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/chat/textexercise/PyrisTextExerciseChatStatusUpdateDTO.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_importssrc/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/competency/PyrisCompetencyExtractionInputDTO.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_importssrc/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/competency/PyrisCompetencyExtractionPipelineExecutionDTO.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_importssrc/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/data/PyrisCompetencyDTO.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_importssrc/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/data/PyrisCourseDTO.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_importssrc/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/data/PyrisExerciseWithStudentSubmissionsDTO.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_importssrc/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/data/PyrisExtendedCourseDTO.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_importssrc/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/data/PyrisLectureDTO.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_importssrc/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/data/PyrisLectureUnitDTO.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_importssrc/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/data/PyrisMessageContentBaseDTO.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_importssrc/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/data/PyrisProgrammingExerciseDTO.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_importssrc/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/data/PyrisResultDTO.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_importssrc/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/data/PyrisSubmissionDTO.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_importssrc/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/data/PyrisTextExerciseDTO.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_importssrc/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/data/PyrisUserDTO.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_importssrc/main/java/de/tum/cit/aet/artemis/iris/service/session/IrisCourseChatSessionService.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_importssrc/main/java/de/tum/cit/aet/artemis/iris/service/session/IrisExerciseChatSessionService.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_importssrc/main/java/de/tum/cit/aet/artemis/iris/service/session/IrisTextExerciseChatSessionService.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_importssrc/test/java/de/tum/cit/aet/artemis/iris/PyrisLectureIngestionTest.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 (35)
src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/data/PyrisResultDTO.java (1)
10-14: LGTM: Improved record declaration formatting.The reformatting of the
PyrisResultDTOrecord declaration significantly enhances readability. Each parameter on its own line makes it easier to scan and understand the structure of the DTO. This change aligns well with our coding guidelines for improved code quality and maintainability.The record adheres to our DTO guidelines:
- Uses Java records
- Contains minimal data
- Follows single responsibility principle
- Follows CamelCase naming convention
Great job on improving the code structure!
src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/data/PyrisUserDTO.java (1)
Line range hint
1-13: Excellent refactoring to improve DTO design and instantiation!The changes to
PyrisUserDTOdemonstrate several improvements:
- The use of a record class aligns with our DTO guidelines, promoting immutability and reducing boilerplate code.
- The
@JsonIncludeannotation helps minimize the DTO by excluding empty fields during serialization.- The static factory method
of(User user)provides a clear and concise way to createPyrisUserDTOinstances fromUserobjects, improving readability and maintainability.- The implementation correctly maps the
Userfields to the DTO fields, ensuring data consistency.These changes adhere to our coding guidelines, particularly in terms of using Java records for DTOs, minimizing data, and following the single responsibility principle.
src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/data/PyrisCourseDTO.java (3)
Line range hint
1-13: LGTM! Well-structured DTO with improvements.The use of a record class for
PyrisCourseDTOaligns well with our coding guidelines for DTOs. The@JsonIncludeannotation helps minimize the DTO by excluding empty fields from JSON serialization, which is a good practice.
10-12: Good addition of static factory method.The introduction of the static factory method
ofis a positive change. It provides a clear and consistent way to createPyrisCourseDTOinstances fromCourseobjects, adhering to the single responsibility principle and improving code readability.
Line range hint
1-13: Verify impact of constructor removal.The AI summary mentions that a direct constructor accepting a
Courseobject has been removed. While the static factory method is a good addition, we should verify that this change doesn't break any existing code that might have been using the removed constructor.Run the following script to check for any remaining usage of the removed constructor:
If this script returns any results, we may need to update those occurrences to use the new static factory method.
✅ Verification successful
Verification Successful: No remaining usages of the removed constructor found.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any remaining usage of the removed PyrisCourseDTO constructor # Test: Search for direct constructor usage of PyrisCourseDTO with Course parameter rg --type java 'new PyrisCourseDTO\s*\(\s*(?:\w+\s*:\s*)?Course\s'Length of output: 68
Script:
#!/bin/bash # Description: Comprehensive search for any usage of PyrisCourseDTO constructor with Course parameter # Search for direct constructor usage with Course parameter, accounting for various whitespace and possible annotations rg --type java 'new\s+PyrisCourseDTO\s*\(\s*(?:@?\w+\s+)?Course\b' # Additionally, search for factory methods or other classes that might instantiate PyrisCourseDTO with Course rg --type java 'PyrisCourseDTO\.of\s*\(\s*Course\s*\)'Length of output: 125
src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/data/PyrisLectureDTO.java (1)
9-18: Improved formatting enhances readability.The reformatting of the
PyrisLectureDTOrecord declaration, with each parameter on a new line, significantly improves code readability. The use of formatter comments (@formatter:offand@formatter:on) ensures that this formatting is preserved, which is a good practice for maintaining consistent code style.src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/data/PyrisSubmissionDTO.java (1)
10-20: Improved code formatting enhances readability.The new formatting with each field on a separate line and proper indentation significantly improves the readability of the
PyrisSubmissionDTOrecord declaration. This change aligns well with our coding guidelines, particularly the principle of maintaining clear and manageable code structures.src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/competency/PyrisCompetencyExtractionInputDTO.java (1)
Line range hint
1-14: LGTM! Well-structured DTO with proper documentation.The
PyrisCompetencyExtractionInputDTOrecord is well-implemented and adheres to the coding guidelines:
- The naming follows CamelCase convention.
- It's implemented as a Java record, which is ideal for DTOs.
- It has a single responsibility (representing competency extraction input).
- The
@JsonIncludeannotation is used correctly to exclude empty values in JSON serialization.- Comprehensive Javadoc is provided, describing the purpose and parameters.
- Imports are specific, avoiding star imports.
Great job on creating a clean and well-documented DTO!
src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/data/PyrisMessageContentBaseDTO.java (1)
8-14: Improved readability with annotation formattingThe reformatting of the
@JsonSubTypesannotation enhances code readability while maintaining the same semantic content. The use of formatter comments (// @formatter:offand// @formatter:on) is a good practice to control automatic code formatting tools, ensuring that the desired multi-line format is preserved.These changes align well with the "kiss:simple_code" principle from our coding guidelines, making the code easier to read and maintain.
src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/chat/PyrisChatStatusUpdateDTO.java (2)
Line range hint
1-7: LGTM: Package declaration and imports are well-structured.The package declaration follows the expected structure, and imports are specific without using wildcard imports, adhering to the coding guidelines.
Line range hint
1-18: Overall, well-implemented DTO with minor suggestions for improvement.The
PyrisChatStatusUpdateDTOis a well-structured DTO that adheres to most of the coding guidelines and best practices. It uses Java records, follows naming conventions, and includes appropriate annotations.Key strengths:
- Proper use of Java record for DTO implementation.
- Clear and concise structure with appropriate fields.
- Use of
@JsonIncludefor efficient serialization.Areas for improvement:
- Enhance Javadoc with more detailed descriptions and address the TODO comment.
- Consider using more specific types and ensuring immutability for collections.
These minor improvements will further enhance the quality and robustness of the DTO.
src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/chat/textexercise/PyrisTextExerciseChatStatusUpdateDTO.java (4)
Line range hint
1-7: LGTM: Package declaration and imports look good.The package declaration and imports are correctly structured. The use of specific imports rather than wildcard imports aligns with the coding guideline to avoid star imports.
9-15: Javadoc comment is informative and well-structured.The Javadoc comment provides clear information about the purpose of the record and its parameters. It also explains why this DTO is used instead of
PyrisChatStatusUpdateDTO, which is helpful for developers.
Line range hint
16-19: Record declaration adheres to coding guidelines.The use of a Java record for this DTO aligns with the coding guideline for DTOs to use Java records. The record name follows the CamelCase naming convention, and its structure supports the single responsibility principle by focusing on representing a specific response type.
The
@JsonIncludeannotation is used appropriately to exclude empty fields from JSON serialization, which helps minimize DTO data as per the guidelines.
Line range hint
1-19: Overall approval: The file is well-structured and adheres to coding guidelines.This new DTO record is correctly implemented and follows the project's coding standards. It uses Java records for DTOs, follows naming conventions, and includes appropriate documentation. The structure supports single responsibility and minimal data transfer, aligning with the specified coding guidelines.
src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/data/PyrisCompetencyDTO.java (1)
13-22: Improved formatting enhances readability.The use of formatter comments and the multi-line formatting of the record declaration significantly improves code readability. This change aligns well with our coding guidelines for maintaining clean and easily understandable code.
src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/chat/textexercise/PyrisTextExerciseChatPipelineExecutionDTO.java (1)
Line range hint
1-9: LGTM: Package declaration and imports are well-structured.The package declaration and imports follow the coding guidelines. Explicit imports are used, avoiding star imports as recommended.
src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/competency/PyrisCompetencyExtractionPipelineExecutionDTO.java (2)
9-9: LGTM: Improved comment consistency.The addition of a period at the end of the comment enhances readability and maintains consistency in documentation.
Line range hint
1-26: Overall, excellent improvements to code quality and readability.The changes in this file enhance the overall code quality and adhere to our coding guidelines. The DTO is well-structured, uses Java records appropriately, and follows the principles of minimal DTOs and constructor injection. The improved formatting and documentation contribute to better readability and maintainability of the code.
src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/data/PyrisTextExerciseDTO.java (1)
25-25: Approved: Good use of static factory methodThe change from using a constructor to a static factory method (
PyrisCourseDTO.of()) is a good improvement. It enhances code readability and potentially encapsulates creation logic within thePyrisCourseDTOclass.To ensure consistency across the codebase, please run the following script:
This script will help identify any inconsistencies in the usage of
PyrisCourseDTOacross the codebase and verify the implementation of theofmethod.✅ Verification successful
Verified: Consistent usage of
PyrisCourseDTO.of()confirmedAll direct constructor calls to
PyrisCourseDTOhave been replaced with the static factory methodof(). Theofmethod exists and is consistently used across the codebase, ensuring improved code readability and maintainability.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for inconsistent usage of PyrisCourseDTO creation # Test 1: Check for any remaining direct constructor calls echo "Checking for direct constructor calls of PyrisCourseDTO:" rg --type java "new PyrisCourseDTO\(" -g "!PyrisCourseDTO.java" # Test 2: Verify the existence and implementation of the 'of' method echo "Verifying the 'of' method in PyrisCourseDTO:" rg --type java "public static PyrisCourseDTO of\(" -g "PyrisCourseDTO.java" # Test 3: Check for consistent usage of the 'of' method echo "Checking for consistent usage of PyrisCourseDTO.of():" rg --type java "PyrisCourseDTO\.of\(" -g "!PyrisCourseDTO.java"Length of output: 1013
src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/chat/course/PyrisCourseChatPipelineExecutionDTO.java (1)
Line range hint
1-11: LGTM: Package declaration and imports are well-organized.The package declaration is correct, and the imports are appropriately specific without using wildcards. The organization of imports is logical, grouping standard Java imports, Jackson annotation, and custom DTOs separately.
src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/chat/exercise/PyrisExerciseChatPipelineExecutionDTO.java (1)
Line range hint
1-11: LGTM: Imports are appropriate and follow guidelines.The imports are well-organized, avoiding star imports as per the coding guidelines. Each import seems necessary for the DTO's functionality.
src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/data/PyrisExerciseWithStudentSubmissionsDTO.java (1)
21-35: LGTM: Improved formatting for better readabilityThe record definition for
PyrisExerciseWithStudentSubmissionsDTOhas been reformatted for improved readability. Each parameter is now on a separate line, which enhances code clarity. The changes adhere to Java naming conventions and DTO principles.src/main/java/de/tum/cit/aet/artemis/iris/service/IrisCompetencyGenerationService.java (2)
29-32: LGTM: Constructor refactored to remove unnecessary dependencyThe constructor has been simplified by removing the
PyrisJobServicedependency. This change:
- Aligns with the single responsibility principle
- Uses constructor injection for dependency management
- Maintains proper naming conventions
These modifications enhance the overall design and maintainability of the class.
Line range hint
1-73: Overall assessment: Improved code quality and designThe changes in this file successfully achieve the following:
- Remove the unnecessary
PyrisJobServicedependency, aligning with the single responsibility principle.- Simplify the job creation process in the
executeCompetencyExtractionPipelinemethod.- Maintain adherence to coding guidelines and best practices.
These modifications contribute to a more maintainable and efficient codebase, which aligns well with the PR's objective of improving code quality within the Iris subsystem.
src/main/java/de/tum/cit/aet/artemis/iris/service/session/IrisTextExerciseChatSessionService.java (3)
58-58: LGTM: Constructor dependency removal improves design.The removal of the
PyrisJobServicedependency from the constructor aligns with the single responsibility principle and simplifies the service's dependencies. This change is a step towards a more modular and maintainable design.
86-86: LGTM: Improved code structure and readability inrequestAndHandleResponse.The changes in this method enhance code readability and maintain good coding practices:
- The introduction of
exerciseDTOimproves code clarity by extracting the DTO creation.- The simplified lambda expression for
TextExerciseChatJobcreation is more concise.- The use of
exerciseDTOin thePyrisTextExerciseChatPipelineExecutionDTOconstruction reduces redundant calls and improves consistency.These modifications align well with the principles of clean code and maintainability.
Also applies to: 102-108
Line range hint
1-170: Verify impact ofPyrisJobServiceremoval on the wider system.The removal of the
PyrisJobServicedependency from this class is a significant change that improves the service's focus and simplifies its responsibilities. However, it's important to ensure that this change doesn't negatively impact other parts of the system.To maintain system integrity:
- Verify that all responsibilities previously handled by
PyrisJobServicein this context are now appropriately managed elsewhere.- Ensure that any components depending on this service are updated to reflect the removal of job-related functionalities, if any.
- Update relevant documentation to reflect these architectural changes.
To help verify the impact, you can run the following script to check for any remaining references to
PyrisJobServicein related files:This will help identify any lingering references that might need to be addressed.
✅ Verification successful
PyrisJobService dependency successfully removed from IrisTextExerciseChatSessionService.java.
All references to PyrisJobService in the reviewed file have been eliminated. No issues detected related to this removal within the context of the changes made.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for PyrisJobService references in related files echo "Searching for PyrisJobService references:" rg "PyrisJobService" src/main/java/de/tum/cit/aet/artemis/iris/ echo "Searching for createTokenForJob method calls:" rg "createTokenForJob" src/main/java/de/tum/cit/aet/artemis/iris/Length of output: 2896
src/test/java/de/tum/cit/aet/artemis/iris/PyrisLectureIngestionTest.java (2)
29-29: Import statement added correctly.The new import for
CourseChatJobis necessary for the changes in thetestRunIdIngestionJobmethod and is placed appropriately among other imports from the same package.
Line range hint
1-307: Overall: Changes align with PR objectives and maintain test integrity.The modifications in this file successfully adapt the
testRunIdIngestionJobmethod to use a refactored API while maintaining the original test intent. These changes contribute to the PR's goal of improving code quality and consistency across Iris features. The test class remains focused and coherent, with other test methods unaffected by these changes.src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/PyrisPipelineService.java (1)
38-41: Good Practice: Reduced Dependencies in ConstructorThe updated constructor now only includes
PyrisConnectorServiceandPyrisJobService. This reduction in dependencies simplifies the class and enhances modularity, adhering to the single responsibility principle.src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/PyrisJobService.java (1)
70-70: Method renaming provides better clarityThe renaming of
createTokenForJobtoregisterJobimproves code readability and more accurately reflects the method's functionality of registering jobs. Ensure that all references to this method in the codebase and documentation have been updated accordingly.Run the following script to verify that no references to the old method name remain:
src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/PyrisProgrammingExerciseDTOService.java (2)
39-39: Adhere to least privilege by reducing logger visibilityThe logger
logis declared asprivate static final, which is appropriate. However, ensure that it's only used within this class and not accessed externally. This follows the principle of least privilege and enhances encapsulation.
Line range hint
60-84: Verify nullability of repository contents to prevent null pointer exceptionsIn the
convertmethod, the variablestemplateRepositoryContents,solutionRepositoryContents, andtestsRepositoryContentsmay potentially be null if the repository fetching fails. Before using these variables, ensure they are not null to preventNullPointerException.Consider adding null checks or defaulting to empty maps:
var templateRepositoryContents = Optional.ofNullable(getFilteredRepositoryContents(exercise.getTemplateParticipation())).orElse(Map.of()); var solutionRepositoryContents = Optional.ofNullable(getFilteredRepositoryContents(exercise.getSolutionParticipation())).orElse(Map.of()); var testsRepositoryContents = Optional.ofNullable(testRepo.map(repositoryService::getFilesContentFromWorkingCopy).orElse(Map.of())).orElse(Map.of());To confirm that these variables are safely initialized, you can run the following script:
src/main/java/de/tum/cit/aet/artemis/iris/service/session/IrisCourseChatSessionService.java (1)
187-201:⚠️ Potential issueAssess performance impact of eager fetching in
loadCourseWithParticipationOfStudentThe method
loadCourseWithParticipationOfStudentperforms eager fetching of multiple associations:Course course = courseRepository.findWithEagerExercisesAndLecturesAndAttachmentsAndLectureUnitsAndCompetenciesAndExamsById(courseId).orElseThrow();Eagerly loading exercises, lectures, attachments, lecture units, competencies, and exams can lead to performance issues, especially with large courses. Additionally, fetching participations separately and then setting them on exercises adds to the overhead.
Consider the following:
- Optimizing Fetch Strategies: Evaluate if all these associations need to be eagerly loaded. Lazy loading or selective fetching might reduce unnecessary data retrieval.
- Reducing Database Calls: Check if the participations can be fetched more efficiently, possibly combining queries or adjusting repository methods.
- Caching Frequently Accessed Data: Implement caching strategies for data that doesn't change frequently to minimize database access.
Would you like assistance in profiling the database queries or refactoring the method to improve performance?
src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/data/PyrisResultDTO.java
Show resolved
Hide resolved
src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/data/PyrisLectureUnitDTO.java
Outdated
Show resolved
Hide resolved
src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/data/PyrisUserDTO.java
Outdated
Show resolved
Hide resolved
src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/data/PyrisSubmissionDTO.java
Show resolved
Hide resolved
...ain/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/data/PyrisProgrammingExerciseDTO.java
Outdated
Show resolved
Hide resolved
src/main/java/de/tum/cit/aet/artemis/iris/service/session/IrisExerciseChatSessionService.java
Show resolved
Hide resolved
src/main/java/de/tum/cit/aet/artemis/iris/service/session/IrisExerciseChatSessionService.java
Outdated
Show resolved
Hide resolved
| private Course loadCourseWithParticipationOfStudent(long courseId, long studentId) { | ||
| Course course = courseRepository.findWithEagerExercisesAndLecturesAndAttachmentsAndLectureUnitsAndCompetenciesAndExamsById(courseId).orElseThrow(); | ||
| List<StudentParticipation> participations = studentParticipationRepository.findByStudentIdAndIndividualExercisesWithEagerSubmissionsResultIgnoreTestRuns(studentId, | ||
| course.getExercises()); | ||
|
|
||
| Map<Long, Set<StudentParticipation>> participationMap = new HashMap<>(); | ||
| for (StudentParticipation participation : participations) { | ||
| Long exerciseId = participation.getExercise().getId(); | ||
| participationMap.computeIfAbsent(exerciseId, k -> new HashSet<>()).add(participation); | ||
| } | ||
|
|
||
| course.getExercises().forEach(exercise -> { | ||
| Set<StudentParticipation> exerciseParticipations = participationMap.getOrDefault(exercise.getId(), Set.of()); | ||
| exercise.setStudentParticipations(exerciseParticipations); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ensure Single Responsibility Principle in loadCourseWithParticipationOfStudent
The method performs multiple tasks:
- Loading the course with several associations.
- Fetching student participations.
- Mapping participations to exercises.
To adhere to the Single Responsibility Principle, consider refactoring the method into smaller, more focused methods. This enhances readability and maintainability.
| private void requestAndHandleResponse(IrisCourseChatSession session, String variant, CompetencyJol competencyJol) { | ||
| var chatSession = (IrisCourseChatSession) irisSessionRepository.findByIdWithMessagesAndContents(session.getId()); | ||
| private void requestAndHandleResponse(IrisCourseChatSession _session, String variant, CompetencyJol competencyJol) { | ||
| var session = (IrisCourseChatSession) irisSessionRepository.findByIdWithMessagesAndContents(_session.getId()); |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Avoid unnecessary casting of session
The casting to IrisCourseChatSession may be redundant if the repository method findByIdWithMessagesAndContents already returns an IrisCourseChatSession instance. Eliminating unnecessary casts enhances code clarity.
Apply this diff to remove the cast:
-var session = (IrisCourseChatSession) irisSessionRepository.findByIdWithMessagesAndContents(session.getId());
+var session = irisSessionRepository.findByIdWithMessagesAndContents(session.getId());Committable suggestion was skipped due to low confidence.
| @@ -120,10 +145,62 @@ | |||
| requestAndHandleResponse(session, variant, null); | |||
| } | |||
|
|
|||
| private void requestAndHandleResponse(IrisCourseChatSession session, String variant, CompetencyJol competencyJol) { | |||
| var chatSession = (IrisCourseChatSession) irisSessionRepository.findByIdWithMessagesAndContents(session.getId()); | |||
| private void requestAndHandleResponse(IrisCourseChatSession _session, String variant, CompetencyJol competencyJol) { | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Rename parameter _session to session for consistency
The parameter name _session does not follow standard Java naming conventions. In Java, variable and parameter names typically do not start with an underscore. Renaming _session to session will improve readability and maintain consistency.
Apply this diff to rename the parameter:
-private void requestAndHandleResponse(IrisCourseChatSession _session, String variant, CompetencyJol competencyJol) {
+private void requestAndHandleResponse(IrisCourseChatSession session, String variant, CompetencyJol competencyJol) {
var session = (IrisCourseChatSession) irisSessionRepository.findByIdWithMessagesAndContents(_session.getId());
// Update the variable usage accordingly
- var session = (IrisCourseChatSession) irisSessionRepository.findByIdWithMessagesAndContents(_session.getId());
+ var session = (IrisCourseChatSession) irisSessionRepository.findByIdWithMessagesAndContents(session.getId());Committable suggestion was skipped due to low confidence.
github-actions
bot
added
the
atlas
There was a problem hiding this comment.
Actionable comments posted: 29
🧹 Outside diff range comments (4)
src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/data/PyrisTextExerciseDTO.java (1)
Line range hint
31-38: Approved: Consistent use of factory methods and improved formatting.The change to use
PyrisCourseDTO.from(...)instead of the constructor improves consistency with the factory method pattern. The addition of formatter comments helps maintain consistent formatting.Consider extracting the date conversion logic into a utility method to improve code reuse:
private static Instant toInstant(ChronoZonedDateTime<?> dateTime) { return Optional.ofNullable(dateTime) .map(ChronoZonedDateTime::toInstant) .orElse(null); } // Usage in the from method: return new PyrisTextExerciseDTO( // ... other fields toInstant(exercise.getStartDate()), toInstant(exercise.getDueDate()) );This change would make the
frommethod more concise and easier to read.src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/data/PyrisMessageDTO.java (1)
Line range hint
25-37: LGTM: Method refactoring improves clarity and follows conventionsThe
frommethod is well-implemented and follows good practices:
- The name change from
oftofromaligns with common Java conventions for conversion methods.- It adheres to the single responsibility principle by focusing solely on conversion.
- The use of streams and functional programming for content processing is appropriate and efficient.
Consider using
instanceofinstead ofgetClass().equals()for type checking. This would make the code more concise and potentially more efficient:public static PyrisMessageDTO from(IrisMessage message) { var content = message.getContent().stream().map(messageContent -> { PyrisMessageContentBaseDTO result = null; - if (messageContent.getClass().equals(IrisTextMessageContent.class)) { + if (messageContent instanceof IrisTextMessageContent) { result = new PyrisTextMessageContentDTO(messageContent.getContentAsString()); } - else if (messageContent.getClass().equals(IrisJsonMessageContent.class)) { + else if (messageContent instanceof IrisJsonMessageContent) { result = new PyrisJsonMessageContentDTO(messageContent.getContentAsString()); } return result; }).filter(Objects::nonNull).toList(); return new PyrisMessageDTO(toInstant(message.getSentAt()), message.getSender(), content); }This change would slightly improve readability and performance.
src/main/java/de/tum/cit/aet/artemis/iris/service/session/IrisTextExerciseChatSessionService.java (2)
Line range hint
114-115: Consider removing unnecessary access checkThe TODO comment suggests that this access check may be redundant since sessions are fetched from the database using the user ID. Removing unnecessary checks can simplify the code and improve maintainability.
Would you like assistance in verifying if this check can be safely removed? I can help by suggesting code changes or opening a GitHub issue to track this task.
Line range hint
117-118: Evaluate redundancy of role checkThe TODO comment indicates that this role check might be unnecessary because the endpoint already enforces the required role via the
@EnforceAtLeastStudentInExerciseannotation. Eliminating redundant checks can reduce code complexity.Would you like assistance in determining if this check can be safely removed? I can help by suggesting code changes or opening a GitHub issue to track this task.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
📒 Files selected for processing (27)
- src/main/java/de/tum/cit/aet/artemis/atlas/dto/CompetencyJolDTO.java (1 hunks)
- src/main/java/de/tum/cit/aet/artemis/atlas/dto/CompetencyJolPairDTO.java (1 hunks)
- src/main/java/de/tum/cit/aet/artemis/atlas/service/competency/CompetencyJolService.java (1 hunks)
- src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/chat/PyrisChatStatusUpdateDTO.java (1 hunks)
- src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/chat/course/PyrisCourseChatPipelineExecutionDTO.java (1 hunks)
- src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/chat/exercise/PyrisExerciseChatPipelineExecutionDTO.java (1 hunks)
- src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/chat/textexercise/PyrisTextExerciseChatPipelineExecutionDTO.java (1 hunks)
- src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/competency/PyrisCompetencyExtractionInputDTO.java (1 hunks)
- src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/competency/PyrisCompetencyExtractionPipelineExecutionDTO.java (1 hunks)
- src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/competency/PyrisCompetencyRecommendationDTO.java (1 hunks)
- src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/competency/PyrisCompetencyStatusUpdateDTO.java (1 hunks)
- src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/data/PyrisCompetencyDTO.java (1 hunks)
- src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/data/PyrisCourseDTO.java (1 hunks)
- src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/data/PyrisExamDTO.java (1 hunks)
- src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/data/PyrisExerciseWithStudentSubmissionsDTO.java (1 hunks)
- src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/data/PyrisExtendedCourseDTO.java (2 hunks)
- src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/data/PyrisLectureDTO.java (1 hunks)
- src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/data/PyrisLectureUnitDTO.java (1 hunks)
- src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/data/PyrisMessageDTO.java (1 hunks)
- src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/data/PyrisProgrammingExerciseDTO.java (1 hunks)
- src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/data/PyrisResultDTO.java (1 hunks)
- src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/data/PyrisSubmissionDTO.java (1 hunks)
- src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/data/PyrisTextExerciseDTO.java (1 hunks)
- src/main/java/de/tum/cit/aet/artemis/iris/service/session/IrisCourseChatSessionService.java (6 hunks)
- src/main/java/de/tum/cit/aet/artemis/iris/service/session/IrisExerciseChatSessionService.java (4 hunks)
- src/main/java/de/tum/cit/aet/artemis/iris/service/session/IrisTextExerciseChatSessionService.java (3 hunks)
- src/test/java/de/tum/cit/aet/artemis/atlas/competency/CompetencyJolIntegrationTest.java (1 hunks)
🧰 Additional context used
📓 Path-based instructions (27)
src/main/java/de/tum/cit/aet/artemis/atlas/dto/CompetencyJolDTO.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_importssrc/main/java/de/tum/cit/aet/artemis/atlas/dto/CompetencyJolPairDTO.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_importssrc/main/java/de/tum/cit/aet/artemis/atlas/service/competency/CompetencyJolService.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_importssrc/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/chat/PyrisChatStatusUpdateDTO.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_importssrc/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/chat/course/PyrisCourseChatPipelineExecutionDTO.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_importssrc/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/chat/exercise/PyrisExerciseChatPipelineExecutionDTO.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_importssrc/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/chat/textexercise/PyrisTextExerciseChatPipelineExecutionDTO.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_importssrc/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/competency/PyrisCompetencyExtractionInputDTO.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_importssrc/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/competency/PyrisCompetencyExtractionPipelineExecutionDTO.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_importssrc/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/competency/PyrisCompetencyRecommendationDTO.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_importssrc/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/competency/PyrisCompetencyStatusUpdateDTO.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_importssrc/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/data/PyrisCompetencyDTO.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_importssrc/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/data/PyrisCourseDTO.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_importssrc/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/data/PyrisExamDTO.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_importssrc/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/data/PyrisExerciseWithStudentSubmissionsDTO.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_importssrc/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/data/PyrisExtendedCourseDTO.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_importssrc/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/data/PyrisLectureDTO.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_importssrc/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/data/PyrisLectureUnitDTO.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_importssrc/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/data/PyrisMessageDTO.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_importssrc/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/data/PyrisProgrammingExerciseDTO.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_importssrc/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/data/PyrisResultDTO.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_importssrc/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/data/PyrisSubmissionDTO.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_importssrc/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/data/PyrisTextExerciseDTO.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_importssrc/main/java/de/tum/cit/aet/artemis/iris/service/session/IrisCourseChatSessionService.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_importssrc/main/java/de/tum/cit/aet/artemis/iris/service/session/IrisExerciseChatSessionService.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_importssrc/main/java/de/tum/cit/aet/artemis/iris/service/session/IrisTextExerciseChatSessionService.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_importssrc/test/java/de/tum/cit/aet/artemis/atlas/competency/CompetencyJolIntegrationTest.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 (32)
src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/data/PyrisCourseDTO.java (1)
10-11: LGTM! The static factory method is well-implemented.The
frommethod is a good addition that follows best practices for DTOs. It provides a clear and concise way to create aPyrisCourseDTOfrom aCourseobject, adhering to the principle of minimal DTOs.src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/data/PyrisLectureUnitDTO.java (1)
10-19: Approved: Improved formatting enhances readability and maintainability.The reformatting of the
PyrisLectureUnitDTOrecord declaration significantly improves code readability and maintainability. Each field on a separate line makes the structure clearer and easier to modify in the future. The use of formatter comments (@formatter:offand@formatter:on) ensures that this custom formatting is preserved.The record adheres to our DTO guidelines by:
- Using a Java record
- Avoiding entities
- Including only necessary data
- Using
@JsonIncludefor data economy in JSON serializationThese changes align well with our coding principles of simplicity and maintainability.
src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/data/PyrisSubmissionDTO.java (1)
9-9: Consider removing formatter commentsThe
@formatter:offand@formatter:oncomments, while ensuring custom formatting is preserved, may conflict with team-wide formatting standards. It's generally better to rely on agreed-upon team-wide formatting rules for consistency across the codebase.This issue was previously identified in a past review comment. If the team has decided to keep these comments, please disregard this comment.
Also applies to: 20-20
src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/data/PyrisResultDTO.java (3)
8-14: Excellent addition of JavaDoc comment!The newly added JavaDoc comment significantly improves the documentation of the
PyrisResultDTOrecord. It provides a clear description of the DTO's purpose and each of its parameters, enhancing code readability and maintainability.
17-21: Improved formatting enhances readability.The reformatting of the
PyrisResultDTOrecord declaration significantly improves code readability. Each parameter on a separate line makes the structure clearer, especially as the DTO grows. The use of formatter comments to preserve this custom formatting is appropriate and was previously approved.
Line range hint
1-22: LGTM! Excellent adherence to coding guidelines.The
PyrisResultDTOrecord demonstrates exemplary adherence to the project's coding guidelines:
- Uses a Java record for the DTO (dtos:java_records).
- Employs the
@JsonIncludeannotation for data economy (dtos:min_data).- Follows CamelCase naming convention (naming:CamelCase).
- Maintains a single responsibility with minimal data (dtos:single_resp, min_data).
The overall structure is clean, concise, and well-documented.
src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/competency/PyrisCompetencyExtractionInputDTO.java (1)
Line range hint
1-17: LGTM! Well-structured DTO with proper annotations.The
PyrisCompetencyExtractionInputDTOrecord is well-defined and adheres to the coding guidelines. It uses CamelCase naming, follows the single responsibility principle, and is appropriately annotated with@JsonInclude. The use of a Java record for a DTO is also in line with best practices.src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/data/PyrisProgrammingExerciseDTO.java (1)
10-23: Approved: Improved formatting enhances readability.The reformatting of the
PyrisProgrammingExerciseDTOrecord declaration significantly improves code readability. Placing each parameter on a new line and using@formatter:offcomments to maintain the desired structure aligns with best practices for complex record declarations.src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/competency/PyrisCompetencyRecommendationDTO.java (2)
15-16: Excellent addition of @JsonInclude annotationThe addition of
@JsonInclude(JsonInclude.Include.NON_EMPTY)is a great improvement. This annotation ensures that only non-empty properties are included in JSON serialization, which aligns with the "minimal_dtos" principle in our coding guidelines. It helps reduce payload size and improves the efficiency of data transfer.
Line range hint
1-22: Overall excellent improvements to the DTOThe changes made to
PyrisCompetencyRecommendationDTO.javaare commendable and align well with our coding guidelines. The file maintains proper naming conventions, adheres to the single responsibility principle, and effectively uses Java records for DTO representation. The addition of the@JsonIncludeannotation and the improved formatting contribute to better code quality and maintainability.These modifications demonstrate a commitment to clean, efficient, and readable code, which will benefit the entire team working on the Iris subsystem.
src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/competency/PyrisCompetencyExtractionPipelineExecutionDTO.java (1)
9-9: LGTM: Improved documentation consistency.The addition of a period at the end of the documentation comment enhances consistency and adheres to best practices for JavaDoc comments.
src/main/java/de/tum/cit/aet/artemis/atlas/dto/CompetencyJolDTO.java (4)
12-13: LGTM: Improved class documentationThe updated documentation clearly describes the purpose of the class as a Pyris DTO mapping for
CompetencyJol. This change enhances code readability and maintainability.
16-23: LGTM: Improved record declaration formattingThe reformatted record declaration enhances readability by placing each field on a separate line. The use of formatter comments ensures consistent styling. This change aligns well with the coding guidelines for DTOs.
26-26: LGTM: Improved method naming and null safetyThe static method has been renamed from
oftofrom, which is more descriptive and aligns with common factory method naming conventions. The addition of the@NotNullannotation enhances null safety. These changes improve code clarity and robustness.
27-36: LGTM: Improved method implementation formattingThe reformatted method implementation enhances readability by placing each constructor parameter on a separate line. The use of formatter comments ensures consistent styling. This change aligns well with the coding guidelines for simplicity and minimal DTOs.
src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/data/PyrisCompetencyDTO.java (2)
12-14: Excellent addition of Javadoc comment!The added Javadoc comment succinctly describes the purpose of the
PyrisCompetencyDTOrecord, improving code documentation as suggested in a previous review. This addition enhances code readability and maintainability.
27-37: Improved method naming and formatting.The renaming of the method from
oftofromis more descriptive and follows common naming conventions for factory methods. The reformatting of the method implementation improves readability and consistency with the record declaration. The use of formatter comments to control IDE formatting is a good practice.The method adheres to the coding guidelines by using constructor injection and maintaining a single responsibility.
src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/data/PyrisExamDTO.java (2)
26-37: LGTM! Improved readability.The reformatting of the
frommethod body enhances readability while maintaining the existing functionality. The use of@formatterannotations is consistent with good coding practices.
25-25: Verify usage of renamed method across the codebase.The rename from
oftofromis a good change as it better describes the method's purpose. However, this change affects the public API.Please run the following script to verify that all usages of this method have been updated:
src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/data/PyrisTextExerciseDTO.java (1)
29-29: Approved: Consistent method naming.The change from
oftofromin the method signature improves consistency with other DTOs in the codebase. This enhances readability and maintainability.src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/data/PyrisMessageDTO.java (1)
Line range hint
1-17: LGTM: Class declaration follows best practicesThe
PyrisMessageDTOclass is well-structured and follows Java best practices:
- It's declared as a record, which is appropriate for DTOs.
- It uses
@JsonIncludeannotation to exclude null or empty fields from JSON serialization.- The fields (sentAt, sender, contents) are appropriately named and typed.
src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/chat/course/PyrisCourseChatPipelineExecutionDTO.java (2)
15-25: Well-structured Javadoc with clear parameter descriptions.The Javadoc is comprehensive and provides clear descriptions for each parameter, which is excellent for maintaining code clarity and aiding developers who will use this DTO.
As previously suggested in past reviews, consider moving the TODO comment about wrapping
competencyJolinOptionaloutside of the Javadoc for improved visibility and maintainability.
26-37: 🛠️ Refactor suggestionConsider implementing the TODO suggestion to improve DTO structure.
The current implementation is functional, but there are opportunities for improvement:
The TODO comment suggests replacing
settingsandinitialStageswith a singlePyrisPipelineExecutionDTO. This change could simplify the API and reduce the number of fields in this record.The record currently has 7 fields, which is approaching the upper limit of what's considered easily maintainable. Implementing the suggested refactoring could help address this.
The use of
@formatter:offand@formatter:onannotations might not be necessary if your IDE is configured correctly. Consider removing these to keep the code cleaner.Would you like assistance in implementing the suggested refactoring to use
PyrisPipelineExecutionDTO?To ensure the impact of these changes, let's check the usage of this DTO:
✅ Verification successful
Verified: The
PyrisCourseChatPipelineExecutionDTOis only used inIrisCourseChatSessionService.java. Implementing the TODO to replacesettingsandinitialStageswithPyrisPipelineExecutionDTOwill simplify the DTO structure without affecting other parts of the codebase.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for usages of PyrisCourseChatPipelineExecutionDTO rg --type java "PyrisCourseChatPipelineExecutionDTO" -A 5Length of output: 2857
src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/chat/exercise/PyrisExerciseChatPipelineExecutionDTO.java (3)
Line range hint
1-12: LGTM: Package declaration and imports are well-structured.The package declaration is correct, and the imports are specific without using wildcard imports, which aligns with the Java coding guideline to avoid star imports. The use of Jackson annotation is appropriate for a DTO.
15-25: 🧹 Nitpick (assertive)Great Javadoc, consider addressing the TODO comments.
The Javadoc is comprehensive and well-written, providing clear descriptions for each parameter. This aligns with good documentation practices.
However, there are two TODO items mentioned in past review comments that might be worth addressing:
- Consider wrapping the
submissionparameter inOptionalfor API clarity.- The comment suggests encapsulating
settingsandinitialStageswithinPyrisPipelineExecutionDTO.Addressing these now could improve the API design and reduce future technical debt.
26-38: 🧹 Nitpick (assertive)Well-structured DTO, consider addressing the TODO comments.
The
PyrisExerciseChatPipelineExecutionDTOrecord is well-defined and follows the coding guidelines:
- It uses CamelCase naming.
- It's implemented as a Java record, which is ideal for DTOs.
- It uses
@JsonIncludeto minimize DTO data.- The formatting improves readability.
However, there are two TODO comments suggesting potential improvements:
- Replace
settingsandinitialStageswithPyrisPipelineExecutionDTO.- Wrap the
submissionparameter inOptionalfor API clarity.Consider implementing these suggested changes to enhance the DTO structure and API design.
Additionally, the use of
@formatter:offand@formatter:oncomments might not be necessary if your IDE is configured correctly. Consider removing these comments and ensuring that your IDE's formatter settings are consistent across the team.src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/data/PyrisExerciseWithStudentSubmissionsDTO.java (1)
19-38: LGTM: Improved documentation and formattingThe addition of Javadoc for the record and the improved formatting of the record definition enhance code readability and documentation. These changes align well with the coding guidelines and address previous review comments.
src/main/java/de/tum/cit/aet/artemis/atlas/service/competency/CompetencyJolService.java (1)
136-136: LGTM! Verify the behavior of the newfrommethod.The change from
CompetencyJolPairDTO.of()toCompetencyJolPairDTO.from()looks good and adheres to the coding guidelines. It appears to be a refactoring of the DTO creation method.To ensure there are no unintended side effects, please verify that the
frommethod inCompetencyJolPairDTOhas the same behavior as the previousofmethod. Run the following script to check the implementation:src/main/java/de/tum/cit/aet/artemis/iris/service/session/IrisCourseChatSessionService.java (3)
Line range hint
75-96: Constructor injection of new dependencies follows best practicesThe addition of
CourseRepository,StudentParticipationRepository, andLearningMetricsServicevia constructor injection adheres to dependency injection best practices. This enhances testability and maintainability.
154-175: Pipeline execution integration is well-implementedThe modifications in
requestAndHandleResponsemethod correctly construct the necessary DTOs and execute the pipeline with clear and organized code. The use of lambda expressions and method references enhances readability.
179-203: MethodloadCourseWithParticipationOfStudentis effectively implementedThe new method efficiently loads the course along with student participations and sets them on the corresponding exercises. This approach addresses the limitations due to the lack of conditional left joins in Spring Boot 3.
src/main/java/de/tum/cit/aet/artemis/iris/service/session/IrisExerciseChatSessionService.java (1)
70-71: Good Use of Constructor InjectionThe addition of
PyrisProgrammingExerciseDTOServicevia constructor injection aligns with the coding guidelinedi:constructor_injection. This promotes better testability and maintainability.Also applies to: 76-88
| @@ -7,7 +7,7 @@ | |||
| @JsonInclude(JsonInclude.Include.NON_EMPTY) | |||
| public record PyrisCourseDTO(long id, String name, String description) { | |||
|
|
|||
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider renaming the 'name' field to 'title' for consistency
While the current implementation is functional, there's a slight inconsistency between the field name in the DTO (name) and the method used to retrieve it from the Course object (getTitle()). To improve clarity and maintain consistency with the Course class, consider renaming the name field to title.
This change would make the relationship between PyrisCourseDTO and Course more explicit and reduce potential confusion for developers working with both classes.
Here's a suggested modification:
- public record PyrisCourseDTO(long id, String name, String description) {
+ public record PyrisCourseDTO(long id, String title, String description) {
public static PyrisCourseDTO from(Course course) {
return new PyrisCourseDTO(course.getId(), course.getTitle(), course.getDescription());
}
}Note that this change doesn't affect the implementation of the from method, as it already uses course.getTitle().
📝 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.
| public record PyrisCourseDTO(long id, String title, String description) { | |
| public static PyrisCourseDTO from(Course course) { | |
| return new PyrisCourseDTO(course.getId(), course.getTitle(), course.getDescription()); | |
| } | |
| } |
| /** | ||
| * Pyris DTO mapping for a {@code LectureUnit}. | ||
| */ |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Approved: Javadoc comment improves documentation.
The addition of the Javadoc comment addresses previous suggestions and enhances the code's documentation. It succinctly describes the purpose of the DTO.
Consider expanding the comment to provide more context:
/**
* Pyris DTO record mapping for a {@code LectureUnit}.
* This record facilitates the transfer of lecture unit data between application layers.
*/This addition would further clarify the DTO's role in the application architecture.
| // @formatter:off | ||
| @JsonInclude(JsonInclude.Include.NON_EMPTY) | ||
| public record PyrisLectureDTO(long id, String title, String description, ZonedDateTime startDate, ZonedDateTime endDate, List<PyrisLectureUnitDTO> units) { | ||
| } | ||
| public record PyrisLectureDTO( | ||
| long id, | ||
| String title, | ||
| String description, | ||
| ZonedDateTime startDate, | ||
| ZonedDateTime endDate, | ||
| List<PyrisLectureUnitDTO> units | ||
| ) {} | ||
| // @formatter:on |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
LGTM! Consider additional improvements for documentation and code coverage.
The reformatting of the PyrisLectureDTO record significantly improves readability and aligns well with the coding guidelines. The use of a Java record for a DTO is appropriate, and the structure follows the single responsibility principle.
To further enhance the code quality and documentation, consider the following suggestions:
- Add a brief Javadoc comment to describe the purpose of this DTO.
- Include
@Schemaannotations for each field to improve API documentation. - Add a
@Generatedannotation to exclude this DTO from code coverage reports.
Here's an example of how you could implement these suggestions:
import io.swagger.v3.oas.annotations.media.Schema;
import javax.annotation.processing.Generated;
/**
* Data Transfer Object representing a lecture in the Pyris system.
*/
@Generated("org.springframework.boot")
@JsonInclude(JsonInclude.Include.NON_EMPTY)
public record PyrisLectureDTO(
@Schema(description = "The unique identifier of the lecture")
long id,
@Schema(description = "The title of the lecture")
String title,
@Schema(description = "A brief description of the lecture content")
String description,
@Schema(description = "The start date and time of the lecture")
ZonedDateTime startDate,
@Schema(description = "The end date and time of the lecture")
ZonedDateTime endDate,
@Schema(description = "A list of lecture units associated with this lecture")
List<PyrisLectureUnitDTO> units
) {}These additions will provide more context for API consumers, maintain consistency across your DTOs, and help manage code coverage metrics.
| public record PyrisCompetencyExtractionInputDTO( | ||
| String courseDescription, | ||
| PyrisCompetencyRecommendationDTO[] currentCompetencies | ||
| ) {} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using List instead of array for currentCompetencies
As suggested in a previous review, using a List<PyrisCompetencyRecommendationDTO> instead of an array for currentCompetencies would be more aligned with Java best practices. Lists provide more flexibility, a richer API, and are consistent with the Collections framework.
Here's the suggested modification:
public record PyrisCompetencyExtractionInputDTO(
String courseDescription,
- PyrisCompetencyRecommendationDTO[] currentCompetencies
+ List<PyrisCompetencyRecommendationDTO> currentCompetencies
) {}Don't forget to add the necessary import:
import java.util.List;| /** | ||
| * A DTO representing the data required from the client to start a Pyris competency extraction job. | ||
| * | ||
| * @param courseDescription the course description (might have been edited in the client) | ||
| * @param currentCompetencies the current competencies (might be unsaved in the client) | ||
| */ |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Minor improvement for Javadoc
The Javadoc is informative and well-structured. However, to fully adhere to Javadoc conventions, consider adding a brief description of the record itself before the parameter descriptions.
Here's a suggested improvement:
/**
* A DTO representing the data required from the client to start a Pyris competency extraction job.
* This record encapsulates the course description and current competencies for the extraction process.
*
* @param courseDescription the course description (might have been edited in the client)
* @param currentCompetencies the current competencies (might be unsaved in the client)
*/| @@ -181,7 +181,7 @@ void shouldReturnValues() throws Exception { | |||
| final var jol1 = competencyUtilService.createJol(competency[1], student, (short) 8, ZonedDateTime.now(), 0.1, 0.2); | |||
| final var jolMap = sendRequest(HttpStatus.OK); | |||
| assertThat(jolMap).isNotNull(); | |||
| final var expectedMap = Map.of(competency[0].getId(), CompetencyJolPairDTO.of(jol01, jol00), competency[1].getId(), CompetencyJolPairDTO.of(jol1, null)); | |||
| final var expectedMap = Map.of(competency[0].getId(), CompetencyJolPairDTO.from(jol01, jol00), competency[1].getId(), CompetencyJolPairDTO.from(jol1, null)); | |||
There was a problem hiding this comment.
🧹 Nitpick (assertive)
LGTM! Consider enhancing test readability.
The change from CompetencyJolPairDTO.of(...) to CompetencyJolPairDTO.from(...) is consistent with the PR objectives to improve code quality and consistency. The test adheres to the provided coding guidelines, using descriptive naming, fixed data, and specific assertions.
To further enhance test readability, consider extracting the expected map creation to a separate method. This would make the test more concise and easier to understand.
Here's a suggestion for improving readability:
private Map<Long, CompetencyJolPairDTO> createExpectedMap(CompetencyJol jol01, CompetencyJol jol00, CompetencyJol jol1) {
return Map.of(
competency[0].getId(), CompetencyJolPairDTO.from(jol01, jol00),
competency[1].getId(), CompetencyJolPairDTO.from(jol1, null)
);
}
@Test
@WithMockUser(username = TEST_PREFIX + "student1", roles = "USER")
void shouldReturnValues() throws Exception {
// ... existing setup code ...
final var jolMap = sendRequest(HttpStatus.OK);
assertThat(jolMap).isNotNull();
final var expectedMap = createExpectedMap(jol01, jol00, jol1);
// ... existing assertion code ...
}This refactoring would improve the test's readability without changing its functionality.
| @@ -88,6 +83,7 @@ public void requestAndHandleResponse(IrisTextExerciseChatSession irisSession) { | |||
| throw new ConflictException("Iris is not enabled for this exercise", "Iris", "irisDisabled"); | |||
| } | |||
| var course = exercise.getCourseViaExerciseGroupOrCourseMember(); | |||
| var exerciseDTO = PyrisTextExerciseDTO.from(exercise); | |||
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Implement handling of client form data
The TODO comment indicates a plan to use client-provided form data via IrisMessageResource instead of fetching the latest submission text. Implementing this feature would improve the accuracy and relevance of the data processed.
Would you like assistance in implementing the handling of client form data? I can help by suggesting code changes or opening a GitHub issue to track this task.
| import de.tum.cit.aet.artemis.iris.service.pyris.PyrisProgrammingExerciseDTOService; | ||
| import de.tum.cit.aet.artemis.iris.service.pyris.dto.chat.PyrisChatStatusUpdateDTO; | ||
| import de.tum.cit.aet.artemis.iris.service.pyris.dto.chat.exercise.PyrisExerciseChatPipelineExecutionDTO; | ||
| import de.tum.cit.aet.artemis.iris.service.pyris.dto.data.PyrisCourseDTO; | ||
| import de.tum.cit.aet.artemis.iris.service.pyris.dto.data.PyrisMessageDTO; | ||
| import de.tum.cit.aet.artemis.iris.service.pyris.dto.data.PyrisUserDTO; |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider Removing Unused Imports
Lines 28-33 include several DTO imports. Please verify if all these imports are necessary. Unused imports can clutter the code and may lead to confusion.
| // @formatter:off | ||
| pyrisPipelineService.executePipeline( | ||
| "tutor-chat", // TODO: Rename this to 'programming-exercise-chat' | ||
| variant, | ||
| token -> new ExerciseChatJob(token, course.getId(), exercise.getId(), session.getId()), | ||
| executionDto -> new PyrisExerciseChatPipelineExecutionDTO( | ||
| latestSubmissionDTO, | ||
| exerciseDTO, | ||
| courseDTO, | ||
| conversationDTO, | ||
| userDTO, | ||
| executionDto.settings(), | ||
| executionDto.initialStages() | ||
| ), | ||
| stages -> irisChatWebsocketService.sendStatusUpdate(session, stages) | ||
| ); | ||
| // @formatter:on |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Minimize the Use of @formatter:off and @formatter:on
Excessive use of formatter directives can reduce code readability. If special formatting is not essential, consider removing these directives to maintain consistent code formatting.
| String variant = irisSettingsService.getCombinedIrisSettingsFor(session.getExercise(), false).irisChatSettings().selectedVariant(); | ||
| var exercise = programmingExerciseRepository.findByIdWithTemplateAndSolutionParticipationElseThrow(session.getExercise().getId()); | ||
| var latestSubmissionDTO = getLatestSubmissionIfExists(exercise, session.getUser()).map(pyrisProgrammingExerciseDTOService::convert).orElse(null); | ||
| var exerciseDTO = pyrisProgrammingExerciseDTOService.convert(exercise); |
There was a problem hiding this comment.
Ensure Proper Null Checks and Exception Handling
Methods like getCombinedIrisSettingsFor, findByIdWithTemplateAndSolutionParticipationElseThrow, and getLatestSubmissionIfExists may return null or throw exceptions. To prevent potential NullPointerExceptions or unexpected crashes, please ensure that you handle these cases appropriately.
Consider adding null checks or handling exceptions:
String variant = irisSettingsService.getCombinedIrisSettingsFor(session.getExercise(), false).irisChatSettings().selectedVariant();
+if (variant == null) {
+ throw new IllegalStateException("Selected variant is null");
+}
var exercise = programmingExerciseRepository.findByIdWithTemplateAndSolutionParticipationElseThrow(session.getExercise().getId());
// Exception handling is already implemented if the exercise is not found.
var latestSubmissionDTO = getLatestSubmissionIfExists(exercise, session.getUser()).map(pyrisProgrammingExerciseDTOService::convert).orElse(null);
+if (latestSubmissionDTO == null) {
+ // Handle the case where there is no latest submission
+}Committable suggestion was skipped due to low confidence.
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range comments (1)
src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/PyrisPipelineService.java (1)
Line range hint
59-83: Refactor nested try-catch blocks for better readabilityThe nested try-catch blocks within the
executePipelinemethod may reduce readability and make error handling more complex. Consider refactoring to simplify the exception handling structure, which will improve maintainability.Here's a suggested refactoring to streamline the exception handling:
public void executePipeline(String name, String variant, Function<String, PyrisJob> jobFunction, Function<PyrisPipelineExecutionDTO, Object> dtoMapper, Consumer<List<PyrisStageDTO>> statusUpdater) { var preparing = new PyrisStageDTO("Preparing", 10, null, null); var executing = new PyrisStageDTO("Executing pipeline", 30, null, null); statusUpdater.accept(List.of(preparing.inProgress(), executing.notStarted())); String jobToken; try { jobToken = pyrisJobService.registerJob(jobFunction); } catch (Exception e) { log.error("Failed to register job for {} pipeline execution", name, e); statusUpdater.accept(List.of(preparing.error("Failed to register job"), executing.notStarted())); return; } var baseDto = new PyrisPipelineExecutionDTO(new PyrisPipelineExecutionSettingsDTO(jobToken, List.of(), artemisBaseUrl), List.of(preparing.done())); var pipelineDto = dtoMapper.apply(baseDto); // Send a status update that preparation is done and pipeline execution is starting statusUpdater.accept(List.of(preparing.done(), executing.inProgress())); try { // Execute the pipeline using the connector service pyrisConnectorService.executePipeline(name, variant, pipelineDto); } catch (PyrisConnectorException | IrisException e) { log.error("Failed to execute {} pipeline", name, e); statusUpdater.accept(List.of(preparing.done(), executing.error("An internal error occurred"))); } catch (Exception e) { log.error("Unexpected error during {} pipeline execution", name, e); statusUpdater.accept(List.of(preparing.done(), executing.error("An unexpected error occurred"))); } }This refactoring removes the nested try-catch blocks by:
- Handling exceptions from
registerJobseparately at the beginning.- Flattening the structure to have a single try-catch for the pipeline execution.
- Adding a general
Exceptioncatch to handle any unforeseen errors.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
📒 Files selected for processing (7)
- src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/PyrisJobService.java (2 hunks)
- src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/PyrisPipelineService.java (4 hunks)
- src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/data/PyrisExtendedCourseDTO.java (1 hunks)
- src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/data/PyrisSubmissionDTO.java (1 hunks)
- src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/data/PyrisUserDTO.java (1 hunks)
- src/main/java/de/tum/cit/aet/artemis/iris/service/session/IrisCourseChatSessionService.java (6 hunks)
- src/main/java/de/tum/cit/aet/artemis/iris/service/session/IrisExerciseChatSessionService.java (4 hunks)
🧰 Additional context used
📓 Path-based instructions (7)
src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/PyrisJobService.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_importssrc/main/java/de/tum/cit/aet/artemis/iris/service/pyris/PyrisPipelineService.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_importssrc/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/data/PyrisExtendedCourseDTO.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_importssrc/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/data/PyrisSubmissionDTO.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_importssrc/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/data/PyrisUserDTO.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_importssrc/main/java/de/tum/cit/aet/artemis/iris/service/session/IrisCourseChatSessionService.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_importssrc/main/java/de/tum/cit/aet/artemis/iris/service/session/IrisExerciseChatSessionService.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/iris/service/session/IrisExerciseChatSessionService.java (1)
Learnt from: alexjoham PR: ls1intum/Artemis#9455 File: src/test/java/de/tum/cit/aet/artemis/iris/IrisTextExerciseChatMessageIntegrationTest.java:401-401 Timestamp: 2024-10-15T11:33:17.915Z Learning: In the Artemis project, when new fields are added to classes like `PyrisChatStatusUpdateDTO`, corresponding tests may be implemented in separate integration test classes such as `IrisChatTokenTrackingIntegrationTest`.
🔇 Additional comments (15)
src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/data/PyrisUserDTO.java (1)
Line range hint
1-9: LGTM! Well-structured DTO classThe
PyrisUserDTOclass is well-designed:
- Appropriate use of a record for a DTO, ensuring immutability.
- Good use of
@JsonInclude(JsonInclude.Include.NON_EMPTY)for minimal JSON serialization.- Follows naming conventions and the single responsibility principle.
The overall structure and annotations of the class are excellent and align well with best practices for DTOs in Java.
src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/data/PyrisSubmissionDTO.java (1)
9-11: LGTM! JavaDoc comment improves documentation.The addition of the JavaDoc comment clearly describes the purpose of the
PyrisSubmissionDTOrecord, improving code documentation as suggested in a previous review. This change aligns with best practices for code readability and maintainability.src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/PyrisPipelineService.java (4)
19-19: Import ofPyrisJobis necessary and appropriateThe addition of the
PyrisJobimport is essential due to its usage in the updated method signatures, ensuring the code compiles and functions correctly.
38-40: Constructor simplification enhances modularityBy reducing the number of dependencies in the constructor, the code adheres to the Single Responsibility Principle, improving modularity and maintainability of the
PyrisPipelineService.
45-46: Also applies to: 55-57
59-60: Method signature update improves flexibilityThe
executePipelinemethod now accepts ajobFunctionparameter, allowing for dynamic creation ofPyrisJobinstances. This enhances flexibility and aligns with best practices for designing extensible code.src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/PyrisJobService.java (3)
64-65: Update JavaDoc to reflect method's functionalityThe JavaDoc for
registerJobstill describes creating a new job token and running a function, but it doesn't fully capture the method's purpose after renaming. Please refer to the previous review comments for more details.
71-74: LGTMThe
registerJobmethod is correctly implemented and follows best practices.
117-117: Clarify JavaDoc forgetAndAuthenticateJobFromHeaderElseThrowThe phrase "The token was previously generated via" in the JavaDoc might be misleading. Consider updating the wording for clarity, as mentioned in previous review comments.
src/main/java/de/tum/cit/aet/artemis/iris/service/session/IrisCourseChatSessionService.java (2)
148-177: Implementation ofrequestAndHandleResponseis correctThe method
requestAndHandleResponseeffectively handles the session data, constructs the necessary DTOs, and executes the pipeline appropriately. The code is clean and follows the best practices.
187-203:loadCourseWithParticipationOfStudentmethod is well-structuredThe method efficiently loads the course and student participations, mapping them correctly to exercises. The implementation adheres to the Single Responsibility Principle and maintains code readability.
src/main/java/de/tum/cit/aet/artemis/iris/service/session/IrisExerciseChatSessionService.java (4)
28-33: Necessary Imports AddedThe newly added imports for DTOs and services are appropriate and necessary for the added functionalities. They enhance the modularity and reusability of the code.
70-71: Dependency Injection Implemented CorrectlyThe
PyrisProgrammingExerciseDTOServiceis properly declared as aprivate finalfield and will be injected via constructor injection, adhering to the coding guidelinedi:constructor_injection.
76-76: Constructor Parameters Updated SuccessfullyThe constructor now includes the
PyrisProgrammingExerciseDTOServiceas a parameter, and it's assigned to the class field. This ensures that all dependencies are correctly managed.Also applies to: 88-88
148-179:requestAndHandleResponseMethod EnhancedThe
requestAndHandleResponsemethod has been updated to utilize the new DTO services and pipeline execution. This change improves data handling by converting entities to their corresponding DTOs and aligns with best practices for modularity and reusability.
| public static PyrisUserDTO from(User user) { | ||
| return new PyrisUserDTO(user.getId(), user.getFirstName(), user.getLastName()); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance robustness with null checks and error handling
The static factory method from is a good addition, providing a clear way to create PyrisUserDTO instances from User objects. However, consider improving its robustness:
- Add a null check for the
Userparameter. - Handle potential
NullPointerExceptions when accessingUserfields.
Here's a suggested implementation:
public static PyrisUserDTO from(User user) {
if (user == null) {
throw new IllegalArgumentException("User cannot be null");
}
return new PyrisUserDTO(
user.getId(),
user.getFirstName() != null ? user.getFirstName() : "",
user.getLastName() != null ? user.getLastName() : ""
);
}This change prevents NullPointerExceptions and provides more informative error messages.
| public record PyrisSubmissionDTO( | ||
| long id, | ||
| Instant date, | ||
| Map<String, String> repository, | ||
| boolean isPractice, | ||
| boolean buildFailed, | ||
| List<PyrisBuildLogEntryDTO> buildLogEntries, | ||
| PyrisResultDTO latestResult | ||
| ) {} |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider renaming boolean field for consistency.
The PyrisSubmissionDTO record adheres well to our coding guidelines:
- It uses CamelCase naming.
- It's implemented as a Java record, following the "dtos:{java_records}" guideline.
- It contains only necessary data, aligning with "dtos:{min_data}".
- It uses
Instantfor thedatefield, following "db:{datetime_not_timestamp}".
However, to better align with Java naming conventions for boolean fields, consider renaming the isPractice field:
public record PyrisSubmissionDTO(
long id,
Instant date,
Map<String, String> repository,
- boolean isPractice,
+ boolean practice,
boolean buildFailed,
List<PyrisBuildLogEntryDTO> buildLogEntries,
PyrisResultDTO latestResult
) {
}This change would improve consistency with common Java practices and make the code more idiomatic.
📝 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.
| public record PyrisSubmissionDTO( | |
| long id, | |
| Instant date, | |
| Map<String, String> repository, | |
| boolean isPractice, | |
| boolean buildFailed, | |
| List<PyrisBuildLogEntryDTO> buildLogEntries, | |
| PyrisResultDTO latestResult | |
| ) {} | |
| public record PyrisSubmissionDTO( | |
| long id, | |
| Instant date, | |
| Map<String, String> repository, | |
| boolean practice, | |
| boolean buildFailed, | |
| List<PyrisBuildLogEntryDTO> buildLogEntries, | |
| PyrisResultDTO latestResult | |
| ) { | |
| } |
| // @formatter:off | ||
| @JsonInclude(JsonInclude.Include.NON_EMPTY) | ||
| public record PyrisSubmissionDTO(long id, Instant date, Map<String, String> repository, boolean isPractice, boolean buildFailed, List<PyrisBuildLogEntryDTO> buildLogEntries, | ||
| PyrisResultDTO latestResult) { | ||
| } | ||
| public record PyrisSubmissionDTO( | ||
| long id, | ||
| Instant date, | ||
| Map<String, String> repository, | ||
| boolean isPractice, | ||
| boolean buildFailed, | ||
| List<PyrisBuildLogEntryDTO> buildLogEntries, | ||
| PyrisResultDTO latestResult | ||
| ) {} | ||
| // @formatter:on |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider team-wide formatting approach and minor alignment adjustment.
The reformatting of the PyrisSubmissionDTO record declaration significantly improves readability, which is great. However, there are a couple of points to consider:
-
The
@formatter:offand@formatter:oncomments, while ensuring custom formatting is preserved, might conflict with team-wide formatting standards. Consider discussing with the team whether to keep these or rely on agreed-upon team-wide formatting rules. -
For consistency with common Java practices, consider aligning the closing parenthesis and curly brace with the record declaration:
public record PyrisSubmissionDTO(
long id,
Instant date,
Map<String, String> repository,
boolean isPractice,
boolean buildFailed,
List<PyrisBuildLogEntryDTO> buildLogEntries,
PyrisResultDTO latestResult
-) {}
+) {
+}These adjustments would further enhance consistency and adhere to typical Java formatting conventions.
📝 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.
| // @formatter:off | |
| @JsonInclude(JsonInclude.Include.NON_EMPTY) | |
| public record PyrisSubmissionDTO(long id, Instant date, Map<String, String> repository, boolean isPractice, boolean buildFailed, List<PyrisBuildLogEntryDTO> buildLogEntries, | |
| PyrisResultDTO latestResult) { | |
| } | |
| public record PyrisSubmissionDTO( | |
| long id, | |
| Instant date, | |
| Map<String, String> repository, | |
| boolean isPractice, | |
| boolean buildFailed, | |
| List<PyrisBuildLogEntryDTO> buildLogEntries, | |
| PyrisResultDTO latestResult | |
| ) {} | |
| // @formatter:on | |
| // @formatter:off | |
| @JsonInclude(JsonInclude.Include.NON_EMPTY) | |
| public record PyrisSubmissionDTO( | |
| long id, | |
| Instant date, | |
| Map<String, String> repository, | |
| boolean isPractice, | |
| boolean buildFailed, | |
| List<PyrisBuildLogEntryDTO> buildLogEntries, | |
| PyrisResultDTO latestResult | |
| ) { | |
| } | |
| // @formatter:on |
| // @formatter:off | ||
| @JsonInclude(JsonInclude.Include.NON_EMPTY) | ||
| public record PyrisExtendedCourseDTO(long id, String name, String description, Instant startTime, Instant endTime, ProgrammingLanguage defaultProgrammingLanguage, | ||
| int maxComplaints, int maxTeamComplaints, int maxComplaintTimeDays, int maxRequestMoreFeedbackTimeDays, Integer maxPoints, Integer presentationScore, | ||
| List<PyrisExerciseWithStudentSubmissionsDTO> exercises, List<PyrisExamDTO> exams, List<PyrisCompetencyDTO> competencies) { | ||
| public record PyrisExtendedCourseDTO( | ||
| long id, | ||
| String name, | ||
| String description, | ||
| Instant startTime, | ||
| Instant endTime, | ||
| ProgrammingLanguage defaultProgrammingLanguage, | ||
| int maxComplaints, | ||
| int maxTeamComplaints, | ||
| int maxComplaintTimeDays, | ||
| int maxRequestMoreFeedbackTimeDays, | ||
| Integer maxPoints, | ||
| Integer presentationScore, | ||
| List<PyrisExerciseWithStudentSubmissionsDTO> exercises, | ||
| List<PyrisExamDTO> exams, | ||
| List<PyrisCompetencyDTO> competencies | ||
| // @formatter:on | ||
| ) { |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
LGTM! Consider adding Javadoc for record parameters.
The reformatting of the record declaration significantly improves readability and aligns well with our coding guidelines for DTOs. The use of @formatter annotations ensures consistent formatting.
To further enhance the documentation, consider adding Javadoc comments for each parameter in the record declaration. For example:
/**
* An extended course DTO for Pyris so it can better answer
* questions regarding the course organization and content.
*
* @param id The unique identifier of the course
* @param name The name of the course
* @param description A brief description of the course
* ...
*/
@JsonInclude(JsonInclude.Include.NON_EMPTY)
public record PyrisExtendedCourseDTO(
long id,
String name,
String description,
...
) {
// ...
}This addition would provide clear documentation for each field, improving code maintainability.
| public static PyrisExtendedCourseDTO from(Course course) { | ||
| List<PyrisExerciseWithStudentSubmissionsDTO> exercises = course.getExercises().stream().map(PyrisExerciseWithStudentSubmissionsDTO::from).toList(); | ||
| List<PyrisExamDTO> exams = course.getExams().stream().map(PyrisExamDTO::from).toList(); | ||
| List<PyrisCompetencyDTO> competencies = course.getCompetencies().stream().map(PyrisCompetencyDTO::from).toList(); | ||
|
|
||
| List<PyrisExamDTO> exams = course.getExams().stream().map(PyrisExamDTO::of).toList(); | ||
| List<PyrisCompetencyDTO> competencies = course.getCompetencies().stream().map(PyrisCompetencyDTO::of).toList(); | ||
|
|
||
| return new PyrisExtendedCourseDTO(course.getId(), course.getTitle(), course.getDescription(), toInstant(course.getStartDate()), toInstant(course.getEndDate()), | ||
| course.getDefaultProgrammingLanguage(), course.getMaxComplaints(), course.getMaxTeamComplaints(), course.getMaxComplaintTimeDays(), | ||
| course.getMaxRequestMoreFeedbackTimeDays(), course.getMaxPoints(), course.getPresentationScore(), exercises, exams, competencies); | ||
| // @formatter:off | ||
| return new PyrisExtendedCourseDTO( | ||
| course.getId(), | ||
| course.getTitle(), | ||
| course.getDescription(), | ||
| toInstant(course.getStartDate()), | ||
| toInstant(course.getEndDate()), | ||
| course.getDefaultProgrammingLanguage(), | ||
| course.getMaxComplaints(), | ||
| course.getMaxTeamComplaints(), | ||
| course.getMaxComplaintTimeDays(), | ||
| course.getMaxRequestMoreFeedbackTimeDays(), | ||
| course.getMaxPoints(), | ||
| course.getPresentationScore(), | ||
| exercises, | ||
| exams, | ||
| competencies | ||
| ); | ||
| // @formatter:on |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
LGTM! Consider extracting mapping logic.
The renaming of the method from of to from improves consistency with other DTOs in the codebase. The reformatting of the method body significantly enhances readability.
To further improve the code structure and adhere to our coding guidelines for small methods and code reuse, consider extracting the mapping logic for exercises, exams, and competencies into separate private methods. For example:
public static PyrisExtendedCourseDTO from(Course course) {
return new PyrisExtendedCourseDTO(
course.getId(),
course.getTitle(),
course.getDescription(),
toInstant(course.getStartDate()),
toInstant(course.getEndDate()),
course.getDefaultProgrammingLanguage(),
course.getMaxComplaints(),
course.getMaxTeamComplaints(),
course.getMaxComplaintTimeDays(),
course.getMaxRequestMoreFeedbackTimeDays(),
course.getMaxPoints(),
course.getPresentationScore(),
mapExercises(course),
mapExams(course),
mapCompetencies(course)
);
}
private static List<PyrisExerciseWithStudentSubmissionsDTO> mapExercises(Course course) {
return course.getExercises().stream().map(PyrisExerciseWithStudentSubmissionsDTO::from).toList();
}
private static List<PyrisExamDTO> mapExams(Course course) {
return course.getExams().stream().map(PyrisExamDTO::from).toList();
}
private static List<PyrisCompetencyDTO> mapCompetencies(Course course) {
return course.getCompetencies().stream().map(PyrisCompetencyDTO::from).toList();
}This refactoring would improve the method's adherence to the Single Responsibility Principle and make the code more maintainable.
Checklist
General
Server
Client
authoritiesto all new routes and checked the course groups for displaying navigation elements (links, buttons).Motivation and Context
The Iris subsystem has some inconsistencies and missing documentation which is a burden for the developer.
Description
This PR refactors some code out of
PyrisPipelineServiceto be more consistent among each Iris feature. It also adds documentation and better formatting to DTOs.Steps for Testing
Prerequisites:
Exam Mode Testing
Prerequisites:
Testserver States
Note
These badges show the state of the test servers.
Green = Currently available, Red = Currently locked
Click on the badges to get to the test servers.
Review Progress
Performance Review
Code Review
Manual Tests
Exam Mode Test
Performance Tests
Test Coverage
Screenshots
Summary by CodeRabbit
New Features
PyrisChatStatusUpdateDTO,PyrisCourseChatPipelineExecutionDTO, andPyrisExerciseChatPipelineExecutionDTO.PyrisCompetencyExtractionInputDTOfor initiating competency extraction jobs.Bug Fixes
Refactor
Documentation