6 ms·

One thing I've learning is code review should never be used just as gate-keeping for the senior developers. Rather, let everyone (juniors, QA, non-developers)

One thing I've learning is code review should never be used just as gate-keeping for the senior developers. Rather, let everyone (juniors, QA, non-developers) in the team perform reviews of everyones code, even your senior code. Not only is it a great way to spread/communicate knowledge and understanding in the team. But everyone looks at your code with a different eye and background. You may have written the greatest opus magnum of your programming career. But if the junior or medior can't make sense of it, they will not be able to maintain it when you're gone. So effectively you have failed.

capableweb 6y ago
> You may have written the greatest opus magnum of your programming career. But if the junior or medior can't make sense of it, they will not be able to maintain it when you're gone. So effectively you have failed. Overall I agree with you, yeah. But also no. Of course the code should be as simple to understand as possible, but if the junior or medior doesn't understand matrices, nothing I do will make them understand them. So I mean, yes, but also no. Sometimes it's OK to expect people to have some domain knowledge. Doesn't mean your code should be spaghetti of course.
- ramy_d 6y ago
  yeah I would say in principle this makes sense but in practice a junior will typically say "this is beyond me but you're the senior so if it works, it works!" It is an opportunity to go through the code and concepts at least.
- aequitas 6y ago
  Maybe they don't have to understand the detailed inner workings of an algorithm and such. But they should be able to deal with that piece of code in abstract. A good comment on what a piece of code is intended to do is perfectly fine to know it's place in a greater whole. Not every developer knows how crypto functions work, but they can make good decisions on how these functions should fit in certain logical scenarios or if an implementation is badly written/documented.
- efsavage 6y ago
  > if the junior or medior doesn't understand matrices, nothing I do will make them understand them. Teaching them will.
  - hinkley 6y ago
    You can explain, you can teach, or you can refactor. Sometimes refactoring is the cheapest of the three, but you won't know until you try to explain/teach it.
    - FroshKiller 6y ago
      It was cheaper for me to ask my junior which of two blocks he understood more easily and eliminate the style he didn't grasp immediately throughout the project than it would've been to try to teach him and every programmer that follows him on the project for sure.
    - efsavage 6y ago
      Agreed, but if cheap is your goal, you shouldn't be hiring junior devs. The leanest companies I've seen/worked for were all senior people. "No passengers".
    - eek04_ 6y ago
      Often, explaining/teaching has only short term value in terms of the product. The code will outlive the team; I try to factor the knowledge into the code, making the code easy to deal with. I also spend a lot of time explaining and teaching, as it is necessary, but it's really a stopgap measure. Give it a few years and there's mostly new people.
- lhorie 6y ago
  > if the junior or medior doesn't understand matrices, nothing I do will make them understand them Oh but there are things we can do. We can add comments that link to resources, we can write long prose (both inline in comments and in out-of-band documents such as RFCs and documentation) explaining why we're using something that nobody else in the team understands vs the alternatives, and we can level people up via informal sessions like lunch-and-learns, info sharing sessions, etc. It's true that many juniors aren't capable of following concepts above a certain level of difficulty, but it's also not rare for people to really flourish if given a gentle push. There's definitely a paretto balance when it comes to knowledge sharing. Don't be that person that writes zero docs and hogs all the knowledge when you could spend 20% effort to level up 80% of people to the point where they can start helping each other.
- Kalium 6y ago
  > Overall I agree with you, yeah. But also no. Of course the code should be as simple to understand as possible, but if the junior or medior doesn't understand matrices, nothing I do will make them understand them. There is a difference between code that is complex because it needs to be (ex: matrix mathematics) and code that is complex because you wanted it to be (ex: microservices and kafka and obscure functional languages for what could have been a Rails CRUD webapp).
- Mauricebranagh 6y ago
  Shurly the point is you a document it and if required walk them though the code and mentor them. And not to what some one did mumble years ago when I was taking over the map part of a map reduce product built in PL1/G and say after 3 days "haven't you learnt it all yet"
- wonder_er 6y ago
  > but if the junior or medior doesn't understand matrices, nothing I do will make them understand them. What if you took a hour or a day to help educate an eager junior developer about what you did in that PR? I, personally, don't know matrices. I can damn well learn them if it becomes relevant to me. If we were on the same team, and I said "what's this stuff?" and you said "nothing I can do will make you understand this" I'd be pissed, and that attitude would _permanently sour_ the relationship. I imagine there was a point in time that _you_ did not understand matrices. I've no doubt that matrices can be tricky, difficult to understand, etc. So, lets work together on sharing that knowledge! Once you've helped me learn it (pointing me to resources, pairing with me on an example problem solving session involving matrices) I will be able to maintain the code, understand it, and apply the pattern to novel problems. But not just that, because I am a compulsive documenter of what I've learned, as you teach me, you help me teach _everyone who comes behind me_ TL;DR the attitude of "if jr/med devs don't already understand matrices, nothing I do will make them understand them." is _extremely_ self-limiting, locking you out of tons of potential value, and causing your entire team to be worse off. It's not your fault, per se. I know this attitude is common in the industry. It just really pisses me off, because so many teams are deeply damaged by it. (I know, I know. Maybe you'd love to slow down and teach early-career devs, but the feature death-slog required by management precludes you taking that time... in that case this isn't directly your fault, etc.)
  - gmfawcett 6y ago
    In defense of the seniors... It generally takes people several semesters of Linear Algebra to become competent with "matrix stuff." It's unrealistic to expect every senior dev to also be an expert teacher -- never mind expecting them to know what the latest and best learning resources are (for a subject they may have learned, themselves, before the Web was invented). Sometimes the right and fair substitute for "nothing I can do will help you" is "get yourself a proper education on this subject, somehow, and then we can talk about the code."
    - peterwoerner 6y ago
      I think the issue is that many of us are poor communicators (myself being number 1) so we say nothing I can do will help you instead of I can't teach linear algebra, numerical analysis and do the code review in an hour. Therefore putting a junior engineer who has a weak math background on a code review for my new algorithm which uses spectral sparsification for approximating matrix inverses in n log(n) isn't a good idea.
- hyperpallium2 6y ago
  Will it be maintained, and whose fault that is, are separate issues.
- zffr 6y ago
  > Rather, let everyone (juniors, QA, non-developers) in the team perform reviews of everyones code, even your senior code. I think the OP is just saying that juniors should be allowed to participate in the reviews of senior engineer code (not that a junior review alone is sufficient for merging a PR). I think this is a great way for juniors to get exposure to the complex topics they might not yet understand, but should learn more about.
- 8note 6y ago
  Do you expect these developers to be able to debug this code after its in production? Or is that bottlenecked to yourself? Having most of your team not be able to understand the code they support sounds dangerous
nitrogen 6y ago
As an opportunity to learn how to read and maintain the code that is being written, and to spot random mistakes that slip past the linter that everyone makes once in a while, yes. In that sense everyone should participate in code review. But if you are giving veto power over code and designs to your most inexperienced developers, you will not have a good time.
- aequitas 6y ago
  Mileage may vary and it depends on the direct impact an approved review has (ie: is it CD to production or are there other quality gatekeepers), but giving junior members this responsibility can greatly help them in personal growth. They can always ask for a co-review if they think they are not able to make a decision, which itself can be another learning moment. And you don't have to tell them you secretly also review the code before it is pushed to prod ;).
- Gh0stRAT 6y ago
  Early in my career, I learned so much from reviewing other more senior peoples' code. I suppose I technically did have "veto power" as my approval was required in order to merge the PR, but nobody ever thought of it in such adversarial terms. We'd comment with questions or things that were unclear, occasionally asking "why are we doing it this way instead of that way?" and more often than not the solution was simply adding/updating comments to make things more clear. For our team at least, it was a casual and collaborative effort where everyone learned from each other. I wouldn't hesitate to give inexperienced devs veto power over my PRs again unless it was for a time-sensitive hot-fix.
thinkingkong 6y ago
You need to redefine success if you think that’s failure. The software is almost by definition not a magnum opus if it isnt understandable or well documented. This attitude is somewhat pervasive but we really need to set better examples through clarity; not cleverness.
- hinkley 6y ago
  Everyone is the hero in their own story.
vbtemp 6y ago
I enthusiastically believed this in the past, but then I started observing the reverse: Code reviews became gatekeeping for junior developers... In other words, junior developers were using it as opportunities to assert power (via obstruction). Of course this was idiosyncratic to the company and team. I've become less positive toward code reviews over the years as well - the low-hanging fruit of style checking, linting, and related stuff has been automated away. Moreover, people are busy and when they do code reviews, they are mostly looking for easy stuff to pick at. What ends up getting avoided is the far more important algorithm, design and architecture reviews that really require a much bigger cognitive load.
- deleted 6y ago
  [deleted]
- rightbyte 6y ago
  I feel code review is overrated. It makes refactoring and small fixes take like 2h longer. I atleast fill up my parallel tasking slots too fast with small things if they linger in review. So either I don't do them or get less done. Code review might be good for a subset of code, like retrofitting bugfixes to release branches or babysitting a team of beginners.
- bowlich 6y ago
  Conversely if you do attempt to do a deeper dive into the design and architecture choices my experience has been you end up getting push back and overruled -- because people are busy and don't want to miss a deadline with code that "works"/ passes whatever the minimum bar was set to hurl it into production.
  - vbtemp 6y ago
    I completely agree. The irony for me has been how immensely helpful toolchains (PRs, awesome unit test harnesses, powerful CI systems, etc etc) actually kind of make things worse and support everyone missing the forest for the trees. Everything has become dislocated from reality. "Issues" and "tickets" does not capture actual work to do. Unit tests and CI do not capture whether something is truly functional. Peer-reviews do not mean a given commit is stronger than otherwise... At the end of the day there's just no replacement for good judgment and experience, despite all the fancy tools that want to try to "gamify" it.
Nursie 6y ago
> even your senior code. Especially your senior code, by the time you get to being an experienced engineer you really should appreciate the value of lots of eyes on your work. You may indeed have written the greatest opus magnum of your programming career, but I bet there are a few little bugs or edge cases or something you've overlooked. There's always something.
eithed 6y ago
I strongly disagree - while juniors are more than welcome to look at the PRs, if you allow juniors to approve PRs of other juniors you quickly find your codebase deteriorating.
- apengwin 6y ago
  That's not what they're saying. Srs. should still review jr. PRs. They're arguing that Jrs should also review Sr. PRs
  - eithed 6y ago
    Well, if you'll allow everybody to review everybody PRs then eventually that's what you'll get. Similarly - junior might look at your PR and see that it's ok. Senior might look at it, see that you've forgot about a thing that junior didn't know about, you omitted to test a certain condition, etc. Again, juniors are more than welcome to look at PRs and be a part of discussion, but if you're merging, you should be wary. I'd personally be against allowing a merge if you get only a junior approval.
    - lowbloodsugar 6y ago
      The difference is "approve" vs "criteria for merging". We have a system that requires two approvals, but one must be from a list of seniors (which doesn't include new hires).
      - eithed 6y ago
        Mhm - if you'll separate your review process from approval process, then yes.
    - CodeMage 6y ago
      > Well, if you'll allow everybody to review everybody PRs then eventually that's what you'll get. If you allow everybody to review every PR, it doesn't imply that you'll allow everybody to approve every PR.
_throwawayyyyyy 6y ago
exactly - was on a team where an individual contributor got promoted vertically to manager and thought they could create a good product by code reviewing everyone to death and demanding that they be the only one allowed to code review. no more tagging other teammates; just this manager
taylodl 6y ago
I've found code reviews to be a waste of time. Here's my primary concern: what evidence do you have that this code actually works? I want you to show me your test plan, I want you to show me you thought through how this code is going to be used and have tests for it. I'm way more interested in that than I am whether variable names use camel case or snake case or whatever. In fact if your tests are complete, and pass, then we can always refactor the code later if we need to.
- mathgladiator 6y ago
  This all depends on your dev environment; a code review should also have tests with the diff as well as test results, and the more automated the better.
- spaetzleesser 6y ago
  I often find stuff and also get suggestions for things that could have been done better. I view code reviews more as a mutual learning experience similar to pair programming. Checking for variable names is silly.
  - aequitas 6y ago
    When I stopped seeing code reviews as gate keepers and started seeing them as collaboration and communication points it finally made sense to me. But reviews only work well if your team shares a common goal, like maintainability or knowledge transfer. I've worked in a place where code reviews where treated to just a box to have checked off. There was no shared responsibility for the codebase. And most people had a mentality of "if you approve mine, I'll approve yours". Still it was kept as a "safeguard" by management because for them it was also a checkbox they could mark on their process checklist. I'm happy I left, there is only so much culture a single person can change in a team/company.
- dougmwne 6y ago
  In QA I have found a code review to be very helpful in identifying test cases. Often I'll think I've covered every edge case I can think of, but once I've read through the code I identify the implementation can fail in a certin scenario I hadn't considered. This is why I think Black Box testing is a bad approach unless something about the circumstance specifically calls for it. I do think it's better to have a test plan first, then supplement it with code review, just to not get overly biased.

capableweb 6y ago

> You may have written the greatest opus magnum of your programming career. But if the junior or medior can't make sense of it, they will not be able to maintain it when you're gone. So effectively you have failed. Overall I agree with you, yeah. But also no. Of course the code should be as simple to understand as possible, but if the junior or medior doesn't understand matrices, nothing I do will make them understand them. So I mean, yes, but also no. Sometimes it's OK to expect people to have some domain knowledge. Doesn't mean your code should be spaghetti of course.

ramy_d 6y ago
yeah I would say in principle this makes sense but in practice a junior will typically say "this is beyond me but you're the senior so if it works, it works!" It is an opportunity to go through the code and concepts at least.
aequitas 6y ago
Maybe they don't have to understand the detailed inner workings of an algorithm and such. But they should be able to deal with that piece of code in abstract. A good comment on what a piece of code is intended to do is perfectly fine to know it's place in a greater whole. Not every developer knows how crypto functions work, but they can make good decisions on how these functions should fit in certain logical scenarios or if an implementation is badly written/documented.
efsavage 6y ago
> if the junior or medior doesn't understand matrices, nothing I do will make them understand them. Teaching them will.
- hinkley 6y ago
  You can explain, you can teach, or you can refactor. Sometimes refactoring is the cheapest of the three, but you won't know until you try to explain/teach it.
  - FroshKiller 6y ago
    It was cheaper for me to ask my junior which of two blocks he understood more easily and eliminate the style he didn't grasp immediately throughout the project than it would've been to try to teach him and every programmer that follows him on the project for sure.
  - efsavage 6y ago
    Agreed, but if cheap is your goal, you shouldn't be hiring junior devs. The leanest companies I've seen/worked for were all senior people. "No passengers".
  - eek04_ 6y ago
    Often, explaining/teaching has only short term value in terms of the product. The code will outlive the team; I try to factor the knowledge into the code, making the code easy to deal with. I also spend a lot of time explaining and teaching, as it is necessary, but it's really a stopgap measure. Give it a few years and there's mostly new people.
lhorie 6y ago
> if the junior or medior doesn't understand matrices, nothing I do will make them understand them Oh but there are things we can do. We can add comments that link to resources, we can write long prose (both inline in comments and in out-of-band documents such as RFCs and documentation) explaining why we're using something that nobody else in the team understands vs the alternatives, and we can level people up via informal sessions like lunch-and-learns, info sharing sessions, etc. It's true that many juniors aren't capable of following concepts above a certain level of difficulty, but it's also not rare for people to really flourish if given a gentle push. There's definitely a paretto balance when it comes to knowledge sharing. Don't be that person that writes zero docs and hogs all the knowledge when you could spend 20% effort to level up 80% of people to the point where they can start helping each other.
Kalium 6y ago
> Overall I agree with you, yeah. But also no. Of course the code should be as simple to understand as possible, but if the junior or medior doesn't understand matrices, nothing I do will make them understand them. There is a difference between code that is complex because it needs to be (ex: matrix mathematics) and code that is complex because you wanted it to be (ex: microservices and kafka and obscure functional languages for what could have been a Rails CRUD webapp).
Mauricebranagh 6y ago
Shurly the point is you a document it and if required walk them though the code and mentor them. And not to what some one did mumble years ago when I was taking over the map part of a map reduce product built in PL1/G and say after 3 days "haven't you learnt it all yet"
wonder_er 6y ago
> but if the junior or medior doesn't understand matrices, nothing I do will make them understand them. What if you took a hour or a day to help educate an eager junior developer about what you did in that PR? I, personally, don't know matrices. I can damn well learn them if it becomes relevant to me. If we were on the same team, and I said "what's this stuff?" and you said "nothing I can do will make you understand this" I'd be pissed, and that attitude would _permanently sour_ the relationship. I imagine there was a point in time that _you_ did not understand matrices. I've no doubt that matrices can be tricky, difficult to understand, etc. So, lets work together on sharing that knowledge! Once you've helped me learn it (pointing me to resources, pairing with me on an example problem solving session involving matrices) I will be able to maintain the code, understand it, and apply the pattern to novel problems. But not just that, because I am a compulsive documenter of what I've learned, as you teach me, you help me teach _everyone who comes behind me_ TL;DR the attitude of "if jr/med devs don't already understand matrices, nothing I do will make them understand them." is _extremely_ self-limiting, locking you out of tons of potential value, and causing your entire team to be worse off. It's not your fault, per se. I know this attitude is common in the industry. It just really pisses me off, because so many teams are deeply damaged by it. (I know, I know. Maybe you'd love to slow down and teach early-career devs, but the feature death-slog required by management precludes you taking that time... in that case this isn't directly your fault, etc.)
- gmfawcett 6y ago
  In defense of the seniors... It generally takes people several semesters of Linear Algebra to become competent with "matrix stuff." It's unrealistic to expect every senior dev to also be an expert teacher -- never mind expecting them to know what the latest and best learning resources are (for a subject they may have learned, themselves, before the Web was invented). Sometimes the right and fair substitute for "nothing I can do will help you" is "get yourself a proper education on this subject, somehow, and then we can talk about the code."
  - peterwoerner 6y ago
    I think the issue is that many of us are poor communicators (myself being number 1) so we say nothing I can do will help you instead of I can't teach linear algebra, numerical analysis and do the code review in an hour. Therefore putting a junior engineer who has a weak math background on a code review for my new algorithm which uses spectral sparsification for approximating matrix inverses in n log(n) isn't a good idea.
hyperpallium2 6y ago
Will it be maintained, and whose fault that is, are separate issues.
zffr 6y ago
> Rather, let everyone (juniors, QA, non-developers) in the team perform reviews of everyones code, even your senior code. I think the OP is just saying that juniors should be allowed to participate in the reviews of senior engineer code (not that a junior review alone is sufficient for merging a PR). I think this is a great way for juniors to get exposure to the complex topics they might not yet understand, but should learn more about.
8note 6y ago
Do you expect these developers to be able to debug this code after its in production? Or is that bottlenecked to yourself? Having most of your team not be able to understand the code they support sounds dangerous

nitrogen 6y ago

As an opportunity to learn how to read and maintain the code that is being written, and to spot random mistakes that slip past the linter that everyone makes once in a while, yes. In that sense everyone should participate in code review. But if you are giving veto power over code and designs to your most inexperienced developers, you will not have a good time.

aequitas 6y ago
Mileage may vary and it depends on the direct impact an approved review has (ie: is it CD to production or are there other quality gatekeepers), but giving junior members this responsibility can greatly help them in personal growth. They can always ask for a co-review if they think they are not able to make a decision, which itself can be another learning moment. And you don't have to tell them you secretly also review the code before it is pushed to prod ;).
Gh0stRAT 6y ago
Early in my career, I learned so much from reviewing other more senior peoples' code. I suppose I technically did have "veto power" as my approval was required in order to merge the PR, but nobody ever thought of it in such adversarial terms. We'd comment with questions or things that were unclear, occasionally asking "why are we doing it this way instead of that way?" and more often than not the solution was simply adding/updating comments to make things more clear. For our team at least, it was a casual and collaborative effort where everyone learned from each other. I wouldn't hesitate to give inexperienced devs veto power over my PRs again unless it was for a time-sensitive hot-fix.

thinkingkong 6y ago

You need to redefine success if you think that’s failure. The software is almost by definition not a magnum opus if it isnt understandable or well documented. This attitude is somewhat pervasive but we really need to set better examples through clarity; not cleverness.

hinkley 6y ago
Everyone is the hero in their own story.

vbtemp 6y ago

I enthusiastically believed this in the past, but then I started observing the reverse: Code reviews became gatekeeping for junior developers... In other words, junior developers were using it as opportunities to assert power (via obstruction). Of course this was idiosyncratic to the company and team. I've become less positive toward code reviews over the years as well - the low-hanging fruit of style checking, linting, and related stuff has been automated away. Moreover, people are busy and when they do code reviews, they are mostly looking for easy stuff to pick at. What ends up getting avoided is the far more important algorithm, design and architecture reviews that really require a much bigger cognitive load.

deleted 6y ago
[deleted]
rightbyte 6y ago
I feel code review is overrated. It makes refactoring and small fixes take like 2h longer. I atleast fill up my parallel tasking slots too fast with small things if they linger in review. So either I don't do them or get less done. Code review might be good for a subset of code, like retrofitting bugfixes to release branches or babysitting a team of beginners.
bowlich 6y ago
Conversely if you do attempt to do a deeper dive into the design and architecture choices my experience has been you end up getting push back and overruled -- because people are busy and don't want to miss a deadline with code that "works"/ passes whatever the minimum bar was set to hurl it into production.
- vbtemp 6y ago
  I completely agree. The irony for me has been how immensely helpful toolchains (PRs, awesome unit test harnesses, powerful CI systems, etc etc) actually kind of make things worse and support everyone missing the forest for the trees. Everything has become dislocated from reality. "Issues" and "tickets" does not capture actual work to do. Unit tests and CI do not capture whether something is truly functional. Peer-reviews do not mean a given commit is stronger than otherwise... At the end of the day there's just no replacement for good judgment and experience, despite all the fancy tools that want to try to "gamify" it.

Nursie 6y ago

> even your senior code. Especially your senior code, by the time you get to being an experienced engineer you really should appreciate the value of lots of eyes on your work. You may indeed have written the greatest opus magnum of your programming career, but I bet there are a few little bugs or edge cases or something you've overlooked. There's always something.

eithed 6y ago

I strongly disagree - while juniors are more than welcome to look at the PRs, if you allow juniors to approve PRs of other juniors you quickly find your codebase deteriorating.

apengwin 6y ago
That's not what they're saying. Srs. should still review jr. PRs. They're arguing that Jrs should also review Sr. PRs
- eithed 6y ago
  Well, if you'll allow everybody to review everybody PRs then eventually that's what you'll get. Similarly - junior might look at your PR and see that it's ok. Senior might look at it, see that you've forgot about a thing that junior didn't know about, you omitted to test a certain condition, etc. Again, juniors are more than welcome to look at PRs and be a part of discussion, but if you're merging, you should be wary. I'd personally be against allowing a merge if you get only a junior approval.
  - lowbloodsugar 6y ago
    The difference is "approve" vs "criteria for merging". We have a system that requires two approvals, but one must be from a list of seniors (which doesn't include new hires).
    - eithed 6y ago
      Mhm - if you'll separate your review process from approval process, then yes.
  - CodeMage 6y ago
    > Well, if you'll allow everybody to review everybody PRs then eventually that's what you'll get. If you allow everybody to review every PR, it doesn't imply that you'll allow everybody to approve every PR.

_throwawayyyyyy 6y ago

exactly - was on a team where an individual contributor got promoted vertically to manager and thought they could create a good product by code reviewing everyone to death and demanding that they be the only one allowed to code review. no more tagging other teammates; just this manager

taylodl 6y ago

I've found code reviews to be a waste of time. Here's my primary concern: what evidence do you have that this code actually works? I want you to show me your test plan, I want you to show me you thought through how this code is going to be used and have tests for it. I'm way more interested in that than I am whether variable names use camel case or snake case or whatever. In fact if your tests are complete, and pass, then we can always refactor the code later if we need to.

mathgladiator 6y ago
This all depends on your dev environment; a code review should also have tests with the diff as well as test results, and the more automated the better.
spaetzleesser 6y ago
I often find stuff and also get suggestions for things that could have been done better. I view code reviews more as a mutual learning experience similar to pair programming. Checking for variable names is silly.
- aequitas 6y ago
  When I stopped seeing code reviews as gate keepers and started seeing them as collaboration and communication points it finally made sense to me. But reviews only work well if your team shares a common goal, like maintainability or knowledge transfer. I've worked in a place where code reviews where treated to just a box to have checked off. There was no shared responsibility for the codebase. And most people had a mentality of "if you approve mine, I'll approve yours". Still it was kept as a "safeguard" by management because for them it was also a checkbox they could mark on their process checklist. I'm happy I left, there is only so much culture a single person can change in a team/company.
dougmwne 6y ago
In QA I have found a code review to be very helpful in identifying test cases. Often I'll think I've covered every edge case I can think of, but once I've read through the code I identify the implementation can fail in a certin scenario I hadn't considered. This is why I think Black Box testing is a bad approach unless something about the circumstance specifically calls for it. I do think it's better to have a test plan first, then supplement it with code review, just to not get overly biased.