Refactored code to resolve implementation as well as design smells and increase maintainability. #233

Drashti2608 · 2024-03-29T18:00:49Z

I introduced below given test cases utilizing Mockito to enhance the testing coverage of the project.

testGenerateNewKey(): To test GenerateNewKey() method of UtilsRandom class.
indexOf_singleOccurrence(): To test indexOf() method of UtilsString class.
testExpectInput(): To test expectInput() method of UtilsLogger class.

Furthermore, I have also attempted to remove some of the implementation code smells by performing below given methods.

Performed 'Extract method' on installMod() method to reduce the Cyclomatic complexity of the method..
Performed 'Decompose conditional' to reduce the complexity of conditional statement in preVisitDirectory() method of FileManager class.
Performed 'Extract method' on SyncFilesManager() method of SyncFileManager class to reduce the Cyclomatic complexity of the method.
Renamed variables 'mods_update_check_name_for_mod_loader' and 'plugins_updater_web_database_min_usages' to remove the code smell of 'Long Identifier'.
I have used 'Designite' tool to detect the smells present in the code.

In further refactoring I have performed below given methods to resolve the design smells present in the code:

Extract class:

FileManager.java class is handling multiple responsibilities. I've refactored the code by extracting related methods into a new class, FileSearcher.java. This enhances modularity and maintainability by focusing each class on specific responsibilities.
ConFileManager.java class has multiple responsibilities, which makes it less cohesive. Refactored the code by creating a new class, FileSystemUtils.java, and extracting the related method to that class.

Replace conditional using polymorphism:

In the MainWindow class, Made changes to improve how we detect the type of theme. Instead of using many if-statements, I introduced ThemeSetup interface. All the theme types implement this interface. This change makes the code easier to manage and ready for adding new types of themes in the future. Hence, MainWindow class also adheres to Open Closed principle.

Move method:

From TaskServerUpdater class, moved a couple of methods to UpdaterConfig class to remove the design smell of Feature envy. These methods belonged more to UpdaterConfig class than TaskServerUpdater class.

Osiris-Team

Thanks a lot for your PR!

If you are interested in larger PRs check out these issues:

Osiris-Team · 2024-03-30T11:26:44Z

src/main/java/com/osiris/autoplug/client/configs/UpdaterConfig.java

These names are long yes, but because they follow a standard like all the other fields in this class, which is to represent the yaml keys. Please revert these.

Osiris-Team · 2024-03-30T11:53:56Z

src/main/java/com/osiris/autoplug/client/console/Commands.java

are all those extra imports are needed?

Osiris-Team · 2024-03-30T11:55:59Z

src/test/java/com/osiris/autoplug/client/ui/utils/HintTextFieldTest.java

+    }
+
+    @Test
+    public void testTextIsClearedOnFocus() {


this isn't an existing feature, this test isnt needed

Osiris-Team · 2024-03-30T11:56:31Z

pom.xml

see HintTextFieldTest.java

Osiris-Team · 2024-03-30T12:00:23Z

src/main/java/com/osiris/autoplug/client/managers/SyncFilesManager.java

        List<File> filesToSendTo = new ArrayList<>();
-        //List<String> ipsToSendTo = new ArrayList<>();


keep these comments since they are relevant for future feature implementations

Thank you so much for reviewing my pull request!
I will look into the code again considering the changes you suggested!

Drashti2608 · 2024-03-31T18:18:32Z

I have made the suggested changes to the code and have committed them. Could you please review them?
I have also performed Move method, Extract class method and Replace conditional with polymorphism method to enhance the code readability and maintainability. Please have a look at them as well.

Osiris-Team

Sorry, I somehow forgot to review this.

Osiris-Team · 2024-04-22T21:51:42Z

src/main/java/com/osiris/autoplug/client/configs/UpdaterConfig.java

revert this.
only functions strictly related to the config should be allowed here.

Osiris-Team · 2024-04-22T21:58:20Z

src/main/java/com/osiris/autoplug/client/console/Commands.java

+    private static MinecraftMod initializeMinecraftMod(File modsDir, String tempName, String input) {
+        MinecraftMod mod = new MinecraftMod(new File(modsDir + "/" + tempName).getAbsolutePath(), tempName, "0", "",
+                "0", "0", "");
+        String repo;


must be initialized: String repo = "modrinth";

Osiris-Team · 2024-04-22T22:00:00Z

src/main/java/com/osiris/autoplug/client/console/Commands.java


-            try { // SPIGOT


keep these comments

Osiris-Team · 2024-04-22T22:03:34Z

src/main/java/com/osiris/autoplug/client/managers/FileManager.java

the FileManager is basically a FileSearcher, no need to split that up, or if you really want to split it up, make each method into its own class, because yes they are pretty long.

Osiris-Team · 2024-04-22T22:05:48Z

src/main/java/com/osiris/autoplug/client/managers/SyncFilesManager.java

these changes I really like, good job

Osiris-Team · 2024-04-22T22:07:14Z

src/main/java/com/osiris/autoplug/client/managers/SyncFilesManager.java


-        Consumer<FileEvent> onFileChangeEvent = event -> {
+    private Consumer<FileEvent> defineFileChangeEventHandler(List<File> filesToSendTo) {


shorter: createFileChangeListener

Osiris-Team · 2024-04-22T22:08:22Z

src/main/java/com/osiris/autoplug/client/network/online/connections/ConFileManager.java

@@ -64,7 +64,7 @@ public boolean open() throws Exception {
                        } else if (requestType == 6) {
                            doProtocolForCopyOrCutFiles();
                        } else if (requestType == 7) {
-                            doProtocolForSendingRoots();
+                            FileSystemUtils.sendRoots(dos);


previous way was fine, no need to split into another class, since this is more related to connection stuff, than filesystem stuff

Osiris-Team · 2024-04-22T22:12:31Z

src/main/java/com/osiris/autoplug/client/ui/MainWindow.java

-            } else {
-                AL.warn("The selected theme '" + generalConfig.autoplug_system_tray_theme.asString() + "' is not a valid option! Using default.");
-                if (!FlatLightLaf.setup()) throw new Exception("Returned false!");
+            ThemeSetup themeSetup = getThemeSetup(generalConfig.autoplug_system_tray_theme.asString());


yes its nicer, but if you compare the additional amount of code needed before this, I don't think its better.
revert.

Osiris-Team · 2024-06-23T18:54:13Z

@Drashti2608 any news?

Drashti Patel added 2 commits March 1, 2024 23:05

Assignment 1 Submission

e506ab1

Refactored code to resolve implementation code smells

441b836

Osiris-Team requested changes Mar 30, 2024

View reviewed changes

Drashti Patel added 2 commits March 30, 2024 23:39

Updated Pull request with suggested changes

abcd281

Refactored code to increase maintainability

7db4d5e

Drashti2608 changed the title ~~Added Test cases to improve branch coverage and Refactored code to resolve implementation code smells.~~ 2)Refactored code to resolve implementation code smells and increase maintainability. Mar 31, 2024

Drashti2608 changed the title ~~2)Refactored code to resolve implementation code smells and increase maintainability.~~ Refactored code to resolve implementation code smells and increase maintainability. Mar 31, 2024

Drashti2608 changed the title ~~Refactored code to resolve implementation code smells and increase maintainability.~~ Refactored code to resolve implementation as well as design smells and increase maintainability. Mar 31, 2024

Drashti2608 requested a review from Osiris-Team March 31, 2024 13:39

Osiris-Team requested changes Apr 22, 2024

View reviewed changes

Osiris-Team closed this Jul 7, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactored code to resolve implementation as well as design smells and increase maintainability. #233

Refactored code to resolve implementation as well as design smells and increase maintainability. #233

Drashti2608 commented Mar 29, 2024 •

edited

Loading

Osiris-Team left a comment

Osiris-Team Mar 30, 2024

Osiris-Team Mar 30, 2024

Osiris-Team Mar 30, 2024

Osiris-Team Mar 30, 2024

Osiris-Team Mar 30, 2024

Drashti2608 Mar 31, 2024

Drashti2608 commented Mar 31, 2024

Osiris-Team left a comment

Osiris-Team Apr 22, 2024

Osiris-Team Apr 22, 2024

Osiris-Team Apr 22, 2024

Osiris-Team Apr 22, 2024

Osiris-Team Apr 22, 2024

Osiris-Team Apr 22, 2024

Osiris-Team Apr 22, 2024

Osiris-Team Apr 22, 2024

Osiris-Team commented Jun 23, 2024

		List<File> filesToSendTo = new ArrayList<>();
		//List<String> ipsToSendTo = new ArrayList<>();


		Consumer<FileEvent> onFileChangeEvent = event -> {
		private Consumer<FileEvent> defineFileChangeEventHandler(List<File> filesToSendTo) {

Refactored code to resolve implementation as well as design smells and increase maintainability. #233

Refactored code to resolve implementation as well as design smells and increase maintainability. #233

Conversation

Drashti2608 commented Mar 29, 2024 • edited Loading

Osiris-Team left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Drashti2608 commented Mar 31, 2024

Osiris-Team left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Osiris-Team commented Jun 23, 2024

Drashti2608 commented Mar 29, 2024 •

edited

Loading