Marcy #16
Marcy #16
Conversation
| var filterVal interface{} | ||
| switch columnKind { | ||
| case "BOOLEAN": | ||
| filterVal, err = strconv.ParseBool(filter[sepIndex:]) |
There was a problem hiding this comment.
Be careful with these methods. If this failed, then filterVal would be false/0 (the default's in go for bool and int is false/0, not nil), and err would be an error. But you aren't checking the err before running the filter, so you wouldn't get the results you expect at this point
| query := datastore.NewQuery("Book") | ||
|
|
||
| if filter != "" { | ||
| splitIndices := regexp.MustCompile("(<=|<|>=|>|=)").FindStringIndex(filter) |
There was a problem hiding this comment.
Be wary of over engineering. This is an impressive bit of code, but it would leave you open to running queries like borrowed > false which doesn't really make much sense. Sometimes simpler is better 😅
There was a problem hiding this comment.
I've just seen the commit message for 3. An incredibly over-engineered way to do Filtering 😄 . As an aside, what would you have done to simplify this? Ie lock it down so you could only filter on the title or author. And it was a string contains match?
There was a problem hiding this comment.
The quickest way to modify the method to only allow strings would be to return a 400 in place of converting the filter string to bool or int.
The simplest code though would be to remove the type checking completely, since before that was added the queries would return nothing if the column type wasn't string (as it would be like doing WHERE Id = "3", which doesn't match the int 3), but that doesn't allow any kind of handling, it just returns empty.
The main reason it's so complicated is because I didn't want to redefine the book model anywhere, so I had to make it dynamically find the type to convert the parameter correctly. If I threw out that requirement (since it's not actually in the exercise), I could just hard code a switch-case statement to check which name is in "fieldName", then either convert based on type or essentially make a passlist that only allows author and title. That would cut 47 lines down to like 12 and be less over-engineered
There was a problem hiding this comment.
Very sensible answer! How about the match being a contains match, rather than an equals?
There was a problem hiding this comment.
Thats fine. Datastore doesn't support contains (or case in-sensitive matches amongst other things). So it was a bit of a trick question (sorry about that)
You would most likely do a Get all from the datastore, then https://pkg.go.dev/strings#Contains check each one in turn. If we know its going to be a small list (100's) and not run often (less than once a minute say) then we would be ok with this brute force approach. If it was more performant, then indexes or tech like big query/elastic search might be more approriate
No description provided.