Code Quality

[ACarlotti]

Originally posted in Oneway Twoway Road Patch for Extended as part of this post. This section is a more general discourse on code quality, both in the context of these changes and the codebase more generally, and about my opinions and aims regarding quality and how they compare with those of THLeaderH, James and the main developers of Standard.


I think I'm broadly in agreement with prissi and Turfit about the quality of the code. For a big software project, it is important that code is made to be as easy to read as possible. Failing to do so means that future developers (including you in the future) will find it harder to work out what the existing code does, slowing down development and making bugs more likely.  Every bit of poorly written code added a project creates a "technical debt". Sooner or later this debt will have to be paid off, whether it's in the additional time spent reading and understanding the code, or the additional time spent fixing bugs that were obscured by the complexity or messiness of the code. As time passes and further changes are made to the code, more and more time is wasted on the technical debt and it becomes harder and harder to fix it later. Many big software products have collapsed (sometimes leading to the collapse of the owning company) due to an accumulation of technical debt making it almost impossible to add new features or fix existing problems without breaking even more things.

Turfit and prissi know this - they have an established project which has been running for over 20 years, and part of the reason it's still going is because a lot of thought has gone into writing the code neatly and in a way which is as easy to understand as is possible. I think this ease of understanding is mentioned explicitly in the coding style guide, possibly more than once.

James, I surmise, is taking a less strict approach, which so far seems to have worked ok so far. However, Extended differs from Standard in that we are not currently producing long-term stable releases, and (as far as I see) the development is much more lead by a single person. Both of these factors make it possible to cope with a bit more technical debt. However, I do wonder if this different ideology is a factor in the large number of signalling bugs that have been arising recently. I suspect if I were to spend some time (probably weeks or more) learning the signalling code I would end up wanting to refactor or rewrite parts of it. I know there are already multiple places in the code where once I've managed to work out what some existing code is trying to do, I've rewritten it to be half the length and more understandable (to me at least, and I hope also to others), and fixed several previously undiscovered bugs in the process.

Anyway, I guess what I'm saying is that if this code were to stay as it is in the codebase for long enough, then I would eventually refactor lots of it to make it easier for future people to read and modify. But in an ideal world I shouldn't have to do that - you should be making an effort to make the code as readable as possible yourself. I spent four hours last night reading the code and noticing (and fixing) some of the issues Turfit mentioned lasted. I did that because it's in master now; I didn't think that would happen with explicitly mentioned issues still present otherwise, so I had assumed (incorrectly) that you'd checked and sorted all the issues mentioned in the Standard thread - otherwise I'd have probably posted something like this before. And I still understand almost nothing about how your code works (the exception being the image refreshing stuff which I rewrote to use existing methods once I was certain that it was just doing the same thing in an unnecessarily different way). However, I know that the changes I've made will make it easier for me or someone else to read and understand your code in the future.

[jamespetts]

Andrew - thank you for your long and thoughtful post, and also for the great amount of work that you have clearly put into fixing the issues and improving the code quality: it is much appreciated.

As to code quality issues more generally: so far, the development of Simutrans-Extended has been largely done by me with others helping on particular projects or sometimes bug fixes ad hoc. My own background is that my day job has nothing to do with coding or IT - I am a barrister - and I am a self-taught amateur (I learnt C++ specifically to work on Simutrans back in 2008 and in some senses am still learning now). The Simutrans code base generally is very old and has a coding style and implementation fitting of its time (the late 1990s), and the original creator of Simutrans himself wrote the project as a way of learning C++, having previously programmed in C. People like Prissi and TurfIt have gone a long way to improve the code over the years in Standard, and I have tried to incorporate these improvements where I can, but 1990s style code is inherently harder to maintain than more modern coding styles.

However, the result of this situation is that there is a chronic defect of time able to be dedicated to work on the code, so maintaining a high quality code base is difficult; plus, I have no experience of professional software development, so perhaps lack the sort of skills and work-flow needed in order optimally to manage contributions. There is also a delicate balance to be drawn between accepting a useful contribution though the code quality may not be the best (no doubt on account of being written by somebody who in many ways be in a similar position to me - a Simutrans enthusiast keen to see additional features but with perhaps limited experience of working on larger projects) and insisting on high quality code. A full code review itself can be time consuming and take away time from other important things, and my own spare time is in any event quite limited.

The signalling code is, I must confess, an example of an erroneous decision on my part about how to handle the project at an early stage. The erroneous decision was to keep as much of the Standard signalling code as possible and build the additional features by adding to that code rather than starting afresh. That seemed to be a sensible idea at the time because one of the signalling methods - cab signalling - was to work in more or less exactly the same way as the old signalling system, and the others at first glance did not seem radically different, needing just a little extra logic. This has resulted in some rather unfortunate non-modular code in rail_vehicle_t::block_reserver(), which is very difficult to work with because it is difficult to understand fully everything that is going on at once: far more detailed logic was needed for the various aspects of the systems than I realised when first creating this. I had thought that the signalling was mostly working well until the large number of bug reports recently (engendered no doubt by the fact that a good multi-player game always tests the code much more thoroughly than single player games seem to test it), which I have been slowly working through.

I have noticed recently, especially since last year when the nightly builds first became available and "Simutrans-Extended" was adopted in place of the former "Simutrans-Experimental" an increase in regular development contributions, from you (Andrew), Ves (mainly in the GUI as Ves is quite new to C++; but this has greatly helped with the passenger and mail classes feature and is greatly helping with the vehicle maintenance/convoy re-combination features currently in progress, albeit stalled pending resolution of the signalling bugs), and also increasing interest from the Japanese Simutrans community, including Phystam and THLeaderH. This is a happy thing indeed, and I wonder whether any improvements to the workflow/project management might be worthwhile to improve the way that Simutrans-Extended is developed. Perhaps it might be worth opening a separate thread to discuss this?

[Ranran(retired)]

First of all I thank all the simutrans developers who contribute to development by taking your time and I also thank those who have try to decipher my broken sentences.

(Sorry, this post is irrelevant to Oneway Twoway Road Patch.)

The signalling code is

I have not reported some signaling bugs which I noticed yet because I have been busy these days. (´・ω・｀)
There are many bugs in these which do not work properly when turning around at the waypoint or canceling reservation or schedule is changed. For example, replacing what I reported so far from "token block" to "absolute block" or "track circuit block" etc. I think these bugs are very similar.
I was afraid that these fixing would be a Cat-and-mouse game.
I am sorry for my poor English comprehension, but James are you trying to make a major renovation of the signaling code they may improve?
If so I will withhold these bug reports and wait until the signaling code modification is done.
However I am very grateful for your achievement. Thank you.

[jamespetts]

We are straying a little off topic here, so I will reply only very briefly: there are no current plans for a total replacement of the signalling code. That would be a project of truly gargantuan scale and might well take years, during which no other progress could be made. It is not at all clear that that would be the most efficient use of my extremely limited time. Instead, I need to work through the existing bug reports one by one to try to address each specific reported issue.

[ACarlotti]

Just a couple of things. Of course, I appreciate that time and experience is limited, and that Simutrans Extended is being developed by a few enthusiasts, and not by a team of trained (and paid) developers, which does somewhat limit what we can ask for. I think there are two concrete things that I would like to see done differently in the future.
1. Where a feature is being developed for and is wanted by Standard, but is currently waiting to be incorporated due to known bugs or issues with the code quality, then I think we should wait for it to be ready for Standard before including it (or perhaps help get it ready for Standard before porting it). This means that we don't merge in a load of known bugs, and also in the long run will involve less work in getting the feature incorporated in both Standard and Extended
2. Check that changesets don't make unrelated changes to whitespace/layout elsewhere in the codebase. I think the only exceptions to this should be if the changes are being made in Extended-only code and the commit message mentions this in some way, or if the changes are in line with changes made in Standard (as is the case with some of the changes I'm importing at the moment). Otherwise they just make the version history more confusing, and introduce unnecessary differences between the Extended and Standard codebases.

In this particular case, as an alternative for waiting for THLeaderH or someone else to improve this code for Standard, I could have considered working to improve the quality of the code and version history in the patch before its inclusion in Extended. (Probably entailing some careful squashing/rebasing of commits to simplify history and remove the whitespace changes, followed by some new commits to improve the code quality, although that still doesn't entirely account for backporting it to Standard.)

Incidentally, I have been glancing over (to varying degrees) all the code changes going into Extended (and also Standard), so might be spotting some issues that way. Other issues can be fixed fairly easily with testing. So I think the main things that should be picked up by some sort of review before making it to master, on the basis that fixing them afterwards is difficult/impossible, are those mentioned above that make version history harder to follow or that make it harder to keep Standard and Extended in sync.

(P.S. I think you should split this discussion into another topic - perhaps if you can copy the second half of my previous post into the start of a new topic 'Code Quality', and move most of the subsequent posts with it.)

[jamespetts]

Thank you for your suggestions - that is helpful. These do seem to be sensible suggestions and should probably be adopted. I wonder whether it is worth having a set of guidelines somewhere to help us to remember these things and help others know the way in which contributions will be treated?

As to splitting the topic, I am afraid that it is somewhat difficult to split parts of posts rather than whole posts; but perhaps we might start a new topic regarding code quality and guidelines?
Edit: Thanks to A. Carlotti's work in disentangling his post, I have now been able to split this so that we can discuss code quality more generally here.