-
Notifications
You must be signed in to change notification settings - Fork 111
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
OP-1150 OP-1151 OP-1153 OP-1154 OP-1155 Refactoring smells #1175
base: develop
Are you sure you want to change the base?
Conversation
…ted for resolution
…ted for resolution
…ted for resolution
@@ -508,4 +507,8 @@ else if (dateOut.isBefore(ad.getAdmDate()) || dateOut.equals(ad.getAdmDate())) { | |||
throw new OHDataValidationException(errors); | |||
} | |||
} | |||
public boolean isAdmissionActiveOnDate(Admission admission, LocalDateTime checkDate) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see little value in extracting the conditional into a method given the overhead that is required to invoke the method. I don't believe this should be done.
@@ -0,0 +1,61 @@ | |||
package org.isf.permissions.test; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing license header in multiple files. Please see the .license-header directory in this repo.
import org.mockito.Mock; | ||
import org.mockito.MockitoAnnotations; | ||
import java.util.List; | ||
import static org.junit.jupiter.api.Assertions.*; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The project standards do not support package level imports. Please see the .ide-settings directory in this repo for import ordering and code formatting. This applies to multiple files in this PR.
Permission result = permissionIoOperations.insertPermission(permission); | ||
|
||
// Assert | ||
assertNotNull(result); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The project standards are to use aspectj
; please adopt the standard. This applies to multiple files in this PR.
…hod refactoring with build success and better readability
@@ -0,0 +1,434 @@ | |||
package org.isf.dicom.manager; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
missing license header
import com.drew.metadata.exif.ExifDirectoryBase; | ||
import com.drew.metadata.exif.ExifIFD0Directory; | ||
import static org.isf.dicom.manager.DicomLoader.getSeriesDateTime; | ||
import static org.isf.dicom.manager.DicomLoader.getStudyDateTime; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
static imports are at the first ones in the list.
@@ -0,0 +1,28 @@ | |||
package org.isf.utils.exception; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
missing license header
@@ -0,0 +1,28 @@ | |||
package org.isf.utils.exception; | |||
import org.junit.jupiter.api.Test; | |||
import static org.junit.jupiter.api.Assertions.assertEquals; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
project standards are to use aspectj (I.e., assertThat()
)
At a minimum please Code Style so there are fewer issues that we need to comment on. |
@Dharmil4602 please read my comment at https://openhospital.atlassian.net/browse/OP-1154?focusedCommentId=15187 |
See OP-1150, OP-1151, OP-1153, OP-1154 and OP-1155.
This pull request addresses the "smell refactoring" by various refactoring techniques like Rename variable, move method. The refactoring effort aims to enhance code readability and maintainability by breaking down complex conditional statements into more modular and understandable components.
Changes Made:
Issue [Complex Conditional]: https://openhospital.atlassian.net/browse/OP-1150
Issue [Feature Envy]: https://openhospital.atlassian.net/browse/OP-1153