Sharing iP code quality feedback [for @RyanNgWH]
Opened this issue · 0 comments
soc-se-bot commented
@RyanNgWH We did an automated analysis of your code to detect potential areas to improve the code quality. We are sharing the results below, to help you improve the iP code further.
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/duke/commands/ArchiveCommand.java lines 23-23:
private boolean toArchive;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/duke/parser/ParserTest.java lines 324-324:
// assertNotEquals(expected, Parser.parse("todo Nights Into Days"));Example from src/test/java/duke/parser/ParserTest.java lines 367-367:
// 2024/01/29 12:00")));Example from src/test/java/duke/parser/ParserTest.java lines 446-446:
// 2024/01/29 12:00 /to 2024/01/29 14:00")));Suggestion: Remove dead code from the codebase.
Aspect: Method Length
Example from src/main/java/duke/parser/Parser.java lines 59-264:
public static Command parse(String input) throws DukeException {
// Remove whitespaces from input
input = input.strip();
// Split input
ArrayList<String> splitInput = new ArrayList<String>(Arrays.asList(input.split(" ")));
// Remove unncessary whitespace
splitInput.removeIf(item -> item == null || "".equals(item));
String description;
List<String> arguments;
switch (splitInput.get(0).toLowerCase()) {
case "bye": // Exit
return new ExitCommand();
case "list": // List tasks
// Check if date filter exists
if (splitInput.size() > 1) { // List filtered tasks
boolean isArchived = false;
if (splitInput.get(1).equals("/archive")) {
isArchived = true;
splitInput.remove(1);
}
if (splitInput.size() <= 1) {
return new ListCommand(isArchived);
}
switch (splitInput.get(1)) {
case "/date": // Filter by date
try {
Instant filterDate = userDateToInstant(splitInput.get(2), "00:00");
return new ListCommand(filterDate, isArchived);
} catch (NumberFormatException | DateTimeException | IndexOutOfBoundsException e) {
throw new InvalidArgumentException(
"Date/time format is invalid. Please enter the date/time in the format 'YYYY/MM/DD'");
}
default: // Invalid filter
throw new InvalidArgumentException(
String.format("Unknown argument '%s' for the 'list' command", splitInput.get(1)));
}
} else { // Return full list
return new ListCommand(false);
}
case "mark": // Mark task
if (splitInput.size() <= 1) {
throw new MissingArgumentException("Missing argument - Index of task required");
}
try {
return new MarkCommand(Integer.parseInt(splitInput.get(1)) - 1, true);
} catch (NumberFormatException e) {
throw new InvalidArgumentException("Index to mark is not an integer");
}
case "unmark": // Unmark task
if (splitInput.size() <= 1) {
throw new MissingArgumentException("Missing argument - Index of task required");
}
try {
return new MarkCommand(Integer.parseInt(splitInput.get(1)) - 1, false);
} catch (NumberFormatException e) {
throw new InvalidArgumentException("Index to unmark is not an integer");
}
case "delete": // Delete task
if (splitInput.size() <= 1) {
throw new MissingArgumentException("Missing argument - Index of task required");
}
try {
return new DeleteCommand(Integer.parseInt(splitInput.get(1)) - 1, false);
} catch (NumberFormatException e) {
throw new InvalidArgumentException("Index to delete is not an integer");
}
case "todo":
if (splitInput.size() <= 1) {
throw new MissingArgumentException("Missing argument - Description of a todo cannot be empty");
}
description = String.join(" ", splitInput.subList(1, splitInput.size()));
// Return new add todo command
return new AddCommand(new Todo(description));
case "deadline":
// Get arguments
arguments = splitInput.subList(1, splitInput.size());
// Get index of '/by' argument
int byIndex = -1;
for (int i = 0; i < arguments.size(); i++) {
if (arguments.get(i).equals("/by")) {
byIndex = i;
break;
}
}
if (byIndex == -1) {
throw new MissingArgumentException("Missing Argument - Argument '/by' missing");
}
// Extract task description & due date
description = String.join(" ", arguments.subList(0, byIndex));
try {
String date = arguments.get(byIndex + 1);
String time = arguments.get(byIndex + 2);
// Create new add deadline command
return new AddCommand(new Deadline(description, Parser.userDateToInstant(date, time)));
} catch (NumberFormatException | IndexOutOfBoundsException e) {
throw new InvalidArgumentException(
"Date/time format is invalid. Please enter the date/time in the format 'YYYY/MM/DD hh:mm'");
}
case "event":
// Get arguments
arguments = splitInput.subList(1, splitInput.size());
// Get index of '/from' and '/to' argument
int fromIndex = -1;
int toIndex = -1;
for (int i = 0; i < arguments.size(); i++) {
if (fromIndex != -1 && toIndex != -1) {
break;
}
if (fromIndex == -1 && arguments.get(i).equals("/from")) {
fromIndex = i;
}
if (toIndex == -1 && arguments.get(i).equals("/to")) {
toIndex = i;
}
}
if (fromIndex == -1) {
throw new MissingArgumentException("Missing Argument - Argument '/from' missing");
} else if (toIndex == -1) {
throw new MissingArgumentException("Missing Argument - Argument '/to' missing");
}
// Extract task description
description = String.join(" ", arguments.subList(0, fromIndex));
try {
// Extract start date
String fromDate = arguments.get(fromIndex + 1);
String fromTime = arguments.get(fromIndex + 2);
// Extract end date
String toDate = arguments.get(toIndex + 1);
String toTime = arguments.get(toIndex + 2);
// Create new add event command
return new AddCommand(new Event(description, Parser.userDateToInstant(fromDate, fromTime),
Parser.userDateToInstant(toDate, toTime)));
} catch (NumberFormatException | IndexOutOfBoundsException e) {
throw new InvalidArgumentException(
"Date/time format is invalid. Please enter the date/time in the format 'YYYY/MM/DD hh:mm'");
}
case "find":
if (splitInput.size() <= 1) { // no arguments
return new FindCommand(false);
}
String keyword = String.join(" ", splitInput.subList(1, splitInput.size()));
// Return new add todo command
return new FindCommand(keyword, false);
case "archive":
if (splitInput.size() <= 1) {
throw new MissingArgumentException("Missing argument - Index of task required");
}
try {
return new ArchiveCommand(Integer.parseInt(splitInput.get(1)) - 1, true);
} catch (NumberFormatException e) {
throw new InvalidArgumentException("Index to archive is not an integer");
}
case "unarchive":
if (splitInput.size() <= 1) {
throw new MissingArgumentException("Missing argument - Index of task required");
}
try {
return new ArchiveCommand(Integer.parseInt(splitInput.get(1)) - 1, false);
} catch (NumberFormatException e) {
throw new InvalidArgumentException("Index to unarchive is not an integer");
}
default:
throw new InvalidCommandException(String.format("Unknown command '%s'", splitInput.get(0)));
}
}Example from src/main/java/duke/storage/Storage.java lines 49-104:
public static void loadFromFile(TaskList taskList, File file)
throws TaskNotSupportedException, FileNotFoundException, TaskCorruptedException, StorageException {
// Check if file is readable
if (!file.canRead()) {
throw new FileNotFoundException("File cannot be read");
}
try {
// Read stored tasks from file
FileInputStream input = new FileInputStream(file);
JSONArray jsonArray = new JSONArray(new JSONTokener(input));
// Populate storage array
for (int i = 0; i < jsonArray.length(); i++) {
// Get json entry
JSONObject entry = jsonArray.getJSONObject(i);
assert entry != null; // Entry should exist
// Parse JSON entry to task
Task task;
switch (TaskType.valueOf(entry.getString("type"))) {
case TODO:
task = new Todo(entry.getString("description"),
entry.getBoolean("isCompleted"),
entry.getBoolean("isArchived"));
break;
case DEADLINE:
task = new Deadline(entry.getString("description"),
entry.getLong("dueDate"),
entry.getBoolean("isCompleted"),
entry.getBoolean("isArchived"));
break;
case EVENT:
task = new Event(entry.getString("description"),
entry.getLong("startDate"),
entry.getLong("endDate"),
entry.getBoolean("isCompleted"),
entry.getBoolean("isArchived"));
break;
default:
throw new TaskNotSupportedException(
String.format("Task '%s' not currently supported", entry.getString("type")));
}
assert task != null; // Task should exist
// Add task to storage array
taskList.addTask(task, task.getIsArchived());
}
} catch (IllegalArgumentException | JSONException e) {
throw new TaskCorruptedException("Task corrupted, will not be loaded");
}
}Example from src/test/java/duke/parser/ParserTest.java lines 151-193:
public void parse_mark_exceptionThrown() throws DukeException {
String expectedMessage = "Missing argument - Index of task required";
MissingArgumentException missingException = assertThrows(
MissingArgumentException.class, () -> Parser.parse("mark"));
assertEquals(expectedMessage, missingException.getMessage());
missingException = assertThrows(
MissingArgumentException.class, () -> Parser.parse("mark "));
assertEquals(expectedMessage, missingException.getMessage());
missingException = assertThrows(
MissingArgumentException.class, () -> Parser.parse(" mark"));
assertEquals(expectedMessage, missingException.getMessage());
missingException = assertThrows(
MissingArgumentException.class, () -> Parser.parse(" mark "));
assertEquals(expectedMessage, missingException.getMessage());
missingException = assertThrows(
MissingArgumentException.class, () -> Parser.parse("Mark"));
assertEquals(expectedMessage, missingException.getMessage());
expectedMessage = "Index to mark is not an integer";
InvalidArgumentException invalidException = assertThrows(
InvalidArgumentException.class, () -> Parser.parse("mark me"));
assertEquals(expectedMessage, invalidException.getMessage());
invalidException = assertThrows(
InvalidArgumentException.class, () -> Parser.parse("mark me "));
assertEquals(expectedMessage, invalidException.getMessage());
invalidException = assertThrows(
InvalidArgumentException.class, () -> Parser.parse(" mark me"));
assertEquals(expectedMessage, invalidException.getMessage());
invalidException = assertThrows(
InvalidArgumentException.class, () -> Parser.parse(" mark me "));
assertEquals(expectedMessage, invalidException.getMessage());
invalidException = assertThrows(
InvalidArgumentException.class, () -> Parser.parse("Mark me"));
assertEquals(expectedMessage, invalidException.getMessage());
}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
src\test\java\duke\parser\ParserTest.java(544 lines)
Suggestion: Consider breaking large classes into smaller ones, if appropriate. A long class is a sign that perhaps it is dong 'too much'.
Aspect: Header Comments
Example from src/main/java/duke/Duke.java lines 30-34:
/**
* Create a Duke instance
*
* @param file File to save and load tasks
*/Example from src/main/java/duke/Duke.java lines 45-49:
/**
* Get the save file of the application
*
* @return Save file of the application
*/Example from src/main/java/duke/Duke.java lines 54-58:
/**
* Set the save file of the application
*
* @param file File to use as the save file of the application
*/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 Message
possible problems in commit 133a37f:
Reset test data file in test cases
File is not reset in some test cases which causes bugs in other test cases
- body not wrapped at 72 characters: e.g.,
File is not reset in some test cases which causes bugs in other test cases
possible problems in commit 71329e4:
Fix bug in getTasksString method
Stream is using wrong list to generate the index which is causing an index out of range exception
- body not wrapped at 72 characters: e.g.,
Stream is using wrong list to generate the index which is causing an index out of range exception
Suggestion: Follow the given conventions for Git commit messages for future commits (do not modify past commit messages as doing so will change the commit timestamp that we used to detect your commit timings).
Aspect: Binary files in repo
No easy-to-detect 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.