current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 2 down vote favorite

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 );
}
share|improve this question
    
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. –  glampert Jul 31 at 13:57

1 Answer 1

up vote 1 down vote accepted

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)

share|improve this answer
    
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. –  CodeDreamer Aug 3 at 3:12
    
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 :) –  insidesin Aug 3 at 8:02

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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