News:

Do you need help?
Simutrans Wiki Manual can help you to play and extend Simutrans. In 9 languages.

Code maintainability

Started by Mariculous, June 13, 2021, 11:55:37 AM

Previous topic - Next topic

0 Members and 1 Guest are viewing this topic.

Mariculous

I just started to refactor way construction and destruction related code to improve maintainability in the future.
Please do not yet pull this, but feel free to note any eventual issues with the code.
https://github.com/irgend42/simutrans-extended/pull/new/way-construction-refactoring

jamespetts

Quote from: Freahk on June 13, 2021, 11:55:37 AM
I just started to refactor way construction and destruction related code to improve maintainability in the future.
Please do not yet pull this, but feel free to note any eventual issues with the code.
https://github.com/irgend42/simutrans-extended/pull/new/way-construction-refactoring

Thank you for this. Reading a code refactoring without an explanation is generally very difficult - can you give an overview of what you are doing here to give some assistance?
Download Simutrans-Extended.

Want to help with development? See here for things to do for coding, and here for information on how to make graphics/objects.

Follow Simutrans-Extended on Facebook.

Mariculous

#2
A brief description can be found in the commit history

Quote
32fb582
way_builder::calc_costs refactoring:
- reuse forge cost calculation
- reuse tree/groundobj removal cost calculation
- improve tree/groundobj removal code (do not iterate the obj list from the start for each single tree/groundobj)

Quote
2baafff
removed code duplication of get_tree_remove_costs and remove_trees

That's, besides minor changes, all it does so far.
1. There had been a function to calculate forge cost, but way_builder::calc_costs used a copy of that code instead of calling the function.
2. There had been a function to calculate the costs of groundobj and tree removal and actually removing those.
  Further, there was different code in way_builder::calc_costs to calculate these costs without removing the obects.
  I used the visitor pattern to share that code.
  That code is based on the code from way_builder::calc_costs instead of the code from remove_trees() because I considered it to be the better implementation.

Minor changes are replacing NULL by nullptr, moving curly braces fromm its own line to the previous one, setting actually const parameters const as well as changes in karte_t::get_forge_cost.
These are:
- moving a condition out of the loop that does not depend on the iteration
- removing an unneccessary is_within_grid_limits check (lookup_kartenboden has its own check for limits and will return null if not in limits)

jamespetts

I worry that this sort of refactoring ends up making the Extended codebase differ from the Standard codebase in places where the functionality is identical or very similar, something which I have always tried to avoid because of how much more difficult that it makes merging desirable changes from the Standard codebase into Extended.

Can I check the extent to which this is likely to be an issue with this patch? If so, it is probably worth submitting it for Standard first. If we are refactoring Extended specific code, we have much more freedom.
Download Simutrans-Extended.

Want to help with development? See here for things to do for coding, and here for information on how to make graphics/objects.

Follow Simutrans-Extended on Facebook.

Mariculous

The forge cost part shouldn't be an issue at all. As far as I know forge costs are an extended feature.
The different approach of remove_trees could be backported to standard. Then, the code difference around tree/groundobj removal shouldn't be an issue.

My intention was to first prepare the rafactoring in extended, then prepare a related refactoring in standard.
That's because some of those refactorisations are quite useful in extended but might not be needed in standard.

jamespetts

Quote from: Freahk on June 13, 2021, 02:13:01 PM
The forge cost part shouldn't be an issue at all. As far as I know forge costs are an extended feature.
The different approach of remove_trees could be backported to standard. Then, the code difference around tree/groundobj removal shouldn't be an issue.

My intention was to first prepare the rafactoring in extended, then prepare a related refactoring in standard.
That's because some of those refactorisations are quite useful in extended but might not be needed in standard.

Ahh, I see, yes. As long as we don't drift further out of sync over time, all should be good, and, yes, indeed, the forge cost is an Extended feature.
Download Simutrans-Extended.

Want to help with development? See here for things to do for coding, and here for information on how to make graphics/objects.

Follow Simutrans-Extended on Facebook.