Outpost Universe Forums

Off Topic => Computers & Programming General => Topic started by: leeor_net on August 25, 2018, 11:08:20 AM

Title: Clean Code
Post by: leeor_net on August 25, 2018, 11:08:20 AM
So... I've been working on OPHD again and I started building the new UI code. After sleeping on it for a night I realized that I was falling into an old trap:

Code: [Select]
void MainReportsUiState::onMouseDown(EventHandler::MouseButton button, int x, int y)
{
    if (button == EventHandler::BUTTON_LEFT)
    {
        for (Panel& panel : Panels)
        {
            if (isPointInRect(MOUSE_COORDS, panel.Rect))
            {
                panel.Selected = true;
            }
            else
            {
                panel.Selected = false;
            }
        }
    }
}

Could be much more effectively written as:

Code: [Select]
void MainReportsUiState::onMouseDown(EventHandler::MouseButton button, int x, int y)
{
    if (button == EventHandler::BUTTON_LEFT)
    {
        for (Panel& panel : Panels)
        {
            panel.Selected = isPointInRect(MOUSE_COORDS, panel.Rect);
        }
    }
}

And of course for those who hate the curly braces:

Code: [Select]
void MainReportsUiState::onMouseDown(EventHandler::MouseButton button, int x, int y)
{
    if (button == EventHandler::BUTTON_LEFT)
        for (Panel& panel : Panels)
            panel.Selected = isPointInRect(MOUSE_COORDS, panel.Rect);
}

And for the extreme minimalist:

Code: [Select]
void MainReportsUiState::onMouseDown(EventHandler::MouseButton button, int x, int y)
{
    if (button == EventHandler::BUTTON_LEFT) for (Panel& panel : Panels) panel.Selected = isPointInRect(MOUSE_COORDS, panel.Rect);
}
Title: Re: Clean Code
Post by: Hooman on August 25, 2018, 12:18:00 PM
A classic  ;D


And for those who don't hate curly braces, but do want more vertical density:
Code: cpp [Select]

void MainReportsUiState::onMouseDown(EventHandler::MouseButton button, int x, int y) {
    if (button == EventHandler::BUTTON_LEFT) {
        for (Panel& panel : Panels) {
            panel.Selected = isPointInRect(MOUSE_COORDS, panel.Rect);
        }
    }
}


The curly braces do help guard against common mistakes, such as revisiting code a long time later to add or remove a line, and thus breaking a loop or if statement because they only apply to the next statement. And also, indentation might lie, but curly braces don't.
Title: Re: Clean Code
Post by: leeor_net on August 25, 2018, 12:45:20 PM
That's why I enforce the braces even for something like if(expression) { return; } -- helps to avoid humiliating mistakes like Apple's goto fail bug (https://en.wikipedia.org/wiki/Unreachable_code#goto_fail_bug)... which aside from the very obvious mistake is also genuinely bad code altogether.