| Thread Previous • Date Previous • Date Next • Thread Next |
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.
-Reece
| Thread Previous • Date Previous • Date Next • Thread Next |
