Sharing iP code quality feedback [for @AlagappanRa] #11
Description
@AlagappanRa 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
Example from src/main/java/components/Storage.java lines 41-41:
//System.out.println("Parent dir " + parentDir.getAbsolutePath());Example from src/main/java/main/Launcher.java lines 10-10:
//Duke.main(args);Suggestion: Remove dead code from the codebase.
Aspect: Method Length
Example from src/main/java/components/Storage.java lines 77-130:
public TaskList readData(Storage storage) throws DukeException {
BufferedReader br;
TaskList result = new TaskList();
try {
br = new BufferedReader(new FileReader(this.store));
assert br != null : "BufferedReader object is null";
String line;
while ((line = br.readLine()) != null) {
//[T][X] read book
if (line.startsWith("[T]")) {
result.addTask(new ToDo(
line.substring(7)),
storage,
true
);
if (line.charAt(4) == 'X') {
result.get(result.size() - 1).markAsDone();
}
//[D][ ] return book (by: Sunday)
} else if (line.startsWith("[D]")) {
int byIndex = line.indexOf("(by: ");
result.addTask(new Deadline(
line.substring(7, byIndex - 1),
line.substring(byIndex + 5, line.length() - 1)),
storage,
true
);
if (line.charAt(4) == 'X') {
result.get(result.size() - 1).markAsDone();
}
//[E][ ] project meeting (from: Mon 2pm to: Fri 4pm)
} else if (line.startsWith("[E]")) {
int fromIndex = line.indexOf(" (from: ");
int toIndex = line.indexOf(" to: ");
result.addTask(new Event(
line.substring(7, fromIndex),
line.substring(fromIndex + 8, toIndex),
line.substring(toIndex + 5, line.length() - 1)),
storage,
true
);
if (line.charAt(4) == 'X') {
result.get(result.size() - 1).markAsDone();
}
}
}
br.close();
return result;
} catch (IOException e) {
throw new DukeException("Something went wrong: " + e.getMessage());
}
}Example from src/main/java/components/Storage.java lines 155-201:
public void deleteLine(int lineNumber) throws DukeException {
List<String> lines = new ArrayList<>();
BufferedReader br = null;
BufferedWriter bw = null;
try {
assert store != null && store.exists() : "Store file does not exist";
// Read file into list
br = new BufferedReader(new FileReader(this.store));
assert br != null : "BufferedReader object is null";
String line;
while ((line = br.readLine()) != null) {
lines.add(line);
}
br.close();
// Remove the line
if (lineNumber < 1 || lineNumber > lines.size()) {
throw new DukeException("Invalid line number");
}
lines.remove(lineNumber);
// Write list back to file
bw = new BufferedWriter(new FileWriter(store));
assert bw != null : "BufferedWriter object is null";
for (int i = 0; i < lines.size(); i++) {
bw.write(lines.get(i));
if (i < lines.size() - 1) {
bw.newLine();
}
}
bw.close();
} catch (IOException e) {
throw new DukeException("Something went wrong: " + e.getMessage());
} finally {
try {
if (br != null) {
br.close();
}
if (bw != null) {
bw.close();
}
} catch (IOException e) {
throw new DukeException("Something went wrong while closing the file: " + e.getMessage());
}
}
}Example from src/main/java/components/Storage.java lines 209-255:
public void replaceLine(int lineNumber, String newData) throws DukeException {
List<String> lines = new ArrayList<>();
BufferedReader br = null;
BufferedWriter bw = null;
try {
assert store != null && store.exists() : "Store file does not exist";
// Read file into list
br = new BufferedReader(new FileReader(this.store));
assert br != null : "BufferedReader object is null";
String line;
while ((line = br.readLine()) != null) {
lines.add(line);
}
br.close();
// Replace the line
if (lineNumber < 1 || lineNumber > lines.size()) {
throw new DukeException("Invalid line number");
}
lines.set(lineNumber, newData);
// Write list back to file
bw = new BufferedWriter(new FileWriter(store));
assert bw != null : "BufferedWriter object is null";
for (int i = 0; i < lines.size(); i++) {
bw.write(lines.get(i));
if (i < lines.size() - 1) {
bw.newLine();
}
}
bw.close();
} catch (IOException e) {
throw new DukeException("Something went wrong: " + e.getMessage());
} finally {
try {
if (br != null) {
br.close();
}
if (bw != null) {
bw.close();
}
} catch (IOException e) {
throw new DukeException("Something went wrong while closing the file: " + e.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
No easy-to-detect issues 👍
Aspect: Header Comments
Example from src/main/java/components/Parser.java lines 8-14:
/**
* Create a ToDo task.
*
* @param command Command to be parsed.
* @return ToDo task.
* @throws DukeException If command is invalid.
*/Example from src/main/java/components/Parser.java lines 30-36:
/**
* Create a Deadline task.
*
* @param fullCommand Command to be parsed.
* @return Deadline task.
* @throws DukeException If command is invalid.
*/Example from src/main/java/main/Duke.java lines 150-154:
/**
* Iteration 2:
* Creates two dialog boxes, one echoing user input and the other containing Duke's reply and then appends them to
* the dialog container. Clears the user input after processing.
*/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 d0cc476:
Add duplicate task handling
Duplicate tasks is addable.
We do not want duplicate tasks to be addable by default.
Lets make the the application reply to the user with a message saying they tried to add a duplicate description upon attempting to add duplicate description.
Allows the user to know if they have accidentally added a overlapping task description.
- body not wrapped at 72 characters: e.g.,
Lets make the the application reply to the user with a message saying they tried to add a duplicate description upon attempting to add duplicate description.
possible problems in commit 203ba33:
Update TaskList
TaskList does not use streams.
Streams provide a declarative way to write code, making out code more understandable.
The tasklist :: printList() method is changed to create a stream.
printList() is the most relevant method to change as the collectors to list method is natural with the print function together with the "\n".
- body not wrapped at 72 characters: e.g.,
Streams provide a declarative way to write code, making out code more understandable.
possible problems in commit f1fe7cc:
Add assertions
Code lacks assertions.
Without assertions, we will not be able to test out code.
Lets add assertions within all abstract classes and to all classes under components package.
Assertions are only added to abstract classes so that the assertions do not have to be repeated for child subclasses.
- body not wrapped at 72 characters: e.g.,
Lets add assertions within all abstract classes and to all classes under components package.
Suggestion: Follow the given conventions for Git commit messages for future commits (no need to modify past commit messages).
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.