Comment 3 for bug 681934

Revision history for this message
Nicolai Hähnle (nha) wrote :

I like the approach of your patch.

Two things I would change in the logic:
1) In the first hunk, change if (trans_steps == 2) to if (trans_steps <= 2) in the interest of robustness.
2) In the same hunk, clamp priority to be >= 0 (or change get_transfer_priority() so that it returns an uint32_t and clamps appropriately). The reason is that your assumption that m_target_building->get_priority() returns something >= 2 might be changed in the future.

Also, to keep the style consistent, please don't use lines like
if (...) do_something();
Instead, put the code in the if() on its own line:
if (...)
    do_something();

Please move variable declarations into the innermost scope where they are used, and as far down as possible. I'm looking at the first hunk: it makes no sense to declare trans_steps and req outside the if-block. Also, you should generally initialize variables within the statement that declares them, unless that is impossible for some reason.

As for your remark about Requests, items that are simply moving into a warehouse because they are not needed should not have an associated Request.

Also for the record, you can push your changes as a bzr branch to Launchpad and then submit a Merge Request. I believe that that would be more in line with the intended workflow.