← Back to team overview

kicad-developers team mailing list archive



Whoops, hit "send" too soon. "Is a simple array" is obviously incorrect. My point was the lack of descriptiveness is a problem. The absence of differentiation between a location and a distance is a missed opportunity. And the abuse of the type as a loosely-related pair of values should be considered a bug.

On 6/21/19 10:54 PM, Reece R. Pollack wrote:
A while back, Jeff emailed me a note suggesting that KiCad was trying to decouple from the wxWidgets type wxPoint except where needed in the UI code. He suggested that I should use VECTOR2I instead.

I understand and support the desire to decouple from wxWidgets. However, looking at the implementation of vector2d.h I was surprised to discover that VECTOR2I is a simple array of /int/ like I would find in an old Fortran program. It's not even a C structure, let alone a well-defined C++ object.

One of the goals of object-oriented programming is to create descriptive objects. Programmers can tell what an object represents by looking at the name and implementation of the object. Well-crafted object help avoid programming errors that could result from performing undefined operations or using data from incompatible objects. In geometry, if we subtract the 2d location of one point from the 2d location of another point the result is a 2d vector describing the distance between these points. However, this vector is not a point. In OOP this vector should be a different object than a point.

The wxWidgets objects wxPoint and wxRealPoint are descriptively named, though poorly implemented. Aside from where code abuses these, they clearly represent locations.  Replacing these with non-OOB typedefs such as VECTOR2I removes the clarity that a variable is intended to represent a location. The name is not descriptive of a location. There is no sanity check to prevent performing a nonsensical operation like adding two locations. It tells the programmer nothing about whether a variable contains a location, a length, or just two loosely-related values.

Rather than replacing wxPoint with VECTOR2I, let us consider how we can make our code more object-oriented rather than more Fortran-like. I believe we should replace wxPoint and wxRealPoint with KiCad-specific objects. These objects should be named such that they clearly indicate that they of represent locations rather than a simple array of integer values. They should be implemented such that nonsensical operations are prohibited and sanity checks put in place where possible. For example, attempting to add two locations should result in a compile-time error. Additional objects should be defined to represent the delta between two locations, and the operations of adding a delta to a location should result in a location.

Just for discussion, let's assume the replacement for wxPoint is named KiPoint. The result of subtracting two KiPoint objects would be another object called KiDelta. Adding two KiPoint objects should be undefined, and result in a compile-time error, as this is a nonsense operation.  Adding a KiPoint and a KiDelta should return a KiPoint. We should never abuse a KiPoint to represent anything else, such as using the X value as the radius of a circle as is currently done. I suspect most of the places where this would be much more than a mechanical substitution is a place where the code abuses the type or is a latent bug.

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.


Mailing list: https://launchpad.net/~kicad-developers
Post to     : kicad-developers@xxxxxxxxxxxxxxxxxxx
Unsubscribe : https://launchpad.net/~kicad-developers
More help   : https://help.launchpad.net/ListHelp

Follow ups