1
\$\begingroup\$

I have the following class for 2D points:

class Point {
private:
    double _x, _y;

public:
    Point(double x, double y) : _x(x), _y(y);

    double x() const { return _x; }
    double y() const { return _y; }
};

I wanted to add a few comparison functions that compared objects of this class in different ways.

One of them is just a simple function:

/// Returns whether A has a smaller y-coordinate than B
/// (and smaller x-coordinate in case of equality).
bool yComp(const Point &A, const Point &B) {
    if (A.y() != B.y()) return A.y() < B.y();
    return A.x() < B.x();
}

The other is a struct with operator() defined on it:

struct angleComp {
    Point origin;
    angleComp(const Point &P);
    bool operator()(const Point &A, const Point &B);
};

// Constructs a new angle comparison with P as origin
angleComp::angleComp(const Point &P) : origin(P) {}

// Compares according to the angles that the lines joining each point
// to a given origin form with the x-axis.
bool angleComp::operator()(const Point &A, const Point &B) {
    GeometricVector u = A - origin, v = B - origin;
    return u.x()*u.x()*u.sqrNorm() < v.x()*v.x()*v.sqrNorm();
}

Do I put the comparisons inside the class definition? The first function could be made a static method, but the problem is I can't put angleComp inside the class (as a static member) since one of its fields is of type Point itself, which would be an incomplete type (inside the class declaration). And I would prefer these comparison functions to be grouped together (instead of one being a static member, and the other being a top-level thing).

So, an alternative would be to put both outside the class, but to retain some grouping, putting all of them in a namespace:

namespace PointComp {
    bool yComp(const Point &A, const Point &B);

    struct angleComp {
        Point origin;
        angleComp(const Point &P);
        bool operator()(const Point &A, const Point &B);
    };
}
\$\endgroup\$

closed as off-topic by user673679, Toby Speight, Mast, яүυк, Cody Gray Mar 9 at 4:56

This question appears to be off-topic. The users who voted to close gave this specific reason:

  • "Lacks concrete context: Code Review requires concrete code from a project, with sufficient context for reviewers to understand how that code is used. Pseudocode, stub code, hypothetical code, obfuscated code, and generic best practices are outside the scope of this site." – user673679, Toby Speight, яүυк
If this question can be reworded to fit the rules in the help center, please edit the question.

  • 1
    \$\begingroup\$ The code is about 2D points. On Code Review, we mainly classify questions according to the task accomplished by the code rather than by your primary concern about the code, because feedback on any aspect of the code is fair game for answers. \$\endgroup\$ – 200_success Mar 7 at 21:42
  • 1
    \$\begingroup\$ No, Code Review reviews concrete code. Answers will suggest better practices, but asking for best practices in general is both off-topic and redundant. \$\endgroup\$ – 200_success Mar 7 at 21:48
  • 4
    \$\begingroup\$ Answers will talk about whether how well your code follows best practices and patterns. Questions should not ask about best practices and design patterns, since they are implicit topics for every review. Also, if your question is specifically about a best practice or design pattern (and not a general critique of your code), then it should go on Software Engineering instead. \$\endgroup\$ – 200_success Mar 7 at 22:23
  • 2
    \$\begingroup\$ It's easy to misread the page like that, I agree. But that's not what it says. 200 is absolutely correct here. \$\endgroup\$ – Mast Mar 8 at 14:08
  • 2
    \$\begingroup\$ Feel free to make a suggestion on our Meta site if you have a suggestion on how to avoid confusion. \$\endgroup\$ – Mast Mar 8 at 16:07
3
\$\begingroup\$

The overall design seems a bit complicated to me. Setters and getters are often more of an hindrance than a real asset, and the benefits of having a simple struct should not be under-estimated. In particular, it reminds you that you don't have to encapsulate everything, and that defining functions at the point of use is often the most convenient way to proceed:

struct Point { float x, y; };

// ...

// here I need to compare points by y and then x
auto best = std::max_element(v.begin(), v.end(), [](auto point_a, auto point_b) {
    return std::tie(point_a.y, point_a.x) < std::tie(point_b.y, point_b.x);
    //      -> tie gives you the lexicographic comparison for free
});

// and there by angle with etc.
auto widest = std::max_element(v.begin(), v.end(), [origin](auto point_a, auto point_b) {
    // ...
});

In a nutshell, why having a namespace and custom classes where you can simply use a lambda?

\$\endgroup\$

Not the answer you're looking for? Browse other questions tagged or ask your own question.