Sharing iP code quality feedback [for @RyanNgWH] - Round 2
Opened this issue · 0 comments
@RyanNgWH 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/ciara/commands/ArchiveCommand.java lines 23-23:
private boolean toArchive;Example from src/main/java/ciara/ui/gui/MainWindow.java lines 73-73:
Boolean toExit = false;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
No easy-to-detect issues 👍
Aspect: Method Length
Example from src/main/java/ciara/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/ciara/parser/ParserTest.java lines 212-254:
public void parse_mark_exceptionThrown() throws CiaraException {
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());
}Example from src/test/java/ciara/parser/ParserTest.java lines 329-371:
public void parse_delete_exceptionThrown() throws CiaraException {
String expectedMessage = "Missing argument - Index of task required";
MissingArgumentException missingException = assertThrows(
MissingArgumentException.class, () -> Parser.parse("delete"));
assertEquals(expectedMessage, missingException.getMessage());
missingException = assertThrows(
MissingArgumentException.class, () -> Parser.parse("delete "));
assertEquals(expectedMessage, missingException.getMessage());
missingException = assertThrows(
MissingArgumentException.class, () -> Parser.parse(" delete"));
assertEquals(expectedMessage, missingException.getMessage());
missingException = assertThrows(
MissingArgumentException.class, () -> Parser.parse(" delete "));
assertEquals(expectedMessage, missingException.getMessage());
missingException = assertThrows(
MissingArgumentException.class, () -> Parser.parse("Delete"));
assertEquals(expectedMessage, missingException.getMessage());
expectedMessage = "Index to delete is not an integer";
InvalidArgumentException invalidException = assertThrows(
InvalidArgumentException.class, () -> Parser.parse("delete me"));
assertEquals(expectedMessage, invalidException.getMessage());
invalidException = assertThrows(
InvalidArgumentException.class, () -> Parser.parse("delete me "));
assertEquals(expectedMessage, invalidException.getMessage());
invalidException = assertThrows(
InvalidArgumentException.class, () -> Parser.parse(" delete me"));
assertEquals(expectedMessage, invalidException.getMessage());
invalidException = assertThrows(
InvalidArgumentException.class, () -> Parser.parse(" delete me "));
assertEquals(expectedMessage, invalidException.getMessage());
invalidException = assertThrows(
InvalidArgumentException.class, () -> Parser.parse("Delete 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\ciara\parser\ParserTest.java(595 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
No easy-to-detect issues 👍
Aspect: Recent Git Commit Message
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.