Sample Code Reviews....
-
@blakeyrat said in Sample Code Reviews....:
@thecpuwizard said in Sample Code Reviews....:
But the scenario could easily be "I am replacing intems in the list, or adding them and it is not important exactly what elements are summed" [perhaps we are trying to get the average of the latest 1000000 samples - though this would be a sub optimal way".
This just confuses me. The only thing the method does is sum the integers. How is it adding or replacing items in the list? How is it trying to get an average?
One does not know what the caller is going to do with the sum. Perhaps divide by the count (that they received, which could be different than the account being summed.
@thecpuwizard said in Sample Code Reviews....:
Apologies, you did mention this before, and I meant to ask for clarification because I am not following....
It's not obvious? You're summing an arbitrary number of integers and putting the result in an integer. You're not doing anything to handle an overflow. You either need to handle overflows smartly (presumably by raising an exception), or put the result in a data type that can handle arbitrarily large integers (like
System.Numerics.BigInteger.)Ahhh... Yes. But that is common to both implementations and would not [IMPO] be involved in evaluating the change do the code.
-
@thecpuwizard said in Sample Code Reviews....:
@blakeyrat said in Sample Code Reviews....:
@thecpuwizard said in Sample Code Reviews....:
But the scenario could easily be "I am replacing intems in the list, or adding them and it is not important exactly what elements are summed" [perhaps we are trying to get the average of the latest 1000000 samples - though this would be a sub optimal way".
This just confuses me. The only thing the method does is sum the integers. How is it adding or replacing items in the list? How is it trying to get an average?
One does not know what the caller is going to do with the sum. Perhaps divide by the count (that they received, which could be different than the account being summed.
Then I think you should give us an exact specification of what the function is supposed to do before we continue discussing the changes. So far, it seems we got:
- May return the sum of the integers in the list.
- May also arbitrarily return something else, indeterministically.
- May throw
IndexOutOfRangeExceptionwhen changed asynchronously. - Must not throw
CollectionModifiedException.
If that's the specification,
return 0;is a valid implementation.
It seems that your points is the programmer doesn't actually know the spec, so any change in code must never cause any observable difference in behavior. This would lead me to the conclusion that you should just never change anything and call it a day.
-
@thecpuwizard said in Sample Code Reviews....:
@blakeyrat said in Sample Code Reviews....:
> > It's not obvious? You're summing an arbitrary number of integers and putting the result in an integer. You're not doing anything to handle an overflow. You either need to handle overflows smartly (presumably by raising an exception), or put the result in a data type that can handle arbitrarily large integers (like
System.Numerics.BigInteger.)Ahhh... Yes. But that is common to both implementations and would not [IMPO] be involved in evaluating the change do the code.
Once again, you're holding out on the context here. If the bug was that "overflows are causing problems" then not addressing that earns a rejection.
You're talking about evaluating something but there's no criteria to evaluate on (as presented)! So I am still rejecting this change as an inexplicable waste of resources.
-
@boomzilla said in Sample Code Reviews....:
Once again, you're holding out on the context here. If the bug was that "overflows are causing problems" then not addressing that earns a rejection.
You're talking about evaluating something but there's no criteria to evaluate on (as presented)! So I am still rejecting this change as an inexplicable waste of resources.
Good points...
I was not "holding out on context", merely looking at a very small focus. But lets take a look at the overflow element that Blakey pointed out.
a) If the driving force (bug report, PBI, requirement, feature, whatever) was specifically about an overflow occurring, then I would hope that everyone would agree to reject as the proposed change since it does nothing to addressed it.
That leaves the alternative of
b) The driving force did not mention overflow. From my perspective it would be better to create a new bug that has the overflow issue identified, and then have a different changeset/commit which addresses only that issue.
@boomzilla said in Sample Code Reviews....:
So I am still rejecting this change as an inexplicable waste of resources.
And yet so many changesets/PR's/etc contain changes that have nothing to do with the specific issue. Which I believe is vadlid reason for rejecting the change...
@topspin said in Sample Code Reviews....:
t seems that your points is the programmer doesn't actually know the spec, so any change in code must never cause any observable difference in behavior. This would lead me to the conclusion that you should just never change anything and call it a day.
If one follows SOLID principles (primarily focused on OCP) then one indeed should never change and observable behavior. One can extend (add new things that do not have any observable impact on existing) or create something new.
Adopting this requires a mindset shift and an approach to software design (I am not saying that one should or should not adopt this - though personally I have found it to be valuable.
-
@thecpuwizard said in Sample Code Reviews....:
I was not "holding out on context", merely looking at a very small focus.
Except that nothing was in focus due to the lack of motivation for the change.
@thecpuwizard said in Sample Code Reviews....:
From my perspective it would be better to create a new bug that has the overflow issue identified, and then have a different changeset/commit which addresses only that issue.
I agree. But they original hypothetical had none of that so there was no reason for the change at all.
@thecpuwizard said in Sample Code Reviews....:
And yet so many changesets/PR's/etc contain changes that have nothing to do with the specific issue. Which I believe is vadlid reason for rejecting the change...
Obviously, I agree, as I expressed this sentiment several times in the thread.
-
@boomzilla said in Sample Code Reviews....:
@blakeyrat said in Sample Code Reviews....:
BlockingCollection
-
@thecpuwizard said in Sample Code Reviews....:
a) If the driving force (bug report, PBI, requirement, feature, whatever) was specifically about an overflow occurring, then I would hope that everyone would agree to reject as the proposed change since it does nothing to addressed it.
That leaves the alternative of
b) The driving force did not mention overflow. From my perspective it would be better to create a new bug that has the overflow issue identified, and then have a different changeset/commit which addresses only that issue.The driving force is Tom from accounts receivable really likes the word "sum" and wants to see it used more in the code. He wrote the ticket on his phone while totaling up receipts and eating dim sum at the restaurant across the street.
Oh look, code change approved, thumbs up, review that shit. Obvious bug about overflowing? Doesn't matter, Tom likes it.
-
@blakeyrat said in Sample Code Reviews....:
@thecpuwizard said in Sample Code Reviews....:
a) If the driving force (bug report, PBI, requirement, feature, whatever) was specifically about an overflow occurring, then I would hope that everyone would agree to reject as the proposed change since it does nothing to addressed it.
That leaves the alternative of
b) The driving force did not mention overflow. From my perspective it would be better to create a new bug that has the overflow issue identified, and then have a different changeset/commit which addresses only that issue.The driving force is Tom from accounts receivable really likes the word "sum" and wants to see it used more in the code. He wrote the ticket on his phone while totaling up receipts and eating dim sum at the restaurant across the street.
Oh look, code change approved, thumbs up, review that shit. Obvious bug about overflowing? Doesn't matter, Tom likes it.
Alas, that type of WTF is far too common in the real world...... But would it not be better to stop the stupidity (or at least slow it down) during the initial submission review, or the execution planning review, or the ..., rather than waiting until after a developer has spent time implementing the code to complete the task that is intended to deliver the value [aka the CODE review]?????
-
@thecpuwizard Ok, here's, as far as I can tell, your intended response to the question:
The B version can throw CollectionChangedException. Which is bad. Even though the A version can throw IndexOutOfRangeException. Which is apparently not at all bad, even though it's equally an Exception and equally unhandled?
(And forgive me if I'm misquoting you, but that's how I read the thing.)
To the question I replied, "a much more pertinent error is the fact that overflow is unhandled in both versions." To which you replied:
Well since that bug is in the original code, you shouldn't point it out in a code review
Which to me makes zero sense. If the goal of the code review is to improve the quality of the code, you improve the quality of the code. Right? Am I insane or are you?
To your intended response to the question, I replied: while it's technically true that that Exception could be thrown by the B version:
- There's an equivalent Exception that could be thrown by the A version in virtually identical circumstances
- If these collections were intended to be changed in another thread, they should be implemented with the concurrent versions designed for that usage
But apparently that was a "wrong" answer too.
Basically this is a long-winded way of saying: I have no fucking clue what this thread is about.
Also:
@thecpuwizard said in Sample Code Reviews....:
during the initial submission review, or the execution planning review, or the ...,
How does your team get around to putting in 37 commits every nanosecond with all these review meetings?
-
@blakeyrat said in Sample Code Reviews....:
To the question I replied, "a much more pertinent error is the fact that overflow is unhandled in both versions." To which you replied:
Well since that bug is in the original code, you shouldn't point it out in a code review
Which to me makes zero sense. If the goal of the code review is to improve the quality of the code, you improve the quality of the code. Right? Am I insane or are you?
Yeah...if the bug wasn't in the original code, then what's the point? Also, it wouldn't be the first time that a bug fix didn't fix the bug.
-
@blakeyrat said in Sample Code Reviews....:
Some good questions, let me try to answer.The B version can throw CollectionChangedException. Which is bad. Even though the A version can throw IndexOutOfRangeException. Which is apparently not at all bad, even though it's equally an Exception and equally unhandled?
The code is already in use, by who knows (could be anyone). They may have code that specifically COUNTS on this behavior. So [IMPO] the focus is about the CHANGE and the ramifications ot that.
Well since that bug is in the original code, you shouldn't point it out in a code review
More accurately, this code review, which is reviewing a CHANGE. It should definitely have been part of the review where the original version was written.
Which to me makes zero sense. If the goal of the code review is to improve the quality of the code, you improve the quality of the code. Right? Am I insane or are you?
99% certain we are both crazy, though likely in different ways and to different degrees. That being said I believe that HOW you improve the quality of the code is important.
Basically this is a long-winded way of saying: I have no fucking clue what this thread is about.
:)
How does your team get around to putting in 37 commits every nanosecond with all these review meetings?
More accurately a commit per developer (who is working on code at the time) every 15-45 minutes... So ensuring that human reviews are extremely fast and focus on things that can not be verified by an automated process is key, and has been a major investment.
-
@thecpuwizard said in Sample Code Reviews....:
More accurately, this code review, which is reviewing a CHANGE.
So you basically reject every commit that changes existing code. Is that really what the open/closed thingy in SOLID is about?
You can't even fix bugs, because someone could rely on them.
-
@topspin said in Sample Code Reviews....:
So you basically reject every commit that changes existing code. Is that really what the open/closed thingy in SOLID is about?
You can't even fix bugs, because someone could rely on them.True to a degree. If you "fix a bug" you risk breaking consumers who have written code that is dependent on the bug!
The alternative is that you use the other principles [SRP, LSP, ISP, DIP] so that your design gives you the ability to provide the "correct" behavior without violation of OCP.
I just checked one project where this principle has been strictly held to [this does not mean zero exceptions, but they are deliberately difficult to get approved]. Over 500K LOC (significant size, not mega huge). 87% of all files in the source control repository are still at their original version [zero edits once committed] even though the project has been running for nearly 5 years of active development.
-
Can someone enlighten me why cross-Thread access to the
Listin question is a big deal when the original post didn't indicate in any way that it was supposed to be threadsafe?edit: Now that I read the thread again, I concur with @boomzilla
What's supposed to be the point behind this?
-
@thecpuwizard said in Sample Code Reviews....:
True to a degree. If you "fix a bug" you risk breaking consumers who have written code that is dependent on the bug!
You're not writing code for MS Windows, are you?
-
@rhywden said in Sample Code Reviews....:
@thecpuwizard said in Sample Code Reviews....:
True to a degree. If you "fix a bug" you risk breaking consumers who have written code that is dependent on the bug!
You're not writing code for MS Windows, are you?
Depends on exactly how you mean that ;)
-
This is why I don't like Code Review Wankery. Something that is quite simple is taken into absurdity.
-
@rhywden It depends sometimes people have written around a bug in a 3rd party API like I did quite a few times, while working at Betfred.
You have never worked in the real world by the looks of it mate.
-
@thecpuwizard said in Sample Code Reviews....:
True to a degree. If you "fix a bug" you risk breaking consumers who have written code that is dependent on the bug!
This. You are correct.
-
@blakeyrat I am sorry your life if so horrible.
It must be terrifying to deliver what you have been asked to do.
-
@thecpuwizard said in Sample Code Reviews....:
True to a degree. If you "fix a bug" you risk breaking consumers who have written code that is dependent on the bug!
I've worked on a project where I fixed bugs that end users relied upon to cook the books in illegal ways, where they then put in change requests to have the buggy way of working something that could be enabled.
Bugs should be corrected wherever found. If this breaks something downstream, congratulations, they have found a bug in their system for free. They now need to fix it to use a later version of the system or resource I was working on, but such is the way of software development.
All software is shit and full of bugs. If you do not fix bugs or make it slightly less shitty because someone depends on it being a particular kind of buggy piece of shit, you are keeping software in a shittier state than need be. I think this is bad.
-
