snippetswiftMinor
Check if objects have coordinates
Viewed 0 times
checkcoordinateshaveobjects
Problem
Here's a function:
It checks if objects have a latitude or a longitude. If they don't, they will be passed to the
Does this function look good to you? Should I avoid side effects in my function?
func checkIfObjectsHaveCoordinates() {
PFQuery(className: "Products").findObjectsInBackgroundWithBlock {
(objects: [PFObject]?, error: NSError?) -> in
if error == nil && objects != nil {
for object in objects! {
if object["Latitude"] == nil || object["Longitude"] == nil {
self.convertObjectLocationToCoordinates(object)
}
object.pinInBackground()
}
} else {
print(error)
}
}It checks if objects have a latitude or a longitude. If they don't, they will be passed to the
convertObjectLocationToCoordinates function. Either way, I save them locally, and this is why I think my function should't have side effects. I feel like this code is hardcoded in some way:if myObject["Latitude"] == nil || myObject["Longitude"] == nil || myObject["Latitude"] as! Float == 0 || myObject["Longitude"] as! Float == 0Does this function look good to you? Should I avoid side effects in my function?
Solution
What constitutes an effect and what constitutes a side-effect may be a matter of framing expectations. The real problem, I think, is that the function name fails to convey what it actually does. A more appropriate name might be something like
If you take my naming suggestion, then pinning products is the effect, and ensuring that they all have coordinates is the side-effect. That's much better than
I'm not quite comfortable with the error handling. I think that rearranging it would clarify both branches of the conditional.
pinProductsInBackgroundWithCoordinates(), because:- "check if objects have coordinates" only vaguely describes what this function is doing. What if it does have coordinates — then what? And if it doesn't, then what?
- If the query succeeds, then wouldn't all of the objects end up having coordinates?
- It looks like
.pinInBackground()is going to be called for all of the objects.
If you take my naming suggestion, then pinning products is the effect, and ensuring that they all have coordinates is the side-effect. That's much better than
checkIfObjectsHaveCoordinates(), in which case both actions would be considered side-effects.I'm not quite comfortable with the error handling. I think that rearranging it would clarify both branches of the conditional.
if error != nil {
print(error)
} else if objects != nil {
for object in objects! {
…
}
}Code Snippets
if error != nil {
print(error)
} else if objects != nil {
for object in objects! {
…
}
}Context
StackExchange Code Review Q#110481, answer score: 2
Revisions (0)
No revisions yet.