Wednesday, October 26, 2016

Code review

Pull request from Alice: Added four "wheels" to the crate thing. It now rolls, so you can pull things in it easily. I think this is awesome.



  • Bob (0): Awesome! But lots of changes, needs code review.
  • Steve (-1): No user story referenced in comments. Also why four wheels? This is obviously redundant. Can you make it one? And nail it to all four sides?
    • Alice: @Steve, it will be unstable on one "wheel".
    • Steve: @Alice, AFAIK, everything was stable without "wheel" things. If it requires duplication to be stable, it is bad design, obviously.
    • Andy: @Steve, @Alice, this will still need four nails, which is redundant too. Why don't we nail it to the bottom with one nail?
    • Steve: @Andy, Yup, got me there. Also I think glue will do better.
    • Alice: @Steve, @Andy, it needs nails to turn freely. And four wheels so that it can roll without falling on a side.
    • Andy:@Alice, it does not. We just glue one "wheel" to the bottom and it will be fine.
  • Emma (-2): These "wheels" - why are they round? We've been making crates out of square parts just fine, now it is a learning curve for everyone.
    • Alice: @Emma, square won't roll, this is why they are round.
    • Emma: @Alice, even worse - it can roll away!
    • Alice: @Emma, this is the whole point. It can roll where you want. You can carry more weight this way.
    • Emma: @Alice, I don't understand. Can't we just make crate smaller?
  • Andy (-1): I think this is a myopic design. What if I want to add or remove these "wheels" at run time? Also, they stick out, so it ruins geometry of the crate and the crate can't lay flat on the ground anymore. Also those "nails" go right across two subsystem boundaries, which violates separation of concerns principle. I think these "wheels" should be separate from the crate.
    • Alice: @Andy, why would you want to add more wheels?
    • Andy: @Alice, good question. You should inquire with product management. But I'll try to explain. The crates are square. Technically speaking, "parallelepiped", a 3-dimensional shape. With your add-ons it is not parallelepiped shape anymore, so it is technically not a crate either.
    • Steve: @Andy, the crate is made of five parts, which, technically are parallelepiped shapes themselves, but once put together they form a more complex geometry.
    • Andy: @Steve, touche!
    • Alice: @Andy, my question was why would you want to change the wheels at run time.
    • Andy: @Alice, as I said, I am not a PM.
  • Bob (+1): I think it still has legs, folks, perhaps we can assume some technical debt to address this in future?
    • Steve: @Bob, the Alice's things are "wheels", not "legs".
    • Emma: @Bob, @Steve, actually legs can be square. @Andy, I think you found the solution to the problem.
    • Andy: @Emma Who did?
    • Bob: @Andy, @Emma, doesn't matter, it is all teamwork after all.
    • Andy: @Bob, right!
    • Emma: @Bob, right! Go team!
    • Alice: Guys, legs do not roll.
    • Bob: @Alice, with all due respect, how did YOU came here today? Apparently they do ;-) 
    • Andy: @Alice, yes, they actually do. Check this out: Chebyshev's Lambda Mechanism.
    • Emma: @Andy, whoa this is way too complex.
  • Bob (-2): Sorry, @Alice, turns out production grade implementation is way to complex. Please don't merge.
... 3 months later ...
  • Chang: Merged to staging.
    • Alice: @Chang, what?!
    • Steve: @Chang, @Alice, this is wrong branch.
    • Chang: @Steve, @Alice, sorry, shall we roll back?
    • Ravvanashan: @Chang, that's OK, I fixed it.
    • Alice: @Ravvanashan how???
    • Ravvanashan: @Alice, took the nails out, as @Andy suggested.
    • Alice: @Ravvanashan, didn't wheels fall off?
    • Ravvanashan: @Alice wheels?
    • Alice: @Ravvanashan round thingies.
    • Ravvanashan: @Alice yeah they did so I just put them in the crate.
    • Alice: @Ravvanashan, @Chang, I still have a feeling we need to roll back...
    • Steve: @Chang, @Alice, rebased on current RC branch.
    • Alice: Oh God.
    • Jeffrey: Folks, some patch is deploying to prod. What is this?
    • Steve: @Jeffrey, don't worry, merge problem, I took care of it.
    • Bob: Great teamwork people!
... 3 months later ... Customer support call:

Customer: Hey what are those round things in my crates for?
Support: Don't worry, this is normal.
Customer: Yes, but why did you put them in there?
Support: Beta feature. Just ignore it.
Customer: But your sales guy said these supposed to make the crates roll.
Support: No, this is not supported yet.
Customer: Seriously? Why the hell did we buy this shit then?!
Sales: Whoa-whoa, let us come down everyone. I'm sure this is misunderstanding of some sort. I'm sure techies can figure something out.
Customer: So how do we make crates roll?
Support: We need to escalate to engineering. Alice?
Alice: *sigh* Just nail them to the sides...

No comments:

Post a Comment