-
Notifications
You must be signed in to change notification settings - Fork 427
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
Reduce code duplication in build.gradle #195
Reduce code duplication in build.gradle #195
Conversation
Click here to submit a new iteration when this PR is ready for review. See this repository's contribution guide for more information. |
698868d
to
9052c65
Compare
Codecov Report
@@ Coverage Diff @@
## master #195 +/- ##
=========================================
Coverage 73.61% 73.61%
Complexity 420 420
=========================================
Files 71 71
Lines 1285 1285
Branches 126 126
=========================================
Hits 946 946
Misses 307 307
Partials 32 32 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
9052c65
to
234dc4e
Compare
implementation group: 'org.openjfx', name: 'javafx-graphics', version: javaFxVersion, classifier: 'linux' | ||
jfxModules.each { mod -> | ||
platforms.each { os -> | ||
implementation group: 'org.openjfx', name: mod, version: javaFxVersion, classifier: os |
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.
It's also possible to write the statement as below which to me seems shorter and more compact.
implementation group: 'org.openjfx', name: mod, version: javaFxVersion, classifier: os | |
implementation "org.openjfx:$mod:$javaFxVersion:$os" |
I used the current version to maintain the same flavor of dependency imports with the other modules.
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.
Can also consider removing the repetition of the "javafx-" prefix in the jfxModules list by adding it within the implementation configuration:
List<String> jfxModules = ['base', 'graphics', 'controls', 'fxml']
jfxModules.each { mod ->
platforms.each { os ->
implementation "org.openjfx:javafx-$mod:$javaFxVersion:$os"
}
}
Other than that LGTM!
Importing dependencies for JavaFX in build.gradle is highly repetitive. This repetition makes it difficult to read, modify, and edit modules if necessary. Currently, for each JavaFX module, the dependencies for 'win', 'mac' and 'linux' are individually added. To address this issue, it would be more efficient to use a nested for loop to handle these imports. By implementing a nested for loop, we can reduce code duplication and improve the ease of modifying modules and platforms. Simplifying the import process for JavaFX dependencies will enhance overall code readability and maintainability. Therefore, let's reduce code duplication by implementing a nested for loop to handle the import of JavaFX dependencies in build.gradle.
234dc4e
to
fb1234c
Compare
v1@Eclipse-Dominator submitted v1 for review. Checkout this PR version locallygit fetch https://github.com/se-edu/addressbook-level3.git refs/pr/195/1/head:BRANCHNAME where |
Do I need to create an issue for this PR? |
@Eclipse-Dominator Not strictly necessary. |
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.
@Eclipse-Dominator good job spotting this opportunity to reduce duplication.
That said, I am a bit hesitant about this change. Yes, it reduces duplication but Gradle is already an additional syntax students need to learn -- we should keep it as simple as possible. Perhaps the simplicity is worth the cost of duplication?
As discussed, let's not do this for now, in the interest of keeping the build.gradle file structure simple. We can reopen this if we decide to go ahead with this in the future. |
Currently, dependencies import for JavaFX is as below:
Importing dependencies for JavaFX in build.gradle is highly repetitive. This repetition makes it difficult to read, modify, and edit modules if necessary. Currently, for each JavaFX module, the dependencies for 'win', 'mac' and 'linux' are individually added. To address this issue, it would be more efficient to use a nested for loop to handle these imports.
By implementing a nested for loop, we can reduce code duplication and improve the ease of modifying modules and platforms. Simplifying the import process for JavaFX dependencies will enhance overall code readability and maintainability.
Therefore, let's reduce code duplication by implementing a nested for loop to handle the import of JavaFX dependencies in build.gradle.