| Thread Previous • Date Previous • Date Next • Thread Next |
On 6/22/19 10:52 AM, Seth Hillbrand wrote:
On 2019-06-21 22:54, Reece R. Pollack wrote:Doing this now, before we go too far down the path of replacing wxWidgets types with non-OOB arrays would enhance readability and make the code more robust. Using VECTOR2I is going the wrong way.Hi Reece-Codebase cleaning like you suggest can go a long way toward improving sustainability and readability. But done the wrong way, it will have the opposite effect as we fight with return types that aren't fully matched.Before we decide to do this type of deep plumbing on the KiCad innards, I'd love to see the proposed specification for what's being implemented. Otherwise, this'll be a bike shedding discussion.
I've been in far too many "code reviews" where the focus became the naming of variables and not on the algorithm being implemented. So I agree with your comment in principle.
That said, the lack of type specificity already makes portions of our code hard to maintain. The DIALOG_GRAPHIC_ITEM_PROPERTIES class already reuses the m_endX UNIT_BINDER to represent both the X-axis endpoint of an object and the radius of a circle. This requires special handling when performing display origin transforms because the X-axis endpoint requires origin transformation while the circle radius does not. This illustrates the risk of treating everything as "just a pair of numbers" because it's "easy".
I'm not suggesting rushing into a major change. But replacing a descriptive object like wxPoint with a non-descriptive object like VECTOR2I isn't an improvement. Especially when its implementation is somewhat non-intuitive, as Hauptmech points out.
-Reece
| Thread Previous • Date Previous • Date Next • Thread Next |
