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

Modify the return value type of canAuthenticate() #25

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

JJson
Copy link

@JJson JJson commented Dec 13, 2018

Modify the return value type of canAuthenticate() to verify that the device and settings are available before authenticating

@rushisangani
Copy link
Owner

@JJson I don't think all these steps are required for canAuthenticate() method.
Just true or false return is fine because anyways user needs to handle all those failure cases.
It would be repetitive handling for authenticateWithBioMetrics() method.
and handling these failure cases in authenticateWithBioMetrics() makes better sense than canAuthenticate() method.

let me know your thoughts.
thanks.

@JJson
Copy link
Author

JJson commented Dec 14, 2018

I think it is better to distinguish the type of error before authentication (for example, the device is not supported, the settings are not authorized, etc.) and the errors in the authentication process (such as fingerprint errors, face errors, etc.).

In my application, the process is like this: first check if the device supports, whether the permission is enabled, if not, prompt the user to open, and then initiate authentication. So I need to call canAuthenticate() first. If there is an error, the prompt needs to give a specific reason, so I need canAuthenticate() to return the specific error type.

@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

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

Successfully merging this pull request may close these issues.

2 participants