195

I am working at a robotics startup on a path coverage team and after submitting a pull request, my code gets reviewed.

My teammate, who has been on the team for more than a year, has made some comments to my code that suggest I do a lot more work than I believe to be necessary. No, I am not a lazy developer. I love elegant code that has good comments, variable names, indentation and handles cases properly. However, he has a different type of organization in mind that I don't agree with.

I'll provide an example:

I had spent a day writing test cases for a change to a transition finding algorithm that I made. He had suggested that I handle an obscure case that is extremely unlikely to happen--in fact I'm not sure it is even possible for it to occur. The code that I made already works in all of our original test cases and some new ones that I found. The code that I made already passes our 300+ simulations that are run nightly. However, to handle this obscure case would take me 13 hours that could better be spent trying to improve the performance of the robot. To be clear, the previous algorithm that we had been using up until now also did not handle this obscure case and not once, in the 40k reports that have been generated, has it ever occurred. We're a startup and need to develop the product.

I have never had a code review before and I'm not sure if I'm being too argumentative; should I just be quiet and do what he says? I decided to keep my head down and just make the change even though I strongly disagree that it was a good use of time.

I respect my co-worker and I acknowledge him as an intelligent programmer. I just disagree with him on a point and don't know how to handle disagreement in a code review.

I feel that the answer I chose meets this criteria of explaining how a junior developer can handle disagreement in a code review.

Doc Brown
  • 199,015
  • 33
  • 367
  • 565
Klik
  • 1,665
  • 2
  • 10
  • 10
  • 7
    Did you write those same arguments in your code review when he asked for that change? What did s/he say? – edalorzo Feb 05 '17 at 00:55
  • 153
    You do realize that unit tests are, in part, meant to catch those one-in-a-million defects when someone checks in a change to your code that breaks it in some obscure way, right? Unit tests are not just about ensuring that your code is correct _now_, but also in three years when someone else is maintaining the code you wrote. –  Feb 05 '17 at 00:58
  • 1
    Do you have a test case for this obscurity? It sounds like you don't as the older algorithm was also not handling it. (You should not program to handle some obscure case, that you cannot exercise even once, to see if the code works.) – Erik Eidt Feb 05 '17 at 01:04
  • 1
    @ErikEidt The obscurity comes from giving it very specific artificial input. Real input will never be like this (IMO). – Klik Feb 05 '17 at 01:17
  • @edalorzo I replied that I don't think we will get input like that and that it is unnecessary to do so. I didn't want to be too stubborn though, so after he replied again I sort of just let it go. – Klik Feb 05 '17 at 01:21
  • 191
    @Klik "Real input will never be like this" That is where you are wrong. I have encountered too many cases of "input will never be like this" and being surprised when it _was_ "like this." In a production environment, all sorts of weird things can happen. Do not think only about how your software will _work_ but also _how will it fail?_ –  Feb 05 '17 at 01:54
  • 38
    @Snowman More to the point *code reviews* are, in part, meant to catch those one-in-a-million defects that unit tests and even randomized testing/fuzzing don't find. – Derek Elkins left SE Feb 05 '17 at 06:47
  • 12
    It is also worth remembering that code reviews aren't just there to catch problems they are also there for you to learn how you might be able to do better so treat them as a learning opportunity. – Steve Barnes Feb 05 '17 at 07:26
  • 5
    FWIW no, I am not an engineer, and no I have never worked on production software. I do it as a hobby, but consider the risk that one takes when producing a product for a return ( on a time schedule ) is prioritized over correctness/robustness. This kind of prioritization appears to be the root cause of most software failure and can lead to vulnerabilities and potential for large liabilities. I am glad to hear that your team seems to have its priorities in order, not for the sake of the company or the team, but for the sake of the end user. Without the end user, there is no company or team. – Nolo Feb 05 '17 at 09:01
  • 1
    @Snowman probably the most important consideration: "*How* will it fail?" Because total test coverage is impossible for non-trivial programs and data. It *will* fail. – Peter - Reinstate Monica Feb 05 '17 at 10:02
  • 74
    hey @Klik, it sounds like the fundamental problem here is you are "scared to speak your mind". NEVER be angry, ALWAYS speak your mind - with a smile. You should have instantly told the guy, "Hmm, that will take me at least two days, is this worth it, let's ask our boss?" and secondly you should have said "Don't forget man we're working on robotics, it's actually *not possible* for the left sensor to be to the right of the right sensor - let's ask the boss how much anti-physical corner cases we want to cover". Your problem is **yours**, your problem is you need to swap **anger** for **talk**. – Fattie Feb 05 '17 at 13:13
  • 8
    BTW, that the reason you cover anti-physical cases in robotics is that *"the physical can change"*. In my humorous example in the previous comment, **indeed, it is entirely possible in the future, the right sensor will be repositioned, will be mobile in itself, will work with mirrors, or whatever, so that in fact *it will be normal* that the 'left sensor will be to the right of the right sensor'**. It's completely and totally normal when working on robotics algorithms that you talk about non-physical coverage. Generalizing beyond current physicality, if you will. – Fattie Feb 05 '17 at 13:15
  • I mean, there will be a "column in your google doc" labelled "currently physical?" "known to be physical soon" "almost certainly will never be physical". This is an absolute basic of working with and thinking about time-space algorithms. If you're not doing this the two of you are totally all over the place. (Indeed, a commonplace joke would be something like, Junior Robot Guy: "But boss, the thing will never be walking upside down" Boss Robot Guy: "OF COURSE it might be walking on a ceiling") – Fattie Feb 05 '17 at 13:20
  • 6
    Just an opinion but not every bug should be fixed. If the likelihood and consequence is low and the cost of the fix high then it may be a business decision to not fix it (or defer fixing it). – paparazzo Feb 05 '17 at 15:52
  • 9
    Just a comment on your example. Even if handling a possible condition is determined to be a waste of time, make sure that it is detected. Report it, log it, abort the program, whatever, but do something other than ignore it. – Zan Lynx Feb 05 '17 at 17:30
  • 2
    If it really truly can never get that input (which my experience says is unlikely to be the case), then can't the test just test that the right exception is thrown when the invalid value is given? – Andy Feb 05 '17 at 23:47
  • 1
    Not testing the corner cases is how Skynet gets created. – Stephen Feb 06 '17 at 02:39
  • Perhaps this obscure case will occur more frequently in the future. It's quite possible that someone more senior than yourself knows the direction of the product more than you. it also depends on how catastrophic a failure would be, usually a junior dev is shielded from these things – Tom Feb 06 '17 at 07:32
  • 1
    A big chunk of my time as a programmer goes into finding and fixing those obscure cases that only happens once in a million times. If you see a defect, make sure it gets handled. – Pieter B Feb 06 '17 at 07:37
  • 3
    Your manager probably decides what a good use of time is. As a junior you have to focus on getting the tasks you get handed done, there is always something you could have done better, but hindsight is 20/20. As you're working with robots we don't know what the failcase would produce. A dead human ? A broken robot costing 2k, 20k or 200k ? – HopefullyHelpful Feb 06 '17 at 08:50
  • 6
    *However, to handle this obscure case would take me 13 hours that could better be spent trying to improve the performance of the robot.* 13 hours used to save headaches and many hours of work in the future. Improving performance can wait, how long is the runway for the startup? Do they have significant performance targets? Because you should value HIGH QUALITY FIRST –  Feb 06 '17 at 17:06
  • 10
    At a bare minimum, your code should throw an error in unsupported use cases. It should *never* silently do the wrong thing. If you don't think you should spend the time to handle the obscure case, at least make that case cause an error and halt the program. – jpmc26 Feb 06 '17 at 21:23
  • 8
    Real input is the worst kind of input. =( – corsiKa Feb 07 '17 at 02:35
  • The decision should take into great account whether if / when the obscure case materializes the outcome will be highly harmful or not. F.ex. if it would "just" cause a crash it's one thing; if it would result in wrong output or data corruption or anything else that could go unnoticed for some time and make users work with unknowingly wrong data and be a nightmare to rectify, it's another thing. Anyway the decision you described about whether to handle or not the obscure case doesn't sound like one that should be taken by a developer alone, there should be a team leader or such to involve. – SantiBailors Feb 07 '17 at 13:16
  • 4
    Depending on the application, acceptable handling of a complex corner case that you believe is thermodynamically ot mathematically impossible can be `assert("This can't happen")` or its equivalent. Emphatically not, if it crashes an airliner rather than a website. – nigel222 Feb 07 '17 at 16:14
  • 4
    I work in an agile environment. My approach would be to argue the unlikelihood of the case being found in practice, propose that we document the case in the code, add a technical debt story to the backlog to create the unit test and modify the code to handle the case and let management decide how to prioritize it relative to other things that need to get done. Changing the conversation from DO/NOT DO to DO NOW/DO LATER might make everyone happier. – Paul Chernoch Feb 08 '17 at 17:21
  • 6
    Your attitude is your error. "I'm not sure it is even possible for it to occur". You are *making decisions to not do recommended work based on your admitted ignorance*! Never ever ever make a coding decision predicated on your ignorance. The right attitude is *I'm not sure what the possible behaviours of this code are so I am going to write zero more lines of code until I am sure*. – Eric Lippert Feb 08 '17 at 17:40
  • 3
    You know what programmers call edge cases that aren’t handled? … BUGS. … The general idea is that a program should NEVER be in an undefined state. As that is a faulty state. So we handle ALL cases. No exceptions. … You know what we call programs that don’t? … HACKED. ^^ – Evi1M4chine Feb 09 '17 at 09:16
  • 1
    If you guys are a startup you also need to _not fuck it up._ Delivering a faulty software is a recipe for disaster. – T. Sar Feb 09 '17 at 14:39
  • When you are working with code which has physical side-effects (e.g., robotics), the bar is raised. "Can't possibly happen," is an anathema to well tested code. That said, the need to get product out the door is at times an existential crisis and maybe this particular case needs to be put further down the list. But it needs a test case eventually. Get it in your issue tracking system. – CuriousRabbit Feb 09 '17 at 20:58
  • I would ask your reviewer to provide a proof of concept test that will exploit the bug to cause the software to behave incorrectly. If a proof of concept cannot be produced, then it is not an immediate issue (but could be in the future if external factors change). Best to document the living crap out of it -- including the discussion that happened in review. – Luke A. Leber Feb 10 '17 at 03:30
  • **Comments are not for extended discussion. Please continue this conversation in the chat room. Thank you.** – maple_shaft Feb 10 '17 at 17:44
  • He caught a bug in your code! You should be happy and grateful for his help. Regarding those "This will never happen" assertions - they do. Would you prefer to fix it now or at 3 am when it cost customers real money while they wait? – Thorbjørn Ravn Andersen Feb 11 '17 at 10:30
  • **13 hours is nothing** except for small projects shorter than say one-man-month. Do take too many shortcut as it cause technical debt accumulation and at some point you will regret the 13 hours you have save if you need say 130 hours to fix the problem it cause because you ignore it. – Phil1970 Feb 11 '17 at 18:50
  • You know what is even more frustrating? Spending hours debugging and then reach code like this: `catch (SomeTypeOfException e) { /* this can't happen in production, so just swallow the exception */ }`. One day, that corner case will happen. – Axel Feb 13 '17 at 13:19
  • I only [want to recommend this blog post](http://www.kalzumeus.com/2010/06/17/falsehoods-programmers-believe-about-names/) to your amusement, and otherwise leave you alone with any conclusions that you (or any other reader) might draw from this. ;-) – JensG Feb 22 '17 at 15:33

22 Answers22

326

I can give you an example of a corner case that could never occur that caused a disaster.

When the Ariane 4 was being developed the values from the lateral accelerometers were scaled to fit into a 16-bit signed integer and because the maximum possible output of the accelerometers, when scaled, could never exceed exceed 32767 and the minimum could never fall below -32768 there was “no need for the overhead of range checking”. In general all inputs are supposed to be range checked before any conversion, but in this case that would be trying to catch an impossible corner case.

Several years later the Ariane 5 was being developed and the code for scaling the lateral accelerometers was reused with minimal testing as it was “proven in use”. Unfortunately the larger rocket could expect larger lateral accelerations so the accelerometers were upgraded and could produce larger 64-bit float values.

These larger values "wrapped" in the conversion code, remember no range checking, and the results on the first launch in 1996 weren't good. It cost the company millions and caused a major hiatus in the program.

Enter image description here

The point that I am trying to make is that you should not ignore test cases as never happening, extremely unlikely, etc.: the standards that they were coding for called for range checking of all external input values. If that had been tested for and handled then the disaster might have been averted.

Note that in Ariane 4 this was not a bug, (as everything worked well for every possible value) - it was a failure to follow standards. When the code was reused in a different scenario it failed catastrophically, while if the range of values had been clipped it would likely have failed gracefully, and the existence of a test case for this might have triggered a review of the values. It is also worth noting that, while the coders and testers came in for some criticism from the investigators following the explosion, the management, QA & leadership were left with the majority of the blame.

Clarification

While not all software is safety critical, nor so spectacular when it fails, my intention was to highlight that "impossible" tests can still have value. This is the most dramatic case that I know of but robotics can also produce some disastrous outcomes.

Personally, I would say that once someone has highlighted a corner case to the test team a test should be put in place to check it. The implementation team lead or project manager may decide to not try to address any failures found but should be aware that any shortcomings exist. Alternatively, if the testing is too complex, or expensive, to implement, an issue can be raised in whatever tracker is in use &/or the risk register to make it clear that this is an untested case — that it may need to be addressed before a change of use or prevent an inappropriate use.

Web_Designer
  • 103
  • 2
Steve Barnes
  • 5,270
  • 1
  • 16
  • 18
  • 103
    Oh my goodness this answer is it. The OP is dealing with software that has physical repercussions. The bar has been raised. The reviewer probably didn't realize this "edge case" until they got looking at the code again. And given enough time nothing is an edge case. You don't want to accidentally swing a robot's arm into someone's head (not that I know a robot arm is involved with this code). – Greg Burghardt Feb 05 '17 at 13:07
  • 12
    Greg & Steve, this is a great example, but it's an example **of a bug**. It's a straightforward obscure bug. Literally "a bug", dividing by zero. When you work on robotics algorithms, it's a central idea that you think about physically-possible and not-physically-possible concepts. (If you will, "not yet" physically possible concepts, wit the current devices.) The confusion between the two developers, under discussion here, is due to them (very surprisingly) not being au fey with this paradigm. Their whole team has sever problems if, the more senior dudes have not embedded that paradigm. – Fattie Feb 05 '17 at 13:25
  • 11
    I think there are real world examples that prove why edge cases should be covered, but this isn't one of them. The example cited is a case of using the *wrong* library for a task. The code for a banana should not be blindly used on an orange. It is the fault of the person who used banana code on an orange, not the person who wrote code for an orange that couldn't handle a banana. (I had to switch Apple for Banana in this analogy due to a big tech company out there...) – Origin Feb 05 '17 at 13:35
  • 53
    @JoeBlow A bug, yes, but a *preventable* one, one that could've been caught with additional test cases and/or code-reviews. God only knows if there was a guy there at some point saying "You know guys, I think we should validate this range..." and others going like "Don't worry about it... what could possibly go wrong with an *impossible* case?" If bugs are not enough proof that our logic has more gaps than we'd like, then I don't know what to say... – code_dredd Feb 05 '17 at 14:00
  • hi @ray. "dividing by zero" (so to speak) is literally a bug. the point under discussion in this QA is an AI in a robotics system. it's not a *bug* being discussed, it's "whether or not the robot handles a certain type of case well". so, you and I are developing a natural language system. You say "it handles English and Chinese", I say "look, it does not handle German, right?" that has nothing to do with a "bug" as such. in the robotics field, it's totally normal / basic to speak of physical versus non-physical coverage. i give many examples of this in my comments above, no need to repeat. – Fattie Feb 05 '17 at 14:17
  • 1
    to make the example the other way: the Ariane failure described was caused by **a bug**, some fool forgot to check a range on an integer. in complete contrast: let's say Ariane was designed to fly autonomously during atmospheric ascent in (say) winds from 0 to 30 knots, from any particular direction. the "code reviewer" comes along and says "what about 35 knots". the example at hand (asked by the OP) is of that nature. – Fattie Feb 05 '17 at 14:20
  • @JoeBlow I understand your point, but I understand the OP's response to be akin to saying "It's impossible for the wind to reach 35 knots, *ever*, so why bother?" simply because he has never observed it or some other arbitrary reason. I don't claim to be familiar with robotics directly, but that seems besides the point. – code_dredd Feb 05 '17 at 14:59
  • 30
    The point that I was trying to make is that you should not ignore test cases as never happening, extremely unlikely, etc., the standards that they were coding for called for range checking of __all__ external input values. If that had been tested for and handled then the disaster might have been averted. Note that in Ariane 3 this was not a bug it was a failure to follow standards. When the code was reused in a different scenario if failed catastrophically while if the range of values had been clipped it would likely have failed gracefully. – Steve Barnes Feb 05 '17 at 15:00
  • hi @ray - well that's exactly correct. the "35 knots" issue has nothing to do with a **bug** per se - if you and I were working on that, and described / thought about the whole issue as a "bug", that would be a (spectacular) basic misunderstanding of our field. {As I further point out, in robotics, this is so completely central to what you're doing that you use language like this *all the time*, it would be completely normal to discuss an issue such as... "oh, is this a real bug or just ignoring a presently non-physical case - hey did you hear the device can now be inverted?!" sort of thing.} – Fattie Feb 05 '17 at 15:04
  • @SteveBarnes that is an important enough point re standards that I think it belongs in your answer, that changes the answer from "cool story but not exactly relevant" to "very important point in answering this question" – sq33G Feb 05 '17 at 20:34
  • 1
    @JoeBlow I'm not sure why in robotics people seem to think of "bugs" differently. I think whenever a piece of software does something it shoundn't or does not do something it should can be reasonably considered to be a "bug". Whether it does or does not have a "physical" consequence is beside the point. If notepad crashes, it's a bug; if a robot goes T-1000 on someone, it's a bug (assuming it's not intentional ;). If people chose to ignore the case where wind speed >= 35 knots, that looks like a bug to me, regardless of whether it ends up in a rocket blowing up, or just a simulation of it. – code_dredd Feb 06 '17 at 00:17
  • hi @ray. say you know the left sensor is always 6" to the left of the right sensor. ie: because it is welded on the device in that position. say you are working with me and I say "oh, you didn't consider the case of the left sensor being 2 inches to the right of the right sensor. what would you say? that's the only point I'm making. – Fattie Feb 06 '17 at 03:44
  • " I'm not sure why in robotics people seem to think of "bugs" differently" robotics is utter, total, crap (at the time of writing). Our absolutely best, cutting edge robots are ... total rubbish. Computers playing chess, A+. mobile robots .. D or F. – Fattie Feb 06 '17 at 03:45
  • 1
    @JoeBlow Again, I get your point. I just don't agree with it. For your sensor example, I would note that you'd be ignoring how temperature changes and other environmental factors affect materials (thermal expansion, etc), such that your assumption *will not always hold true* (a few millimeters can make a diff in some contexts). Your opinion about some things being "utter total crap" is besides the point, IMO. – code_dredd Feb 06 '17 at 03:53
  • @sq33G Thanks an expansion on the comment added to the answer. – Steve Barnes Feb 06 '17 at 07:10
  • 1
    Minor nitpicking: Shouldn't the upper bound for 16-bit signed integers be 32767? – Margaret Bloom Feb 06 '17 at 08:57
  • Quite right @MargaretBloom corrected. – Steve Barnes Feb 06 '17 at 09:11
  • 4
    Indeed. OP seems to be thinking of testcases as something you do only to prove that previously-seen bugs are fixed. They are also there to ensure you don't have future-seen bugs. – Lightness Races in Orbit Feb 06 '17 at 14:57
  • I think I already saw that sample somewhere in SEng.SE along with some others in an older post by the way. – Walfrat Feb 06 '17 at 15:59
  • 2
    I disagree that the Ariane 5 bug has anything to do with testing. The original Ariane 4 code _was_ sufficiently tested. It just should have never compiled in the new Ariane 5 setting with the larger accelerometer type. What this bug illustrates is that type safety matters and implicit type conversions are a bad idea. – leftaroundabout Feb 06 '17 at 17:25
  • 1
    @SteveBarnes: If input is read from a sensor by clocking in 16 bits and interpreting the result as a 16-bit two's-complement integer, there's no need to consider input values outside the range -32768 to 32767 because no combination of input stimuli could yield such a value. If the input routine is changed so that it can read more than 16 bits, then downstream code will need to be inspected to ensure the new range of values doesn't push computations outside the range that can be handled. – supercat Feb 07 '17 at 00:55
  • 2
    Minor point: Had the out-of-range data simply aborted the calculation the result would have been totally harmless. The offending routine returned a value that was necessary for liftoff but not used in flight. It was left running past T-0 in case there was a delay as the calculation took hours to restart. – Loren Pechtel Feb 07 '17 at 03:03
  • 4
    @supercat The standards involved say that __all__ external data must be range checked. As you point out the original coders omitted this because it was an __impossible__ error just like the OPs case but one of the reasons for that rule is to catch situations where the _impossible_ does happen. Actually for space certified code you are supposed to handle things like single event upset where a value changes midway through its processing, possibly to an "impossible" value such as an out of range enum. – Steve Barnes Feb 07 '17 at 04:17
  • Indeed @LorenPechtel if the values had been clipped or processing aborted with a "safe" neutral value returned things would have been very different. – Steve Barnes Feb 07 '17 at 04:20
  • 3
    Another example of this sort of thing is the [Air France 447 crash](https://en.wikipedia.org/wiki/Air_France_Flight_447). "The angle of attack had then reached 40 degrees...The stall warnings stopped, as all airspeed indications were now considered invalid by the aircraft's computer due to the high angle of attack...Roughly 20 seconds later...the pilot decreased the aircraft's pitch slightly, airspeed indications became valid and the stall warning sounded again...but stopped when the pilot increased the aircraft's nose-up pitch." The computers regarded the _improbable_ as _impossible_. – Kyralessa Feb 07 '17 at 12:29
  • @SteveBarnes: If one reads a `volatile uint8_t`, is it necessary to validate that the result is in the range 0 to 255? From my read of what happened, the original code did ensure that it read 16 bits, each of which was 0 or 1? If your point is that individual modules are supposed to validate data, I'd agree that would be sound practice, albeit one that some compilers try to undermine in the name of "optimization". – supercat Feb 07 '17 at 15:29
  • @supercat the original code received an input value from an accelerometer and scaled it up (up IIRC) so as to get the maximum definition in a signed 16 bit integer, i.e. max +ve value close as possible to 16767, etc., the sensors on the later rocket had a greater range so the scaled value was able to wrap. If there had been a check `if (inval > MAXVAL) inval = MAXVAL;` and similar for -ve values the problem would probaly not have occurred. N.B. The aviation and space industries have long mandated turning off optimisation unless it is shown to be essential. – Steve Barnes Feb 07 '17 at 17:07
  • 2
    The Ariane example justifies great care in code re-use, but writing the original code to anticipate all possible future re-use is not justified -- it was perfectly correct in that original application with that original data type. – donjuedo Feb 08 '17 at 14:41
  • Not downvote worthy, but this answer doesn't address the fact that not every software application does such critical tasks, and not every bug results in catastrophic failure. Time is money, and there are applications where it isn't worth chasing certain corner cases because the benefit of avoiding them isn't worth the cost of dealing with it. – whatsisname Feb 08 '17 at 17:30
  • @whatsisname - added a clarification to address your point. – Steve Barnes Feb 08 '17 at 17:51
  • @SteveBarnes where did you get the quote *there was “no need for the overhead of range checking”* and where are you getting the information about *The standards involved say that all external data must be range checked.*? Are the Ariane4 coding standards public? It sounds like they made a considered trade-off for performance - otherwise what overhead were they saving? Not writing two greater-than/less-than lines? Why would they ignore standards to save so little effort, unless there was some pressing need? – TessellatingHeckler Feb 08 '17 at 19:21
  • @TessellatingHeckler the "no need" quote came from a co-worker of mine who had worked on the project - we also discussed the standards involved as we were working to very similar standards on the project that we together on. I have also worked on many other projects where, despite standards to the contrary there was felt that there was "no need for checks" _until the auditors arrived_ I personally don't believe that it was a considered trade off. – Steve Barnes Feb 08 '17 at 19:27
  • 2
    I’m pretty sure that Ariane 5 reused a **physical component** (the inertial reference system) from A4, complete with its embedded firmware. And the A4 IRS development team, with tight performance constraints, considered which conversions could be done without range checks, and decided (at a senior level, with many reviews) that this one didn’t need the checks. – Simon Wright Feb 09 '17 at 08:32
  • 1
    [Here is another corner case](https://ntrs.nasa.gov/archive/nasa/casi.ntrs.nasa.gov/20100024127.pdf). On Apollo 6 the #2 engine auto-shutdown due to degraded performance. The #3 engine subsequently shutdown because a certain wiring harness was cross-wired to the #3 engine. – radarbob Feb 09 '17 at 19:06
  • 1
    This is a good answer. Whenever I see questions like this, around edge-cases etc. I'm always inclined to suggest reading about Therac-25: https://computingcases.org/case_materials/therac/case_history/Case%20History.html – Hywel Rees Feb 10 '17 at 08:41
  • **Comments are not for extended discussions. If you would like to discuss this answer further then please visit the chat room. Thank you.** – maple_shaft Feb 10 '17 at 17:43
  • 1
    Perfect answer. No other way to put it. I always try to cover all scenarios I can come up so that the application doesn't fail miserably but fail gracefully. – dokgu Feb 10 '17 at 19:49
229

an obscure case that is extremely unlikely to happen--in fact I'm not sure it is even possible to occur

Not having untested behaviors in code can be very important. If a piece of code is run e.g. 50 times a second, a one in a million chance will happen approximately every 5.5 hours of runtime. (In your case, the odds seem lower.)

You may talk about the priorities with your manager (or whoever is a more senior person in charge for the unit you work in). You will better understand whether e.g. working on code performance or code being bullet-proof is the top priority, and how improbable that corner case may be. Your reviewer may also have a skewed idea of priorities. Having talked with the person in charge, you'll have an easier time (dis)agreeing with your reviewer suggestions, and will have something to refer to.

It is always a good idea to have more than one reviewer. If your code is only reviewed by one colleague, ask someone else who knows that code, or the codebase in general, to take a look. A second opinion, again, will help you to more easily (dis)agree with the reviewer's suggestions.

Having a number of recurring comments during several code reviews usually points to a bigger thing not being clearly communicated, and the same issues crop up again and again. Try to find out that bigger thing, and discuss it directly with the reviewer. Ask enough why questions. It helped me a lot when I started the practice of code reviews.

Community
  • 1
9000
  • 24,162
  • 4
  • 51
  • 79
  • 5
    Second code reviewer is a good idea. Is it annoying to ask a lot of "why" questions though? I have heard about the technique of asking 5 whys and think it's a good idea, but I've never tried it in fear of being a bother. – Klik Feb 05 '17 at 01:25
  • The 5 whys technique is amazing, but you can drive the other party out of their depth. Often 2-3 whys in depth is enough. – 9000 Feb 05 '17 at 04:10
  • 10
    @9000 It can also be frustrating when the answer is "because life is that way." For example, this is one I recently had to go through: "Use spaces over tabs." "Why?" "Because our style guide says to." "Why?" "I don't know; I didn't write it." "Ha, clearly you're wrong and I'm right, and I'm going to leave my code with tabs!" Then a post-commit hook changed it anyway, but still -- if you use the 5 whys technique, go until you've gotten a sensible answer, not until you've made the other person frustrated. – Nic Feb 05 '17 at 08:47
  • 114
    @QPaysTaxes: Everyone should know the why of spaces over tabs (or tabs over spaces) argument: because both are correct but consistency is more important so just pick one, be consistent and don't waste time bikeshedding – slebetman Feb 05 '17 at 10:19
  • @slebetman - that's a useful and fun term I hadn't heard, thanks for that! https://en.wiktionary.org/wiki/bikeshedding The bike shed on a nuclear plant eh? Good one. – Fattie Feb 05 '17 at 13:32
  • There are a lot of good answers and useful information. I like this one the best because you focused on the issue of code reviews and provided actionable suggestions. – Klik Feb 05 '17 at 17:03
  • 2
    "If a piece of code is run e.g. 50 times a second, a one in a million chance will happen approximately every 5.5 hours of runtime." -- Clearly this is not actually the case, though. – David Aldridge Feb 05 '17 at 17:09
  • I second the second opinion. Our DoD was to have at least two members on each code review. – Baldrickk Feb 06 '17 at 09:39
  • 1
    @9000 One question I have with your answer, is your part "e.g. working on code performance or code being bullet-proof is the top priority". I'm fairly certain that in the first part you meant improving the performance (processing speed) of the code that was written. However, writing new code should be of higher priority over the other two. Definitely not at the expense of the other two, but if testing this edge case will take 13 hours of work, that is a lot of new functionality that can be added. Still, +1! – krillgar Feb 06 '17 at 12:40
  • 2
    @DavidAldridge or it might be a 1 in a billion defect. The point stands regardless.. When you run a piece of code over varying imputs frequently enough rare cases become common. I work on an application server in my day job. Last week I ran a series of tests running 8K transactions per second for 2 hours. A 1 in 1 Million bug would have occured 60+ times in 2 hours at that load. – Leliel Feb 06 '17 at 16:20
  • 1
    @krillgar: There are such cases as show-stopper bugs, when you stop working on new shiny things and fix a problem that undermines the foundation of your work. In the OP's particular case, the problem, if there is one, is not of the show-stopper variety. Being a junior, the OP is not in a position to decide on priorities. This is why he should take his code review to someone more senior, who is in a position to declare the priorities. _Most likely_ spending the 13 hours was not strictly necessary. 13 hours being ~2 days, contacting a senior person is practically past due. – 9000 Feb 06 '17 at 18:45
  • 31
    `Not having untested behaviors in code can be very important. If a piece of code is run e.g. 50 times a second, a one in a million chance will happen approximately every 5.5 hours of runtime. (In your case, the odds seem lower.)` -- What? No. Not unless you're running a Monte-Carlo simulation, or your code includes some randomized element. Computers don't run software according to a bell curve or standard deviation unless you tell them specifically to do so. – Robert Harvey Feb 06 '17 at 19:35
  • 13
    @RobertHarvey: consider something like a race condition, which is exactly a randomized element. Also consider a data-dependent bug while processing streamed data; while data may not be quite random, but unless it's highly repetitive, a particular combination of bytes may occur time and again. One in a million = triggered by a particular combination of 20 bits, a bug like this would be immediately visible if processing a video stream; something that happens rarely but regularly might involve e.g. 40 bits. – 9000 Feb 06 '17 at 20:19
  • @9000: Lock-free programming is the answer to race conditions, not more tests. The likelihood of a race condition drops to zero. – Robert Harvey Feb 06 '17 at 20:25
  • 7
    @RobertHarvey: Quite possibly! I just wanted to point that there are pieces of code that may have a 1e-6 chance (not 0 and not 1) to break under a particular invocation, referring to the «obscure case that is extremely unlikely to happen». While bugs like that usually are not visible under tests, they _are_ visible in production. – 9000 Feb 06 '17 at 20:29
  • 7
    @RobertHarvey arguably locks are more standard way of dealing with race conditions - especially if you don't need a throughput guarantees of hard real time algorithms. Neither (locks or lock-free data structures) would helpif person applying them does not know the problems of distributed/multi-threaded computing. "Not unless you're running a Monte-Carlo simulation (...) Computers don't run software according to a bell curve or standard deviation" - timings (at least in non-embedded platorms) usually follow some distribution and normal distribution is often nice approximation. – Maciej Piechotka Feb 07 '17 at 00:47
  • 2
    Agree 100% with "talk... with your manager" - they're there to set your priorities. At the same time though, it was totally valid of the reviewer to point out an edge case, however unlikely. The edge case represents a risk, however small, and it is up to management to decide whether to accept the risk or spend the time/money to eliminate it. And further, any acceptance of risk should be *explicitly documented*. – Blackhawk Feb 08 '17 at 15:06
  • @slebetman For the record, my response was supposed to mean "I don't know why the author of the style guide chose one over the other"; the person I was explaining it to ignored any attempt to say why consistency is important, so I just gave up and let the post-commit hook fix it. – Nic Feb 08 '17 at 19:24
  • 3
    Even if the manager agrees that fixing the bug is a priority, if the fix is expected to take 13 hours (a third of a work-week!), then it's almost certainly better off in its own pull request anyway. – Kyle Strand Feb 08 '17 at 19:28
  • 1
    Asking “why?” is never ”annoying”! The only annoying thing is idiots projecting their disliking of their own ineptness onto the messenger, because they can’t handle not being perfect little snowflakes. → The *only* allowed “I don’t know why.” answers are literal fundamental research. And even there, that answer means you have ZERO guarantees on it, and throwing dice is just as (in)valid. → „Spaces over tabs“ has the reason that tabs have a configurable with, which can result in a mess if it differs for different editors. – Evi1M4chine Feb 09 '17 at 09:27
  • 1
    Writing software to run a physical machine is much different than writing software that is forms over data. – Matthew Whited Feb 09 '17 at 15:51
87

Since it wasn't handled before, it's out of scope for your effort. You or your colleague can ask your manager if it's worth the effort to cover this case.

kevin cline
  • 33,608
  • 3
  • 71
  • 142
  • 21
    I think this is the key point- although covering the extra case is indeed a useful and valid improvement, there's no need for it to be spludged into an existing PR for different reasons. – DeadMG Feb 05 '17 at 10:48
  • 7
    Wether or not it was handled before is also irrelevant IMO, it simply wasn't in the task-description beforehand. – Jasper N. Brouwer Feb 05 '17 at 18:39
  • 1
    I agree that it should be tackled separately, however if *detecting* the situation is easy, I would ensure that (1) the case is detected (report the error/abort appropriately) and (2) a test case is created to demonstrate that the detection works. – Matthieu M. Feb 06 '17 at 12:43
  • 6
    To echo this sentiment a good answer to pull review comments can be "I'll raise a ticket to cover that, good point." That acknowledges that the 'thing' needs to get done, while also allowing it to get prioritised alongside all the other work that needs to get done. – Edd Feb 06 '17 at 22:18
  • 17
    Could not disagree more strongly. The fact that this issue might not have been brought up during the initial implementation of the feature might just as easily -- since we don't know specifics -- be indicative of having been incredibly lucky until now as it could be of being unnecessary. Code review of a modification that changes how this algorithm works -- potentially increasing the likelihood of this edge case -- is _exactly_ the time to bring up potential issues. Whether it's out of scope should be decided after proper evaluation of exactly that, not decided out of hand with little context. – Matthew Read Feb 06 '17 at 23:38
  • 2
    @Matthew: I agree that it should not be ignored. However it is not reasonable for the reviewer to insist that it be corrected as part of the present work. – kevin cline Feb 07 '17 at 02:45
  • 6
    This is terrible advice! `Since it wasn't handled before, it's out of the scope for your effort` would be enough to mark every single bug report as `wontfix`, assuming your spec was bad enough to begin with that it didn't consider the edge cases, if you even had a proper spec. – Filip Haglund Feb 07 '17 at 14:30
  • 2
    Our jobs as prefessionals and engineers is to make sure things are done correctly. Not to just copy how they were done before. – Matthew Whited Feb 07 '17 at 17:31
  • 4
    Agreed. Out of scope. Does that mean you should ignore and walk away? No! Discuss the issue with the reviewer / manager, and decide if the original scope should be altered to include this case or whether you will handle it as a separate issue / prioritize against other important issues that you as a junior dev may not be aware of. – p e p Feb 07 '17 at 18:16
  • 5
    @FilipHaglund This isn't a bug report though, it's a code review comment to handle a hypothetical and it will supposedly take a non-trivial amount of time to fix. It shouldn't be ignored, but it should be addressed as a separate ticket and prioritized as a new piece of work. – Ben Feb 08 '17 at 04:22
  • 3
    Handle it as a feature request, which is what it is. All new behaviour should be a feature request. It should go onto your backlog and be specced up. – NibblyPig Feb 08 '17 at 13:58
  • 1
    This would be the best answer if instead of saying "ask your manager" the answer suggested handling this a bit more formally and recommend the OP "raise a ticket to cover the issue". That way the review comment doesn't get forgotten and an explicit informed decision can be made as to whether (or when) the issue needs to be resolved or not. The first step would be to get concurrence with the co-worker that it doesn't need to be resolved, but since there's disagreement then it should be elevated to the next level. – Dunk Feb 08 '17 at 18:49
  • 3
    For those saying that it should be fixed, just because an issue was found aren't being realistic. There's always errors to be found, it doesn't mean they all need to be fixed or that every issue is worth the effort to fix. Writing bug free code is really hard. 90% of developers can't write something as simple as an issue-free hello world program. Surely that means that anything more complicated is almost always going to have some edge case that someone can imagine. Just because it can be imagined and could possibly occur, doesn't mean it is worth the price to fix versus the cost of failure. – Dunk Feb 08 '17 at 18:57
  • 1
    @FilipHaglund You may have a different meaning for what "effort" means in this context than what the answerer intended. It seems to me that what this answer is saying (which I agree with) is that the effort of *the task corresponding to this single PR* does not necessarily include fixing bugs that weren't part of the impetus for the PR. A bug report would be the impetus for a *new, separate* "effort". – Kyle Strand Feb 08 '17 at 19:23
  • @JasperN.Brouwer On the contrary, if the old code handled the edge-case, then the PR would be introducing a *new* bug, i.e. a regression. That would be unacceptable. As it is, however, it sounds like the PR is a strict improvement over the old code, precisely because it does *not* introduce any new bug and (presumably) it addresses the original task for which it was created. – Kyle Strand Feb 08 '17 at 19:25
  • @KyleStrand Yeah, it just depends on how you want to categorize it. If you want to increase the size of this item to account for this change, or if you want to create a new one to account for the bugfix. I feel like fixing the current implementation is probably lost effort, since it's going to be replaced/modified a lot "soon" anyway by the bugfix. – Filip Haglund Feb 08 '17 at 19:48
  • @FilipHaglund By "fixing the current implementation", do you mean updating the PR code, or developing a bugfix for the old code? – Kyle Strand Feb 08 '17 at 20:53
  • Not always true. If the usage of your code implies behavior X is handled but behavior X is not handles then this is usually alarming. – Harrichael Feb 08 '17 at 21:42
  • @Harrichael Yes, uncovering a new bug may indeed be alarming, and fixing it may become a top priority. That does *not* in any way imply that the fix belongs in a pull request addressing a *separate issue*, though. – Kyle Strand Feb 08 '17 at 22:06
  • Way to ignore what I said. Consider function checkAllStatuses(), and the function handles all statuses but has a rare corner case where a status might not be handled. This is problematic because this is not obvious when using the function. The idea of a function is to abstract away some procedure, but if you do not implement the implied behavior, you are forcing every programmer who uses the function to ignore function abstraction and remember that this rare condition may not be handled. – Harrichael Feb 08 '17 at 22:09
  • @Harrichael Yes, I understand, and no, I didn't ignore what you said. You're saying that this particular bug is a leaky abstraction, and therefore it's alarming. But all non-fatal bugs are leaky abstractions. I am accepting your statement that the newly discovered leak is "alarming" but nevertheless asserting that the fix belongs in a separate PR. – Kyle Strand Feb 09 '17 at 05:33
  • It's good to see so many UI developers ignoring edge cases. That's great in a UI/webapp as you most likely aren't going to kill someone with a minor glitch. In the physical world with senor data and real actuators bad things can cause injury and death. Sensor can report bad data, events can be called out of order, missed or triggered multiple times. – Matthew Whited Feb 09 '17 at 15:55
  • @MatthewWhited I don't see anyone recommending *ignoring* edge cases; the closest I see is Jasper Brouwer's comment that fixing the defect would still be out of scope even if the PR introduced the defect. As I understand them, everyone else is just saying that the decision to fix the issue should be separated from the PR code review, and/or that the actual fix should go in a separate PR. Are you interpreting these comments and this answer differently? – Kyle Strand Feb 09 '17 at 16:31
  • 3
    @FilipHaglund Out of scope for current feature != wontfix. It just means it's a separate task. – Jan Feb 10 '17 at 01:57
  • 1
    I think everyone is missing this crucial point and are instead reacting to the "it-will-probably-never-happen"-part (which rightfully needs a reaction as well). Core reviews are not meant to extend scope, if it was not in the original spec of your task then it's a new ticket and should be introduced as such and prioritized with the rest of the backlog. – kb. Feb 13 '17 at 11:23
53

With complex algorithms, it's very difficult to prove you have thought of every test case that will come up in the real world. When you intentionally leave a test case broken because it won't come up in the real world, you are potentially leaving other test cases broken that you haven't even thought of yet.

The other effect that often happens is when you handle additional test cases, your algorithm necessarily becomes more general, and because of that you find ways to simplify it and make it more robust for all your test cases. This saves you time in maintenance and troubleshooting down the road.

Also, there are umpteen cases of "this should never happen" bugs happening in the wild. That's because your algorithm might not change, but its inputs might change years down the road when no one remembers about this one unhandled use case. That's why experienced developers handle these sorts of things when they are fresh in their minds. It comes back to bite you later if you don't.

Karl Bielefeldt
  • 146,727
  • 38
  • 279
  • 479
  • 7
    You make a very good point in your second paragraph. I've experienced that before, but articulating it is very helpful. – Klik Feb 05 '17 at 17:14
  • 7
    Third case = bingo. I work mostly in legacy code, and there are projects where 80% of the work is just fixing places that made assumptions. I like this answer as is, but I would add that it's sometimes okay to cover such an impossible edge case by setting up a graceful failure with an exact and detailed error message, especially when you're in a time crunch. – Jeutnarg Feb 06 '17 at 17:52
  • I like to log exceptions that say "[Error description] According to the specification [version] signed off by [person who signed it off] this can't happen." – Peter Wone Feb 13 '17 at 05:53
  • But since there was no test case for it before, the code (assuming the specs was to replace the previous) shouldn't have to fit new test cases in that iteration. And if there is no test for that corner case already it should be a new ticket/task to be done, not a code review feedback imo. – kb. Feb 13 '17 at 11:25
38

This is not a technical question but a business strategy decision. You notice the suggested fix is for a very obscure case which will almost never happen. Here it makes a big difference if you are programming a toy or if you are programming say medical equipment or an armed drone. The consequences of a rare malfunction will be very different.

When doing code reviews you should apply a basic understanding of the business priorities when deciding how much to invest in handling rare cases. If you disagree with your colleague in your interpretation of the priorities of the business you may want to have a discussion with someone on the business side of things.

JacquesB
  • 57,310
  • 21
  • 127
  • 176
  • 5
    Exactly, ask somebody else if it's worth covering. I'd at least write the test case and then mark it as "skipped" with comments saying that somebody else made the decision that it wasn't worth implementing. CYA – Sean McSomething Feb 06 '17 at 19:33
  • 2
    Ya, whenever something large, out-of-scope yet not urgent comes up in a code review we tend to create an issue for it in our tracker and then prioritize it alongside the rest of the other stuff we need to get done. Occasionally this means that the task gets banished to the far end of the priority list, but if that's the case then it's really not important. – Tacroy Feb 06 '17 at 20:03
  • 1
    "It's not the odds, it's the stakes!" – user1068 Feb 07 '17 at 19:38
  • 2
    This is the best answer. The question is really about setting priorities and managing risks. – wrschneider Feb 08 '17 at 19:51
  • I agree with this being business decision. Create a ticket with your estimate of time needed to fix it, assign it to your boss (whoever in chain of command is responsible for allocation of your time), and ask for it to have a priority assigned for it (or rejected, as case may be) – Matija Nalis Feb 13 '17 at 12:17
23

Code reviews are not purely about code correctness. In reality, that is quite far down the list of benefits, behind knowledge sharing and being a slow-but-steady process towards creating a team style/design consensus.

As you are experiencing, what counts as "correct" is often debatable, and everyone has their own angle on what that is. In my experience, limited time and attention can also make code reviews highly inconsistent - the same issue might be picked up or not depending on different developers and at different times by the same developer. Reviewers in the mindset of "what would improve this code?" will often suggest changes that they would not add to their own efforts naturally.

As an experienced coder (more than 15 years as a developer), I am often reviewed by coders with fewer years of experience than me. Sometimes they ask me for changes that I mildly disagree with, or think unimportant. However, I still make those changes. I fight battles over changes only when I feel the end result would cause a weakness in the product, where the time cost is very high, or where a "correct" point of view can be made objective (e.g. the change being asked for won't work in the language we are using, or a benchmark shows a claimed performance improvement is not one).

So I suggest pick your battles carefully. Two days coding a test case that you think is not necessary is probably not worth the time/effort to fight. If you are using a review tool, such as GitHub pull requests, then maybe comment there about cost/benefit that you perceive, in order to note your objection, but agree to still do the work. This counts as a gentle push back, so the reviewer knows they are hitting a boundary, and more importantly include your rationale so cases like this can be escalated if they get into deadlock. You want to avoid escalating written disagreements - you don't want to have an Internet forum style argument over work differences - so it may be helpful to talk the issue through first and record a fair summary of the outcome of the discussion, even if you still agree to disagree about the need for the work (it is entirely possible a short friendly discussion will decide for you both anyway).

After the event, this is a good topic for sprint review or development team planning meetings, etc. Present it as neutrally as you can e.g. "In code review, Developer A identified this additional test case, it took extra two days to write, does the team think the extra coverage was justified at this cost?" - this approach works much better if you actually do the work, as it shows you in a positive light; you have done the work, and just want to poll the team for risk aversion vs. feature development.

Peter Mortensen
  • 1,050
  • 2
  • 12
  • 14
Neil Slater
  • 459
  • 2
  • 9
  • 2
    "Present it as neutrally as you can" .. quite. I would go further than NeilS and suggest, as they say, you need to swap "anger for talk". Just instantly and openly say (for example, in the specific example at hand), "Steve, your corner case is non-physical with the current mechanical design, don't you think dude? Let's first decide on whether it's non-physical, and even if it is whether it's worth spending two days on." – Fattie Feb 05 '17 at 13:27
  • 1
    *"If you are using a review tool"* is a key observation. IME, these tools are adequate for keeping a record of the review, but the real work gets done face-to-face with discussion. It's the most efficient way for a two-way flow of information. If you disagree with a review, have that disagreement constructively in person, and only then enter the agreed outcome into the tool. – Toby Speight Feb 06 '17 at 10:56
  • @TobySpeight: I agree and tried to incorporate that into the answer. – Neil Slater Feb 06 '17 at 11:24
  • 1
    2 hours is more what I would not fight. 2 days and I'm not going to be able to finish all of the other tasks I've committed to for the week. – Matthew Read Feb 06 '17 at 23:43
15

I'd advise you to at least assert against the obscure case. That way, not only do future developers see that you actively decided against the case, but with a good failure handling, that should already be in place, this would also catch surprises.

And then, make a test case that asserts that failure. This way, the behaviour is better documented and will show up in unit tests.

This answer obviously assumes that your judgement of the even being "extremely unlikely if even possible" is correct and we can not judge that. But if it is, and your colleague agrees, then an explicit assertion against the event should be a satisfactory solution for both of you.

WorldSEnder
  • 288
  • 1
  • 11
  • Totally agree. If there is **any** case that your code can't handle, check the input as early as possible and fail there. "The program crashes if we do X" is always better than "Our robot kills people if we do X" – Josef Feb 06 '17 at 14:08
  • 1
    Good suggestion. Good code is code that's proven to never fail but, if it does inexplicably fail regardless, fails in a well-defined way that can be repaired. Code that can not possibly go wrong but, [if it does go wrong](https://sites.google.com/site/h2g2theguide/Index/g/704396) turns out to be impossible to get at or repair, is not so great... – leftaroundabout Feb 06 '17 at 17:41
  • This, I was going to post the exact same thing. The reviewer is pointing out a possible failure, how to handle it is a different question. – SH- Feb 09 '17 at 15:51
  • 2
    Oh, no, do not do an assert in your code. If the assert is not compiled in, no one will ever see it. If the assert is compiled in then your robot will crash. I've seen more than one case where an assert for 'something that could never happen' in production code triggered and brought down not only that system but everything that depended on it. – Tom Tanner Feb 13 '17 at 10:28
  • @TomTanner "with a good failure handling, that should already be in place". I am assuming that the code is already able to handle failing assertions here. Which might not be much of a stretch, since safe failure strategies should be part of any physical system. – WorldSEnder Feb 13 '17 at 15:27
13

Since you seem to be new there, there is only one thing you can do - check with the team leader (or project leader). 13 hours is a business decision; for some firms/teams, a lot; for some, nothing. It's not your decision, not yet.

If the lead says "cover that case", fine; if he says "nah, screw it", fine - his decision, his responsibility.

As for code reviews in general, relax about it. Having a task returned to you once or twice is perfectly normal.

Tom
  • 139
  • 4
7

One thing I don't think I've seen addressed in kind, although it was kind of brought up in @SteveBarnes answer:

What are the repercussions of a failure?

In some fields a failure is an error on a web page. A PC blue screens and reboots.

In other fields it's life or death - self driving car locks up. Medical pace maker stops working. Or in Steve's answer: stuff blows up resulting in the loss of millions of dollars.

There is a world of difference in those extremes.

Whether or not 13 hours to cover a "failure" is worth ultimately shouldn't be up to you. It should be up to management and owners. They should have a feel for the greater picture.

You should be able to give a good guess at what is going to be worth it. Will your robot simply slow down or stop? Degraded performance? Or will a failure of the robot cause monetary damage? Loss of life?

The answer to THAT question should drive the answer to "is it worth 13 hours of the companies time". Notice: I said the Companies time. They pay the bills and ultimately decide what's worth it. Your management should have the final say either way.

WernerCD
  • 822
  • 6
  • 10
6

The best way to handle disagreement is the same, regardless of if you're a junior developer or a senior developer, or even a CEO.

Act like Columbo.

If you've never watched any Columbo, it was a pretty fantastic show. Columbo was this very unassuming character - most people thought that he was a little bit crazy and not worth worrying about. But by appearing humble, and just asking people to explain he was able to get his man.

I think it's also related to the Socratic method.

In general you want to ask questions of yourself and others to make sure that you're making the right choices. Not from a position of, "I'm right, you're wrong," but from a position of honest discovery. Or at least as best as you can.

In your case you have two ideas here, but they have fundamentally the same goal: to make the code better.

You're under the impression that skimping on code coverage for a potentially improbable (impossible?) case in favor of development of other features is the best way to do that.

Your coworker is under the impression that being more careful about the corner cases is more valuable.

What do they see that you don't see? What do you see that they don't see? As a junior developer you're actually in a great position because you naturally should be asking questions. In another answer someone mentions how surprisingly likely a corner case is. So you can start out with, "Help me understand - I was under the impression that X, Y, and Z - what am I missing? Why will the widget framboozle? I was under the impression that it would fintuble under cromulent circumstances. Will the swizzle stick actually embiggen the ANZA brushes?"

As you question your assumptions and your findings you will dig down, uncover biases, and eventually figure out what the correct course of action is.

Start with the assumption that everyone on your team is perfectly rational and they have the best interests of the team and the product in mind, just like you. If they're doing something that doesn't make sense, then you need to figure out what you don't know that they do, or what you know that they don't.

Wayne Werner
  • 2,340
  • 2
  • 23
  • 23
5

Maybe talk to the person who is responsible for prioritizing work? In startup could be CTO or product owner. He could help to find if this extra work is required and why. Also you could bring your worries during daily standups (if you have them).

If there is no clear responsibility (e.g. product owner) for planing work, try to talk to people around you. Later it could become an issue that everyone is pushing product to opposite direction.

Konstantin Petrukhnov
  • 375
  • 1
  • 9
3

13 hours is not such a big deal, I'd just do it. Remember you're getting paid for it. Just chalk it up as "job security". Also it's best to keep good karma among the team. Now if it was something that'd take you a week or more, then you could involve your manager and ask him if that's the best use of your time, especially if you don't agree with it.

However, you sound like you need leverage in your group. Here's how you get leverage: ask for forgiveness don't ask for permission. Add stuff to the program as you see fit (within scope of'course, ie make sure it completely solves the problem the boss wants..), and tell the manager or your colleagues after the fact. Don't ask them: "Is it ok if I add feature X". Rather, just add features that you personally want in the program. If they get upset at a new feature or don't agree with, be ok to remove it. If they like it, keep it.

In addition whenever they ask you to do something, go the "extra mile" and add alot of things they forgot to mention or things that would work better than what they said. But don't ask them if it's "ok" to go the extra mile. Just do it and casually tell them about it after it's done. What you're doing is training them..

What happens will be your manager will peg you as a "go-getter" and will start putting his trust in you, and your colleagues will start seeing you as the lead bc you're starting to own the program. And then when things like what you mention happen in the future you'll have more say because you're essentially the star of the team and teammates will back down if you don't agree with them.

  • 8
    While I'm all for programmers being proactive and not merely "taking orders from on high", the way you present this is professionally irresponsible and unethical. You're basically saying the OP should spend the employer's time and money not working on requested features but instead working on features s/he "personally wants", and then spend the employer's time and money removing those features. This doesn't even factor in potential defects that get added or other developer's time to code review/maintain this. I would fire a developer with the attitude you describe, particularly a junior one. – Derek Elkins left SE Feb 05 '17 at 06:59
  • 1
    Well you're right in a way. Especially if the engineer goes off on his own without any sensibility to what the client's vision is. But don't sabotage what I said completely-- I merely said to go the "extra mile". So that means you take what your boss says and you expand on it. And in software, that's the difference between software that looks like trivial shit and software that looks like it's crafted by a professional. I know many developers that do "exactly what they're told" even if what they're told is complete garbage. Those developers never amount to anything. –  Feb 05 '17 at 07:11
  • 2
    There are responsible ways to do what you describe. Many times requirements leave a lot of room and using your judgment there to produce a more polished result (balanced against the effort, including maintenance effort, to achieve it) is a good thing. Proactively pointing out and fixing bugs is usually fine. Spending *your own time* to prototype a feature that you think is in *the company's* interest and presenting it to the team for possible inclusion is also fine. Your "personal wants" are irrelevant, the focus while your being paid should be on the business' interests. – Derek Elkins left SE Feb 05 '17 at 07:12
  • See my second comment, for how I would present what I believe the idea you're trying to get at is. As I said, my issue is more with the way you presented it. Developers need some pride to know that they can make meaningful decisions, but humility to know that they (usually) don't have a broad picture on the business's goals or priorities. More senior developers are less likely to make bad decisions and more likely to know what the business's goals are and how to move toward them. – Derek Elkins left SE Feb 05 '17 at 07:15
  • also note that my comment is for those wanting to transition to lead or consultant level. Companies specifically hire me FOR my opinion. –  Feb 06 '17 at 17:32
3

Code can ALWAYS be better.

If you're in a code review and you don't see anything that might be better or a unit test that might catch a bug, it's not the code that's perfect, but the reviewer that isn't doing their job. Whether you chose to mention the improvement is a personal choice. But almost any time your team does a code review there should be things that someone notices that could be better or everyone likely just wasted their time.

That said, whether you act on the comments or not is up to your team. If your changes fix the issue or adds enough value without change that your team accepts them then merge them in and log their comments into the backlog for someone to address later. If the team finds your changes add more risk or complexity than value then you have to resolve the issues accordingly.

Just remember that any code has at least one more edge case that could be tested and could use at least one more refactoring. This is why code reviews are best done as a group with everyone looking at the same code at the same time. So that at the end everyone can come to a consensus on whether or not the code in review is acceptable (as it is) and adds enough value to merge into the community base or if certain things must be done before there is enough value to merge.

Since you are asking this question I assume you're not actually doing "code reviews", but instead creating a pull request or other submission mechanism for others to comment in a non-deterministic way. So now you're into a management issue and a definition of done. I'd guess your management is indecisive and doesn't actually understand the process and purpose of code reviews and likely doesn't have a "definition of done" (DOD). Because if they did your DOD would explicitly answer this question and you wouldn't have to come here and asked.

How do you fix it? Well, ask you manager to give you a DOD and tell you if you have to always implement all comments. If the person in question is your manager then the answer is self-evident.

Peter Mortensen
  • 1,050
  • 2
  • 12
  • 14
user261610
  • 41
  • 1
3

Code review serves several purposes. One you are obviously aware of is, "Is this code fit for purpose?" In other words, is it functionally correct; is it adequately tested; are the non-obvious parts appropriately commented; does it conform to the project's conventions?

Another part of code review is sharing of knowledge about the system. It's an opportunity for both the author and the reviewer to learn about the changed code and how it interacts with the rest of the system.

A third aspect is that it can provide a review of problems that existed before any changes are made. Quite often, when reviewing someone else's changes, I'll spot something I missed in a previous iteration (quite often something of my own). A statement like, "Here's an opportunity to make this more robust than it was," is not a criticism, and don't take it as one!

My current team regards code review not just as a gateway or hurdle that the code must pass unscathed before commit, but primarily as an opportunity for a somewhat structured discussion of a particular area of functionality. It's one of the most productive occasions for information sharing. (And that's a good reason to share the reviewing around the team, rather than always the same reviewer).

If you perceive code reviews as a confrontational activity, that's a self-fulfilling prophecy. If instead you consider them as the most collaborative part of your work, they will stimulate continual improvements to your product and to how you work together. It helps if a review can be clear about the relative priorities of its suggestions - there's a mile of difference between "I'd like a helpful comment here" and "This breaks if x is ever negative", for example.

Having made a whole lot of general statements above, how does that apply to your specific situation? I hope it's now obvious that my advice is to respond to the review with open questions, and to negotiate what approach has the most value. For your example case where an extra test is suggested, then something like, "Yes, we can test for that; I estimate it will take <time> to implement. Do you think the benefit is worth it? And is there something else we can do to guarantee the test isn't necessary?"

One thing that does strike me when I read your question: if it takes two days' effort to write a new test case, then either your new test is a very different scenario to your existing tests (in which case it probably has a lot of value) or you have identified a need for improved code re-use in the test suite.

Finally, as a general comment on the value of code reviews (and as a pithy summary of what I've said above), I like this statement, in Maintainers Don't Scale by Daniel Vetter:

At least for me, review isn’t just about ensuring good code quality, but also about diffusing knowledge and improving understanding. At first there’s maybe one person, the author (and that’s not a given), understanding the code. After good review there should be at least two people who fully understand it, including corner cases.

Toby Speight
  • 550
  • 3
  • 14
3

This question is not about about the virtues of defensive programming, the dangers of corner cases, or the catastrophic risks of bugs in physical products. In fact it's not even really about software engineering at all.

What it's really about is how a junior practitioner handles an instruction from a senior practitioner when the junior can't agree or appreciate it.

There are two things you need to appreciate about being a junior developer. Firstly, it means that whilst it's possible that you're in the right, and him in the wrong, it is - on balance of probabilities - not likely. If your coworker is making a suggestion that you can't see the value of, you need to seriously entertain the possibility that you're just not experienced enough to understand it. I don't get that sense from this post.

The second thing to appreciate is that your senior partner is so-called because he has more responsibility. If a junior breaks something important, they're not going to be in trouble if they followed instructions. If a senior allowed them to break it, however - by not raising issues in code review, for instance - they would quite rightly be in a lot of bother.

Ultimately, it's an implicit requirement of your job that you observe instructions from those the business trust to lead their projects. Are you generally unable to defer to seniors when there's good reason to value their experience? Do you intend to follow no instruction you cannot understand? Do you think the world should stop until you are convinced? These values are incompatible with working in a team.

A final point. Think back to the projects you wrote six months ago. Think of the projects you wrote at university. See how bad they now seem - all the bugs and upside-down design, the blind spots and the misguided abstractions? What if I told you that in six months' time you'll count the very same flaws in work you're doing today? Does that help demonstrate how many blind spots are in your current approach? Because that is the difference that experience makes.

Jimmy Breck-McKye
  • 448
  • 4
  • 9
2

constantly makes comments to my code that suggest me to do a lot more work than is necessary.

You can give recommendations, but ultimately it's not your role to decide what's necessary. It's your job to implement what management or (in this case your reviewer) decides is necessary. If you disagree with what is necessary too much or too strongly, you will likely find yourself out of a job. Consequently it's part of your professionalism to come to terms with this and be at peace with it.

He had suggested that I handle an obscure case that is extremely unlikely to happen

There are other great answers here that show how even non-bugs (ie. something that can provably not fail ever) still should be re-worked sometimes. (e.g. in the case of building in future safety of the product, following standards etc.) Part of the role of a great developer is to have as much confidence as possible that your code will be robust in every conceivable situation every time, and future-proofed as well, not just working under tested situations under present conditions most of the time

Bradley Thomas
  • 5,090
  • 6
  • 17
  • 26
2

Suggestions to code reviewers to increase the business usefulness of your code review (you as OP should propose such a change):

  • Mark down your comments by type. "Critical"/"Must-Do"/"Optional"/"Suggested improvements"/"nice to have"/"I'm musing".

    If this seems too CDO/anal/complicated, at least use 2 levels: "Must fix to pass review and be allowed to merge your change" / "All others".

Suggestions for handling code review suggestions that seem less critical to make:

  • Create an open ticket in your ticketing system of choice (your team uses one, hopefully?), tracking the suggestion

  • Put the ticket # as a response comment to the code review item if your process allows responses to comments like Fisheye or email reviews do.

  • Reach out to reviewer and explicitly ask if that item is of the "must fix or won't be merged/released" type.

    • If the answer is "Yes", but you disagree, let the person responsible for the project management (PM, your manager, etc...) make a decision - present the disagreement fully and honestly. This isn't about which of you is "right" but about what's better for the project, so PM/manager's job.

Now, treat that ticket as any other development request.

  1. If it's decided to be urgent after escalation, treat it as any urgent dev request. Deprioretize other work and work on this.

  2. Otherwise, work on it according to priority it was assigned and its ROI (which can differ based on your business line as explained in other answers).

DVK
  • 3,576
  • 2
  • 24
  • 28
2

You should not escalate this to the management.

In most companies the management guy will always chose to not write that extra test, to not spend time marginally improving the code quality, to not lose time refactoring.

In a lot of cases the code quality depends on the unwritten rules in the development team and on the extra effort that the programmers put in.

You are a junior developer and this is your first code review. You must accept the advice and do the work. You can only improve the workflow and the rules in your team if you first know and respect them for a while so that you can understand why they are there. Otherwise you'll be that new guy who doesn't respect the rules and becomes the lone wolf of the team.

You are new to the team, follow the advices you get for a while, find out why they are there, don't bring the first advice that you get into question in the scrum meeting. The real business priorities will be evident to you after a while without asking (and may be not what the management guy will tell you face to face).

Claudiu Creanga
  • 298
  • 1
  • 10
1

It is very important you make code that satisfies what your lead / management request. Any other detail would just be a "nice to have". If you are an expert (read that: "if you are not a junior developer") in your field, then you are "eligible" to address minor issues you find along the way without consulting your leaders every time.

If you think something is wrong and you are relatively expert in your field then chances are high you are right.

Here are some statements that could be useful to you:

  • I was requested to do X, code reviewer suggests also doing Y, should I do it or should I move on to more important stuff?
  • You are suggesting Y to me, so can you figure out at least one test case that would catch that behaviour so I can test it? I believe that code won't be called.
  • Maybe shouldn't we develop a safe fallback for uncovered cases first? So we catch them early and fix on the go so we can focus on more important stuff.
  • OK, while I am implementing Y, could you be so kind to write some test cases for it so we get that thing done once and for all?
  • I was requested to do X, and I think I could do Y unless there are other priorities. Next time why don't you file a feature request instead of putting it as a review comment on my code? At least we can hear further opinions from other team members on that feature before implementing it (generally any important stuff should be a feature and should be handled by more than one person; usually code review should be about reviewing code and solutions approaches; the rest is bug-fixing and features).

Do you seriously think that the attitude of your reviewer is damaging the project or do you think he's right most times (except maybe sometimes he just makes minor errors in evaluation and exaggerates that)?

To me it sounds more like he's writing stuff that does not belong in a code review, which is bad practice because it makes it harder for everyone to track stuff. However I don't know what other review comments he made, so I can't say anything on other cases.

In general try to avoid both of the following:

  • You are not doing what has been requested
  • You make your reviewer feel dumb

If he's actually reviewing your code, it's because management trust him more than you.

Peter Mortensen
  • 1,050
  • 2
  • 12
  • 14
CoffeDeveloper
  • 551
  • 1
  • 4
  • 10
-1

When there's disagreement during code review about scope:

  1. Document the scope that's actually covered by the code. Nobody likes nasty surprises.
  2. Realize that scope is a business decision. Scope should already be known by the time you start working on a feature, but if it isn't you can always ask for clarification later.

If the code reviewer is the one making the business decisions, he can change the scope at any time, even during code review, but he isn't doing so in his role as code reviewer.

Peter
  • 3,718
  • 1
  • 12
  • 20
  • this doesn't seem to offer anything substantial over points made and explained in prior 20 answers – gnat Feb 11 '17 at 05:54
-1

If you can't prove the edge case to be impossible, you must assume it to be possible. If it is possible, then it is inevitable that it will eventually occur, and sooner rather than later. If the edge case hasn't occurred in testing, that may be a hint that test coverage is incomplete.

  1. Accept the feedback.
  2. Before making any changes to your code, try your best to build a test for the edge case and see if you can get a test failure (proof that the issue exists). If it's impossible to create a such a test case and get a test failure, then you may be able to conclude that the edge case is actually impossible (although I'd be hesitant to draw such a conclusion).
  3. If you can get a test failure, apply the appropriate code change to pass the test.

For the good of the product, you probably want to be able to produce the edge case and induce a failure, so that you can apply the fix and have confidence that a potential problem has been averted.

The whole point of code reviews is to put additional eyes on the code. None of us is immune from mistakes or oversights. It's all too common to look at a piece of code many times and not notice an obvious error, where a fresh pair of eyes can pick it up immediately.

As you said, you've implemented a new algorithm. It would be unwise to draw conclusions about the behavior of your new algorithm based upon the behavior of or observations about its predecessor.

Zenilogix
  • 309
  • 1
  • 3
  • this doesn't seem to offer anything substantial over points made and explained in prior 21 answers – gnat Feb 13 '17 at 07:39
-2

There are code reviewers who know what they are doing, and if they say something needs to be changed, then it needs to be changed, and when they say something needs to be tested, then it needs to be tested.

There are code reviewers who need to justify their own existence by creating useless work for others.

Which one is which is for you to decide, and how to handle the second kind is more a question for workplace.stackexchange.

If you use scrum, then the question is whether your work does what it is supposed to do (apparently it does), and you can put handling the extremely rare and maybe impossible case on the backlog, where it will be prioritised, and if it goes into a sprint, then your reviewer can feel free to pick it up and do the 13 hours work. If you do job X and because you do job X you realise job Y also needs doing, then job Y doesn't become part of job X, it is its own independent job.

xmedeko
  • 176
  • 1
  • 9
gnasher729
  • 42,090
  • 4
  • 59
  • 119