code review
-
try
{
Be_Born();
while (1)
{
try
{
Live();
Make_Money();
}
catch (FinancialException const& e)
{
// handle financial exception
}
catch (MedicalException const& e)
{
// handle medical exception
}
}}
catch (DeathException const& e)
{
Die();
}Freedom is the freedom to say that two plus two make four. If that is granted, all else follows. -- 6079 Smith W.
-
I once wrote a program with a variable int hell_freezes_over = 0; The loop control was do ... until hell_freezes_over The languange didn't support infinite loops.
-
well.. lucky you. the larger the team the higher the chance to have that coworker (those coworkers?) who wants to micromanage reviews
A new .NET Serializer All in one Menu-Ribbon Bar Taking over the world since 1371!
-
Super Lloyd wrote:
Anyway, someone was updating my code
One of my earlier co-workers discovered that my code contained a '#define ever ;;' so that I could write an infinite loop as 'for (ever) { ...'. A few years earlier, I was working in a CHILL environment; in CHILL 'DO FOR EVER' is a part of the base language. This construction made my coworker so upset that he not only changed it where he had discovered it; he searched through the entire code base of the company for other uses, and found a good handful; I had been programming with 'for (ever) {' for a couple of years by then (in embedded software, infinite loops are commonplace), changing every one of them to 'while (0) {', adding an angry commit comment that requested everybody to refrain from making such 'jokes' in our program code - we are a serious company! Then he brought it up at the scrum, to make sure that everybody would know that in our company, we do not code that way. I asked if we could make it slightly less cryptic by using 'while (true) {', but this fellow would not under any circumstances accept that. According to him, there is one, single way of coding an infinite loop, that is immediately recognized by every programmer in the world, and that is 'while (0)'. So he would not tolerate anything else. He certainly had no formal authority, completing his degree about three years earlier, having worked in the company for half a year, I was 25 years his senior. I didn't have to accept his dictate. But he had an unbearable arrogance and self confidence that I didn't care to fight against, so I just nodded "OK!". At least that episode gave me a story to tell - this is certainly not the first time :-)
trønderen wrote:
According to him, there is one, single way of coding an infinite loop, that is immediately recognized by every programmer in the world, and that is 'while (1)'
(Changed the 0 to 1 per your earlier replies) Lint may disagree with his assertion that while(1) {} is the only and preferred way to write a loop - I have had it prefer the for(;;) construct and both are versions that are easily recognised. I wouldn't personally be a fan of "#define ever ;;" as it "feels" fragile both in requiring the definition be included consistently and the risk someone may one day want to name a variable or function as that. Would be something for an interesting private discussion rather than "show and tell" at a scrum meeting, however!
-
I often feel like code review are mostly filled with unneeded comment for the sake of commenting or to take some sort of "ownership" that doesn't do anything special beside burdening the reviewee with a special cosmetic change udpate. Anyway, someone was updating my code and I was feeling revengeful so I also asserted my opinion on purely cosmetic and totally irrelevant code! ;P Although, come to think of it, the guy was doing unnecessary work in Dispose() because "that's what is done everywhere", even though it's not needed and closing documents take godamn too long (due to all those unnecessary piece of code running in all those Dispose() methods), mmmrrppphhh... I stick to my gun!
A new .NET Serializer All in one Menu-Ribbon Bar Taking over the world since 1371!
Years ago, I had the same. A colleague wrote an export routine to export a number of tables to an XML file. The tables where already loaded in memory. He came up with an over the top export routine of about 20.000 lines of code. He had been working on it for 3 weeks. I had to do the code review. I looked at it and rejected ALL of it. (And I wasn't revengeful) I told him we could do it with ONE little statement. (ds.GetXml() - where ds is the already loaded DataSet):cool: He never spoke to me again until I left the company. :-D
-
I often feel like code review are mostly filled with unneeded comment for the sake of commenting or to take some sort of "ownership" that doesn't do anything special beside burdening the reviewee with a special cosmetic change udpate. Anyway, someone was updating my code and I was feeling revengeful so I also asserted my opinion on purely cosmetic and totally irrelevant code! ;P Although, come to think of it, the guy was doing unnecessary work in Dispose() because "that's what is done everywhere", even though it's not needed and closing documents take godamn too long (due to all those unnecessary piece of code running in all those Dispose() methods), mmmrrppphhh... I stick to my gun!
A new .NET Serializer All in one Menu-Ribbon Bar Taking over the world since 1371!
You feel this way partly because a lot of people do code review wrong. Y'all need to check out Dr Michaela Greiler: 1. Follow [https://twitter.com/mgreiler\](https://twitter.com/mgreiler) if you use the bird site 2. Read her [code review checklist](https://www.michaelagreiler.com/code-review-checklist-2/) 3. Perhaps book a [code review workshop](https://www.awesomecodereviews.com/) if your team is really getting bumpy around code review time (I haven't worked through any of her workshops, but have a lot of faith they will be good based on her newsletter) Where I work, code review is an integral part of the dev cycle. No code gets back into the main line without review. When I joined the place I work at now, code review was new to me. Often it felt like brutal, unnecessary nitpicking. What's changed since then is two-fold: 1. all of us are striving to be better reviewers, because (a) that's good for the product, (b) that's good for the reviewee (fostering good relationships and the ability to learn), (c) that's good for the reviewer (not wasting time on pointless bull💩) 2. I think we've all learned to detach the _self_ from the _code_. You are, in the words of Tyler Durdin, not your fscking car keys. You are not your fscking code. You are fallable, like everyone else, and can, when open to it, learn, both to be a better coder, and to be humble, which makes you a better _person_. _Just because your code has an issue, it doesn't make you less of a person, or even less of a coder. It just grants you an opportunity to learn, when someone helps you discover that issue._ Of course, that "someone" has to learn how to review without personal attack - see recommendations above.
------------------------------------------------ If you say that getting the money is the most important thing You will spend your life completely wasting your time You will be doing things you don't like doing In order to go on living That is, to go on doing things you don't like doing Which is stupid. - Alan Watts https://www.youtube.com/watch?v=-gXTZM\_uPMY
-
I often feel like code review are mostly filled with unneeded comment for the sake of commenting or to take some sort of "ownership" that doesn't do anything special beside burdening the reviewee with a special cosmetic change udpate. Anyway, someone was updating my code and I was feeling revengeful so I also asserted my opinion on purely cosmetic and totally irrelevant code! ;P Although, come to think of it, the guy was doing unnecessary work in Dispose() because "that's what is done everywhere", even though it's not needed and closing documents take godamn too long (due to all those unnecessary piece of code running in all those Dispose() methods), mmmrrppphhh... I stick to my gun!
A new .NET Serializer All in one Menu-Ribbon Bar Taking over the world since 1371!
Too many people treat code reviews as a way to pick apart someone's efforts. Personally I look at them as a way to ask questions, offer suggestions and even as a learning opportunity for myself and the rest of the team. If someone has found a better way of performing a task, it gives us all a chance to absorb that lesson and use for ourselves. It should be a two-way street, with the reviewers having that opportunity to see and understand the nature of the new/added code. Egos need to be left at the door when doing these reviews, and the participants need to keep an open mind...
-
Too many people treat code reviews as a way to pick apart someone's efforts. Personally I look at them as a way to ask questions, offer suggestions and even as a learning opportunity for myself and the rest of the team. If someone has found a better way of performing a task, it gives us all a chance to absorb that lesson and use for ourselves. It should be a two-way street, with the reviewers having that opportunity to see and understand the nature of the new/added code. Egos need to be left at the door when doing these reviews, and the participants need to keep an open mind...
Occasionally, a review of proposed algorithms for solutions (not VS solutions) would have pseudo code presented to the group. It helped establish the flow chart of logic to the solution. No review of code was required. As professionals it was assumed we knew what we were doing. There were exceptions such as debugging problems, etc. but rare as a group. Our shop was not that large so meetings were more often 1 on 1. Picking apart code was something done by our professors in school, or by a selected colleague. You were responsible for picking apart your own code. If your code had to be plugged into a system, then a dummy system was used to test it. Here a group meeting might be called to review that test, before going live. It has been awhile and things have changed including coding. Richard Hamming, American Mathematician (Hamming code fame) "Mathematicians stand on on each other's shoulders and computer scientists stand on each other's toes."
"A little time, a little trouble, your better day" Badfinger
-
I often feel like code review are mostly filled with unneeded comment for the sake of commenting or to take some sort of "ownership" that doesn't do anything special beside burdening the reviewee with a special cosmetic change udpate. Anyway, someone was updating my code and I was feeling revengeful so I also asserted my opinion on purely cosmetic and totally irrelevant code! ;P Although, come to think of it, the guy was doing unnecessary work in Dispose() because "that's what is done everywhere", even though it's not needed and closing documents take godamn too long (due to all those unnecessary piece of code running in all those Dispose() methods), mmmrrppphhh... I stick to my gun!
A new .NET Serializer All in one Menu-Ribbon Bar Taking over the world since 1371!
Super Lloyd wrote:
I often feel like code review are mostly filled with unneeded comment for the sake of commenting or to take some sort of "ownership" that doesn't do anything special beside burdening the reviewee with a special cosmetic change udpate.
All I can say in my experience of many years is that reviewers often do nothing but rubberstamp the review process. So if you are seeing this from many people I would be curious exactly what they are commenting on so frequently in your code.
Super Lloyd wrote:
was doing unnecessary work in Dispose() because "that's what is done everywhere", even though it's not needed and closing documents take godamn too long (due to all those unnecessary piece of code running in all those Dispose()
If you have a "document" which is taking too long to close and you have actually profiled the problem to be a problem with Dispose method then I would question your architecture and design. Or perhaps the definition of "document".
-
Super Lloyd wrote:
Anyway, someone was updating my code
One of my earlier co-workers discovered that my code contained a '#define ever ;;' so that I could write an infinite loop as 'for (ever) { ...'. A few years earlier, I was working in a CHILL environment; in CHILL 'DO FOR EVER' is a part of the base language. This construction made my coworker so upset that he not only changed it where he had discovered it; he searched through the entire code base of the company for other uses, and found a good handful; I had been programming with 'for (ever) {' for a couple of years by then (in embedded software, infinite loops are commonplace), changing every one of them to 'while (0) {', adding an angry commit comment that requested everybody to refrain from making such 'jokes' in our program code - we are a serious company! Then he brought it up at the scrum, to make sure that everybody would know that in our company, we do not code that way. I asked if we could make it slightly less cryptic by using 'while (true) {', but this fellow would not under any circumstances accept that. According to him, there is one, single way of coding an infinite loop, that is immediately recognized by every programmer in the world, and that is 'while (0)'. So he would not tolerate anything else. He certainly had no formal authority, completing his degree about three years earlier, having worked in the company for half a year, I was 25 years his senior. I didn't have to accept his dictate. But he had an unbearable arrogance and self confidence that I didn't care to fight against, so I just nodded "OK!". At least that episode gave me a story to tell - this is certainly not the first time :-)
trønderen wrote:
his construction made my coworker so upset that he not only changed it where he had discovered it; he searched through the entire code base of the company for other uses, and found a good handful; I had been programming with 'for (ever)
Myself I take the view that I am working in and for a business. So I write code with the view that someone in the future will need to read my code, understand it, and modify it to meet the needs which I cannot possibly predict when I write the code. So I try to make the code as easy to understand as possible. So it is not what I want but rather how to best produce something that is more useable for the future.