2
\$\begingroup\$

Recently I've written some code which implements snap-to-edge functionality for some windows.

There is some code duplication as you can see. I think this code can be simpler or possibly improved.

// If 'rtParent' and 'rtChild' are closed each other as 'ENABLE_STICK_VALUE', return true;  
auto IsNearEachOther = []( const QRect& rtParent, const QRect& rtChild ) -> std::tuple< bool, QPoint>
{
    bool isNear = false;
    QPoint ptMoveTarget;

    // If 'rtChild's left position is closing to 'rtParent's right position.
    if ( rtChild.left() > rtParent.right() )
    {
        if ( ( ( rtChild.left() - rtParent.right() ) <= ENABLE_STICK_VALUE )
            && ( rtChild.top() >= ( rtParent.top() - ENABLE_STICK_VALUE ) )
            && ( rtChild.top() <= ( rtParent.bottom() + ENABLE_STICK_VALUE ) ) )
        {
            isNear = true;
            ptMoveTarget = QPoint( rtParent.right(), rtParent.top() );
        }
    }

    // If 'rtChild's right position is closing to 'rtParent's left position.
    if ( rtChild.right() < rtParent.left() )
    {
        if ( ( ( rtParent.left() - rtChild.right() ) <= ENABLE_STICK_VALUE )
            && ( rtChild.top() >= ( rtParent.top() - ENABLE_STICK_VALUE ) )
            && ( rtChild.top() <= ( rtParent.bottom() + ENABLE_STICK_VALUE ) ) )
        {
            isNear = true;
            ptMoveTarget = QPoint( rtParent.left() - rtChild.width() , rtParent.top() );
        }
    }

    return std::make_tuple( isNear, ptMoveTarget );
};

QRect rtParent = m_pParentChatRoom->geometry();
QRect rtCurrent = geometry();

bool result = false;
QPoint ptMoveTarget;
std::tie( result, ptMoveTarget ) = IsNearEachOther( rtParent, rtCurrent );
if ( result )
{
    move( ptMoveTarget );
}
Improve this question
\$\endgroup\$
1
  • \$\begingroup\$ That function is large for a lambda. Regardless of being some inner function inside some other function, it is probably time to make it a proper member of some class or an ordinary namespace-level function. \$\endgroup\$
    – glampert
    Jul 31 '15 at 13:57
1
\$\begingroup\$

Definitely extract method for the following code:

if ( rtChild.right() < rtParent.left() )
{
    if ( ( ( rtParent.left() - rtChild.right() ) <= ENABLE_STICK_VALUE )
        && ( rtChild.top() >= ( rtParent.top() - ENABLE_STICK_VALUE ) )
        && ( rtChild.top() <= ( rtParent.bottom() + ENABLE_STICK_VALUE ) ) )
    {
        isNear = true;
        ptMoveTarget = QPoint( rtParent.left() - rtChild.width() , rtParent.top() );
    }
}

Where you can pass through a left/right variable however you want and then change:

rtParent.left() - rtChild.width() to rely on the given parameter, falling back onto rtParent.right()

The same can be said for any other small changes in the code, such as

rtChild.right() < rtParent.left() versus rtChild.left() > rtParent.right()

I'm not going to give you the exact solution, but that if condition holds most of the repeated code and can be brought out into something like snapTo(right/left)

Improve this answer
\$\endgroup\$
2
  • \$\begingroup\$ Thank you for your answer. I've accept your answer, however I've altered the code using other api, such as QRect::adjusted(), QRect::contains(). If you have some spare time, I want to you check my altered code, and listen to your thought. Thank you for your time. \$\endgroup\$
    – hyun
    Aug 3 '15 at 3:12
  • \$\begingroup\$ Yeah that looks nice. Very readable. I don't have much time to got through it, but it definitely is an improvement, granted it works how you wanted to still :) \$\endgroup\$
    – insidesin
    Aug 3 '15 at 8:02

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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