0
\$\begingroup\$

My Rectangle class contains one vertex (x,y), its width a, and its height b; the opposite vertex will be (x+a,y+b). I had a task to take 2 rectangles and compute the minimal 3rd rectangle to contain 2 smaller ones. How can I simplify this code?

Rectangle operator+(Rectangle &o){
    double x1 = x, x2 = o.Gx(), y1 = y, y2 = o.Gy(), a1 = a, a2 = o.Ga(), b1 = b, b2 = o.Gb();
    double retx, rety, reta, retb;
    if (x2 > x1){
        retx = x1;
        if (x1 + a1 <= x2)
            reta = a1 + a2 + x2 - (x1 + a1);
        else if (x1 + a1 > x2 + a2)
            reta = a1;

        else
            reta = x2 + a2 - x1;
    }
    else{
        retx = x2;
        if (x1 <= x2 + a2)
            reta = x1 + a1 - x1;
        else if (x2 + a2 > x1 + a1)
            reta = a2;
        else
            reta = a1 + a2 + x1 - (x2 + a2);
    }
    if (x1 == x2 && a1 > 0 && a2 > 0){
        retx = x1;
        if (a1 > a2)
            reta = a1;
        else
            reta = a2;
    }

    if (y2 > y1){
        rety = y1;
        if (y1 + b1 <= y2)
            retb = b1 + b2 + y2 - (y1 + b1);
        else if (y1 + b1 > y2 + b2)
            retb = b1;

        else
            retb = y2 + b2 - y1;
    }
    else{
        rety = y2;
        if (y1 <= y2 + b2)
            retb = y1 + b1 - y1;
        else if (y2 + b2 > y1 + b1)
            retb = b2;
        else
            retb = b1 + b2 + y1 - (y2 + b2);
    }
    if (y1 == y2 && b1 > 0 && b2 > 0){
        rety = y1;
        if (b1 > b2)
            retb = b1;
        else
            retb = b2;
    }
    return Rectangle(retx,rety,reta,retb);
}
\$\endgroup\$

closed as unclear what you're asking by Toby Speight, Dannnno, Graipher, t3chb0t, Donald.McLean Jan 26 '18 at 13:48

Please clarify your specific problem or add additional details to highlight exactly what you need. As it's currently written, it’s hard to tell exactly what you're asking. See the How to Ask page for help clarifying this question. If this question can be reworded to fit the rules in the help center, please edit the question.

  • 3
    \$\begingroup\$ Please provide the code for the rest of the Rectangle class and a minimal example program demonstrating it so that other users can compile and run the program. This will help with the review process. \$\endgroup\$ – Null Jan 23 '18 at 23:16
  • 1
    \$\begingroup\$ Could a and b be negative? \$\endgroup\$ – vnp Jan 23 '18 at 23:36
5
\$\begingroup\$

For the moment, I'm assuming a typical graphics system with the origin at the top left. At the very least, the variable names I've used depend upon that to really make any sense. Anyway, I believe the logic you want can be embodied in code something like this (please forgive the sexist variable names):

Rectangle operator+(Rectangle &o) {
    double my_left = std::min(x, x + a);
    double his_left = std::min(o.x, o.x + o.a);
    double left = std::min(my_left, his_left);

    double my_right = std::max(x, x + a);
    double his_right = std::max(o.x, o.x + o.a);
    double right = std::max(my_right, his_right);

    double my_top = std::min(y, y + b);
    double his_top = std::min(o.y, o.y + o.b);
    double top = std::min(my_top, his_top);

    double my_bottom = std::max(y, y + b);
    double his_bottom = std::max(o.y, o.y + o.b);
    double bottom = std::max(my_bottom, his_bottom);

    return Rectangle(left, top, right - left, bottom - top);
}

If you can be sure that a and b are non-negative, you can simplify that quite a bit to just:

Rectangle operator+(Rectangle &o) {
    double left = std::min(x, o.x);
    double right = std::max(x + a, o.x + o.a);
    double top = std::min(y, o.y);
    double bottom = std::max(y + b, o.y + o.b);

    return Rectangle(left, top, right - left, bottom - top);
}

As for the rest of the code, I see two obvious things. First of all, the variable names aren't particularly helpful. I guess I can sort of keep things like x1 and x2 straight, but only just.

Second, the logic strikes me as almost a complete mess. I guess I can understand what it needs to do, and if I was willing to spend enough time on it I could verify that it works correctly--but it's not immediately obvious to me at first glance, and it would take some fairly exhaustive (and exhausting) analysis to be sure that it works for every possible case, not to mention being sure exactly how it works.

There's also one final caveat I feel obliged to add: an IEEE floating point number can be set to a NaN, which means all comparisons to that value will return false (even if (a == a) will yield false). If either of your rectangles contains a NaN, all of these will produce garbage results.

\$\endgroup\$
  • \$\begingroup\$ Assuming C++14 or later, those 3-line stanzas can be simplified using the std::min() that takes an initializer list. E.g. auto left = std::min({x, o.x, x+a, o.x+o.a}); etc. It probably makes more sense to provide some methods so we could just write std::min(left(), o.left()) &c, since they are probably useful for other code, too. \$\endgroup\$ – Toby Speight Jan 24 '18 at 9:28
  • \$\begingroup\$ @TobySpeight: quite possibly--but hard to be at all sure from looking at this code in isolation. Assuming the rest of the code is sane at all, the second version is what I'd normally expect to see. \$\endgroup\$ – Jerry Coffin Jan 24 '18 at 14:23
  • \$\begingroup\$ Agreed - there's not enough code for a good review (and I'm impressed with this answer built on very little). We might also mention std::minmax() FWIW. I haven't given consideration to how the standard algorithms behave when given NaNs - I find it easiest to declare that "not a rectangle" and exit early (perhaps via an is_valid() member). I'm flagging the question as "unclear" until we can see a definition of Rectangle. \$\endgroup\$ – Toby Speight Jan 24 '18 at 14:39

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