Author Topic: Clean Code  (Read 1506 times)

Offline leeor_net

  • Administrator
  • Hero Member
  • *****
  • Posts: 2350
  • OPHD Lead Developer
    • LairWorks Entertainment
Clean Code
« 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);
}

Offline Hooman

  • Administrator
  • Hero Member
  • *****
  • Posts: 4954
Re: Clean Code
« Reply #1 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.

Offline leeor_net

  • Administrator
  • Hero Member
  • *****
  • Posts: 2350
  • OPHD Lead Developer
    • LairWorks Entertainment
Re: Clean Code
« Reply #2 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... which aside from the very obvious mistake is also genuinely bad code altogether.
« Last Edit: August 25, 2018, 12:47:15 PM by leeor_net »