News:

Simutrans.com Portal
Our Simutrans site. You can find everything about Simutrans from here.

Patch: factor out some network code from karte_t::interactive()

Started by ArthurDenture, August 27, 2013, 02:19:24 AM

Previous topic - Next topic

0 Members and 1 Guest are viewing this topic.

ArthurDenture

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.


prissi

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, ... )

kierongreen

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?

ArthurDenture

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...


ArthurDenture

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.

prissi