TFS CodeReview question
-
Taking 2 weeks is just bad. Either your review process way too slow/under-prioritized. Or your commits are way to big. Smaller commits are easier to review. If you have a rule saying "Only 100% functional units can be committed to master" then that rule is counter-productive.
... such stuff as dreams are made on
megaadam wrote:
Smaller commits are easier to review.
:thumbsup:
-
At my new work they like each new commit to go through a code review in a shelveset before making it to the dev branch. Unfortunately code review are done very slowly, sometimes taking up to 2 weeks, and that cause me a lot of trouble when merging back since incompatible change have been introduce since then... How do you tackle that problem? One idea which just strikes me is that maybe I could delay my own reviews of my team mate's code, until my own code has been reviewed... this way I prevent introduction of incompatible change...
A new .NET Serializer All in one Menu-Ribbon Bar Taking over the world since 1371!
-
At my new work they like each new commit to go through a code review in a shelveset before making it to the dev branch. Unfortunately code review are done very slowly, sometimes taking up to 2 weeks, and that cause me a lot of trouble when merging back since incompatible change have been introduce since then... How do you tackle that problem? One idea which just strikes me is that maybe I could delay my own reviews of my team mate's code, until my own code has been reviewed... this way I prevent introduction of incompatible change...
A new .NET Serializer All in one Menu-Ribbon Bar Taking over the world since 1371!
Super Lloyd wrote:
code review are done very slowly, sometimes taking up to 2 weeks
Two things - 1) that's just stupid, 2) how stupid are the people you work with to tolerate this? Sorry about my bluntness. ;) Marc
Latest Article - Create a Dockerized Python Fiddle Web App Learning to code with python is like learning to swim with those little arm floaties. It gives you undeserved confidence and will eventually drown you. - DangerBunny Artificial intelligence is the only remedy for natural stupidity. - CDP1802
-
At my new work they like each new commit to go through a code review in a shelveset before making it to the dev branch. Unfortunately code review are done very slowly, sometimes taking up to 2 weeks, and that cause me a lot of trouble when merging back since incompatible change have been introduce since then... How do you tackle that problem? One idea which just strikes me is that maybe I could delay my own reviews of my team mate's code, until my own code has been reviewed... this way I prevent introduction of incompatible change...
A new .NET Serializer All in one Menu-Ribbon Bar Taking over the world since 1371!
Super Lloyd wrote:
sometimes taking up to 2 weeks
:wtf:
Super Lloyd wrote:
How do you tackle that problem?
Change the process or get a new job. You can't possibly keep dealing with conflicts like that.
There are two kinds of people in the world: those who can extrapolate from incomplete data. There are only 10 types of people in the world, those who understand binary and those who don't.
-
At my new work they like each new commit to go through a code review in a shelveset before making it to the dev branch. Unfortunately code review are done very slowly, sometimes taking up to 2 weeks, and that cause me a lot of trouble when merging back since incompatible change have been introduce since then... How do you tackle that problem? One idea which just strikes me is that maybe I could delay my own reviews of my team mate's code, until my own code has been reviewed... this way I prevent introduction of incompatible change...
A new .NET Serializer All in one Menu-Ribbon Bar Taking over the world since 1371!
-
At my new work they like each new commit to go through a code review in a shelveset before making it to the dev branch. Unfortunately code review are done very slowly, sometimes taking up to 2 weeks, and that cause me a lot of trouble when merging back since incompatible change have been introduce since then... How do you tackle that problem? One idea which just strikes me is that maybe I could delay my own reviews of my team mate's code, until my own code has been reviewed... this way I prevent introduction of incompatible change...
A new .NET Serializer All in one Menu-Ribbon Bar Taking over the world since 1371!
To make a blanket statement that 2 weeks to do a code review is wrong is, well... wrong. What if it is an a nuclear or other critical environment? Having said that, the code review time frame should be as fast or faster than the development environment. If the code review is taking longer, then management needs to adjust priorities. I am working on a project that has multiple event based calculations; the process was falling behind because the calculations could to be processed as fast as they were received. We had to rework the calculations to get them processed in a more efficient manner... you have the same situation; your 'events' are code waiting to be reviewed; your 'review' is your calculation... something needs to be adjusted.
-
To make a blanket statement that 2 weeks to do a code review is wrong is, well... wrong. What if it is an a nuclear or other critical environment? Having said that, the code review time frame should be as fast or faster than the development environment. If the code review is taking longer, then management needs to adjust priorities. I am working on a project that has multiple event based calculations; the process was falling behind because the calculations could to be processed as fast as they were received. We had to rework the calculations to get them processed in a more efficient manner... you have the same situation; your 'events' are code waiting to be reviewed; your 'review' is your calculation... something needs to be adjusted.
Yeah, the tech lead told us code review are important and must be done first and quick. I must insist on it being resolved with the other developer when submitted. plus there is a developer who is particularly slow at them.... and he is going to another team!
A new .NET Serializer All in one Menu-Ribbon Bar Taking over the world since 1371!
-
To make a blanket statement that 2 weeks to do a code review is wrong is, well... wrong. What if it is an a nuclear or other critical environment? Having said that, the code review time frame should be as fast or faster than the development environment. If the code review is taking longer, then management needs to adjust priorities. I am working on a project that has multiple event based calculations; the process was falling behind because the calculations could to be processed as fast as they were received. We had to rework the calculations to get them processed in a more efficient manner... you have the same situation; your 'events' are code waiting to be reviewed; your 'review' is your calculation... something needs to be adjusted.
Tim Carmichael wrote:
What if it is an a nuclear or other critical environment?
Wow, I can't imaging a code review taking more than a couple of hours, tops. How long does a nuclear code review take, if you don't mind me asking?
-
Tim Carmichael wrote:
What if it is an a nuclear or other critical environment?
Wow, I can't imaging a code review taking more than a couple of hours, tops. How long does a nuclear code review take, if you don't mind me asking?
Think of the complexity of a simple change and everything it touches. Remember DLL hell? Imagine that at a nuclear site; the change must be reviewed from a code perspective, but it must also be reviewed from the perspective of what it can impact. What happens if the code is put into production with an error? So... a two week review is not unheard of... there is a reason nuclear sites are not running the latest and greatest versions of operating systems. Now, from a colleague that worked nuclear... So , for example when I would write and engineering change (EC) package... You are required to get a peer review - they would formally document their review and you had to document how you disposioned each of their comments. After they were happy with how you addressed their comments then they would sign off on your package. The time it took for someone to review the package would take 1/3 - 2/3 the time it took to write it. Because your review is your signature of the work just as you did it. It is what I know as a true peer review. After you get a peer review you then would have to get discipline specific reviews done. For example, you have to get a 10CFR50.69 review, I&C Engineering Review, Operations Review, Maintenance review, sometimes even Chemistry and Radiation depends on what I was putting in and who was impacted by my change to the plant design You have to disposition the comments for each person and they are captured in the package.
-
Think of the complexity of a simple change and everything it touches. Remember DLL hell? Imagine that at a nuclear site; the change must be reviewed from a code perspective, but it must also be reviewed from the perspective of what it can impact. What happens if the code is put into production with an error? So... a two week review is not unheard of... there is a reason nuclear sites are not running the latest and greatest versions of operating systems. Now, from a colleague that worked nuclear... So , for example when I would write and engineering change (EC) package... You are required to get a peer review - they would formally document their review and you had to document how you disposioned each of their comments. After they were happy with how you addressed their comments then they would sign off on your package. The time it took for someone to review the package would take 1/3 - 2/3 the time it took to write it. Because your review is your signature of the work just as you did it. It is what I know as a true peer review. After you get a peer review you then would have to get discipline specific reviews done. For example, you have to get a 10CFR50.69 review, I&C Engineering Review, Operations Review, Maintenance review, sometimes even Chemistry and Radiation depends on what I was putting in and who was impacted by my change to the plant design You have to disposition the comments for each person and they are captured in the package.
Wow! I am so glad I don't work on nuclear projects. :wtf:
-
At my new work they like each new commit to go through a code review in a shelveset before making it to the dev branch. Unfortunately code review are done very slowly, sometimes taking up to 2 weeks, and that cause me a lot of trouble when merging back since incompatible change have been introduce since then... How do you tackle that problem? One idea which just strikes me is that maybe I could delay my own reviews of my team mate's code, until my own code has been reviewed... this way I prevent introduction of incompatible change...
A new .NET Serializer All in one Menu-Ribbon Bar Taking over the world since 1371!
2 weeks code review is your impediment, you can ask your boss to remove it. Be agile. :-)
-
At my new work they like each new commit to go through a code review in a shelveset before making it to the dev branch. Unfortunately code review are done very slowly, sometimes taking up to 2 weeks, and that cause me a lot of trouble when merging back since incompatible change have been introduce since then... How do you tackle that problem? One idea which just strikes me is that maybe I could delay my own reviews of my team mate's code, until my own code has been reviewed... this way I prevent introduction of incompatible change...
A new .NET Serializer All in one Menu-Ribbon Bar Taking over the world since 1371!
The flaw in the current process is that the code should really be reviewed "post integration", not pre integration. What if the review is fine, but due to the two week lag the merging/integration introduces a defect? What good did the review do? "Works on my branch" will become the new mantra. Maybe all of the merging needs to happen on a "review" branch. Items are migrated from "review" to "dev" in the order they are committed on "review" Or else flip it. Integrate on "dev", migrate via code review to "review" in commit order and build masters from "review" instead of dev. If someone really wants the fifth package in the "queue" to make it into a build, the preceding four packages will have to be reviewed first. Another approach would be to perform weekly reviews post label/milestone/sprint. Depending on the churn in certain files, some people would end up reviewing impacts from multiple packages at one time, but I do not see why that would be a problem. This might be more efficient vs. making developers switch more frequently between writing/reviewing. Also, per another comment. It sounds like the team is not blocking off enough time to perform reviews. For 3 days of coding, you probably need 1 day of review. Make Monday review day... no new dev work until the review backlog is cleared out.
-
At my new work they like each new commit to go through a code review in a shelveset before making it to the dev branch. Unfortunately code review are done very slowly, sometimes taking up to 2 weeks, and that cause me a lot of trouble when merging back since incompatible change have been introduce since then... How do you tackle that problem? One idea which just strikes me is that maybe I could delay my own reviews of my team mate's code, until my own code has been reviewed... this way I prevent introduction of incompatible change...
A new .NET Serializer All in one Menu-Ribbon Bar Taking over the world since 1371!
Change your organization OR Change your organization (Fix it, or leave, LOL). Okay, 2 weeks is insane. I would BEG at the minimum for a Peer-Review to get things moving. And more complete review later (could it be that you are NEW?) When we get new developers, we do DAILY code reviews with them, and do NOT let them check anything in unless reviewed. But this is done more as mentoring and training to get them up to speed, and to identify their strengths and weaknesses... Is this temporary?
-
To make a blanket statement that 2 weeks to do a code review is wrong is, well... wrong. What if it is an a nuclear or other critical environment? Having said that, the code review time frame should be as fast or faster than the development environment. If the code review is taking longer, then management needs to adjust priorities. I am working on a project that has multiple event based calculations; the process was falling behind because the calculations could to be processed as fast as they were received. We had to rework the calculations to get them processed in a more efficient manner... you have the same situation; your 'events' are code waiting to be reviewed; your 'review' is your calculation... something needs to be adjusted.
I've worked in medical devices for the past 10 years. The longest code review we ever did was a day, and that included checking out the code and performing a full regression test with the physical devices. Small checkins are good, large checkins are bad. Merging can be challenging enough with a small checkin, depending on what's being changed. Then add a two week time frame and that's a huge disaster waiting to happen. Two weeks? Time to bring this up with the manager or whoever is in charge. That's ridiculous. :doh:
"Computer games don't affect kids; I mean if Pac-Man affected us as kids, we'd all be running around in darkened rooms, munching magic pills and listening to repetitive electronic music." -- Marcus Brigstocke, British Comedian
-
At my new work they like each new commit to go through a code review in a shelveset before making it to the dev branch. Unfortunately code review are done very slowly, sometimes taking up to 2 weeks, and that cause me a lot of trouble when merging back since incompatible change have been introduce since then... How do you tackle that problem? One idea which just strikes me is that maybe I could delay my own reviews of my team mate's code, until my own code has been reviewed... this way I prevent introduction of incompatible change...
A new .NET Serializer All in one Menu-Ribbon Bar Taking over the world since 1371!