AY1920S2-CS2103T-W17-2 / main

Notably
https://ay1920s2-cs2103t-w17-2.github.io/main/
MIT License
3 stars 4 forks source link

Auto Correction Engine #9

Open ljiazh3ng opened 4 years ago

ljiazh3ng commented 4 years ago

Handled by 1 person.

This epic deals with the following:

kevinputera commented 4 years ago

Proposed API:

EDIT:

/**
 * Represents the status of the correction.
 */
enum CorrectionStatus {
  UNCHANGED, // If the `actual` string supplied to the `correct` method is already a valid one
  CORRECTED, // If the `actual` string supplied to the `correct` method is altered (corrected)
  FAILED, // If the `actual` string supplied to the `correct` method is too far away each of the `possibilities` supplied
}

/**
 * Represents the result of a correction.
 */
interface CorrectionResult<T> {
  CorrectionStatus getCorrectionStatus();
  Optional<T> getCorrectedItem(); // When status is `FAILED`, this will return `Optional.empty()`. Otherwise, this will return the corrected item
}

/**
 * Represents the correction engine instance.
 * This should be the class you interact with when doing corrections.
 */
interface CorrectionEngine<T> {
  CorrectionResult<T> correct(T actual);
}

@johannagwan @firzanarmani @ljiazh3ng guys please let me know if this works for you!

johannagwan commented 4 years ago

Thanks Kevin for the suggestion! I have some clarifications:

Thank you!

kevinputera commented 4 years ago
  • When the CorrectionStatus is FAILED, do you also wanna consider displaying a statement stating something along the line of "No results found."? As a user, I think it's clearer when the GUI shows this kind of statement. You may consider creating a new method for this.

Yup, that's why I introduced the CorrectionStatus enum rather than just using a boolean value for that! I won't need to implement another method for this; instead, you can add this message in your SuggestionModel (in a way, the error message is also a suggestion), in case you got a CorrectionStatus.FAILED when calling the CorrectionEngine's correct() method. What do you think?

  • Just a small typo, possiblilities should be possibilities

Ah right, thanks for catching! I'll update the comment.

johannagwan commented 4 years ago

Please correct me if I'm wrong, but what I understand from reading your API is you would just return Optional.empty() when the CorrectionStatus is FAILED without any message. So the "No results found" message should be put inside my SuggestionModel instead of in your CorrectionModel? Why do you think it's better that way?

Thanks for the clarification, Kevin!

kevinputera commented 4 years ago

Please correct me if I'm wrong, but what I understand from reading your API is you would just return Optional.empty() when the CorrectionStatus is FAILED without any message. So the "No results found" message should be put inside my SuggestionModel instead of in your CorrectionModel? Why do you think it's better that way?

Exactly. Think of it this way:

By design, CorrectionEngine doesn't care (and shouldn't care) about the context in which it's used. You should think of it as being a string matching engine, without any context on its use case for the product we're building. This is important because:

  1. Doing it this way, we can separate: (1) the concern of running an algorithm for string matching (think: a search engine, without any context of the app) to CorrectionEngine, and (2) the concern of creating suggestions (think: this part knows how to respond to the user, as per the product's design) to SuggestionEngine.
  2. By making the CorrectionEngine context-unaware (that means it has no knowledge of product-specific instructions, such as error messages, success messages, etc.), we can ensure that CorrectionEngine and SuggestionEngine are not tightly coupled. Doing so, CorrectionEngine can be easily used by Parser too, without much modification. In addition, as developers, you know that all product-specific instructions, like showing suggestions, showing the "Do you mean?" texts, showing error messages, are part of the SuggestionEngine, while the matching-specific instructions, like the Levenshtein distance algorithm, the configuration for the matching, etc are part of the CorrectionEngine. We now have a clear separation between the modules and thus can work in parallel without much dependency.

If the above is not clear enough, perhaps it helps to think in terms of consistency:

One last thing! As of now, my plan is to make all the CorrectionEngine-related classes exist in the 'Logic' level. That is, there won't be any CorrectionEngine-related models. This is because, in a way, the CorrectionEngine acts more like a utility tool rather than a product-specific function (similar to the difference between a JSON parser and an AddressBook model, for example).

firzanarmani commented 4 years ago

If the above is not clear enough, perhaps it helps to think in terms of consistency:

  • If we were to ask CorrectionEngine to return the "No results found" response text, shouldn't we ask CorrectionEngine to also return the "Do you mean?" response text and the suggestion list for the sake of consistency (CorrectionEngine should return a response text whether or not it fails)?
  • If that's the case, then don't you think that the boundary between the CorrectionEngine and SuggestionEngine becomes unclear?

One last thing! As of now, my plan is to make all the CorrectionEngine-related classes exist in the 'Logic' level. That is, there won't be any CorrectionEngine-related models. This is because, in a way, the CorrectionEngine acts more like a utility tool rather than a product-specific function (similar to the difference between a JSON parser and an AddressBook model, for example).

I agree with this. This gives @johannagwan the flexibility to decide which to give priority to (correction or suggestion) when displaying the results in the dropdown.

On my end, I feel this API will be sufficient as a utility tool (as you mentioned) so far. What about you @ljiazh3ng ?

kevinputera commented 4 years ago

I agree with this. This gives @johannagwan the flexibility to decide which to give priority to (correction or suggestion) when displaying the results in the dropdown.

Yep, totally agree 👍

On my end, I feel this API will be sufficient as a utility tool (as you mentioned) so far. What about you @ljiazh3ng ?

Awesome, thanks!

johannagwan commented 4 years ago

Sorry for the late reply. Thank you for the super clear explanation @kevinputera @firzanarmani!