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

Enum valueForString methods are not consistent #91

Closed
mikeburke106 opened this issue Jan 30, 2015 · 1 comment · Fixed by #118
Closed

Enum valueForString methods are not consistent #91

mikeburke106 opened this issue Jan 30, 2015 · 1 comment · Fixed by #118
Labels
best practice Not a defect but something that should be improved anyway bug A defect in the library REVIEW - scheduled The reviewer has scheduled a review of this PR for the current sprint

Comments

@mikeburke106
Copy link
Contributor

See PR #82 for more details.

Most enum classes simply use the name() method to define the value of the JSON value for each particular enum value. Therefore, the valueForString method typically calls and returns the result of the valueOf() method.

If valueForString is called with a JSON value that is not contained in the enum, the valueOf method will cause an IllegalArgumentException.

However, in enum classes that define their own internal name to represent the JSON value, the valueForString method loops through all enum values and returns the value if it's found. If it's not found, it will return null.

All enums should behave identically in this case, so we need to determine if the correct course of action is to throw a RuntimeException (causing a crash) or to return null.

@mikeburke106 mikeburke106 added bug A defect in the library best practice Not a defect but something that should be improved anyway labels Jan 30, 2015
@mikeburke106
Copy link
Contributor Author

Ok, so let me put this in a form of a question. When we get an invalid JSON enum value as part of a message from SDL core and call valueForString() on the enum class in question, do we expect:

  1. Return null - it will be as if the value was omitted from the JSON string, but app runs as normal
  2. Throw IllegalArgumentException - app will crash with Runtime exception

Similarly, what should happen if we pass in null to valueForString()?

  1. Return null - it will be as if the value was omitted from the JSON string, but app runs as normal
  2. Throw NullPointerException - app will crash with Runtime exception

This case "should never happen", but in case it does, I'm of the opinion that we should return null instead of crashing the application. Does anyone else have an opinion?

This would involve re-writing the valueForString() methods to look something like this:

public static AppInterfaceUnregisteredReason valueForString(String value) {
    try{
        return valueOf(value);
    }
    catch(Exception e){ //IllegalArgumentException or NullPointerException
        return null;
    }
}

@mikeburke106 mikeburke106 mentioned this issue Feb 18, 2015
5 tasks
@mikeburke106 mikeburke106 self-assigned this Feb 18, 2015
mikeburke106 pushed a commit that referenced this issue Feb 18, 2015
Signed-off-by: Mike Burke <[email protected]>
@mikeburke106 mikeburke106 added the REVIEW - scheduled The reviewer has scheduled a review of this PR for the current sprint label Feb 20, 2015
mikeburke106 pushed a commit that referenced this issue Feb 23, 2015
Signed-off-by: Mike Burke <[email protected]>

Conflicts:
	sdl_android_lib/src/com/smartdevicelink/proxy/rpc/enums/VrCapabilities.java
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
best practice Not a defect but something that should be improved anyway bug A defect in the library REVIEW - scheduled The reviewer has scheduled a review of this PR for the current sprint
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant