4 ms·

To be fair, the first step towards refactoring is understanding the existing code -- ideally, knowing everywhere it is used, all of its behaviors, and important

by iaaan 5y ago

To be fair, the first step towards refactoring is understanding the existing code -- ideally, knowing everywhere it is used, all of its behaviors, and importantly, its history, so that you don't break anything, and so that you don't reintroduce bugs that have already been fixed over the years. Or, in lieu of all that, a robust automated test suite. This cannot be done with a file containing 28k lines of code. That is an insurmountable task. They may as well have been asked to start from scratch and build a new engine. I'm curious what the purpose of this ritual was. Was it just hazing, or was the thought that someone might actually be able to accomplish this?

lupire 5y ago
A 28Kloc file can be modular.
hulahoof 5y ago
I would expect people new to the code base would look at this gigantuan file and ask why it hasn’t been refactored, and this probably was seen as an opportunity to familiarise new hires with the code base as well as get them on the same level as to the effort involved in a refactor. I agree with you, a rewrite is probably how they should have tackled this one.
z3t4 5y ago
It is possible to write tests in a single-file program. You could for example have a -test flag that runs all the tests when the program starts. It's never too late to introduce tests - the next bug you fix, start by writing a test that detects the bug, then fix the bug - and confirm that the bug is fixed by running the test. Then you never have to worry about a bug you already fixed will show up again. And the tests will build up to a decent test suit over time. The next step is to also write a new test for each new change or feature. Keep doing this and your program will soon have full test coverage. The trick is to not test small units - instead write the tests as if a user was using the program, so that the actual tests covers a lot of code all over the place, not just the code you added. Mock key presses, and button presses. Then when a user reports a bug, you can write the test so that it repeats the user actions. What makes testing hard is a lot of side effects, like the program writing to databases or calling external API's. Not the LOC of the source file. You might want to mock an automatic call to the fire department for a fire control program, but for API calls and databases, just have the test write to the prod environment, but include rollback/cleanup in the test. That way you don't need a separate testing environment.
- branko_d 5y ago
  > but for API calls and databases, just have the test write to the prod environment, but include rollback/cleanup in the test I disagree with that. Automated testing should be done on a test database. Holding a lock for too long could effectively block the entire production. This could happen while debugging through a test (e.g. by hitting a breakpoint or by just stepping line-by-line). Or by simply having a bug in the test causing a "transaction leak" - depending on the tech stack, this could keep the transaction and the associated locks alive until all tests have finished running, not just the one which had the bug. Or you could commit instead of rollback by mistake. Or you could simply put unexpected strain on the database, affecting the performance of real users.
  - z3t4 5y ago
    It can be a lot of work creating an isolated dev/test environment if you already have a large app that communicate with a lot of services. But if you can that is preferable as the test will create strange artifacts - but those systems is probably someone elses job to make sure they dont get corrupted ;)
cookiengineer 5y ago
> Was it just hazing, or was the thought that someone might actually be able to accomplish this? Actually no. Most devs that just got started familarizing themselves with the codebase wanted to refactor the file and came up with the idea themselves. Usually they thought this is a crappy file and this must be an easy task to do because they saw all the nested if/elseif/else statements in the code. The problem, architecture-wise, was that the road logic was the glue code that integrated a lot of different parts, layers, and NPC behaviours from the rest of the codebase as it was changing the surrounding game world. If there was a hospital placed with a non-squared ground tile next to it, if it was placed with a 1 offset (roads were 3x3 tiles), if it was placed with a 2 offset next to another road... It went as far as influencing the path heatmap that was necessary for the A* guessing algorithm to make the NPCs walk correctly on the sidewalks. The permutations of possible sidewalks alone were enough complexity on their own... So in a lot of ways necessary features that historically had no place in the Entity/Component based engine at some point made it in there. The next best thing (and also spaghetti code) was the Cursor Entity, which had to have line tracing algorithms to be able to select things that are visible under a donut-like shape when the user was hovering the hole, or say, a tree in the game world. Convex and Concave shapes were integrated, and lots of edge cases in there, too, which are actually huge mathematical problems in terms of available performance once you dig more into it, so we ended up with binary height map sprites that helped both the slicing and the cursor at some point. The important lessons learned from the road logic were very valueable for newbies, as it was teaching the practical problems of isometric game worlds. So afterwards everyone was able to grasp why the complexity was added, and what was necessary to remove it (in the sprints in the future). At some point we decided a couple of things because of the road logic and cursor entity for new iterations of the engine, like: - always use a 1x1 road tile - always use square based tiles for all objects - dont make sidewalks, use just road tiles - dont make trees with holes in their leaves - dont make trees higher than the buildings - no artist can ever request crosswalks. Never ever. ...etc

by iaaan 5y ago

lupire 5y ago

A 28Kloc file can be modular.

hulahoof 5y ago

I would expect people new to the code base would look at this gigantuan file and ask why it hasn’t been refactored, and this probably was seen as an opportunity to familiarise new hires with the code base as well as get them on the same level as to the effort involved in a refactor. I agree with you, a rewrite is probably how they should have tackled this one.

z3t4 5y ago

It is possible to write tests in a single-file program. You could for example have a -test flag that runs all the tests when the program starts. It's never too late to introduce tests - the next bug you fix, start by writing a test that detects the bug, then fix the bug - and confirm that the bug is fixed by running the test. Then you never have to worry about a bug you already fixed will show up again. And the tests will build up to a decent test suit over time. The next step is to also write a new test for each new change or feature. Keep doing this and your program will soon have full test coverage. The trick is to not test small units - instead write the tests as if a user was using the program, so that the actual tests covers a lot of code all over the place, not just the code you added. Mock key presses, and button presses. Then when a user reports a bug, you can write the test so that it repeats the user actions. What makes testing hard is a lot of side effects, like the program writing to databases or calling external API's. Not the LOC of the source file. You might want to mock an automatic call to the fire department for a fire control program, but for API calls and databases, just have the test write to the prod environment, but include rollback/cleanup in the test. That way you don't need a separate testing environment.

branko_d 5y ago
> but for API calls and databases, just have the test write to the prod environment, but include rollback/cleanup in the test I disagree with that. Automated testing should be done on a test database. Holding a lock for too long could effectively block the entire production. This could happen while debugging through a test (e.g. by hitting a breakpoint or by just stepping line-by-line). Or by simply having a bug in the test causing a "transaction leak" - depending on the tech stack, this could keep the transaction and the associated locks alive until all tests have finished running, not just the one which had the bug. Or you could commit instead of rollback by mistake. Or you could simply put unexpected strain on the database, affecting the performance of real users.
- z3t4 5y ago
  It can be a lot of work creating an isolated dev/test environment if you already have a large app that communicate with a lot of services. But if you can that is preferable as the test will create strange artifacts - but those systems is probably someone elses job to make sure they dont get corrupted ;)

cookiengineer 5y ago

> Was it just hazing, or was the thought that someone might actually be able to accomplish this? Actually no. Most devs that just got started familarizing themselves with the codebase wanted to refactor the file and came up with the idea themselves. Usually they thought this is a crappy file and this must be an easy task to do because they saw all the nested if/elseif/else statements in the code. The problem, architecture-wise, was that the road logic was the glue code that integrated a lot of different parts, layers, and NPC behaviours from the rest of the codebase as it was changing the surrounding game world. If there was a hospital placed with a non-squared ground tile next to it, if it was placed with a 1 offset (roads were 3x3 tiles), if it was placed with a 2 offset next to another road... It went as far as influencing the path heatmap that was necessary for the A* guessing algorithm to make the NPCs walk correctly on the sidewalks. The permutations of possible sidewalks alone were enough complexity on their own... So in a lot of ways necessary features that historically had no place in the Entity/Component based engine at some point made it in there. The next best thing (and also spaghetti code) was the Cursor Entity, which had to have line tracing algorithms to be able to select things that are visible under a donut-like shape when the user was hovering the hole, or say, a tree in the game world. Convex and Concave shapes were integrated, and lots of edge cases in there, too, which are actually huge mathematical problems in terms of available performance once you dig more into it, so we ended up with binary height map sprites that helped both the slicing and the cursor at some point. The important lessons learned from the road logic were very valueable for newbies, as it was teaching the practical problems of isometric game worlds. So afterwards everyone was able to grasp why the complexity was added, and what was necessary to remove it (in the sprints in the future). At some point we decided a couple of things because of the road logic and cursor entity for new iterations of the engine, like: - always use a 1x1 road tile - always use square based tiles for all objects - dont make sidewalks, use just road tiles - dont make trees with holes in their leaves - dont make trees higher than the buildings - no artist can ever request crosswalks. Never ever. ...etc