CS2103JAN2017-T15-B1 / main

Taskcrusher
MIT License
1 stars 1 forks source link

Code Quality Feedback for A0127737X #81

Closed xpdavid closed 7 years ago

xpdavid commented 7 years ago

Main

    public final boolean eventListToShowEmpty;
    public final boolean taskListToShowEmpty;

    public ListsToShowUpdatedEvent(boolean eventListToShowEmpty, boolean taskListToShowEmpty) {
        this.eventListToShowEmpty = eventListToShowEmpty;
        this.taskListToShowEmpty = taskListToShowEmpty;
    }

You may want to follow the boolean variable naming conversion in the coding standard.

/** loads a new xml storage file. If the file does not exist, create a new one and set it as the storage file
 *  This is achieved by posting LoadNewStorageFileEvent which is handled at the high-level MainApp instance.
 */
public class LoadCommand extends Command {
    public static final String COMMAND_WORD = "load";

The Javadoc format is not correct.

public LoadCommand(String filenameToLoad) {
        assert filenameToLoad != null;
        this.filenameToLoad = filenameToLoad.trim();
    }
if (!filenameToLoad.endsWith(XML_EXTENSION)) {
            throw new CommandException(MESSAGE_INVALID_EXTENSION);
        }

Using of this keywords is not consistent.

        public boolean run(ReadOnlyUserToDo item) {
            if (item instanceof ReadOnlyEvent) {
                ReadOnlyEvent event = (ReadOnlyEvent) item;
                if (event.isComplete()) {
                    return false;
                } else if (event.hasOverlappingTimeslot(userInterestedTimeslot)) {
                    return true;
                } else {
                    return false;
                }
            } else if (item instanceof ReadOnlyTask) {
                ReadOnlyTask task = (ReadOnlyTask) item;
                if (task.isComplete()) {
                    return false;
                } else if (task.getDeadline().isWithin(userInterestedTimeslot)) {
                    return true;
                } else {
                    return false;
                }
            }
            assert false;
            return false; //should not reach here
        }

The two branches are almost the same. You may want to reduce the duplications.

    public XmlAdaptedTimeslot(Timeslot timeslot) {
//        startDate = timeslot.start.toString();
//        endDate = timeslot.end.toString();
        startDate = DateUtilApache.dateAsStringForStorage(timeslot.start);
        endDate = DateUtilApache.dateAsStringForStorage(timeslot.end);
    }

Comment out code?

Test

Where is your test collate file? Or you should work more on writing test code.

yoshi-1224 commented 7 years ago

Noted. thanks for the review