Sharing iP code quality feedback [for @FreakkMe] #1
Description
@FreakkMe 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
No easy-to-detect issues 👍
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/freaky/storage/Storage.java lines 133-200:
private Task parseLine(String line) {
// Format: TYPE | STATUS | DESCRIPTION | OPTIONAL
String[] parts = line.split(" \\| ");
String type = parts[0];
boolean isDone = parts[1].equals("1");
String description = parts[2];
switch (type) {
// "to-do" case
case "T":
ToDo todo = new ToDo(description);
if (isDone) {
todo.markAsDone();
}
return todo;
// "deadline" case
case "D":
String by = parts[3];
LocalDateTime time;
// Checks if the format of the task stored in hard disk is valid, returns a message if not
try {
time = Parser.parseLocalDateTime(parts[3]);
} catch (DateTimeParseException e) {
throw new IllegalArgumentException("Invalid deadline datetime in file", e);
}
Deadline deadline = new Deadline(description, time);
if (isDone) {
deadline.markAsDone();
}
return deadline;
// "event" case
case "E":
String fromTo = parts[3];
String[] times = fromTo.split(" -> ");
LocalDateTime startTime;
LocalDateTime endTime;
// Checks if the format of the task stored in hard disk is valid, returns a message if not
try {
startTime = Parser.parseLocalDateTime(times[0]);
endTime = Parser.parseLocalDateTime(times[1]);
} catch (DateTimeParseException e) {
throw new IllegalArgumentException("Invalid event datetime in file", e);
}
Event event = new Event(description, startTime, endTime);
if (isDone) {
event.markAsDone();
}
return event;
// Format error case
default:
throw new IllegalArgumentException("Unknown task type");
}
}Example from src/main/java/freaky/main/Freaky.java lines 228-295:
private String handleAddTask(String input) {
boolean hasBy = input.contains(" /by ");
boolean hasFrom = input.contains(" /from ");
boolean hasTo = input.contains(" /to ");
// Checks if the input after "to-do", "deadline" or "event" is valid, returns a message if not
if (input.trim().equals("todo")) {
return ui.toDoFormatMessage();
} else if (input.startsWith("deadline") && !hasBy
|| input.replaceFirst("deadline ", "").replaceFirst(" /by ", "").matches(" *")) {
return ui.deadlineFormatMessage();
} else if (input.startsWith("event") && (!hasFrom || !hasTo)
|| input.replaceFirst("event ", "").replaceFirst(" /from ", "")
.replaceFirst(" /to ", "").matches(" *")) {
return ui.eventFormatMessage();
}
Task task;
// To-do case
if (input.startsWith("todo ")) {
task = new ToDo(input.split("todo ", 2)[1]);
// Deadline case
} else if (input.startsWith("deadline ")) {
String[] parts = input.split("deadline ", 2)[1].split(" /by ", 2);
LocalDateTime time;
// Checks if the input date for deadline is valid, returns a message if not
try {
time = Parser.parseLocalDateTime(parts[1].trim());
} catch (DateTimeParseException e) {
return ui.deadlineDateTimeErrorMessage();
}
task = new Deadline(parts[0], time);
// Event case
} else if (input.startsWith("event ")) {
String[] parts = input.split("event ", 2)[1].split(" /from ", 2);
String[] times = parts[1].split(" /to ", 2);
LocalDateTime startTime;
LocalDateTime endTime;
// Checks if the input date for event is valid, returns a message if not
try {
startTime = Parser.parseLocalDateTime(times[0].trim());
endTime = Parser.parseLocalDateTime(times[1].trim());
} catch (DateTimeParseException e) {
return ui.eventDateTimeErrorMessage();
}
task = new Event(parts[0], startTime, endTime);
// Command invalid case
} else {
return ui.eventDateTimeErrorMessage();
}
tasks.add(task);
storage.save(tasks.getTasks());
// Returns task info
return ui.taskAddedMessage(task, tasks);
}Example from src/main/java/freaky/main/Freaky.java lines 311-411:
private String handleCheck(String input) {
// Default checks both task, one per task
enum CheckType { BOTH, DEADLINE, EVENT }
CheckType checkType;
int check;
// Split input into tokens to check the match type
String[] tokens = input.trim().split(" ");
// The default "check" case
if (tokens.length == 1 && input.equals("check")) {
checkType = CheckType.BOTH;
check = 1;
// Can be in "check n" format or "check deadline/event" format
} else if (tokens.length == 2) {
// "check deadline" case
if (tokens[1].equals("deadline")) {
checkType = CheckType.DEADLINE;
check = 3;
// "check event" case
} else if (tokens[1].equals("event")) {
checkType = CheckType.EVENT;
check = 3;
// "check n" case
} else {
try {
checkType = CheckType.BOTH;
check = Parser.parseInteger(tokens[1]);
} catch (NumberFormatException e) {
return ui.checkFormatMessage();
}
}
// In "check deadline/event n" format
} else if (tokens.length == 3) {
if (tokens[1].equals("deadline")) {
checkType = CheckType.DEADLINE;
} else if (tokens[1].equals("event")) {
checkType = CheckType.EVENT;
} else {
return ui.checkFormatMessage();
}
// Checks if the input after "check deadline/event ", returns a message if it is invalid
try {
check = Parser.parseInteger(tokens[2]);
} catch (NumberFormatException e) {
return ui.checkFormatMessage();
}
// Incorrect format
} else {
return ui.checkFormatMessage();
}
// Checks if the number to check is valid (non-positive), no errors even if it
// exceeds the max number of deadlines/events left
if (check <= 0) {
return ui.negativeValueError();
}
// Extracts deadlines and events that weren't marked as done before
TaskList deadlineList = Helper.getClosestDeadlines(tasks, check);
TaskList eventList = Helper.getClosestEvents(tasks, check);
StringBuilder response = new StringBuilder();
switch (checkType) {
// "check" or "check n" case which checks both type of tasks
case BOTH:
response.append(ui.checkDeadlineList(check, deadlineList))
.append("\n")
.append(ui.checkEventList(check, eventList));
break;
// "check deadline" or "check deadline n" case which checks deadline tasks
case DEADLINE:
response.append(ui.checkDeadlineList(check, deadlineList));
break;
// "check event" or "check event n" case which check event tasks
case EVENT:
response.append(ui.checkEventList(check, eventList));
break;
// This line shouldn't be reached
default:
return ui.checkFormatMessage();
}
return response.toString().trim();
}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
No easy-to-detect issues 👍
Aspect: Recent Git Commit Messages
possible problems in commit 1a58e78:
CheckStyle included and several changes made to follow Java coding standard
- Longer than 72 characters
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@nus.edu.sg if you want to follow up on this post.