The International Simutrans Forum

 

Author Topic: Patch: factor out some network code from karte_t::interactive()  (Read 2218 times)

0 Members and 1 Guest are viewing this topic.

Offline ArthurDenture

  • Coder/patcher
  • *
  • Posts: 86
This just comes from me trying to understand how the main loop works :-). I was able to split out about 150 lines of standalone command processing from karte_t::interactive() in simworld.cc and move it into its own functions.

In theory, it's just a refactoring, without any behavioral changes.

I've pushed the change to https://github.com/artdent/simutrans/tree/interactive, and it's also attached as a patch.

Update: found one more tiny function to extract. Pushed as another commit to the same place.
« Last Edit: August 27, 2013, 02:46:07 AM by ArthurDenture »

Offline kierongreen

  • Dev Team, Coder/patcher
  • Devotee
  • *
  • Posts: 2346
Re: Patch: factor out some network code from karte_t::interactive()
« Reply #1 on: August 27, 2013, 07:37:07 AM »
Seems tidier :)

Offline prissi

  • Developer
  • Administrator
  • *
  • Posts: 10822
  • Languages: De,EN,JP
Re: Patch: factor out some network code from karte_t::interactive()
« Reply #2 on: August 27, 2013, 03:07:39 PM »
Personally I am not so fond of splitting functions called only a single time. But it seems the trend, and kieron like it so it may be a good idea.

Honestly, karte_t need badly a split between the timing and interaction and the working on the geometry (init, rotation, hills, ... )

Offline kierongreen

  • Dev Team, Coder/patcher
  • Devotee
  • *
  • Posts: 2346
Re: Patch: factor out some network code from karte_t::interactive()
« Reply #3 on: August 27, 2013, 07:27:37 PM »
I'm not fond of overly splitting functions myself - however where a function is extremely long it does make it clearer what each part does. Also there is one part where ArthurDenture has removed duplicate code which is always good in my book...

distribute_groundobjs_cities could also be naturally split into groundobjs and cities. You think splitting the file itself as well is also a way forward then prissi?

Offline ArthurDenture

  • Coder/patcher
  • *
  • Posts: 86
Re: Patch: factor out some network code from karte_t::interactive()
« Reply #4 on: August 28, 2013, 04:19:17 AM »
Functions are more understandable when they are reasonably short, do one thing, and have relatively few dependencies. It becomes easier to figure out the function's beginning state, end state, and side effects are. Functions that have a single purpose also are easier to move around. For example, I agree with you that karte_t is pretty huge and has lots of pieces that don't have much to do with each other -- see what parts of it have to do with the network code without having to understand all of the 400-line interactive() function at once. Large complicated functions make it it harder to tell what data actually depends on what other data.

As for the ambient sound logic in my second patch, if you have a large block of code that exists to set exactly one variable, extracting that into a function allows you to potentially write tests for that function. Which doesn't exist yet but is achievable one piece at a time.

I didn't even look at distribute_groundobjs_cities, but yeah, that's another hefty one...

Offline prissi

  • Developer
  • Administrator
  • *
  • Posts: 10822
  • Languages: De,EN,JP
Re: Patch: factor out some network code from karte_t::interactive()
« Reply #5 on: September 07, 2013, 09:45:43 PM »
Incorporated.

Offline ArthurDenture

  • Coder/patcher
  • *
  • Posts: 86
Re: Patch: factor out some network code from karte_t::interactive()
« Reply #6 on: September 08, 2013, 03:17:53 AM »
Yay, thanks!

Did you also want to include the second one in the series that moves ambient sound calculation into its own function? I've attached it here. Sorry, I should have explicitly attached it the first time around instead of just mentioning its existence on the branch.

Offline prissi

  • Developer
  • Administrator
  • *
  • Posts: 10822
  • Languages: De,EN,JP
Re: Patch: factor out some network code from karte_t::interactive()
« Reply #7 on: September 08, 2013, 10:23:34 PM »
Also this is in, thank you