Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Should we returns meaningful error or should we keep it simple, stupid? #3

Open
aloisdg opened this issue Sep 16, 2021 · 10 comments
Open
Labels
enhancement New feature or request question Further information is requested

Comments

@aloisdg
Copy link
Member

aloisdg commented Sep 16, 2021

Hello,

Instead of an empty list maybe we could return a Result of data. Where Result can contains an error type (like Luhn check error or length error) or a all good with data something like

enum ResultCode {
    Ok,
    LengthError, // card with 50 digits
    ValidationAlgorithmError, // luhn fail
    ImplementationError, // unkown card
    etc...
}

class Result {
    public ResultCode ResultCode { get; }
    public CardType Card { get; }
}

This could be nice in the front-end to show message to the end user like "This card seems to be a visa be the length does not match. A visa should have 13 or 16 numbers."

image

Of course we wont handle the message, but this issue will provide the tool to unlock the feature to developers

Thoughts? Would it be useful?

cc @4gq @fighou @torendil

@aloisdg aloisdg added enhancement New feature or request good first issue Good for newcomers question Further information is requested labels Sep 16, 2021
@aloisdg aloisdg changed the title Returns meaning full error Should we returns meaningful error or should we keep it simple, stupid? Sep 17, 2021
@torendil
Copy link
Collaborator

As discussed yesterday, I believe it would be beneficial to output the type of error encountered. By the way, I think instead of a simple case we could output several in case you have an electron Visa

@aloisdg
Copy link
Member Author

aloisdg commented Sep 20, 2021

That's right! So the output would be a IEnumerable<Result<T>>. For example "4026320594033" would returns (in pseudo json):

[
  {
    "issuingNetwork": "visaElectron",
    "resultCode": "LengthError"
  },
  {
    "issuingNetwork": "visa",
    "resultCode": "Ok"
  }
]

Is it what you were thinking?

@torendil
Copy link
Collaborator

torendil commented Sep 20, 2021

I don't think it's useful to output all networks with their associated error codes, but besides that, yes

@aloisdg
Copy link
Member Author

aloisdg commented Sep 20, 2021

Not all networks but all the matching one by IIN ranges. It could even be a nullable value when resultCode is null/undefined then there is no error (in this case we will have to rename resultCode into errorCode).

@torendil
Copy link
Collaborator

Ok as a result seems good for me.
The result by IIN range is skewed if the client mistakes on the first letter. We could do with a score that shows how close you are from a valid card and has a threshold at ~70% and only display those?
In any case, displaying an error message will lead to questions such as that. Another possibility would be to return only the matching patterns (useful in the case of the electron VISA for instance)

@aloisdg
Copy link
Member Author

aloisdg commented Sep 21, 2021

I think the percent way is going to be arbitrary. I am not in favor of that. As per #2, the first one is the most plausible one. We follow the enum's order (which is a bit, well, a bit arbitrary too, but less).

As a library I don't really want to take this kind of decision for the developer. They can still wrap the api with their own logic if they want to.

@ytyukhnin
Copy link
Member

It would be better to have something meaningful as a return type instead of Result, for example having a Card class.

@torendil
Copy link
Collaborator

torendil commented Oct 4, 2021

This is also linked to issue #5, I believe it's better to return an object rather than an enum, but I have yet to draft a complete answer as to why for @thinkbeforecoding. I had a tough week ;)

@hey24sheep
Copy link
Contributor

In response to my #48, return of empty is fine as long as the reason for empty is given along with it like (not found or invalid input). Otherwise empty return is a swallowed error for user. Reading at this, I feel a lot has already been discussed. Glad to be a part of this ^^

@aloisdg
Copy link
Member Author

aloisdg commented Oct 21, 2021

@hey24sheep Glad to have you as a contributor. This is a on going discussion and your input is more than welcome :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request question Further information is requested
Projects
None yet
Development

No branches or pull requests

4 participants