You may not like it, but this is peak programming. A level of perfection attainable only by undergoing the rite of writing a single if statement with 100+ conditions.
I'm a second-year CS student that is still learning in this realm. Is there a general way that most developers would rewrite lines 5261-5284? My assumption is I would look to find some rules I can apply to simplify the code but I'm also aware a CS course is a bit of a bubble and what I've learnt so far might not be the way things are handled in industry.
For example, line 5284 has a 8 inequality operators checking every 4th element (140. 144, 148,...). A single "blocks[1][y][x] % 4 != 0" would remove them all. There also appears to be 3 main segments in that huge block of code (111-118, 119-126, 137-168) which would allow it to be simplified.
A second question: Why is there no comments? Is this common?
It's frighteningly common, and more likely if only 1 developer is working on something. CS students comment far too much, but I would expect a monstrous chain like this to have at least -some- documentation.
One of the best refactoring tools is naming. Arlo Banshee wrote and interesting article about the subject some time ago (http://arlobelshee.com/good-naming-is-a-process-not-a-single...). This is a really long post, but the overall theme is iterate over code until everything has a name. To apply this to your question, the first thing you would do is take those lines, put it in a function with a bad name like “doesSomething” then keep iterating until someone can actually read the code and it makes sense.
In the beginning of learning to program it’s easy to get stuck on the “clever” side of programming, but a lot of the time it can remove readability. The earlier you learn this, the more your future coworkers will appreciate you.
My first major project (at an internship) had all the comments on every file (even getters / setters, and it even had getters/setters because someone told me to). Interestingly enough it was a project that had to read static analysis tools' output and combine it into a single report / webapp, so I was very aware of all the output that was reported and all the niggly little things that these tools (think PMD, CPD, FindBugs, etc) pointed out. Including how a getter wasn't commented.
As mindfulgeek mentions, doing some sort of extract method refactor to slop a name onto it can be a good first step. Then do some sleuthing thru other number name pairings, inserting names in place of the numbers, until you have a solid grasp of what the names should actually be. Feathers in Working Effectively With Legacy Code brings up a concept called seams... u can insert a seam to map to objects and then do object manipulations with polymorphism.
Iirc that particular bit of code is the power logic. You can file an issue on my fork if ys like me to write more on this when im not on a broken phone.
I don't have time to go insane today, so I have tried not to look at the code directly. But, "hundreds of distinct cases" would cause me to reach for a data-directed method using a lookup table or other decision-tree data structure.
The way I'm reading this code is that the cases aren't that distinct. Look at lines 4367 to 4390 and you see 23 distinct cases that could be handled by a range and Mod 3.
It appears the author is well aware of this issue! From the readme:
"Back when I was first learning to program in Java, I decided to try to make a clone of the excellent PC game Terraria. Of course, I was convinced that my version would have many more features than the official one.
But before I realized how silly that idea was, I produced 11,000 lines of, to date, the most atrocious code I have ever seen in my life. I make it available here mostly as a cautionary tale of what can happen if you don't pay attention to the quality of your code. (Lesson learned, in my case!) Here are some of the highlights..."
I wouldn’t ever want to work with this code, but there’s a level of persistence here that I kind of respect. Sure, the code sucks, but I can’t help but feel like I couldn’t have written this even if I wanted to. There’s usually a point when I’m too deep in a nested conditional to remember all the variables, and that’s usually before 23 tabs in.
Yeah. This is the code of a very smart person who didn't take the time to make abstractions, either through laziness or naivete. I've seen much more deeply-wrong code that would be much harder to refactor into something maintainable than this would be.
Yes, this kind of "naive coding technique" occasionally pops up, and I think it's a good discussion topic, because its mere existence demonstrates that you can get extremely far in terms of delivering features with a minimal amount of abstraction: "sometimes all you need is a function."
Further, that the code probably would not really be hideously miserable for a more experienced coder to refactor(it's just very duplicated) shows that copy-paste is reasonable for deferring some kinds of architectural decisions. It's not what you would bias towards if you have a whole team pounding away at the same file and rapidly evolving it, but as a solo practitioner who wants to get the feature out and put it away for the day, this style is convenient for letting the code as a whole develop some maturity and functionality before it gets refactored with abstraction.
But isn't this the beauty of programming to begin with. Barring certain things like medical devices, cars, etc., where it has to be very orthodox, for stuff like Web back/frontends, and even gaming, this is how people learn, I guess. Sure, that code could be re-factored for speed, cleaner in appearance, etc.
Reminds me of the video driver (forget card) some Linux developers wrote that was ~30k lines of code. OpenBSD wanted to use it because the HW manufacturer wouldn't release the specs, but because the Linux code was GPL, the OpenBSD guys re-wrote it from scratch in less than 1/4 of the code.
About 12 years ago I was working at a company in which a developer had recently left, and left behind some code that had a bug in it that needed fixing, the code was processing some XML and I knew a lot about XML so I was called over to look at this code.
The code was actually very simple, it consisted of for loops over a bunch of element nodes in which child nodes had to be compared with child nodes in other element nodes, or in which attribute nodes needed to be compared with other attribute nodes, whenever one of these nodes that needed to be compared with other nodes was found a new for loop would be started to loop over the whole set of nodes once again to find the attribute or element with children that needed to be compared with the currently selected attribute or element. I am describing this so succinctly and clearly for you so that you may experience a deeper understanding of just how succinct and clear the code itself was.
Whenever values where found that matched they were added to an array that was later looped over to get the values to actually doing something with them. They had printed out the oode, it was nearly 100 pages - I was pretty sure I could have done it in less than 2 pages (it was in C# and I sucked at C#), I mean the actual XML that was being analyzed would be less than 5 pages if printed.
After looking at it for a few minutes I announced that it should be rewritten. The other developers wanted to continue on with it to find the bug. I think they had been driven insane.
on edit: by less than 2 pages I mean that it might be more than 1 page but I suppose not 2 full pages.
Reminded of a time I took the code of our Data Scientists (2 physicists that "coded". They decided to make their own ORM in Ruby (called "Mondongo") instead of using one available. No wonder, the code was a real mess...
For example, they needed to get a numeric value from a dropdown box that got several text options. The way they did it was:
and so on with all the options. And in theory they looked for a 0, 1, 2... etc.
Another WTF was that, when we told them we would be upgrading from Ruby 1.8 to 2.0, they told us that they were not sure, because 2.0 did not work that well... As it happens, at some point in the code they were using the .object_id property of a boolean value and testing it == 2 or 0. Well... ruby 2.0 made true.object_id == 20 , so things did not work as expected.
I have actually done a similar WTF myself when I had been programming professionally for about a year, I had to do a validation on SOAP requests for some EU project in ASP in 1999-2000 (thankfully only one of the acronyms in that statement are still relevant) and the parameters for doing it had changed a couple days before.
There was a requirement that we had to deliver all validation code in VBScript that would run in the ASP but also should handle possible future changes to the request, and I had examples of all valid possible requests (which as I said had just been changed a couple days before), so I wrote an XSLT that went over all the XML templates and generated a bunch of VBScript if statements and for loops for each possible valid request and if it didn't find it returned an error message that it was invalid.
So anyway based on the theory that I can understand any stupid coding wtf I myself have done - if the wtf code is generated by some other better code I understand why the code is so awful, but if it is handwritten I still don't understand. And my guy handwrote his code. I'm guessing yours too. crazy.
Terraria extensively used God objects with hundreds or thousands of properties, and used naive C# serialization for its netcode. This meant that sending the state of a single object would take over 1KB, leading to truly outrageous network requirements for a 2D platformer game, on the order of >100KB/s/player.
The local variable names look like the code was decompiled with ILSpy.
All the magic numbers probably (hopefully?) were constants in the original code.
The crazy indentation looks like the decompiler just doesn't use "else if".
The goto's were just old ILSpy versions being bad at control flow reconstruction.
But neither the C# compiler nor the ILSpy decompiler moves code to different methods/classes (only the JIT does stuff like inlining), so yes it's really a single enormous class handling every possible item in the game via cascading if statements.
Yes it's decompiled, so the chains of if statements would likely be switches instead, but basic things like an object having thousands of properties are exactly as the programmer wrote it.
Honestly the only insane thing is that it's all in one file. Could've also been broken up into more functions (it seems like this person didn't actually want to be using Java), but it's reasonably named and formatted and they did a decent job separating out static data at the top instead of littering it throughout the code.
Oh, and that stream of comments by polytomous. That's... something.
This kind of thing in particular is really interesting:
public static void print(String text) {
System.out.println(text);
}
This person cares enough about brevity to make little wrapper functions like this, but at the same time everything is very procedural and monolithic. This gives a vague, weird sense of refined taste, despite some obvious problems with the artifact as a whole.
I'm going to go out on a limb and guess that the programmer came from a C background (where brevity tends to be valued and abstractions tend not to be) and used Java because it's cross-platform.
That's not bad, actually. It's one reason why I hated leaving Perl for Python. Python is a little too orthodox sometimes.
What I'm seeing is where frameworks are becoming the new "programming languages". People now bicker over what framework to use rather than first master the language they are using for a task. I tend to favor the method of "do the simplest thing that works".
I can understand that slightly. Back in college when I was doing a lot of Java if the IDE hadn't had a shortcut to auto complete System.out.println() I probably would have either made it as a macro or defined a function like that in a library (or in a block I would paste in every single project if we're being completely honest).
That Polytomous is a magnificent troll. They acknowledge the code is awful in their fork's README. Also, their prose is much clearer and more comprehensible in the same README. They must have been putting on a character in that thread.
It’s actually pretty great because they’re a person who has worked on the code, acting insane in an issue “Anyone who looks at this code instantly becomes insane”.
If that’s not some very clever trolling, I’m not sure what is.
I think i had a polytomous account on the orange site, but forgot the creds. This is what i settled on in the interim. I dont put on a character, if my acting theory is anything its a derivation of neofuturism.
Even OP is telling them to pump their brakes. Anyone who doesn't have a sense of humor about their younger self's code probably still writes the same quality spaghetti.
> The TerrariaClone.init() method, which is over 1,300 lines long, actually grew so large that the Java compiler started running out of memory trying to compile it! The solution? Copy half of the init() code into a new method, called codeTooLarge(), and call that from init()
I found this in the readme, hilarious (not in a mean way).
Setting up a game world is notoriously imperative and makes every OOP-minded dev rip out their hair trying to do it how they think it should be done before realizing the truth that, for this kind of problem, ugly and imperative is good.
In my estimation, the sin is the cyclomatic complexity.
I've seen tons of code, including code at companies famous for rigorous coding-interview processes to weed out weak programmers, that was far far worse than this. For all its bad properties, this code at least doesn't succumb to over-abstraction, obscure control flows or object-lifecycle rules, or desperate attempts to use every possible combination of language features in a (failed) attempt to look smart. Those things will kill readability and maintainability far more effectively than a bit of linearity or bad variable naming.
I've seen different kinds of comparably atrocious over the years.
This is at least obvious and easy to explain badness... And the lesson was learned.
Some people don't learn it or are forced to ignore it all the time by middle management.
Doesn't look that bad. Could probably be optimized to run faster. Making a Terraria clone is not easy. If you manage to make it run smoothly and its a decent game, no player will complain about the source code.
I have a project from last year where I made my own little pokemon game using processingjs. I thought I had some decent knowledge of developing but the code consists of a file with 1900+ lines of code. It's terrible.. Also because I had barely any knowledge of how I should structure code.
You may not like it, but this is peak programming. A level of perfection attainable only by undergoing the rite of writing a single if statement with 100+ conditions.