Sharing iP code quality feedback [for @Eilyss] - Round 3 #4
Description
@Eilyss We did an automated analysis of your code to detect potential areas to improve the code quality. We are sharing the results below, so that you can avoid similar problems in your tP code (which will be graded more strictly for code quality).
IMPORTANT: Note that the script looked for just a few easy-to-detect problems only, and at-most three example are given i.e., there can be other areas/places to improve.
Aspect: Tab Usage
No easy-to-detect issues 👍
Aspect: Naming boolean variables/methods
Example from src/main/java/com/neokortex/Chatbot.java lines 38-38:
private boolean exitProgram = false;Example from src/main/java/com/neokortex/ui/cli/Cli.java lines 42-42:
boolean lineEmpty = true;Example from src/main/java/com/neokortex/core/Storage.java lines 49-49:
private boolean pathSet;Suggestion: Follow the given naming convention for boolean variables/methods (e.g., use a boolean-sounding prefix).You may ignore the above if you think the name already follows the convention (the script can report false positives in some cases)
Aspect: Brace Style
No easy-to-detect issues 👍
Aspect: Package Name Style
No easy-to-detect issues 👍
Aspect: Class Name Style
No easy-to-detect issues 👍
Aspect: Dead Code
Example from src/test/java/com/neokortex/time/dateparser/CompositeDateParserTest.java lines 1-1:
//package com.elsria.time.dateparser;Example from src/test/java/com/neokortex/time/dateparser/CompositeDateParserTest.java lines 3-3:
//import org.junit.jupiter.api.Test;Example from src/test/java/com/neokortex/time/dateparser/CompositeDateParserTest.java lines 5-5:
//import java.time.LocalDate;Suggestion: Remove dead code from the codebase.
Aspect: Method Length
Example from src/main/java/com/neokortex/DialogueMap.java lines 41-133:
public DialogueMap() {
this.dialogueMap = new HashMap<>();
this.dialogueMap.put(DialoguePath.STARTUP,
"Welcome to the NeoKortex! "
+ "I'm {name}, and I am your host in this space. "
+ "So, what do you wanna do today? "
);
this.dialogueMap.put(DialoguePath.TOO_MANY_ARGUMENTS, "Woah, that's too many arguments for this command.");
this.dialogueMap.put(DialoguePath.INVALID_ARGUMENTS, "That's not how you use this command.");
this.dialogueMap.put(DialoguePath.GREET,
"Hi! I'm {name}, your trusty mechanical companion." + SEP
+ "I'm here to walk you through the ins and outs of the NeoKortex."
+ "Let me know if you need anything! I'm always happy to help."
);
this.dialogueMap.put(DialoguePath.STARTUP_FAILURE,
"Welcome to the NeoKortex! "
+ "I'm {name}, and I am your host in this space. "
+ "Hmm... there seems to be an error with loading your data. "
+ "Could you please check it out for me? "
+ "In any case, welcome!"
);
this.dialogueMap.put(DialoguePath.GENERIC_FAILURE, "Sorry wut");
this.dialogueMap.put(DialoguePath.INVALID_TIME_SPECIFIED, "That's not a valid time :/");
this.dialogueMap.put(DialoguePath.NO_TASK_SPECIFIED, "Erm... so what task?");
this.dialogueMap.put(DialoguePath.NO_DEADLINE_SPECIFIED, "But when?");
this.dialogueMap.put(DialoguePath.NO_START_TIME_SPECIFIED, "But from when?");
this.dialogueMap.put(DialoguePath.NO_END_TIME_SPECIFIED, "But to when?");
this.dialogueMap.put(DialoguePath.TOO_MANY_TIMES_SPECIFIED, "Woah that's too many! Which one?");
this.dialogueMap.put(DialoguePath.NO_TASK_ID_SPECIFIED, "Wait which task?");
this.dialogueMap.put(DialoguePath.SUCCESSFULLY_ADDED_TASK,
"Added:" + SEP
+ "{r:0}" + SEP);
this.dialogueMap.put(DialoguePath.ADD_TASK_STORAGE_FAILURE,
"Added:" + SEP
+ " {r:0}" + SEP
+ "to the list" + SEP
+ "Woah, hold on..." + SEP
+ "I seem to be unable to save your changes." + SEP
+ "Could you run the Save command?"
);
this.dialogueMap.put(DialoguePath.TASK_ID_OOB, "Hey! Task number {r:0} is out of bounds.");
this.dialogueMap.put(DialoguePath.SUCCESSFULLY_DELETED_TASK,
"Deleted:" + SEP
+ " {r:0}");
this.dialogueMap.put(DialoguePath.DELETE_TASK_STORAGE_FAILURE,
"Deleted:" + SEP
+ " {r:0}" + SEP
+ "from the list" + SEP
+ "Woah, hold on..." + SEP
+ "I seem to be unable to save your changes." + SEP
+ "Could you run the Save command?"
);
this.dialogueMap.put(DialoguePath.ECHO, "{r:0}");
this.dialogueMap.put(DialoguePath.FAREWELL, "Okey dokey, see you soon!");
this.dialogueMap.put(DialoguePath.FIND_RETURNS_EMPTY_LIST, "Hmm... there are no tasks that match your search");
this.dialogueMap.put(DialoguePath.FIND_SUCCESSFUL,
"Here are all the tasks that contain: \"{r:0}\"" + SEP
+ "Here you go!" + SEP
+ "{r:1}"
);
this.dialogueMap.put(DialoguePath.LIST_EMPTY, "Hmm... there's nothing in your list right now.");
this.dialogueMap.put(DialoguePath.LIST_NON_EMPTY,
"Here are the tasks on your list:" + SEP
+ "{r:0}"
);
this.dialogueMap.put(DialoguePath.TASK_ALREADY_MARKED, "Task {r:0} is already marked!");
this.dialogueMap.put(DialoguePath.SUCCESSFULLY_MARKED_TASK, "Okay! I have marked the task \"{r:0}\" as done.");
this.dialogueMap.put(DialoguePath.MARK_TASK_STORAGE_FAILURE,
"Okay! I have marked the task \"{r:0}\" as done." + SEP
+ "Woah, hold on..." + SEP
+ "I seem to be unable to save your changes." + SEP
+ "Could you run the Save command?"
);
this.dialogueMap.put(DialoguePath.TASK_NOT_MARKED, "Task {r:0} has not been marked yet!");
this.dialogueMap.put(DialoguePath.SUCCESSFULLY_UNMARKED_TASK,
"Okay! The task \"{r:0}\" is no longer marked as done."
);
this.dialogueMap.put(DialoguePath.UNMARK_TASK_STORAGE_FAILURE,
"Okay! I have marked the task \"{r:0}\" as done." + SEP
+ "Woah, hold on..." + SEP
+ "I seem to be unable to save your changes." + SEP
+ "Could you run the Save command?"
);
this.dialogueMap.put(DialoguePath.SUCCESSFULLY_SAVED_TASK,
"Okay! I have saved the current list into storage");
this.dialogueMap.put(DialoguePath.MARK_TASK_STORAGE_FAILURE,
"Hmm... I can't seem to save the the list to storage :/"
);
this.dialogueMap.put(DialoguePath.INVALID_PATH,
"That is not a valid path"
);
}Suggestion: Consider applying SLAP (and other abstraction mechanisms) to shorten methods e.g., extract some code blocks into separate methods. You may ignore this suggestion if you think a longer method is justified in a particular case.
Aspect: Class size
No easy-to-detect issues 👍
Aspect: Header Comments
Example from src/main/java/com/neokortex/Chatbot.java lines 72-77:
/**
* Unused at the moment as there is only 1 {@code DialogueMap}
*
* @param dialogueMap the dialogueMap associated with the Chatbot. Contains all possible
* responses to command execution responses as returned by {@link CommandHandler}
*/Example from src/main/java/com/neokortex/task/TaskType.java lines 66-74:
/**
* TODO: Scrap this
* Checks if the given character represents a valid task type.
*
* @param taskType the character to validate
* @return true if the character corresponds to a registered task type, false otherwise
*
* @see #getTaskType(char)
*/Suggestion: Ensure method/class header comments follow the format specified in the coding standard, in particular, the phrasing of the overview statement.
Aspect: Recent Git Commit Messages
No easy-to-detect issues 👍
Aspect: Binary files in repo
No easy-to-detect issues 👍
❗ You are not required to (but you are welcome to) fix the above problems in your iP, unless you have been separately asked to resubmit the iP due to code quality issues.
ℹ️ The bot account used to post this issue is un-manned. Do not reply to this post (as those replies will not be read). Instead, contact cs2103@comp.nus.edu.sg if you want to follow up on this post.