Sharing iP code quality feedback [for @geraldneo567] - Round 2 #5
Description
@geraldneo567 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/duke/Task.java lines 10-10:
private boolean completed;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/duke/Storage.java lines 45-86:
public List<Task> load() {
List<Task> ls = new ArrayList<>();
File directory = new File("../../../data/");
if (!directory.exists()) {
directory.mkdir();
}
try {
File saved = new File(filepath);
if (!saved.exists()) {
saved.createNewFile();
} else {
Scanner scan = new Scanner(saved);
while (scan.hasNextLine()) {
String line = scan.nextLine();
String[] splitted = line.split("\\|");
String firstToken = splitted[0];
String name = splitted[2];
boolean completed = splitted[1].equals("1");
if (firstToken.equals("D")) {
String timing = splitted[3];
Deadline toAdd = new Deadline(name, timing);
ls.add(toAdd);
if (completed) toAdd.setDone();
}
if (firstToken.equals("T")) {
Todo toAdd = new Todo(splitted[2]);
ls.add(toAdd);
if (completed) toAdd.setDone();
}
if (firstToken.equals("E")) {
String timing = splitted[3];
Event toAdd = new Event(name, timing);
ls.add(toAdd);
if (completed) toAdd.setDone();
}
}
}
} catch(IOException e) {
System.out.println(e);
}
return ls;
}Example from src/main/java/duke/Ui.java lines 31-86:
public void start(Stage stage) {
//The container for the content of the chat to scroll.
scrollPane = new ScrollPane();
dialogContainer = new VBox();
scrollPane.setContent(dialogContainer);
userInput = new TextField();
sendButton = new Button("Send");
AnchorPane mainLayout = new AnchorPane();
mainLayout.getChildren().addAll(scrollPane, userInput, sendButton, label);
scene = new Scene(mainLayout);
stage.setScene(scene);
stage.show();
//Step 2. Formatting the window to look as expected
stage.setTitle("Duke");
stage.setResizable(false);
stage.setMinHeight(600.0);
stage.setMinWidth(400.0);
mainLayout.setPrefSize(400.0, 600.0);
scrollPane.setPrefSize(385, 535);
scrollPane.setHbarPolicy(ScrollPane.ScrollBarPolicy.NEVER);
scrollPane.setVbarPolicy(ScrollPane.ScrollBarPolicy.ALWAYS);
scrollPane.setVvalue(1.0);
scrollPane.setFitToWidth(true);
// You will need to import `javafx.scene.layout.Region` for this.
dialogContainer.setPrefHeight(Region.USE_COMPUTED_SIZE);
userInput.setPrefWidth(325.0);
sendButton.setPrefWidth(55.0);
AnchorPane.setTopAnchor(scrollPane, 1.0);
AnchorPane.setBottomAnchor(sendButton, 1.0);
AnchorPane.setRightAnchor(sendButton, 1.0);
AnchorPane.setLeftAnchor(userInput , 1.0);
AnchorPane.setBottomAnchor(userInput, 1.0);
sendButton.setOnMouseClicked((event) -> {
handleUserInput();
});
userInput.setOnAction((event) -> {
handleUserInput();
});
// more code to be added here later
}Example from src/main/java/duke/Ui.java lines 88-218:
private void handleUserInput() {
this.label.setVisible(false);
String userText = userInput.getText();
String response = "";
CommandType commandType = parser.parse(userText);
switch (commandType) {
case LIST:
response = taskList.getTasksAsString();
break;
case MARK:
try {
String[] splitted = userText.split(" ");
int index = Integer.valueOf(splitted[1]);
Task set = taskList.setDone(index - 1);
response += "Nice! I've marked this task as done: \n";
response += set;
break;
} catch (IndexOutOfBoundsException e) {
response += "ERROR: The index you have provided is invalid";
break;
}
case UNMARK:
try {
String[] splitted = userText.split(" ");
int index = Integer.valueOf(splitted[1]);
Task unset = taskList.setUndone(index - 1);
response += "OK, I've marked this task as not done yet: \n";
response += unset;
break;
} catch (IndexOutOfBoundsException e) {
response += "ERROR: The index you have provided is invalid";
break;
}
case TODO:
try {
Task toAdd = new Todo(userText.split("todo")[1].trim());
taskList.addTask(toAdd);
response += "Got it. I've added this task: \n";
response += toAdd + "\n";
response += "Now you have " + taskList.getSize() + " tasks in the list.";
break;
} catch (ArrayIndexOutOfBoundsException e) {
response = "☹ OOPS!!! The description of a todo cannot be empty.";
break;
}
case DEADLINE:
try {
String[] splitted = userText.split("deadline")[1].split("/by");
Deadline toAdd = new Deadline(splitted[0].trim(), splitted[1].trim());
taskList.addTask(toAdd);
response += "Got it. I've added this task: \n";
response += toAdd + "\n";
response += "Now you have " + taskList.getSize() + " tasks in the list.";
break;
} catch (ArrayIndexOutOfBoundsException e) {
response += "Oops! The correct format is : `deadline /by <type time here>`";
break;
}
case EVENT:
try {
String[] splitted = userText.split("event")[1].split("/at");
Event toAdd = new Event(splitted[0].trim(), splitted[1].trim());
taskList.addTask(toAdd);
response += "Got it. I've added this task: \n";
response += toAdd + "\n";
response += "Now you have " + taskList.getSize() + " tasks in the list.";
break;
} catch (ArrayIndexOutOfBoundsException e) {
response += "Oops! The correct format is : `event /at <type time here>`";
break;
}
case DELETE:
try {
String[] splitted = userText.split(" ");
int index = Integer.valueOf(splitted[1]);
Task removedTask = taskList.deleteTask(index - 1);
response += "Noted. I've removed this task: \n";
response += removedTask + "\n";
response += "Now you have " + taskList.getSize() + " tasks in the list.";
break;
} catch (IndexOutOfBoundsException e) {
response += "The index you have provided is invalid";
break;
}
case FIND:
String[] splitted = userText.split(" ");
response += "Here are the matching tasks in your list:\n";
taskList.findAndGetTasks(splitted[1]);
break;
case UPDATE:
String[] splittedString = userText.split(" ");
int index = Integer.valueOf(splittedString[1]);
String command = splittedString[2];
if (command.startsWith("name")) {
String name = userText.split("name")[1].trim();
taskList.findAndSetName(name, index);
response += "Noted. I have updated the name of task " + index + " to " + name + ".";
}
if (command.startsWith("time")) {
String time = splittedString[3].trim();
if (splittedString.length == 5) {
time += " " + splittedString[4].trim();
}
response += "Noted. I have updated the time of task " + index + " to " + time + ".";
taskList.findAndSetTime(time, index);
}
default:
break;
}
Label userTextLabel = new Label(userText);
Label dukeTextLabel = new Label(response);
dialogContainer.getChildren().addAll(
new DialogBox(userTextLabel, new ImageView(user)),
new DialogBox(dukeTextLabel, new ImageView(duke)).flip()
);
userInput.clear();
/*
Label userText = new Label(userInput.getText());
Label dukeText = new Label(getResponse(userInput.getText()));
dialogContainer.getChildren().addAll(
new DialogBox(userText, new ImageView(user)),
new DialogBox(dukeText, new ImageView(duke))
);
userInput.clear();
*/
}Suggestion: Consider applying SLAP (and other abstraction mechanisms) to shorten 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/duke/Task.java lines 33-35:
/**
* Set a task as done.
*/Example from src/main/java/duke/Task.java lines 40-42:
/**
* Set a task as not done.
*/Example from src/main/java/duke/TaskList.java lines 135-138:
/**
* Obtain the current size of the list.
* @return int containing size of the list
*/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 (Subject Only)
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.