10 ms·

You need to get the programmers together and agree on a coding guideline, a linter and other architectural fundamentals. Then the gardening phase begins, which

• crdoconnor 9y ago
  IME linting picks up about 99% fluff (ooh look there's some whitespace on a blank line) and 1% potential issues highlighted (usually minor). The reason people like to default to linting when "fixing code quality issues" is purely because it's 100% objective and measurable, not because it picks up the truly important issues. It doesn't. Edit: I am aware that coders have OCD impulses which can be sated with a linter, but priority #1 when fixing spaghetti code is unraveling the spaghetti, not sating your need to see }s in a place that makes you feel better.
  • mathgeek 9y ago
    Most linters allow you to disable whitespace linting. Good tools allow you to learn to use them better.
  • simonpantzare 9y ago
    I'd argue that there's a significant mental overhead reading code when it doesn't follow the same conventions, and that enforcing a style guide is a matter of having no broken windows. But of course there's no point trying to make shit look good.
    • dep_b 9y ago
      A linter and code conventions are a way to make code more consistent and easier to switch between. At times it becomes hard to see what code you wrote and what somebody else wrote, which is a good thing. If I open a file and everything "just feels wrong" but I can't change it because enforcing my personal preferences are just my personal preferences is just making things worse that really takes some valuable energy away I could use for actually improving things. But I can get used to a coding guideline that's not 100% my preference but a well enforced and consistent one. Knowing that if I fix indenting and naming the fix will not enrage someone else.
      • dasmoth 9y ago
        At times it becomes hard to see what code you wrote and what somebody else wrote, which is a good thing. I suspect this is the big point of disagreement between lint-advocates and many of those who are dubious. I generally don’t see big wins from collectivising code, and on the who prefer to treat people I’m working with as individuals who can be interacted with 1:1 vs an amorphous “the team” writing “the codebase”.
        • dep_b 9y ago
          Usually there’s a style that’s mandated by large authorities like C# formatting is mandated by Microsoft and JetBrains. Copying their style gets you there for 80%. The rest is whatever you agree on and usually not something that bothers me. If you’re the guy that thinks having a 300 line function in an 8000 line class is just an individual way of expressing yourself you’ll have to fight me. If you think real tabs is better than 4 spaces I shrug and let you have it your way.
          • dasmoth 9y ago
            The fact that you’ve named an IDE vendor rather emphasises that there is a substantial gray area. By no means everyone wants to use an IDE (let alone that specific one...)
            • 9y ago
  • lloeki 9y ago
    We did that (rubocop). Lots of fluff, a dozen or three minor to medium issues, and a couple of downright critical ones that slipped through production cracks out of sheer luck. Not even counting the last ones, the holistic effect of having a code base normalized is unmeasurable (i.e == through the roof), because suddenly moving from one part of the code to another is consistent enough that it removes a mental barrier to seeing patterns and enabling refactoring.
    • crdoconnor 9y ago
      IMO there are two features which linting tools need before I can start treating them as serious "necessities" on a code base rather than "nice to haves". lint [cosmetic | medium | serious] - you must select one to run the tool, and the rule defaults must be sensible. No "100 character line length" in 'medium'. That's strictly cosmetic. Serious is for initialized and unused variables and the like - things which you might plausibly say "I'm not unhappy that broke the build because it could have indicated a bug". lint medium --against=branch - the linting tool should be able to intelligently compare against a baseline branch and only warn you about issues on code you have actually touched. I do not agree that the holistic effect of having a code base "normalized" is "through the roof". I think the cost is relatively high while the benefits are relatively low. Right now at work if I run a linter against my code there are about 10,000 "issues" - the top 50 of which are completely cosmetic and I'd be remiss to invest time in fixing them. I'm not opposed to linters on principle and I do use them, I just think that there's almost always bigger fish to fry.
      • icebraining 9y ago
        pylint has different levels you can select (convention, refactor, warning, error), not sure if it's a common feature. For the second, see git-lint: https://github.com/sk-/git-lint https://github.com/sk-/git-lint
      • dasmoth 9y ago
        Yep, a tool that did all of this might be enough to convince me.
      • 9y ago
  • troels 9y ago
    Depends. Some programmers have absolutely no hygiene wrt formatting. You'd want that sorted for readability. But the bigger point is that you reduce merge conflicts since you won't have all sorts of unrelated formatting changes mixed in with the real changes.
    • ionforce 9y ago
      > Some programmers have absolutely no hygiene wrt formatting. I don't know how these kinds of people live with themselves or are tolerated by others. I hate it.
  • bunderbunder 9y ago
    I agree, linting doesn't pick up much. But, at places I've worked where the codebase was kept linted, the humans did a better job of catching bugs in code review. My guess is that, when you have a cosmetically messy code base, people start to develop a lower standard for what counts a understanding the code they're reading. Probably as a self-preservation mechanism. There's only so much time in a day.
    • ryanianian 9y ago
      Also: if your coworkers care enough about quality to add and maintain a linter, they're the kind of coworkers that care to do good code-reviews and maintain a high quality bar.
  • jerf 9y ago
    "IME linting picks up about 99% fluff (ooh look there's some whitespace on a blank line) and 1% potential issues highlighted (usually minor)." What language are you using? Dynamic languages are fairly difficult to meaningfully lint because it's hard enough just to analyze a function for problems, let alone extend past that. Static languages are easier to write non-trivial linters for, because there's more information just sitting there in the source, waiting for something to grab it. But even for dynamic languages, linters can still do things like enforce documentation standards, code testing/coverage standards, and some other basics that may sound trivial, but still add up to very useful things.
    • spc476 9y ago
      I find luacheck (a linter for Lua) invaluable for picking up typos, unused variables, variables shadowing other variables and unintentional global use (since Lua defaults to global by default). The types of checks seems mostly reasonable to me [1][2]. [1] http://luacheck.readthedocs.io/en/stable/warnings.html http://luacheck.readthedocs.io/en/stable/warnings.html [2] I'm not entirely happy with the formatting issues, but they can be easily disabled. Edit: hit submit by mistake.
  • sjellis 9y ago
    "The reason people like to default to linting when "fixing code quality issues" is purely because it's 100% objective and measurable, not because it picks up the truly important issues. It doesn't." I think that ideally, linting and especially formatting help quality by taking the trivial issues off the table. If your code is currently "clean", and the tools reformat or flag the code that you just wrote, then hopefully those issues will never be committed into the repository, so human code review can be about substantive points.
• aalleavitch 9y ago
  This is absolutely the situation I am in right now, and I desperately need all the guidance I can get. We’re talking thousands of lines of React components that rely regularly on manipulating global js variables, mixed with MVC 5 written in Razor delivering variables in script tags, mixed with outdated jQuery, that’s all just concat’d together with gulp and that hasn’t seen a refactor in years of changing hands with no style guide. The company started in 2004 and I swear some of this code is still floating around from the day it started. We have three sets of scss breakpoints, three or four layers of “common.scss” files that act like a piece of furniture that has been painted over dozens of time (with shit like * {padding:5px} floating around), and an unknown amount of dead code. There’s lines written back-to-back with vastly different styles (with a random distribution of semi-colons and arrow functions and etc), and major global variables named with misspellings. And while I clean this all up, I am expected to continue pushing out features and addressing serious bugs with stop-gap measures. Please advise.
  • jlongster 9y ago
    You need to tell your manager that it's simply impossible to both clean all that up and also make progress on the project. You need to stop the erosion in order to fix it; fixing will require fundamental changes that will absolutely block progress. If aren't able to freeze the project, your cleanup progress will be constantly thwarted by new ugly hacks. There's just no way around it. If your managers won't freeze the project until you make meaningful progress, you're going to be stuck working with this ugly code for a long time. Try to convince them that in the long-term a cleanup will pay dividends. Without their support though, you won't get anywhere. If you're able to freeze the code for a bit, it sounds like it might just be quicker to rewrite some parts like the CSS. Don't take on the whole project at once - find pieces that can be rewritten individually, even if it's a large part like all the styling.
    • aalleavitch 9y ago
      Unfortunately there are business reasons why freezing the code is impossible. We’re ecommerce and working on a timeline dictated by consumer behavior. So if we need a promo tomorrow it’s going live or else we’re not making budget. I kind of assume this is how we got into this state to begin with.
• infectoid 9y ago
  Great advice here. Agree on a process. Do it slowly. I work with a lot of legacy code and have felt overwhelmed at times by all the different coding styles that have been used through the code base and not know when to refactor or simply clean up. Over the last year or two I've been using a linter to help me with all this. Basically any code I touch gets cleaned to the point the linter isn't complaining. This is usually a fairly quick and fun process and I've actually learned a lot along the way. I would refactor as soon as too many things were touching and the code started to have a smell about it even though the linter wasn't complaining. It has made maintaining legacy code more fun (almost like a game) and given me some better practices when moving into new code.