6 ms·

I left my last company because one of my co-devs would always do crazy hack-job things, and when I complained to them or higher-ups, the excuse was: < "Well al

I left my last company because one of my co-devs would always do crazy hack-job things, and when I complained to them or higher-ups, the excuse was: < "Well all the work was already developed, and it would take too much time to rewrite it. You should have said something earlier" > "When?" I asked, considering she had just put up the (big) PR's and PR's ARE the time to review... < "Check her commits as she pushes them to the repo" - as in her bugfix/feature branches, not master... My jaw dropped. Especially since I was hired on as "Lead" and had all the accountability but no actual power.

jack_h 6y ago
Yeah, I'm in a similar situation at the moment. It's incredibly frustrating because during code reviews I will request changes so it's not such a broken hack job, and the response will basically be "No, it's not worth changing". At which point I'm the one "holding up development". We wasted hundreds of development hours during the last project because of this persons "inventive" code, and nobody seems to understand what's going on. Shame the job market is a bit crap right now.
- munk-a 6y ago
  It's hard to get more strength to push back with out of thin air. I'd encourage you to try pushing for more detailed post-mortems (if you don't already have them) and just keep an eye out on how much curtailed reviews cost the company. You also really want an advocate for code maintenance and if you don't have one of these with a loud voice there isn't a really feasible way to solve it except becoming it yourself and earning the trust of those above you. Two pieces of actual useful advice I can offer are: 1. A review style I picked up based off of RFC 2119[1] basically the reviewing software we use allows us to mark particular comments as blocking of non-blocking and I pair that with the usage of MAY/SHOULD/MUST within the comment language i.e. "We're using the old `array()` syntax here instead of `[]` we MAY wish to use the more modern syntax" this allows me some room to elevate necessary change while keeping in the nitpicks I really want to throw in (and I do try and minimize them) without lowering the power of the strong comments. I've used MUST maybe three times always for something incredibly terrible like pages not loading or migrations to the DB that are unsafe and cause data loss. 2. Agree on syntax and style rules and enforce them. It's easier to get people to agree to rules once than try and argue for them on each PR - anything like brace placement or line limit shouldn't come up repeatedly since it wastes everyone time and makes folks feel belittled. 1. https://www.ietf.org/rfc/rfc2119.txt https://www.ietf.org/rfc/rfc2119.txt
  - gen220 6y ago
    This is great advice. Just have some minor thoughts to tack on. Post-mortems are great for many reasons. For the case of GP, one particular advantage is that they align senior peoples' understanding: we shouldn't do X again. If you have a strong narrative for why a project failed, post-mortems are a formal setting in which you can present this narrative with concrete evidence to higher-ups. In the future, when you see warning signs that a mistake is approaching repetition, you can raise the concern up the chain, invoking the memory of the post-mortem to motivate their intervention. I also totally agree that a sincere and high-quality code review process is required for high quality code. Your 2119 recommendation is excellent. I'd also recommend doing some reading on commit message templates that smart people follow, they've improved my commit game, big-time.
munk-a 6y ago
At our company no commits get into the trunk without going by another set of eyes. We're probably creeping up to mid-sized right now so those eyes can vary in stringency and reliability more so than they would have when it was just a handful of devs, but I think mandatory code reviews are a good habit to get into - so long as you empower every reviewer to be critical and make it clear that both the reviewer and dev are owning the code and must ensure it is acceptable during the process. We've had that process on for quite a while, and while there are some big weaknesses and holes in it we've also adopted a principle to keep PRs as small as possible[1] with those two tools we've had some pretty reasonable success with a lot of our biggest incidents being related to times when we've made large changes or a review was skimped on. 1. Even if that isn't measure in LoC - moving a dependency and updating references to it is something I'd count as a single action - but one I'd want isolated from any logic changes.
- mkhalil 6y ago
  Yeah, commit's weren't going to trunk/master without the extra eyes/PR. The commits I was told to review if I wanted to stop thecraziness were the personal ones going to the bugfix/feature branch.
  - gen220 6y ago
    This might be a separate issue! :) Many good companies enforce a no-origin-branches policy, with rare and well-justified exceptions. Because, used as you describe, a "feature branch" is just a future massive diff in disguise (when it's eventually merged), and massive diffs are a big no-no because they're a huge pain to iterate on via code review.
    - geitir 6y ago
      Doesn't every git repo have an origin branch? What is the alternative to creating a feature branch for developing something you don't want in production until it's ready?
      - 6y ago

jack_h 6y ago

Yeah, I'm in a similar situation at the moment. It's incredibly frustrating because during code reviews I will request changes so it's not such a broken hack job, and the response will basically be "No, it's not worth changing". At which point I'm the one "holding up development". We wasted hundreds of development hours during the last project because of this persons "inventive" code, and nobody seems to understand what's going on. Shame the job market is a bit crap right now.

munk-a 6y ago
It's hard to get more strength to push back with out of thin air. I'd encourage you to try pushing for more detailed post-mortems (if you don't already have them) and just keep an eye out on how much curtailed reviews cost the company. You also really want an advocate for code maintenance and if you don't have one of these with a loud voice there isn't a really feasible way to solve it except becoming it yourself and earning the trust of those above you. Two pieces of actual useful advice I can offer are: 1. A review style I picked up based off of RFC 2119[1] basically the reviewing software we use allows us to mark particular comments as blocking of non-blocking and I pair that with the usage of MAY/SHOULD/MUST within the comment language i.e. "We're using the old `array()` syntax here instead of `[]` we MAY wish to use the more modern syntax" this allows me some room to elevate necessary change while keeping in the nitpicks I really want to throw in (and I do try and minimize them) without lowering the power of the strong comments. I've used MUST maybe three times always for something incredibly terrible like pages not loading or migrations to the DB that are unsafe and cause data loss. 2. Agree on syntax and style rules and enforce them. It's easier to get people to agree to rules once than try and argue for them on each PR - anything like brace placement or line limit shouldn't come up repeatedly since it wastes everyone time and makes folks feel belittled. 1. https://www.ietf.org/rfc/rfc2119.txt https://www.ietf.org/rfc/rfc2119.txt
- gen220 6y ago
  This is great advice. Just have some minor thoughts to tack on. Post-mortems are great for many reasons. For the case of GP, one particular advantage is that they align senior peoples' understanding: we shouldn't do X again. If you have a strong narrative for why a project failed, post-mortems are a formal setting in which you can present this narrative with concrete evidence to higher-ups. In the future, when you see warning signs that a mistake is approaching repetition, you can raise the concern up the chain, invoking the memory of the post-mortem to motivate their intervention. I also totally agree that a sincere and high-quality code review process is required for high quality code. Your 2119 recommendation is excellent. I'd also recommend doing some reading on commit message templates that smart people follow, they've improved my commit game, big-time.

munk-a 6y ago

At our company no commits get into the trunk without going by another set of eyes. We're probably creeping up to mid-sized right now so those eyes can vary in stringency and reliability more so than they would have when it was just a handful of devs, but I think mandatory code reviews are a good habit to get into - so long as you empower every reviewer to be critical and make it clear that both the reviewer and dev are owning the code and must ensure it is acceptable during the process. We've had that process on for quite a while, and while there are some big weaknesses and holes in it we've also adopted a principle to keep PRs as small as possible[1] with those two tools we've had some pretty reasonable success with a lot of our biggest incidents being related to times when we've made large changes or a review was skimped on. 1. Even if that isn't measure in LoC - moving a dependency and updating references to it is something I'd count as a single action - but one I'd want isolated from any logic changes.

mkhalil 6y ago
Yeah, commit's weren't going to trunk/master without the extra eyes/PR. The commits I was told to review if I wanted to stop thecraziness were the personal ones going to the bugfix/feature branch.
- gen220 6y ago
  This might be a separate issue! :) Many good companies enforce a no-origin-branches policy, with rare and well-justified exceptions. Because, used as you describe, a "feature branch" is just a future massive diff in disguise (when it's eventually merged), and massive diffs are a big no-no because they're a huge pain to iterate on via code review.
  - geitir 6y ago
    Doesn't every git repo have an origin branch? What is the alternative to creating a feature branch for developing something you don't want in production until it's ready?
    - 6y ago