Refactoring: Enhancing code maintainability by reducing code smells. #1336

Yukta026 · 2024-03-23T12:33:53Z

By this pull request, I am reaching out to submit my refactoring improvements for your consideration.
These changes aim to improve readability, maintainability, and performance while aligning with best practices and the project's objectives.

A semicolon were missing in the Standalone reply code. The reply signature changed and needs a BiConsumer<BaseAbilityBot, Update> instead of Consumer<Update> in the Ability reply

Add getMessageId to MaybeInaccessibleMessage According to https://core.telegram.org/bots/api#maybeinaccessiblemessage

…ed from 'Boolean' to 'String' (according to TelegramBotAPI Official Documentation). (rubenlagus#1315)

* fix force=true for WriteAccessAllowed * + test and fixtures on write access allowed

* Fix path for GetMyName method * Add missing sticker methods from API 6.6 * Fix possible NPE in InputSticker validation * Add ReplyParameters to SendInvoice API method * Removed mysterious fields in SetChatTitle

…g explaining variables technique

…ing decomposing conditional technique

…ing renaming method and introducing explaining variable technique

… using explaining variables technique

…clause using explaining variables technique

…ier using renaming variables technique

…on using extract class technique

…sing moving pull-up method technique

…larization using change bidirectional association to unidirectional association technique

Chase22 · 2024-03-25T11:04:13Z

Something is not right with the git commits in this PR. It's trying to merge a lot more commits than it should. Please make sure you branch is up to date and based on the devbranch.

I'll be looking through these but please tell your professor that open-source developers aren't outsourced teachers and that we have to invest quite significant time into the reviews.

Yukta026 · 2024-03-26T09:04:14Z

Thank you for taking the time to review my pull request and providing feedback. To address the issue regarding the git commits, I've conducted a thorough analysis of the changes made. In total, there are 11 commits originating from my assignment2 branch, affecting a total of 21 files.

Furthermore, I've ensured that my branch is up to date and based on the dev branch by incorporating the latest changes from the dev branch into my own. This includes resolving any merge conflicts that arose, such as the one encountered in the Story.java file.

I greatly appreciate the time and effort you've dedicated to reviewing my contributions to this repository. My ultimate goal is to make meaningful contributions, and I'm committed to ensuring the quality and integrity of my work. Please feel free to take as much time as needed for further review.

Senderman

I want to help the developer of this library so I did my own code review. See the github code comments.
Also, why do you leave explanations to your refactor in the code comments? Keep in mind that you request to merge your code (including comments) into the upstream repository. Which means people who would like to inspect the code in the future and create their own pull requests, will have to read through your changelog in the code, when there's VCS (git) for this

Senderman · 2024-03-26T22:51:37Z

telegrambots-abilities/src/main/java/org/telegram/abilitybots/api/objects/Ability.java

+    final String METHOD_NAME_CONSTRAINT = "cannot be longer than 31 characters, and cannot be null";
+
+    // isValueCommandName renamed to isValCmdName
+    checkArgument(isValCmdName(name), INVALID_METHOD_NAME_ERROR+METHOD_NAME_CONSTRAINT, name);


in java, it's inconvenient to use abbreviations like here. use isValidCommandName

Senderman · 2024-03-26T22:53:13Z

telegrambots-abilities/src/main/java/org/telegram/abilitybots/api/objects/MessageContext.java

@@ -22,6 +22,9 @@ public class MessageContext {
  private final Update update;
  private final BaseAbilityBot bot;

+  // Refactored code-Added thirdArg as explaining variable to remove magic number 2
+  int thirdArg = 2;


when did array index become a magic number?

Senderman · 2024-03-26T22:55:29Z

telegrambots-abilities/src/main/java/org/telegram/abilitybots/api/sender/DefaultSender.java

+  /*
+  *  Refactored code - Insufficient Modularization design smell refactored by moving the methods related to send functionality
+  * and extracting class DefaultSender1.
+   */


This makes more "code smell" by creating non-obvious DefaultSender1 class (how end-users should understand the difference?) why methods related to sending are moved from DefaultSender class?

Senderman · 2024-03-26T22:55:59Z

telegrambots-abilities/src/main/java/org/telegram/abilitybots/api/sender/MessageSender1.java

@@ -0,0 +1,39 @@
+package org.telegram.abilitybots.api.sender;


See my comments about DefaultSender1

Senderman · 2024-03-26T22:56:44Z

telegrambots-abilities/src/main/java/org/telegram/abilitybots/api/util/AbilityUtils.java

-  public static boolean isValidCommandName(String commandName){
+
+  // Refactored code by renaming isValueCommandName to isValCmdName method
+  public static boolean isValCmdName(String commandName){


in java, it's inconvenient to use abbreviations like here. No need for renaming

Senderman · 2024-03-26T22:58:29Z

.../test/java/org/telegram/telegrambots/extensions/bots/commandbot/commands/BotCommandTest.java

+    }
+
+    @Test
+    public void testcommandIdentifierStartsWithCOMMAND_INIT_CHARACTER()


Only String objects and its methods are involved in this test. What are you trying to test here is core java's String.substring method, but it's not related to the library code, because this library doesn't hack into JVM

Senderman · 2024-03-26T22:59:01Z

...g/telegram/telegrambots/extensions/bots/commandbot/commands/TimedSendLongPollingBotTest.java

+        };
+
+        // Act
+        Long chatId = 002L;


Why not to use 2L or just 2?

Senderman · 2024-03-26T23:00:22Z

...eta/src/main/java/org/telegram/telegrambots/meta/api/methods/invoices/CreateInvoiceLink.java

    @Override
    public String getMethod() {
        return PATH;
    }

-    @Override
-    public void validate() throws TelegramApiValidationException {
+    // Old code - validate() method was containing complex method and complex conditional code smells


Why do you store old code in comments when using VCS (git)?

Senderman · 2024-03-26T23:02:12Z

telegrambots-meta/src/main/java/org/telegram/telegrambots/meta/api/objects/Chat.java

@@ -30,7 +30,8 @@
 @AllArgsConstructor
 public class Chat implements BotApiObject {

-    private static final String ID_FIELD = "id";
+    // Variable name changed from CHAT_FIELD to CHAT_ID_FIELD for refactoring
+    private static final String CHAT_ID_FIELD = "id";


In Telegram api, the Chat object has the "id" field, not "chat_id". It would be convenient to leave it as is

Senderman · 2024-03-26T23:03:19Z

telegrambots-meta/src/main/java/org/telegram/telegrambots/meta/api/objects/stories/Story.java

+    * Refactored code - Using CHAT_ID_FIELD instead of CHAT_FIELD to change bidirectional association to unidirectional association
+     */
+
+    private static final String CHAT_ID_FIELD = "chat_id";


This breaks the Story feature of the library since the Story object in Telegram Api has the Chat object, but not chat_id. See https://core.telegram.org/bots/api#story
You will fail to deserialize any Story object

…smell -2

… using replace conditional with polymorphism technique

Yukta026 · 2024-03-29T18:33:44Z

I want to help the developer of this library so I did my own code review. See the github code comments. Also, why do you leave explanations to your refactor in the code comments? Keep in mind that you request to merge your code (including comments) into the upstream repository. Which means people who would like to inspect the code in the future and create their own pull requests, will have to read through your changelog in the code, when there's VCS (git) for this

Thank you for your insightful feedback on the code review. I have thoroughly reviewed all the suggested changes and made every effort to incorporate them. If you believe any additional modifications are necessary, please don't hesitate to inform me.

aNNiMON and others added 20 commits January 22, 2024 20:10

Add default constructor to WriteAccessAllowed (rubenlagus#1273)

d2ffe8b

Update AllowedUpdates (rubenlagus#1276)

ac32664

fix ReactionType validations (rubenlagus#1303)

47be445

Update Using-Replies.md (rubenlagus#1282)

b0d3989

A semicolon were missing in the Standalone reply code. The reply signature changed and needs a BiConsumer<BaseAbilityBot, Update> instead of Consumer<Update> in the Ability reply

Update MaybeInaccessibleMessage.java (rubenlagus#1310)

870d1e5

Add getMessageId to MaybeInaccessibleMessage According to https://core.telegram.org/bots/api#maybeinaccessiblemessage

Fixed incorrect Chat#profileBackgroundCustomEmojiId field type. Chang…

6b50d68

…ed from 'Boolean' to 'String' (according to TelegramBotAPI Official Documentation). (rubenlagus#1315)

Missing fields and constructor update for API 7.0 (rubenlagus#1322)

d615fbf

* fix force=true for WriteAccessAllowed * + test and fixtures on write access allowed

Add missing methods from API 6.6 (rubenlagus#1323)

ed75ff9

* Fix path for GetMyName method * Add missing sticker methods from API 6.6 * Fix possible NPE in InputSticker validation * Add ReplyParameters to SendInvoice API method * Removed mysterious fields in SetChatTitle

API 7.1

0e96778

API 7.1

e344f9c

Test cases for ASDC assignment-1

6406b1f

Implementation code smell - 1 - Refactored code for Magic number usin…

11b5569

…g explaining variables technique

Implementation code smell - 2 - Refactored code for Complex Method us…

e80a519

…ing decomposing conditional technique

Implementation code smell - 3 - Refactored code for Long Statement us…

3e8d8c7

…ing renaming method and introducing explaining variable technique

Implementation code smell - 4 - Refactored test code for Magic number…

aafebe6

… using explaining variables technique

Implementation code smell - 5 - Refactored test code for Empty Catch …

4344b84

…clause using explaining variables technique

Implementation code smell - 6 - Refactored test code for Long Identif…

b9c94fd

…ier using renaming variables technique

Design code smell - 1 - Refactored code for Insufficient Modularizati…

3d3099b

…on using extract class technique

Design code smell - 2 - Refactored code for Unnecessary Abstraction u…

848266c

…sing moving pull-up method technique

Design code smell - 3 - Refactored code for Cyclically-dependent Modu…

240fb70

…larization using change bidirectional association to unidirectional association technique

Yukta026 changed the title ~~Refactoring: Enhancing code maintainability by reducing implementation and design smells(as part of Course Assignment).~~ Refactoring: Enhancing code maintainability by reducing code smells. Mar 26, 2024

Fix: merge conflict for story file fixed

2b9aea9

Senderman reviewed Mar 26, 2024

View reviewed changes

Yukta026 added 5 commits March 29, 2024 14:13

Removed/renamed files for design smell-1

5fba3b2

Reset code changed in implementation smell-1

e70155a

Removed unnecessary comments from refactored code for implementation …

1b6598a

…smell -2

Made necessary changes for refactoring implementation smell-3

423ceca

Reset code changed for refactoring implementation smell-4

3729e7b

Yukta026 added 6 commits March 29, 2024 14:26

Removed unncessary comments

fb128ba

Removed unnecessary comments

f4cef77

Removed unnecessary comments

3c98bfa

Removed unnecessary comments

5b97c30

Code reset for story and chat files

6451919

Design code smell - 4 - Refactored code for unexploited encapsulation…

1278586

… using replace conditional with polymorphism technique

rubenlagus force-pushed the dev branch 2 times, most recently from c4394aa to 7df1524 Compare April 1, 2024 19:59

rubenlagus force-pushed the dev branch from 1977b5c to 19372e2 Compare April 13, 2024 10:43

rubenlagus force-pushed the dev branch 4 times, most recently from bc4fd52 to 65c33e0 Compare June 16, 2024 01:14

rubenlagus force-pushed the dev branch from 232c62b to 1337142 Compare June 22, 2024 11:28

rubenlagus force-pushed the dev branch from 1337142 to 4d8c975 Compare July 6, 2024 11:00

rubenlagus force-pushed the dev branch from 0b6d307 to 665cd14 Compare August 4, 2024 14:45

rubenlagus force-pushed the dev branch 2 times, most recently from 68a6f61 to 5fc7d88 Compare August 18, 2024 22:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactoring: Enhancing code maintainability by reducing code smells. #1336

Refactoring: Enhancing code maintainability by reducing code smells. #1336

Yukta026 commented Mar 23, 2024 •

edited

Loading

Chase22 commented Mar 25, 2024

Yukta026 commented Mar 26, 2024

Senderman left a comment

Senderman Mar 26, 2024

Senderman Mar 26, 2024

Senderman Mar 26, 2024

Senderman Mar 26, 2024

Senderman Mar 26, 2024

Senderman Mar 26, 2024 •

edited

Loading

Senderman Mar 26, 2024

Senderman Mar 26, 2024

Senderman Mar 26, 2024

Senderman Mar 26, 2024

Yukta026 commented Mar 29, 2024

		@@ -0,0 +1,39 @@
		package org.telegram.abilitybots.api.sender;

Refactoring: Enhancing code maintainability by reducing code smells. #1336

Are you sure you want to change the base?

Refactoring: Enhancing code maintainability by reducing code smells. #1336

Conversation

Yukta026 commented Mar 23, 2024 • edited Loading

Chase22 commented Mar 25, 2024

Yukta026 commented Mar 26, 2024

Senderman 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

Senderman Mar 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Yukta026 commented Mar 29, 2024

Yukta026 commented Mar 23, 2024 •

edited

Loading

Senderman Mar 26, 2024 •

edited

Loading