d-edge / Cardidy

A .net library to identify credit card number and cvv
MIT License
34 stars 13 forks source link

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

Open aloisdg opened 3 years ago

aloisdg commented 3 years ago

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

torendil commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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

aloisdg commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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

torendil commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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