nus-cs2103-AY2223S2 / forum

12 stars 0 forks source link

Better exceptions: how far down should you split your exceptions? #239

Closed ChickenChiang closed 1 year ago

ChickenChiang commented 1 year ago

Sorry for the poor wording in the title I can't seem to find the right words, I believe my examples will clarify it

Currently I have a single MissingArgumentException that handles the following issues:

If any of the above issues are found when creating parsing the user input, I throw a new MissingArgumentException(String Message), where the message corresponds to the issue. For example:

private static ToDoCommand buildToDoCommand(String[] words, String userInput)
            throws MissingArgumentException {
        if (words.length < 2) {
            throw new MissingArgumentException(Messages.MESSAGE_DESCRIPTION_MISSING); // <-- Look here!
        }
        return new ToDoCommand(userInput.substring(5));
    }

Would it be better to create exceptions for each of the three issues mentioned instead?

private static ToDoCommand buildToDoCommand(String[] words, String userInput)
            throws MissingArgumentException {
        if (words.length < 2) {
            throw new MissingDescriptionException(); // <-- Look here!
            // the exception will have the error message within it's class, and nothing needs to be passed as and argument
        }
        return new ToDoCommand(userInput.substring(5));
    }

Which method is better? And if the second method is chosen, will there come a point where there are too many exceptions in our project?

Would love to hear your thoughts, thanks in advance!

jiasheng59 commented 1 year ago

From my point of view, it depends on your purpose of using exception and the ease of dealing with more possible exceptions in future. Exception is either shown to user or developer, if it's supposed to be shown to user, e.g. for the purpose of telling user which action is not allowed, then using the former one might be clearer in the sense of telling user what exactly went wrong (As a user, I'll be happier to receive the warning like "Hey, deleting task index beyond existing list is not allowed" instead of "Hey, invalid input"). On the other hand, if it's for debugging purpose of current / future developer, I think it's better to make the exception clear in conveying the situation that causes the exception for the ease of fixing bugs.

Of course, the trade-off is now the code becomes much longer and may even look unnecessary to have so many exceptions (MissingIndexException, MissingDatesException, MissingDescriptionException) just to deal with one type of error (for instance, MissingArgumentException).

Whether you should spend time enhancing your existing feature or organizing your exception handling logic (which is kind of related to your code quality), I believe you have (and are given) the flexibility to decide, at least in this module.

damithc commented 1 year ago

Good question @ChickenChiang and some valid points @jiasheng59

A general advice is 'when in doubt, use the simpler option'. You can always change it to a more sophisticated option later if the simple option is not sufficient.

Relevant https://nus-cs2103-ay2223s2.github.io/website/se-book-adapted/chapters/codeQuality.html#practice-kissing