[Li Shunyang/lishunyang12] ip#196
Conversation
HalFentise
left a comment
HalFentise
left a comment
There was a problem hiding this comment.
i like your implementation in task class
thanks |
sevenseasofbri
left a comment
sevenseasofbri
left a comment
There was a problem hiding this comment.
Good job so far! Some methods are a little long but those can be refactored to improve readability.
| import engineer.task.*; | ||
| import engineer.command.*; |
There was a problem hiding this comment.
Avoid using wildcard imports, explicitly import each class
| static TextDivider myTextDivider = new TextDivider(); | ||
|
|
||
| public static void printResponse() throws EngineerException{ | ||
| boolean isKeepAsking = true; |
There was a problem hiding this comment.
Think of the name of a boolean variable as a question. I am not quite sure if "is keep asking?" a good name in this case?
Maybe something like isRunning or shouldContinue would be more grammartically sound?
| if (inputMessage.isEmpty()) { | ||
| throw new EngineerException(myChatBotManager.askForNonEmptyValue()); | ||
| } |
There was a problem hiding this comment.
Good job making the happy path prominent.
| static ChatBotManager myChatBotManager = new ChatBotManager(); | ||
| static TextDivider myTextDivider = new TextDivider(); | ||
|
|
||
| public static void printResponse() throws EngineerException{ |
There was a problem hiding this comment.
Few things about this method
- The method is quite long (>30LOC). Consider chunking it into multiple methods to maintain redability
- The naming is not apt because it is not only printing the response but doing several other tasks within it.
| public class ChatBotManager { | ||
| private String indent = " "; | ||
| private String line = indent + "___________________________"; | ||
| // TaskManager myTaskManager = new TaskManager(); |
There was a problem hiding this comment.
Avoid commenting out dead code. Just delete it instead
| public String[] divideEvent(String[] words){ | ||
| String inputTaskEvents = ""; | ||
| String from = ""; | ||
| String to = ""; | ||
| String[] textString = new String[3]; | ||
|
|
||
| int firstDivisionIndex = 0; | ||
| int secondDivisionIndex = 0; | ||
| for(int i = 0; i < words.length; i++) { | ||
| if(words[i].equals("/from")) { | ||
| firstDivisionIndex = i; | ||
| } | ||
| if(words[i].equals("/to")) { | ||
| secondDivisionIndex = i; | ||
| } | ||
| } | ||
| for(int i = 0; i < firstDivisionIndex; i++) { | ||
| inputTaskEvents += words[i] + " "; | ||
| } | ||
| for(int i = firstDivisionIndex+1; i < secondDivisionIndex; i++) { | ||
| from += words[i] + " "; | ||
| } | ||
| for(int i = secondDivisionIndex+1; i < words.length; i++) { | ||
| if(i == words.length-1) { | ||
| to += words[i]; | ||
| } else { | ||
| to += words[i] + " "; | ||
| } |
There was a problem hiding this comment.
Avoid using magic literals by using constants or variables in place of them
| List<String> lines = Files.readAllLines(Paths.get(FILE_PATH)); | ||
| for (String line : lines) { | ||
| if(!line.trim().isEmpty()) { | ||
| Task task = Task.fromFileString(line); | ||
| if(task != null) { | ||
| taskList.add(task); | ||
| taskCount++; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
This is around 3 levels of indentation, consdier moving the inner lines into a separate method
| writer.write(taskList.get(i).toFileString() + "\n"); | ||
| } | ||
| writer.close(); | ||
| }catch(IOException e) { |
There was a problem hiding this comment.
| }catch(IOException e) { | |
| } catch(IOException e) { |
3 participants
No description provided.