Some code comments

[joto]

I am just looking at the code with focus on the use of vtzero. Here are some things:

• https://github.com/mapbox/vtquery/blob/master/src/geometry_processors.hpp#L12 Why the check? Don't we want to reserve if count == 1?
• https://github.com/mapbox/vtquery/blob/master/src/geometry_processors.hpp#L29 can be const
• Consider rewriting the geometry processors to create the mapbox::geometry types internally instead of taking them by reference and then returning it through the result() function moving the result into query_geometry. (Look for result in https://github.com/mapbox/vtzero/blob/master/doc/reading.md#geometries)
• If you declare the copy/move constructors, always also declare copy/move assignment! https://github.com/mapbox/vtquery/blob/master/src/vtquery.cpp#L46-L50
• Comment is unclear: https://github.com/mapbox/vtquery/blob/master/src/vtquery.cpp#L49 If copy doesn't work anyway, you dont' need to specify the constructor anyway. But in this case I think what is meant is: "This is a huge object, make sure it never gets copied."
• Don't write comments that don't tell the reader anything more than the code already does. https://github.com/mapbox/vtquery/blob/master/src/vtquery.cpp#L33 . This just makes the code longer and it can easily get out of sync with the code when changes are made later.
• https://github.com/mapbox/vtquery/blob/master/src/vtquery.cpp#L134 creates an unnecessary std::string. Something like this should work: { const auto svalue = value.string_value(); return Nan::New<v8::String>(svalue.data(), svalue.size()).ToLocalChecked();
• The execute() function is too long. https://github.com/mapbox/vtquery/blob/master/src/vtquery.cpp#L163-L294 Consider refactoring it, for instance by moving this part into its own function. Then comments like these https://github.com/mapbox/vtquery/blob/master/src/vtquery.cpp#L278-L280 are not needed any more.
• Consider making https://github.com/mapbox/vtquery/blob/master/src/vtquery.cpp#L31 a std::vector<vtzero::property>. It is unclear what https://github.com/mapbox/vtquery/blob/master/src/vtquery.cpp#L262-L268 is actually bying us. There are all these conversions to std::string` why?
• Use the type defined in https://github.com/mapbox/vtquery/blob/master/src/vtquery.cpp#L31 in https://github.com/mapbox/vtquery/blob/master/src/vtquery.cpp#L35 and https://github.com/mapbox/vtquery/blob/master/src/vtquery.cpp#L55 .
• What does the comment in https://github.com/mapbox/vtquery/blob/master/src/vtquery.cpp#L35 tell me? If I don't understand what an rvalue is, that comment is probably not going to help...
• ResultObject, TileObject and QueryData are just structs. Consider making the code more object oriented and turn them into real classes. For instance this line https://github.com/mapbox/vtquery/blob/master/src/vtquery.cpp#L190 is somewhat confusing and parts of it at least could be a method on QueryData called find_layer_by_name() or so. Maybe utils::convert_vt_to_ll (https://github.com/mapbox/vtquery/blob/master/src/vtquery.cpp#L256) should be a method of TileObject? It already takes three parameters from the TileObject.

[mapsam]

Thanks @joto! Sorry I totally missed this. Mind tagging @mapsam next time? I'll respond to your comments below from the perspective of #42 landing soon.

all comments about geometry_processors.cpp

src/geometry_processors.cpp has been removed in place of vector-tile's 2.x branch - so these should be cleared up.

Don't write comments that don't tell the reader anything more than the code already does

Regarding some comments be unnecessary, I hear you. I've been learning a lot with this library, and have been pairing with @flippmoke and writing notes as code comments. They are helpful to me! I'll remove them before a release, though. Regarding the rest of your comments on the code comments, it's safe to assume they'll be removed. They are very much for me, not intended for a release or anyone else.

https://github.com/mapbox/vtquery/blob/master/src/vtquery.cpp#L134 creates an unnecessary std::string. Something like this should work: { const auto svalue = value.string_value(); return Nan::Newv8::String(svalue.data(), svalue.size()).ToLocalChecked();

We now have a property_value_visitor and this is no longer the case, unless vector-tile does this internally. https://github.com/mapbox/vtquery/blob/vector-tile-2x/src/vtquery.cpp#L131-L158

The execute() function is too long

👍 I'll work on breaking things out. Introducing vector-tile slimmed it down a bit, but we can do better.

Consider making https://github.com/mapbox/vtquery/blob/master/src/vtquery.cpp#L31 a std::vectorvtzero::property. It is unclear what https://github.com/mapbox/vtquery/blob/master/src/vtquery.cpp#L262-L268 is actually bying us. There are all these conversions tostd::string` why?

Use the type defined in https://github.com/mapbox/vtquery/blob/master/src/vtquery.cpp#L31 in https://github.com/mapbox/vtquery/blob/master/src/vtquery.cpp#L35 and https://github.com/mapbox/vtquery/blob/master/src/vtquery.cpp#L55 .

This is now a mapbox::feature::properties_map.

Consider making the code more object oriented and turn them into real classes.

I like this a lot, will work on it!

---

My todos

• Turn ResultObject, TileObject and QueryData into real classes. For instance this line https://github.com/mapbox/vtquery/blob/master/src/vtquery.cpp#L190 is somewhat confusing and parts of it at least could be a method on QueryData called find_layer_by_name() or so. Maybe utils::convert_vt_to_ll (https://github.com/mapbox/vtquery/blob/master/src/vtquery.cpp#L256) should be a method of TileObject? It already takes three parameters from the TileObject.
• clean up code comments - make them helpful for more than just myself
• clean up ResultObject - If you declare the copy/move constructors, always also declare copy/move assignment! https://github.com/mapbox/vtquery/blob/master/src/vtquery.cpp#L46-L50
• make the Execute() function smaller by breaking pieces into functions

[mapsam]

@joto a majority of these have been updated and are now in master. I'm going to refrain from turning the structs into classes for now, since we are planning on breaking things out into more modules over at #14 (prior art mapbox/vtvalidate#14)

Last up is to clean the code comments!

[mapsam]

Comments cleaned up in #60