Code Quality Feedback for A0163962X

[xpdavid]

A0163962X-unused.md is put into a wrong folder. Should put inside unused folder.

Main

        if (this.flag.equals(Task.TASK_FLAG)) {
            UnmodifiableObservableList<ReadOnlyTask> lastShownList = model.getFilteredTaskList();

            if (lastShownList.size() < targetIndex) {
                throw new CommandException(Messages.MESSAGE_INVALID_TASK_DISPLAYED_INDEX);
            }

            ReadOnlyTask taskToDelete = lastShownList.get(targetIndex - 1);

            try {
                model.deleteTask(taskToDelete);
            } catch (TaskNotFoundException tnfe) {
                assert false : "The target task cannot be missing";
            }

            return new CommandResult(String.format(MESSAGE_DELETE_TASK_SUCCESS, taskToDelete));
        } else {
            UnmodifiableObservableList<ReadOnlyEvent> lastShownList = model.getFilteredEventList();

            if (lastShownList.size() < targetIndex) {
                throw new CommandException(Messages.MESSAGE_INVALID_EVENT_DISPLAYED_INDEX);
            }

            ReadOnlyEvent eventToDelete = lastShownList.get(targetIndex - 1);

            try {
                model.deleteEvent(eventToDelete);
            } catch (EventNotFoundException enfe) {
                assert false : "The target event cannot be missing";
            }

            return new CommandResult(String.format(MESSAGE_DELETE_EVENT_SUCCESS, eventToDelete));
        }

Duplicated or very similar code blocks. Also in EditCommandParser (as the code is too long, I won't put it here)

/**
     *
     * @param date
     * @return
     * @throws IllegalValueException
     */
    private Timeslot parseDateRange(String date) throws IllegalValueException {
        Timeslot dateRange;

Javadoc without description but with @param?

try {
            if (preambleFields.get(0).get().equals(Task.TASK_FLAG)) {

                Optional<Integer> index = preambleFields.get(1).flatMap(ParserUtil::parseIndex);
                if (!index.isPresent()) {
                    return new IncorrectCommand(
                            String.format(MESSAGE_INVALID_COMMAND_FORMAT, EditCommand.MESSAGE_USAGE));
                }

                EditTaskDescriptor editTaskDescriptor = new EditTaskDescriptor();
                try {
                    editTaskDescriptor.setName(ParserUtil.parseName(preambleFields.get(2)));
                    editTaskDescriptor.setPriority(ParserUtil.parsePriority(argsTokenizer.getValue(PREFIX_PRIORITY)));
                    editTaskDescriptor.setDeadline(ParserUtil.parseDeadline(argsTokenizer.getValue(PREFIX_DATE)));
                    editTaskDescriptor
                            .setDescription(ParserUtil.parseDescription(argsTokenizer.getValue(PREFIX_DESCRIPTION)));
                    editTaskDescriptor
                            .setTags(parseTagsForEdit(ParserUtil.toSet(argsTokenizer.getAllValues(PREFIX_TAG))));
                } catch (IllegalValueException ive) {
                    return new IncorrectCommand(ive.getMessage());
                }

try-catch in try-catch?

    /**
     * Parses a {@code Optional<String> deadline} into an
     * {@code Optional<Deadline>} if {@code deadline} is present.
     */
    public static Optional<List<Timeslot>> parseTimeslots(Optional<String> timeslots) throws IllegalValueException {
        assert timeslots != null;

        if (timeslots.isPresent()) {

            if (timeslots.get().equals(Timeslot.NO_TIMESLOT)) {
                throw new IllegalValueException(Timeslot.MESSAGE_TIMESLOT_DNE);
            }

            // TODO again, could refactor this somewhere
            String[] timeslotsAsStrings = timeslots.get().split("\\s+or\\s+");
            List<Timeslot> timeslotsParsed = new ArrayList<>();
            for (String t : timeslotsAsStrings) {
                String[] dates = t.split("\\s+to\\s+");
                if (dates.length != 2) {
                    throw new IllegalValueException(Timeslot.MESSAGE_TIMESLOT_PAIRS);
                }
                timeslotsParsed.add(new Timeslot(dates[0], dates[1]));
            }

            return Optional.of(timeslotsParsed);

        } else {
            return Optional.empty();
        }
    }

Poor SLAP here? Too deep nesting?

    @Override
    public boolean equals(Object other) {
        return other == this // short circuit if same object
                || (other instanceof Description // instanceof handles nulls
                        && this.description.equals(((Description) other).description)); // state
        // check
    }

Not meaningful comment? // check

Test

    @Test
    public void execute_add_invalidArgsFormat() {
        String expectedMessage = String.format(MESSAGE_INVALID_COMMAND_FORMAT, AddCommand.MESSAGE_USAGE);
        assertCommandFailure("add ", expectedMessage);
        assertCommandFailure("add noflag", expectedMessage);
        assertCommandFailure("add z Valid Name p/3 //valid description", expectedMessage);
    }

Test case naming not following coding standard? featureUnderTest_testScenario_expectedBehavior()

    @Test
    public void execute_confirm_successful() throws Exception {
        execute_add_successful();

        UserInbox expectedInbox = new UserInbox();
        TestDataHelper helper = new TestDataHelper();
        Task toBeAdded = helper.homework();
        expectedInbox.addTask(toBeAdded);
        Event toBeAdded2 = helper.reviewSession();
        expectedInbox.addEvent(toBeAdded2);
        Event confirmed = helper.reviewSessionConfirmed();
        expectedInbox.addEvent(confirmed);

        assertCommandSuccess("confirm e 2 1", String.format(ConfirmCommand.MESSAGE_CONFIRM_EVENT_SUCCESS, confirmed),
                expectedInbox, expectedInbox.getTaskList(), expectedInbox.getEventList());
    }

Test case nests test case?

    @Test
    public void execute_confirm_eventIndexNotFound() throws Exception {
        String expectedMessage = MESSAGE_INVALID_EVENT_DISPLAYED_INDEX;
        TestDataHelper helper = new TestDataHelper();
        List<Task> taskList = helper.generateTaskList(2);
        List<Event> eventList = new ArrayList<>();
        eventList.add(helper.reviewSession());

        model.resetData(new UserInbox());
        for (Task t : taskList) {
            model.addTask(t);
        }

        for (Event e : eventList) {
            model.addEvent(e);
        }

        assertCommandFailure("confirm e 2 1", expectedMessage);
    }

Consider SLAP here regarding to the two for loop.

List<Event> unchangedEvents = new ArrayList<>();
        List<Task> changedTasks = new ArrayList<>();

        // keep if want to add more tests
        // Event editedEvent = new Event(preexistingEvents.get(0).getName(),
        // preexistingEvents.get(0).getTimeslots(),
        // preexistingEvents.get(0).getLocation(),
        // preexistingEvents.get(0).getDescription(),
        // preexistingEvents.get(0).getTags());

        Task editedTask = new Task(new Name("editedName"), new Deadline(""), new Priority(Priority.NO_PRIORITY),
                new Description("editedDescription"), preexistingTasks.get(0).getTags());

Comment out code?

[bdioni]

Thank you!