Validate if the statement ID is of the right format #500
Conversation
|
Hi, I have fixed a few tests as of now. Once the validate function is approved, I will commit the fixes for the rest of the cases. Please let me know what you think. Thank you. |
| throw new TypeNotPresentException("Query,Property or Lexeme", null); | ||
|
|
||
| if(!statementParts[1].matches(statementFormat)) | ||
| throw new IllegalStateException("Statement part does not have a correct format"); |
There was a problem hiding this comment.
The indentation of this line is inconsistent with the rest. Also, I do not think IllegalStateException is appropriate: this should only be used when you detect that some data structure has reached an inconsistent state. To validate arguments, IllegalArgumentException is the one to use.
| */ | ||
| public static boolean validateStatementID(String statementID) | ||
| { | ||
| if(statementID.equals("")) |
There was a problem hiding this comment.
Please always use curly braces with if to avoid ambiguities.
| * @return true if the statement ID is of the valid format | ||
| */ | ||
| public static boolean validateStatementID(String statementID) | ||
| { |
There was a problem hiding this comment.
Please put curly brace in the same line like all the code base.
| * @param statementID | ||
| * @return true if the statement ID is of the valid format | ||
| */ | ||
| public static boolean validateStatementID(String statementID) |
There was a problem hiding this comment.
If you raise an exception if the ID is not valid, then returning a boolean is not useful. If the function returns, then you aready know the ID is valid.
| return true; | ||
|
|
||
| String separator = "\\$"; | ||
| String statementFormat ="^\\{?[A-Za-z0-9]{8}-[A-Za-z0-9]{4}-[A-Za-z0-9]{4}-[A-Za-z0-9]{4}-[A-Za-z0-9]{12}\\}?$"; |
There was a problem hiding this comment.
We will validate millions of IDs. I belive it would be better to precompile the pattern using Pattern.compile and store it in a final static class field.
| if(statementParts.length != 2) | ||
| throw new IllegalArgumentException("Statement ID does not have the correct number of parts"); | ||
|
|
||
| if(statementParts[0].charAt(0) != 'Q' && statementParts[0].charAt(0) != 'P' && statementParts[0].charAt(0) != 'L' && statementParts[0].charAt(0) != 'M') |
There was a problem hiding this comment.
There are actually more types (forms, senses, media-info...). You could use EntityIdValueImpl.guessEntityTypeFromId that will does the same thing and supports all types.
|
@kanikasaini do you have the intention to come back to this PR? |
Resolves #166