Hacker News Reader

Comment by manicennui | Hacker News Reader

In the vast majority of cases, writing good, maintainable code does not require more time. The real problem is that the majority of people working as software engineers barely know what they are doing, and use excuses like this because it makes some amount of sense to the incompetent managers in charge of them.

View on HN

I work with someone who regularly opens PRs for untested code. I'm talking stuff that hasn't even been run once: missing imports, undefined variables, etc... not bugs. I'm fine with bugs. I'm not fine with not testing your work in the most basic sense.

PR reviews don't mean throwing crap over the wall and hoping the reviewer figures it out. With this guy, there is so much back-and-forth hand holding, it would be simpler to close the PR and do it myself. But I don't...

Sounds like something to raise with their manager. Or with this person, before even reading the PR, "Hey - just to check before I review this, does it work on your machine? Have you tested it?". If they say no, close the PR and tell them to do that before opening it. If they lie, call them on it. If they aren't learning, don't waste your time hiding this useless coworker's failures.

I have raised it in the past and will do so again. One of the problems is the manager has no software engineering experience and his view of "working norms" often go against mine.

Some of my complaints are fairly basic: test/review your own work before asking someone else to review it. This should be applicable regardless of industry.

time to go next?

I've been thinking about that for a while, for a variety of reasons. This is one of the smaller ones. Probably after the holidays.

It is applicable regardless of industry. If your manager isn’t technical, drag the nearest senior engineer into the conversation too.

As for the PR, don’t bother. “Hey this code you’ve submitted for code review doesn’t even compile. I’m your colleague, not a human compiler. Please don’t waste my time with this again” -> Close issue.

I'm 100% fine with opening a PR for "I just typed it" code...

with two caveats:

- a HUGE disclaimer at the top saying "DO NOT MERGE: not tested"

- also, you'll probably want to politely ask someone for a review, and be specific about what you're looking for

"Draft PRs" are fine for discussing topics or code or goals with a team mate.

It's all about getting feedback!

That's fine. What I'm talking about isn't that at all. This was a "this feature is done, here you go" PR.

Looping back to TFA: I've been trying to use this workflow more to get quality code written more quickly. I definitely have a habit of spending too much time polishing code - getting a draft/do-not-merge PR up with the core of my proposed changes up for review helps me avoid that, plus getting an earlier review helps with finding issues.

This sounds like what I do, except I open them as draft PRs in some functional state that I've run locally first in order to solicit feedback as early as possible asynchronously.

Most teams I've seen don't practice any sort of iterative development, despite claiming to do so.

You are not alone. It is not fun.

If you are in charge of the review process, simply start closing their PRs, list the reasons why, or better yet create a document you can reference establishing your guidelines. Don't back and forth; if they can't meet a simple set of guidelines, the PR is not ready for review.

Let their manager take issue with it. Fight it, bring it to their manager's manager, whatever. A company like that is not worth working for, so try to make it a company that is, or leave, but do yourself a favor and find a team you love.

> better yet create a document you can reference establishing your guidelines.

This. Remove as much ambiguity and grey area as possible.

> better yet create a document you can reference establishing your guidelines. How about working on a document that establishes the companies guidelines?

I can not imagine having to meet everyone's personal quality bar.

You don't have to meet everyone's personal quality bar, just the person who is in charge of reviewing your code. Write good code and you won't have a problem!

My expectations here were a bit lower, given the individual involved: write code that actually runs.

Ha! I think you might be asking for too much these days!

In addition to what the others have said, make sure you have a CI and don’t review the PR until that’s showing green. The CI should ideally include tests, linting according to a common code standard and other “grunt tasks” that are unnecessary in a PR review.

Wouldn't a linter catch most of this?

You can also block commits with listing errors

Isn't this what draft PRs, CI/CD and automated integration tests are for?

> In the vast majority of cases, writing good, maintainable code does not require more time

Yep. Especially with practice. You can pretty much get to a point where you build things reasonably well by default without even thinking too hard about it. You have to want to attain it, and be willing to ruthlessly evaluate and file down your design repeatedly.

I believe there's a compounding effect at play here, which accounts for super-linear gains in ability, given enough focus, time, maturity, and number of projects.

I equate it a lot to writing an essay. Once you’ve gotten enough practice, you know the general structure. You’ll write a draft and then do some edits with some red ink. Then, if you have people willing to reas it, you get their eyes on it.

> You can pretty much get to a point where you build things reasonably well by default without even thinking too hard about it.

That's mastery. There are probably about as many master programmers as there were master... let's say blacksmiths. The problem is there are 10, 20, maybe 50 times as many programmers as we ever had journeymen blacksmiths. And they all seem to think that tenure equals mastery. If we had 10, 20, 50 times as many masters, we'd have enough people to keep an eye on things. But we don't.

> that's mastery

Sounds more like competence

Nah, competence means that with some effort you can make things that work well enough, if you don't need to put in such effort it's mastery.

This.

And it's often simple techniques like preferring pure functions or using immutable data structures that enable a huge improvement in maintainability, yet few seem to be employing them.

Occasionally the difference comes purely from the fact that someone looked into the library docs and chose the appropriate API method.

A large fraction of people doing woodworking videos are overtly or covertly pushing the fundamentals. If you don't have the fundamentals down what are you doing trying to do complicated stuff?

If I had a dollar for everytime I heard “we had a tight deadline” as an answer to a question “why is this code so shi*y” I’d have more money than Musk.

There is no good code vs. bad code, there are just good programmers and bad ones.

And given how many programmers there are in total, roughly 98.76% of them are the bad ones :)

I am in my 26th year of this career and I can count on one had situations where a bad programmer wrote good code and good programmer wrote bad code.

I’d consider myself a good coder, and though I have the standard number of digits, I do not have enough to count the number of times I’ve written bad code. But I’ve written a lot of code, and over the decades I’ve learned a lot. I think that process requires some bad code along the way.

And this is not a total narcissistic opinion at all. Just say all programmers are bad except you.

If you actually read my comment I never said I was the good one mate ;)

I would love to see an example of your code, please!

$450/hr - nothing comes free

Sure, if it's as good as you say! Let's make a deal :)

I think one of the exceptions is heavily optimized code, though the surrounding code can still be maintainable and clear, with the weird stuff to tickle the compiler or inline assembly being heavily commented.

For those kinds of cases, I love having as-simple-as-possible reference code checked in alongside it, even if it's just #ifdef'd out for common builds. Having a baseline makes it sooo much easier to understand and debug the optimized code.

Ideally, the slow code is available alongside the fast code and the unit tests confirm they are sufficiently equivalent. If that's not possible, the go-fast bits need to be refactored until it is possible. Don't forget that error handling is part of the equivalency testing.

Beyond unit tests, this seems like a great opportunity to leverage property-based testing, at least for pure functions; checking that both versions return the same results is an easy property to think of.

Agree completely. In fact it often takes less time to write code properly because you won’t have accidentally introduced a regression that you need to fix before shipping.

Reply on news.ycombinator.com