Code review comment for lp:~inkscape.dev/inkscape/pathVectorSatellites

Revision history for this message
Krzysztof Kosinski (tweenk) wrote :

- 2Geom is a separate library, not part of Inkscape. It should only contain code which is useful outside of Inkscape, and the changes should be submitted to 2Geom.
- 2geom/pointwise.h has a dependency on helper/geom.h, which is unacceptable.
- The Pointwise and Satellite classes are poorly undocumented - I don't understand what they are supposed to do. Are they supposed to some store per-node data? Their interface is rather unintelligible.
- The computational complexity comments are misleading. This code is obviously O(N) in the number of satellites and should either use a map or a simple vector, not a vector of pairs of size_t and Satellite. What is the intended purpose?
- The coding style does not match Inkscape's and is in fact inconsistent even within this patchset (some methods are in snake_case, some local variables are in lowerCamelCase, invalid indent in class definitions, etc.)
- The getters and setters on Satellite are pointless, they could simply be replaced by public fields.
- The way the satellite parameters are stored is very un-XML. I possible, each of the boolean parameters should be a separate attribute.

review: Needs Fixing

« Back to merge proposal