4 ms·

The number of atrocities committed in that code are mind numbing -- this is what you get for writing such unmaintainable code.

mturmon 7y ago
Could have been said more kindly, but the point is accurate. This reads like C code that happens to be written in Python. There is a lot of fiddly bit-twiddling operations. The main issue — besides not using 2to3 — is that / in Python3 is interpreted as floating point division, whereas it used to be integer division if operands were integers. Replacing / by // in the relevant code blocks might help? Or getting another implementation of the RNG?
misnome 7y ago
It really isn't that bad. I regularly see much, much worse, and much more unidiomatic python, from people who have gone on to work at FAANG.
strenholme 7y ago
The code base in question is not one I wrote; it is one I decided to maintain years after the original author lost interest in it. [1] This is not too different from what I have seen in multiple jobs, where the new hire doesn’t get to write all the code they use from scratch, but instead has to maintain someone else’s mess. Often times, management and market conditions demand more from programmers than they have time to properly implement with test cases, well structured code, good comments, etc. Technical debt is a big problem in code used by businesses, and the technical debt this particular 6000-line script has is a lot less than some stuff I have seen and have been paid to maintain. The fact of the matter is that most open source code out there is quite a bit cleaner and maintainable than the code which is behind firewalls, running important businesses. [1] Edit: To the original author’s credit, this particular code base is an incredibly popular and successful map generator for Civilization 4, with some 40 thousand downloads; it got messy because the author was more interested in adding new features requested by users than in keeping the code base clean. It is a single script because that made it a lot easier for users to install. And, newer versions of the same generator are written in Lua and not Python because that is what newer versions of Civilization use for scripting.
- diminoten 7y ago
  No, this code "got messy" because the person writing it has no clue what they're doing in Python. This code was never clean.
  - strenholme 7y ago
    Please keep https://news.ycombinator.com/newsguidelines.html https://news.ycombinator.com/newsguidelines.html in mind when commenting.
    - diminoten 7y ago
      A) Not your job to police this site, B) just because your feelings were somehow hurt about code you didn't even write, doesn't actually mean I violated commenting guidelines, and C) maybe you ought to read the guidelines yourself, considering one of them specifically says not to comment when you think a post is inappropriate. You want a "good critical comment"? Here: * Try breaking your code up into more than one file. There are other/better ways of releasing Python besides "one file". * Lay off the code commenting, it's vastly overdone here. * Use version control tools to version your code, not comments. * Python convention is to use underscore_case, not camelCase. * Variable/function/class names should be descriptive, not random letters/numbers/incomprehensible abbreviations. * Delete commented out code -- it's stored in version control, you're not going to lose it. * The "initialize" func is a terrible concept and represents a fundamental misunderstanding of how Python classes work. This whole misadventure is a cautionary tale in, "Why a light architecture pass matters even in 'throwaway' scripts." * Add tests. * The massive if/else chains can be replaced in a number of different, more readable and therefore superior, ways. That's some of what I saw in 2 minutes of review. This code is in very poor condition, and if you think the state of this code is an indictment on how hard it is to convert python from 2 to 3, you're more than just a little mistaken. You would need to pay me substantial amounts of money to spend more of my time going deeper into what's wrong with this code, as there is a lot and it is not generally enjoyable to work with code so thoroughly broken as this. The script (somehow) works. That matters. However, this is not a good representation of why migrating from Python2 to Python3 is hard; this code is in no shape to be used to represent anything other than how not to write Python.

mturmon 7y ago

Could have been said more kindly, but the point is accurate. This reads like C code that happens to be written in Python. There is a lot of fiddly bit-twiddling operations. The main issue — besides not using 2to3 — is that / in Python3 is interpreted as floating point division, whereas it used to be integer division if operands were integers. Replacing / by // in the relevant code blocks might help? Or getting another implementation of the RNG?

misnome 7y ago

It really isn't that bad. I regularly see much, much worse, and much more unidiomatic python, from people who have gone on to work at FAANG.

strenholme 7y ago

The code base in question is not one I wrote; it is one I decided to maintain years after the original author lost interest in it. [1] This is not too different from what I have seen in multiple jobs, where the new hire doesn’t get to write all the code they use from scratch, but instead has to maintain someone else’s mess. Often times, management and market conditions demand more from programmers than they have time to properly implement with test cases, well structured code, good comments, etc. Technical debt is a big problem in code used by businesses, and the technical debt this particular 6000-line script has is a lot less than some stuff I have seen and have been paid to maintain. The fact of the matter is that most open source code out there is quite a bit cleaner and maintainable than the code which is behind firewalls, running important businesses. [1] Edit: To the original author’s credit, this particular code base is an incredibly popular and successful map generator for Civilization 4, with some 40 thousand downloads; it got messy because the author was more interested in adding new features requested by users than in keeping the code base clean. It is a single script because that made it a lot easier for users to install. And, newer versions of the same generator are written in Lua and not Python because that is what newer versions of Civilization use for scripting.

diminoten 7y ago
No, this code "got messy" because the person writing it has no clue what they're doing in Python. This code was never clean.
- strenholme 7y ago
  Please keep https://news.ycombinator.com/newsguidelines.html https://news.ycombinator.com/newsguidelines.html in mind when commenting.
  - diminoten 7y ago
    A) Not your job to police this site, B) just because your feelings were somehow hurt about code you didn't even write, doesn't actually mean I violated commenting guidelines, and C) maybe you ought to read the guidelines yourself, considering one of them specifically says not to comment when you think a post is inappropriate. You want a "good critical comment"? Here: * Try breaking your code up into more than one file. There are other/better ways of releasing Python besides "one file". * Lay off the code commenting, it's vastly overdone here. * Use version control tools to version your code, not comments. * Python convention is to use underscore_case, not camelCase. * Variable/function/class names should be descriptive, not random letters/numbers/incomprehensible abbreviations. * Delete commented out code -- it's stored in version control, you're not going to lose it. * The "initialize" func is a terrible concept and represents a fundamental misunderstanding of how Python classes work. This whole misadventure is a cautionary tale in, "Why a light architecture pass matters even in 'throwaway' scripts." * Add tests. * The massive if/else chains can be replaced in a number of different, more readable and therefore superior, ways. That's some of what I saw in 2 minutes of review. This code is in very poor condition, and if you think the state of this code is an indictment on how hard it is to convert python from 2 to 3, you're more than just a little mistaken. You would need to pay me substantial amounts of money to spend more of my time going deeper into what's wrong with this code, as there is a lot and it is not generally enjoyable to work with code so thoroughly broken as this. The script (somehow) works. That matters. However, this is not a good representation of why migrating from Python2 to Python3 is hard; this code is in no shape to be used to represent anything other than how not to write Python.