27 ms·
I want to fault this but I can't. Code reviewer who's not afraid to reject code? Check. Proper testing of code changes? Check. The process probably looks silly
by hacker_9 10y ago
I want to fault this but I can't. Code reviewer who's not afraid to reject code? Check. Proper testing of code changes? Check.
The process probably looks silly in this case because nothing went wrong. But with a hard-coded variable being changed, who is to say it won't break some other system by being changed? And if the variable was then moved to a config file, who's to say again that it might not break in some obscure way? Such as if the config file is not found in the uat/test environment? Additionally after all this, the manager can change the variable in the config file as they please without this process need going through again.
- zzalpha 10y agoMy reaction exactly. And 2 days were lost to escalation while 2 were spent in test. Everything else didn't look crazy to me if the change is small but has a high system impact. For an industry that is implicated in downed spacecraft, massive security vulnerabilities, and a reputation for unstable products, it's amazing how happy we are to optimize for developer satisfaction over all other metrics (I'm looking at you, Agile). Edit: And before anyone says it, I'm not claiming this is a good process. But coding velocity is just one metric by which we should be measuring our success.
- dreamfactory2 10y agowe should measure value, that is all. and that's what agile is supposed to aim for, not developer comfort. these processes offer negative value (prohibitive costs of change and inability to execute in time which massively outweigh any perceived security). even a vaguely leaner organisation will eat their lunch. and that error-proofing is entirely imaginary - they have added a ton of moving parts, handoffs, and manual processes and so will have way more screw ups than a small tight team practising continuous delivery, as anyone who has worked in such environments will know.
- delinka 10y ago"value." I'm getting this BS from a PM right now over a refactor job that needs to happen. Sustainability and tech debt be damned. Every time they want to support a new file type, it's editing several files, most of which are source, recompile, put QA resources on it, get IT to sign the binaries ... If you'll get off my back, let me refactor and get my "developer comfort" on, you'll never have to involve me or QA or IT in supporting Just One More File Type ever again - you can push that from policy on the server side. But "developer comfort" isn't "value for the customer." _sigh_
- softawre 10y agoSo...learn to make your case more effectively. PMs are people, too.
- delinka 10y agoDo I really have to explain the backstory here on HN? OK, the case has been made. Repeatedly. By me, my team lead, the research intern who wrote the original code ... Project Management is in agreement. This Product Manager refuses to accept that we need to refactor. Doesn't want the spend the time to do it. "I understand technical debt, but..." "I understand sustainability, but..." Please allow me to disagree with you that this is somehow my fault (which can apparently be corrected by my "learning to make my case more effectively.")
- nitrogen 10y agoI'm curious, have you put it into numbers? You can probably even make stuff up (AKA estimate), as long as it has a rational basis. Something like, "Over the next six months, not refactoring will cost us 450 hours of lost developer productivity, based on how much time was spent fighting tech debt over the last two weeks. The refactor will take six weeks and 300 hours of developer time. If we do not refactor, we will have 20% developer turnover within two years." Edit: It's possible that your PM will then say, "This feature will earn $250k per week. Delaying six weeks will cost $1.5m. Over two years, 1800 hours of wasted developer time is ~$150k, and recruiting 20% new devs will cost $250k. Let's spend $400k in lost productivity to earn $1.5m in additional revenue."
- RandomOpinion 10y ago>...it's amazing how happy we are to optimize for developer satisfaction over all other metrics (I'm looking at you, Agile). Developer comfort is not a goal of agile; the goal of agile is always having the project in a known good state. Any developer comfort is a side effect of the prerequisites for having the project always be in a known good state.
- mikekchar 10y agoI don't really disagree with you, but it is not unreasonable to come away with the point of view that the parent had. The agile manifesto has caused as much harm as it has good, I think. People use the manifesto as a methodology, rather than using it as a way to reflect on a methodology. In the extreme, some people will use the manifesto as an excuse to not only avoid a methodology, but a method as well! As long as my intentions are pure, then I can do no wrong, kind of thinking. Having said that, I think to say that "the goal of agile is always having the project in a known good state" is simplifying things a bit too much. A very important part of agile is preferring artefacts that are executable over artefacts that are not. More relevant to this discussion is the idea that people should be making judgements rather than relying on hard coded rules of process. If someone has made the decision that the risks of doing something is not worth the benefit, then avoiding the problem is not a waste of time. Importantly, though, a process or rule can not make judgements -- only people can. Following a rule blindly certainly can waste a lot of time. Setting up a team is not easy. If you put yourself in a situation where every single person on your team can and will press the big red "Stop The Presses!" button, then you only have yourself to blame. Every person on a team should have responsibility appropriate for their experience, skill and view of the situation. If you have disputes, then you don't want an arbitrary process determining the winner -- you want a real, thinking, feeling human being to make the judgement call. I'm rambling on here, but most of the breakdowns I see in teams come from the idea that either employees are stupid and so we should shield ourselves from them with layers of process, OR the idea that my colleagues are stupid and so I should make sure to politically rig decisions so I don't have to talk to them. This is basically guaranteed to result in failure.
- Pxtl 10y agoAgile isn't about developer comfort, its about the fact that nobody knows what they want until they see it and decide that they hate everything about it. It's about the fact that requirements are made of mercury. E-signatures on specs aren't worth the e-paper they're written on, and the stakeholders want it yesterday with today's changes to the requirements. Agile isn't a process so much as it is conceding defeat on the very concept of process.
- cestith 10y agoI like the mercury metaphor. Here's a another one for Agile.: Don't build a stone fortress for a nomadic tribe. If someone wants to give you a full specification and stick to it, you can build something fully specified. If they give you a vague idea that will change a lot, build them as little as necessary and make it flexible enough to change with the idea. If your code's going along for the trip, it may as well be easy to pack.
- ArkyBeagle 10y agoI don't think the example is about optimizing for developer satisfaction. 1) He changed a number. 2) There was ( apparently ) a very simple test for the effect of the change being correct. 3) This was high-priority.
- ubernostrum 10y agoYeah, this is the kind of "everybody's empowered to push back and insist on doing things correctly" environment that people normally praise. Or at least, they praise until they realize that people will push back and insist on quality, and then they decide it's stifling and prevents them from getting anything done.
- omouse 10y agoThe problem is when the push back is not standardized. I've seen bad code go through with comments like "we can deal with these things in a later PR or two" but then other code gets flagged for "it fixes one problem but it could fix three other things too" which increases scope. An inconsistent standard is better than no standard but it's just as frustrating.
- jocro 10y agoIt's all a balance though, right? And really, about the flexibility of the environment. You have to be able to recognize and deal with a broad spectrum of priorities - this, at least, was not prioritized highly enough across the environment and that meant it wasn't handled gracefully.
- user5994461 10y agoThey're not pushing on quality. They are plainfully pushing against it and deliberately risking to introduce more bugs.
- drewg123 10y agoI think the fault lies in review procedures that require bringing the rest of the code up to standards if you just touch a tiny part of it. In a shop like this, these rules mean that nobody will ever touch legacy code if they can possibly help it.
- Posibyte 10y agoFor the company I work in the daytime for and the company I moonlight for, this has been the case in both situations. The looming nature of legacy code, especially with prohibitively work-expensive policies lead to much more than bad programmer effectiveness. It lead to lots of extra lead-time on deployments, heavily decreased morale, and when we lost our two rockstar-20/30-year developers, turnover exploded. There's more to it more often than not than just policies and the amount of legacy code, but even when we had the most supportive of managers, morale was very low over it. I definitely think companies, as they age and go from startup to a firmly placed company, should really take stock of their code assets and policies and really make sure they're kept up-to-standard yearly, and really look at every policy and make sure it's absolutely needed. It's a lot easier than taking a 1 line project on a 9 year old piece of code and turning it from a half-day full-review to a 6 day haltingly slow process.
- jrs235 10y ago> this has been the case in both situations. Can you please clarify what the case has been? Has it been the case that procedures that require bringing all code up to standards if just a tiny bit is touched? Or has it been the case that nobody will ever touch legacy code if they can possibly help it? Thanks.
- rietta 10y agoThat is a valid criticism, but this is basically institutionalized boy scouting, which is a good practice.
- softawre 10y ago> institutionalized boy scouting, which is a good practice You brought up the name for it. Okay. Now can we get back to the debate on whether it is a good practice? These people are debating whether boy-scouting is 100% required every time, or whether there are critical issues that require making it sometimes-optional.
- rietta 10y agoAmen! For an enterprise system that doesn't have end-to-end automated tests, this sounds like a sane organization running as best it can. The process is optimized to not let incomplete code make it into production. This is a company doing it right given what they have to start with. Changing that ship won't happen overnight.
- rqebmm 10y agoI agree. Requiring things like stakeholder signoffs, code review, good test processes, and respect for team resource management is annoying, but it can also be incredibly important when you're trying to keep a big ship afloat. Without those steps there's another article waiting to be written: "Rogue developer brings down business for six days with a seemingly innocuous 1-line change"
- serge2k 10y agoExcept the end result was the boss said DO IT NOW and it got done now. This seems to be a pretty clear example of not making the process work for you. Unwritten policies. Having to escalate twice because of silly reasons. Hot fixes being miscategorized as features. Is a very urgent fix really the place to be fixing a bunch of other tech debt?
- dreamfactory2 10y agocost to business of occasional goof is less here than prohibitively high costs of making any change from these inappropriate and frankly insane processes (I've held management positions at such companies and easily recognise these kind of processes, in fact the post may have triggered some mild PTSD)
- bunderbunder 10y agoI've worked in two kinds of companies: Places where it's kind of a hassle to get code past a code review, and places where everyone's always having to put out fires all the time. Of the two, I've had to unexpectedly work nights, weekends or even vacations at only one kind. Guess which one I decided I'd rather work at.
- user5994461 10y agoI don't get it. The two groups are not mutually exclusive. It can be annoying to go past code reviews and yet there are fire everywhere all the time.
- lisivka 10y ago
- pbh101 10y agoFor established businesses, I wonder if you could come up with a useful metric for how much value you add by rejecting a commit and thus avoiding an operational incident. Mature, high-profit systems warrant much greater scrutiny on incoming changes than businesses/systems which aren't spinning off money yet. You can err in both ways: too slow where you need to be fast, and too fast where you need to be more careful. And AFAICT companies/organizations build processes, habits, and platforms around one mode and have a hard time switching to the other.
- user5994461 10y agoYes. Here, every second of downtime is over $10, if it affects all users. Last time we had a partial outage that only affected a piece of the base for a few hours, we got a first estimate at over 4 million dollar direct loss of revenues, and some percentage of that as direct guaranteed profit loss.
- vacri 10y agoI'll go further and say that this is what people say they want: time to refactor out tech debt. The headline is a lie - there was tech debt that was company policy to take time to fix (parametising some hardcoded variables, amongst other things). "mission critical LoC" is not the same as "LoC" in their little summary. People beg for time to fix tech debt, and right here is someone complaining about having to do just that.
- x0x0 10y agoAs I read it, he's complaining about (1) gating high-priority releases on tech debt (priority inversion); (2) the fetishization of rules over judgement (no hard-coded variables that have been changed once in 10 years with no near-term updates planned); and (3) the systemic lack of urgency.
- vacri 10y agoYeah, but this is "top of the queue" priority, not "must get done today" priority. The managers were talking about lead times and the potential of laying someone off, not "if this doesn't get fixed today, someone is going home hungry!". There's no mention anywhere in the story of someone actually at risk of being let go due to the delay. I've also worked support at a startup that was all about the whole "we don't really need tests for trivial changes!" factor. Oh, what fun it was when the devs would push straight to production at 1am, with an "ah, it'll get tested in the morning sometime, maybe"... in the meantime our farmer clients were using the tool well before office hours to plan their days around irrigation requirements. Those changes that devs thought so trivial were usually trivial, but sometimes were quite catastrophic due to unintended interactions.
- x0x0 10y agoAs per the article, there were both unit tests and a pre/post test run initially provided for this change.
- KKKKkkkk1 10y agoIt's nice to reject code, but code reviewers must also be mindful of opportunity costs when wasting developer time on trivial concerns. (Being the cynic that I am, I guess at some point Corporate America will introduce code review reviews to solve this problem.)
- yongjik 10y agoIf we're dealing with a company-wide hard-coded variable that only changes when the company is in danger of layoff, I'm pretty sure that: (1) Making the variable a configuration variable will do nothing: it's not like you want two different versions of company policies at the same time. (2) It won't help with testing, because any realistic test of the variable's change will involve changing the company policy and probably the way all systems behave. In my opinion, making that into a config variable is a mistake. It's perfectly OK to keep as a hard-coded constant.
- mynegation 10y agoIt is not a mistake. One literal 3 is not distinguishable from another literal 3. If you change it in one place but forget about another - that can lead to the inconsistent behaviour. Configuration variables are much less error prone
- hughw 10y agoIt was a hard-coded value before. The OP just wanted to leave it as a hard-coded value. The world would have been just as hard-coded before his change as after, and it would have been improved as little by the change. The policies in place seem to set the perfect against the good.
- Gibbon1 10y agoMy take on this is modifying the hard coded constant was not a functional change to the code base. Making it configurable was a functional change. One shouldn't make functional changes to old battle tested code unless you have to. If you do that it's just risk.
- quelltext 10y agoNobody set it needs to be a magic number. A descriptive constant name will address that. However, a setting in a config file invites tampering, which in turn means more complicated tests to account for that. It might make sense if frequent changes are anticipated. However, not every hard coded constant should or needs to be in a config file.
- justinsaccount 10y agoThe process is fine, but sometimes shit is broken and you need to fix it now and clean up the existing mess later. Why not instead of > Shirley (Code Review): It is now against company policy to have any hard-coded variables. You will have to make this a record in the Parameters file. Also, there are 2 old Debug commands, an unassigned variable warning message, and a hard-coded Employee ID that will all have to be fixed before this module can be moved to production. Add at the end: However, I'll allow the change for now because this is an important update that affects peoples jobs. I want to see every issue fixed before the end of the week and I will not approve any other changes until these issues are resolved. There, win-win.
- absrnd 10y agoBecause everything is urgent, all of the time.. and so nothing gets improved.
- aiiane 10y agoThis is what escalation is for - the job of a VP or Director of Engineering is to decide what is actually urgent and approve fast-tracking it, and what should actually follow process. Having an efficient escalation process is critical.
- dsr_ 10y agoVery much this: there should be a process for "emergency change" and that process has to be used for every actual emergency and not for anything else.
- droopyEyelids 10y agoThe word "should"
- chewyfruitloop 10y agoI've found that those in charge also have a budget to run you on... how do they justify you spending x hours updating code to do exactly 0 as far as anyone is concerned? ..plus all the regression testing? They just don't... it gets left as it is
- misterbowfinger 10y agoThis is one of those "all of software is a tradeoff" kind of thing. Here, the company optimized for safety, with the tradeoff being that it takes much longer to ship. Startups optimize for shipping, and consequently ship broken code way more often. OP demonstrates the fatal flaw in having process, in that poorly created process can be crippling. In this case, it may have led to significantly less profits, where if they had a way to fast-track something immediately, they could have avoided the delayed change for something so simple. Another flaw is that process, if it's really bad, almost encourages people to circumvent it. At large, slow companies, the devs who push the most code are not necessarily the best and the brightest but rather those who can bend process in their favor.
- mannykannot 10y agoI realized this as I was writing another reply, but it can be faulted for forcing a system upgrade onto an independent issue. Separation of concerns, people! Messing with legacy code is not a trivial matter, and if you are going to do it, make it its own project.
- paulddraper 10y agoWhat about > Also, there are 2 old Debug commands, an unassigned variable warning message, and a hard-coded Employee ID that will all have to be fixed before this module can be moved to production. F* that s* indeed.
- Clubber 10y agoThis sounds like a place programmers go to die.
- whack 10y agoHere's some. David/Julie: Having processes that allow critical issues to sit idle in a backlog for 2 days. Shirley: Delaying a code change because of completely unrelated issues. Code changes that introduce bad code should be fixed first. But demanding that the person fix all unrelated issues in the file is not the way to go. Tony: Not having tools in place for quick, completely automated testing. If a one line change requires 3 days of manual testing, something is wrong.
- bhntr3 10y agoBut will anyone change it again? How many times would they have to change it for this to be better? Maybe the developer should build a UI so the business owners can change it themselves. After all, business people editing config files directly on production machines is dangerous. Clearly the conf file should be in source control and subject to code review. And so on and so forth. Reasonable changes accrete. The real question is: What is this policy and process protecting against? Does it work? We're all risk averse. Bad things happen. We come up with processes and systems to prevent them. We write our postmortems and say "How could this have been prevented?" and we say "Heavyweight process X and automated, difficult to maintain, high overhead system Y" and everyone nods their head sagely and makes plans to implement X and Y. Then we make a new mistake and postmortem and build process Z and system W. But you know that old saying "Lightning never strikes the same place twice"? Well, it's wrong. Sometimes it does. Especially over a long enough timeline. But it's pretty rare. We're the soldiers preparing to fight the last war and losing the next one. Personally, when I see stuff like this I always think of https://medium.com/@webseanhickey/the-evolution-of-a-software-engineer-db854689243#.8afhpt89p https://medium.com/@webseanhickey/the-evolution-of-a-softwar... It's a cartoon. But I'm getting old and angry. 15 years in, I think that most of this stuff that we do, these pedantic code reviews, processes, policies, best practices and test plans we're always beating each other over the head with, they don't always pay for themselves. We still write bad software. We still make stupid human mistakes. There is no answer but good judgment, trust and respect. You need as much of it as possible. I think it's easier if you believe Clayton Christensen and build protected small teams that can operate in a limited bureaucracy and limit the connectivity between them and groups viewing them as "others". But again, trust, respect, autonomy and good judgment. Or just make shittons of money. With lots of money, you can afford to hire lots of smart people to do stupid stuff and take forever doing it.
- hvidgaard 10y agoThere are two paths issues can take. One is the regular one, that follow procedures for good reason. The other is the fast one, this is the things where time is of the essence, e.g. a bug that takes the entire system down so noone can do anything - issues like this must be solved asap, and the codemess, edge-edge-case testing, and documentation will have to wait until later (but will be top priority). This change sounded like it was the latter, and putting it through 6 days wait to get something out that needs to be changed yesterday is too slow.
- nnq 10y agoThe fault is that they're missing the "fuck the process, let's get this done by yesterday morning or we're all gonna die" button. (Of course, the rule for this button when you have is that you can only press it like once per month or some other arbitrary long time. Oh, and maybe having a physical button that actually sets off an alarm in the office could help too. Maximum annoyance to prevent abuse.)
- chuckcode 10y agoThe pain for me here isn't that process can take some extra time, it is that the developer doesn't know what is expected and keeps getting surprised at every step. It can soak up a lot of calendar time and developer motivation to have a code change swatted back and forth as different people weigh in with various undocumented guidelines/policies/standards over a period of days or even weeks. This could have been a very different experience if developer had known what was expected from the start. Ideally I'd like to see code reviews culturally in an organization be mostly about catching bugs and collaboratively learning the craft of coding better. It can be very frustrating and demotivating when it devolves into a series of undocumented style and naming requirements. I think it is good to have standards and process but seems like most places I've worked are woefully short on documenting these hidden requirements.
- nottrobin 10y agoI think there are certain things here that should go and things that should stay. Reviewers should not be afraid to reject code, changes should be tested. However, there are both bureaucracy and code reviewing things that I would change here to not slow things down unnecessarily: - If the code practices document hasn't been updated, pending future changes shouldn't block your PR - You should never have any trouble finding someone to review your changes - the whole team should be empowered to review. - Strictly, only the lines you touch should need to follow any existing code practices, not the whole module because you happened to change one line. Otherwise people will be scared to do small fixes. - You should only be forced to write new tests for functionality you've actually added, for the same reason. As well as of course fixing tests you break with your code. - Don't even get me started on Ed not having access to Marge... - Why does this piece of work need to be done in the first place? Why would a backlog be limited to 3 months? TL;DR: There should only be as much bureaucracy & strict rules as strictly necessary. Unnecessary bureaucracy is the death of companies, large and small.
- anon1209 10y ago>- Why does this piece of work need to be done in the first place? Why would a backlog be limited to 3 months? If it's a literal factory they may want minimize storage costs. You have to rent space to keep the product until the customer expects it delivered. I'm a layman so take my guess with a grain of salt.
- snovv_crash 10y agoSo when stackoverflow or github goes down, and they trace the bug to some legacy component, they should refactor the entire thing, taking a week, instead of increasing the memory on their VM, bypassing the release process, then doing full postmortem once the fire is out? This isn't dev. Running a factory is ops. It can't take 6 days to renew your domain name when you realise it expires in 2. It can't take 6 days to patch your server when some script kiddie takes it down the first time. Not everyone has a job that gives them the luxury of making sure everything is perfect before they commit to anything.