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

Suggestion to add a result model instead of [cardtype] #48

Closed
hey24sheep opened this issue Oct 21, 2021 · 4 comments
Closed

Suggestion to add a result model instead of [cardtype] #48

hey24sheep opened this issue Oct 21, 2021 · 4 comments

Comments

@hey24sheep
Copy link
Contributor

hey24sheep commented Oct 21, 2021

Identify function should return a result model class (serializable) instead of an IEnumerable. It will be scalable and better.

A few suggestions, when return card types from Identify function, there are 3 scenarios

1 - Card is invalid or symbols or shady

  • return should be CardType.Invalid

2 - Card is not yet supported, so not found

  • return should be CardType.Unkown or CardType.NotYetSupported

3 - Wrap everything inside Identify with try - catch

  • return CardType.ErrorIdentifying from catch block

To add new type

  • Invalid
  • ErrorIdentifying (this should be instead wrapped by another class instead)

Now, cardtype should only have card types(imo). A new model should be used for Identify result which could have

[Serializable]
IdentifyResult.cs

{
"Card Number": user_inputed numbe
"Identified Card Type" : {result}
"Error Identifying" : // bool? some string error? discuss?
"IsInvalid" : // if do not want to use cardtype for error handling
}

this should also in future could be wrapped in a bulkIdentify function which could have another property "Count".

@aloisdg
Copy link
Member

aloisdg commented Oct 21, 2021

Related to #3

@aloisdg
Copy link
Member

aloisdg commented Oct 21, 2021

I will close this issue to continue this thread on #3. One point though, I think CardType.NotYetSupported should be CardType.NotImplemented to follow NotImplementedException Class.

@aloisdg aloisdg closed this as completed Oct 21, 2021
@hey24sheep
Copy link
Contributor Author

@aloisdg oh, I didn't see the already discussed one. Thanks 👍🏼

@aloisdg
Copy link
Member

aloisdg commented Oct 21, 2021

@hey24sheep no problem it is a deep old one. Glad you share some of our ideas ^^

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants