Fix bug where task commands work with room index instead of room numbers #115
Conversation
w-yeehong
force-pushed
the
fix-issue#102
branch
from
a13a1c6
to
6c253cc
Compare
w-yeehong
changed the title
w-yeehong
changed the title
There was a problem hiding this comment.
Good job and thank you for refactoring the messages! Can see that you tried to add some assertions in your code. However, if I am not mistaken, some of your assertions are used in cases when user input is invalid. In such cases, I am not sure if assertions are appropriate since assertions cause the application to crash. (Imagine keying in a room number of -5 and the app crashes ><)
|
|
||
| if (roomIndex.getZeroBased() >= rooms.size()) { | ||
| throw new CommandException(Messages.MESSAGE_INVALID_ROOM_INDEX); | ||
| if (roomNumber < 0) { |
There was a problem hiding this comment.
Actually now I'm wondering if you do your assertion early on in the constructor, then the app will directly crash and will never reach here :X
There was a problem hiding this comment.
Yup that's true. Good observation here 😄
That said, assertions may not necessarily be enabled, especially when an application goes into production.
The roomNumber < 0 check is to prevent the application from crashing if this method somehow gets called when assertions are off.
| } | ||
|
|
||
| @Override | ||
| public CommandResult execute(Model model) throws CommandException { | ||
| requireNonNull(model); | ||
| if (roomNumber < 0) { |
There was a problem hiding this comment.
Same issue, if you assert in constructor, will this Message Invalid room number be received by user?
| Task taskToDelete = tasks.get(taskIndex.getZeroBased()); | ||
| Optional<Task> optionalTask = model.getTaskFromRoomWithTaskIndex(taskIndex, room); | ||
| Task taskToDelete = optionalTask.orElseThrow(() -> | ||
| new CommandException(Messages.MESSAGE_INVALID_TASK_INDEX)); | ||
| assert taskToDelete != null : "The task to delete should never be null."; |
There was a problem hiding this comment.
If the task to delete is null, the app will just crash?
There was a problem hiding this comment.
Yup, I think the app is meant to crash in that case. model.getTaskFromRoomWithTaskIndex(taskIndex, room) should return either an empty Optional or an Optional-wrapped task. It should never be null here.
The app will crash with a NullPointerException if taskToDelete is null either way.
| return roomList.containsRoom(room); | ||
| } | ||
|
|
||
| @Override | ||
| public Optional<Room> getRoomWithRoomNumber(int roomNumber) { | ||
| assert (roomNumber > 0) : "Room number should be greater than 0."; |
There was a problem hiding this comment.
Yup, it will crash the app provided assertions are enabled. If getRoomWithRoomNumber(...) is called with a roomNumber greater than 0, it would be a programming error. Crashing in that case is helpful for debugging.
| requireAllNonNull(task, roomIndex); | ||
| public AddTaskCommand(Task task, int roomNumber) { | ||
| requireNonNull(task); | ||
| assert roomNumber > 0 : "Room number should be greater than 0."; |
There was a problem hiding this comment.
i think you should be asserting if the roomNumber is null? Because if the roomNumber is <0 then it should be the user fault and not the system fault no?
There was a problem hiding this comment.
roomNumber cannot actually be null though, because it is a primitive (i.e. int).
addtask,deletetask, andedittasknow work with room number instead of index.