-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Refactoring: Enhancing code maintainability by reducing code smells. #1336
base: dev
Are you sure you want to change the base?
Changes from 21 commits
d2ffe8b
ac32664
47be445
b0d3989
870d1e5
6b50d68
d615fbf
ed75ff9
0e96778
e344f9c
6406b1f
11b5569
e80a519
3e8d8c7
aafebe6
4344b84
b9c94fd
3d3099b
848266c
240fb70
2b9aea9
5fba3b2
e70155a
1b6598a
423ceca
3729e7b
fb128ba
f4cef77
3c98bfa
5b97c30
6451919
1278586
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. when did array index become a magic number? |
||
|
||
private MessageContext(Update update, User user, Long chatId, BaseAbilityBot bot, String[] arguments) { | ||
this.user = user; | ||
this.chatId = chatId; | ||
|
@@ -88,7 +91,9 @@ public String secondArg() { | |
*/ | ||
public String thirdArg() { | ||
checkLength(); | ||
return arguments[2 % arguments.length]; | ||
|
||
// Replaced magic number 2 with thirdArg | ||
return arguments[thirdArg % arguments.length]; | ||
} | ||
|
||
/** | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -27,6 +27,10 @@ public class DefaultSender implements MessageSender { | |
|
||
private DefaultAbsSender bot; | ||
|
||
/* | ||
* Refactored code - Insufficient Modularization design smell refactored by moving the methods related to send functionality | ||
* and extracting class DefaultSender1. | ||
*/ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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? |
||
public DefaultSender(DefaultAbsSender bot) { | ||
this.bot = bot; | ||
} | ||
|
@@ -106,34 +110,4 @@ public void getWebhookInfoAsync(SentCallback<WebhookInfo> sentCallback) throws T | |
public Message sendDocument(SendDocument sendDocument) throws TelegramApiException { | ||
return bot.execute(sendDocument); | ||
} | ||
|
||
@Override | ||
public Message sendPhoto(SendPhoto sendPhoto) throws TelegramApiException { | ||
return bot.execute(sendPhoto); | ||
} | ||
|
||
@Override | ||
public Message sendVideo(SendVideo sendVideo) throws TelegramApiException { | ||
return bot.execute(sendVideo); | ||
} | ||
|
||
@Override | ||
public Message sendSticker(SendSticker sendSticker) throws TelegramApiException { | ||
return bot.execute(sendSticker); | ||
} | ||
|
||
@Override | ||
public Message sendAudio(SendAudio sendAudio) throws TelegramApiException { | ||
return bot.execute(sendAudio); | ||
} | ||
|
||
@Override | ||
public Message sendVoice(SendVoice sendVoice) throws TelegramApiException { | ||
return bot.execute(sendVoice); | ||
} | ||
|
||
@Override | ||
public Message sendVideoNote(SendVideoNote sendVideoNote) { | ||
return null; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,53 @@ | ||
package org.telegram.abilitybots.api.sender; | ||
import org.telegram.telegrambots.meta.api.methods.BotApiMethod; | ||
import org.telegram.telegrambots.meta.api.methods.send.*; | ||
import org.telegram.telegrambots.meta.api.objects.Message; | ||
import org.telegram.telegrambots.bots.DefaultAbsSender; | ||
import org.telegram.telegrambots.meta.exceptions.TelegramApiException; | ||
import org.telegram.telegrambots.meta.updateshandlers.SentCallback; | ||
|
||
import java.io.Serializable; | ||
public class DefaultSender1 implements MessageSender1{ | ||
private static final String TAG = MessageSender.class.getName(); | ||
|
||
private DefaultAbsSender bot; | ||
|
||
@Override | ||
public <T extends Serializable, Method extends BotApiMethod<T>, Callback extends SentCallback<T>> void executeAsync(Method method, Callback callback) throws TelegramApiException { | ||
|
||
} | ||
|
||
@Override | ||
public <T extends Serializable, Method extends BotApiMethod<T>> T execute(Method method) throws TelegramApiException { | ||
return null; | ||
} | ||
|
||
@Override | ||
public Message sendPhoto(SendPhoto sendPhoto) throws TelegramApiException { | ||
return bot.execute(sendPhoto); | ||
} | ||
|
||
@Override | ||
public Message sendVideo(SendVideo sendVideo) throws TelegramApiException { | ||
return bot.execute(sendVideo); | ||
} | ||
|
||
@Override | ||
public Message sendSticker(SendSticker sendSticker) throws TelegramApiException { | ||
return bot.execute(sendSticker); | ||
} | ||
|
||
@Override | ||
public Message sendAudio(SendAudio sendAudio) throws TelegramApiException { | ||
return bot.execute(sendAudio); | ||
} | ||
|
||
@Override | ||
public Message sendVoice(SendVoice sendVoice) throws TelegramApiException { | ||
return bot.execute(sendVoice); | ||
} | ||
@Override | ||
public Message sendVideoNote(SendVideoNote sendVideoNote) { | ||
return null; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,39 @@ | ||
package org.telegram.abilitybots.api.sender; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. See my comments about DefaultSender1 |
||
|
||
|
||
import org.telegram.telegrambots.meta.api.methods.BotApiMethod; | ||
import org.telegram.telegrambots.meta.api.methods.send.*; | ||
import org.telegram.telegrambots.meta.api.objects.Message; | ||
import org.telegram.telegrambots.bots.DefaultAbsSender; | ||
import org.telegram.telegrambots.meta.exceptions.TelegramApiException; | ||
import org.telegram.telegrambots.meta.updateshandlers.SentCallback; | ||
|
||
import java.io.Serializable; | ||
|
||
/** | ||
* A sender interface that replicates {@link DefaultAbsSender} methods. | ||
* | ||
* @author Abbas Abou Daya | ||
*/ | ||
public interface MessageSender1 { | ||
/* | ||
* Refactored code - Interfaces related to send functionality. | ||
*/ | ||
<T extends Serializable, Method extends BotApiMethod<T>, Callback extends SentCallback<T>> void executeAsync(Method method, Callback callback) throws TelegramApiException; | ||
|
||
<T extends Serializable, Method extends BotApiMethod<T>> T execute(Method method) throws TelegramApiException; | ||
|
||
|
||
Message sendPhoto(SendPhoto sendPhoto) throws TelegramApiException; | ||
|
||
Message sendVideo(SendVideo sendVideo) throws TelegramApiException; | ||
|
||
Message sendAudio(SendAudio sendAudio) throws TelegramApiException; | ||
|
||
Message sendVoice(SendVoice sendVoice) throws TelegramApiException; | ||
|
||
Message sendVideoNote(SendVideoNote sendVideoNote) throws TelegramApiException; | ||
|
||
Message sendSticker(SendSticker sendSticker) throws TelegramApiException; | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -302,7 +302,9 @@ public static boolean isValidCommand(String command){ | |
* @param commandName the command name to be checked for validity | ||
* @return whether the command name is valid | ||
*/ | ||
public static boolean isValidCommandName(String commandName){ | ||
|
||
// Refactored code by renaming isValueCommandName to isValCmdName method | ||
public static boolean isValCmdName(String commandName){ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. in java, it's inconvenient to use abbreviations like here. No need for renaming |
||
if (commandName == null || commandName.length() > 31) return false; | ||
return commandName.matches("[A-Za-z_0-9]+"); | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -10,9 +10,11 @@ | |
import static org.telegram.abilitybots.api.bot.DefaultBot.getDefaultBuilder; | ||
|
||
class AbilityTest { | ||
// Refactored magic number -4 using extract method technique | ||
final int inputNegativeNumber = -4; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. When did array index become a magic number? |
||
@Test | ||
void argumentsCannotBeNegative() { | ||
Assertions.assertThrows(IllegalArgumentException.class, () -> getDefaultBuilder().input(-4).build()); | ||
Assertions.assertThrows(IllegalArgumentException.class, () -> getDefaultBuilder().input(inputNegativeNumber).build()); | ||
} | ||
|
||
@Test | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,44 @@ | ||
package org.telegram.telegrambots.extensions.bots.commandbot.commands; | ||
import org.junit.jupiter.api.Test; | ||
import org.telegram.telegrambots.meta.api.objects.Chat; | ||
import org.telegram.telegrambots.meta.api.objects.User; | ||
import org.telegram.telegrambots.meta.bots.AbsSender; | ||
import static org.junit.jupiter.api.Assertions.assertEquals; | ||
|
||
|
||
public class BotCommandTest | ||
{ | ||
|
||
@Test | ||
public void testToString() | ||
{ | ||
// Act | ||
BotCommand commandidentifier = new TestBotCommand("test","Description"); | ||
|
||
// Assert | ||
assertEquals("<b>/test</b>\nDescription",commandidentifier.toString()); | ||
} | ||
|
||
@Test | ||
public void testcommandIdentifierStartsWithCOMMAND_INIT_CHARACTER() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
{ | ||
// Arrange | ||
String commandidentifier = "/YuktaGurnani"; | ||
|
||
// Act | ||
String result = commandidentifier.substring(1); | ||
|
||
// Assert | ||
assertEquals("YuktaGurnani",result); | ||
} | ||
private static class TestBotCommand extends BotCommand | ||
{ | ||
public TestBotCommand(String commandIdentifier, String description){ | ||
super(commandIdentifier,description); | ||
} | ||
public void execute(AbsSender absSender, User user, Chat chat, String[] arguments) | ||
{ | ||
|
||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,42 @@ | ||
package org.telegram.telegrambots.extensions.bots.commandbot.commands; | ||
|
||
import org.junit.jupiter.api.Test; | ||
import org.telegram.telegrambots.extensions.bots.timedbot.TimedSendLongPollingBot; | ||
import org.telegram.telegrambots.meta.api.objects.Update; | ||
import static org.mockito.Mockito.mock; | ||
|
||
|
||
public class TimedSendLongPollingBotTest | ||
{ | ||
@Test | ||
public void testMessageProcessing() | ||
{ | ||
// Arrange | ||
TimedSendLongPollingBot testbot = new TimedSendLongPollingBot() | ||
{ | ||
@Override | ||
public String getBotUsername() { | ||
return null; | ||
} | ||
|
||
@Override | ||
public void onUpdateReceived(Update update) | ||
{ | ||
|
||
} | ||
@Override | ||
public void sendMessageCallback(Long chatId, Object messageRequest) | ||
{ | ||
|
||
} | ||
}; | ||
|
||
// Act | ||
Long chatId = 002L; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why not to use 2L or just 2? |
||
Object messageRequest = mock(Object.class); | ||
|
||
// Assert | ||
testbot.sendTimed(chatId,messageRequest); | ||
testbot.finish(); | ||
} | ||
} |
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.
in java, it's inconvenient to use abbreviations like here. use isValidCommandName