Where do you draw the line on cleaning up someone elses code
-
Beyond a certain point you cross over from something being obviously crap, to "I like style X better than style Y"; at which point the main thing you're doing becomes adding noise to your source control diffs? Does it matter if other devs are still working on it, or if it's all you now? Does having a tool like Resharper, whose utility in finding stuff the compiler warnings miss is badly degraded if it's spazzing out over style issues, affect where you draw the line? What if only part of the team has the tool?
3x12=36 2x12=24 1x12=12 0x12=18
Dan Neely wrote:
Where do you draw the line?
You draw the line through all of their code and start over from scratch. :)
It was broke, so I fixed it.
-
Beyond a certain point you cross over from something being obviously crap, to "I like style X better than style Y"; at which point the main thing you're doing becomes adding noise to your source control diffs? Does it matter if other devs are still working on it, or if it's all you now? Does having a tool like Resharper, whose utility in finding stuff the compiler warnings miss is badly degraded if it's spazzing out over style issues, affect where you draw the line? What if only part of the team has the tool?
3x12=36 2x12=24 1x12=12 0x12=18
I generally don't mess with style unless it is totally messed up. In which case I check-out, reformat the whole document, check in with the comment that all I did was reformat. Then on with code changes. As a general rule though all code should be formatted in the default style within VS. The real formatting issues here are in the SQL stored procedures where we seem to have many distinctly different styles to the point where you can tell who wrote what by the way it's formatted. I've tried to get people to standardize on any of the formats, but have so far received no support on that. If only there was a tool that did as good of a job formatting SQL as VS does with C# code.
-
Beyond a certain point you cross over from something being obviously crap, to "I like style X better than style Y"; at which point the main thing you're doing becomes adding noise to your source control diffs? Does it matter if other devs are still working on it, or if it's all you now? Does having a tool like Resharper, whose utility in finding stuff the compiler warnings miss is badly degraded if it's spazzing out over style issues, affect where you draw the line? What if only part of the team has the tool?
3x12=36 2x12=24 1x12=12 0x12=18
In a professional setting, you should use your company's coding standards to draw the line. If they don't have one, work with the team to develop one you like. As a hobbyist, I tend to use the SharpDevelop Guidelines (PDF)[^]. About the only thing I change is I always put a brace on a newline instead of only on classes and interfaces. So, my typical if/then statement would be:
if (a + b)
{
// code block;
}instead of:
if (a + b) {
// code block;
}Sometimes I slip back to Hungarian notation for controls and variables, but I usually catch myself early enough to adjust the source accordingly. :-\ Flynn
-
Beyond a certain point you cross over from something being obviously crap, to "I like style X better than style Y"; at which point the main thing you're doing becomes adding noise to your source control diffs? Does it matter if other devs are still working on it, or if it's all you now? Does having a tool like Resharper, whose utility in finding stuff the compiler warnings miss is badly degraded if it's spazzing out over style issues, affect where you draw the line? What if only part of the team has the tool?
3x12=36 2x12=24 1x12=12 0x12=18
If I can read the code and understand it whatever style is fine. I don't care about casing or variable names. I've seen some people blow a gasket over naming conventions and such. To me it's just not that important. What the code is doing matters. How it looks, not so much.
-
If I can read the code and understand it whatever style is fine. I don't care about casing or variable names. I've seen some people blow a gasket over naming conventions and such. To me it's just not that important. What the code is doing matters. How it looks, not so much.
See my new signature. :)
Martin Fowler wrote:
Any fool can write code that a computer can understand. Good programmers write code that humans can understand.
-
In a professional setting, you should use your company's coding standards to draw the line. If they don't have one, work with the team to develop one you like. As a hobbyist, I tend to use the SharpDevelop Guidelines (PDF)[^]. About the only thing I change is I always put a brace on a newline instead of only on classes and interfaces. So, my typical if/then statement would be:
if (a + b)
{
// code block;
}instead of:
if (a + b) {
// code block;
}Sometimes I slip back to Hungarian notation for controls and variables, but I usually catch myself early enough to adjust the source accordingly. :-\ Flynn
Flynn Arrowstarr / Regular Schmoe wrote:
In a professional setting, you should use your company's coding standards to draw the line. If they don't have one, work with the team to develop one you like.
:laugh: :laugh: :laugh: I'd like to introduce you to my friend Bugfree the flying pig; he shares a cube (specifically a space adjacent to the back wall) with me and is, among other things, in charge of the coding standard development project. The first draft is expected on the 32nd of never. :sigh:
3x12=36 2x12=24 1x12=12 0x12=18
-
Flynn Arrowstarr / Regular Schmoe wrote:
In a professional setting, you should use your company's coding standards to draw the line. If they don't have one, work with the team to develop one you like.
:laugh: :laugh: :laugh: I'd like to introduce you to my friend Bugfree the flying pig; he shares a cube (specifically a space adjacent to the back wall) with me and is, among other things, in charge of the coding standard development project. The first draft is expected on the 32nd of never. :sigh:
3x12=36 2x12=24 1x12=12 0x12=18
I hear you. That's why I'm glad I only code for me. I can keep to my standards and not want to strangle the people around me. :laugh: Flynn
-
I generally don't mess with style unless it is totally messed up. In which case I check-out, reformat the whole document, check in with the comment that all I did was reformat. Then on with code changes. As a general rule though all code should be formatted in the default style within VS. The real formatting issues here are in the SQL stored procedures where we seem to have many distinctly different styles to the point where you can tell who wrote what by the way it's formatted. I've tried to get people to standardize on any of the formats, but have so far received no support on that. If only there was a tool that did as good of a job formatting SQL as VS does with C# code.
Yeah, were I to force a standard I'd probably go with resharpers (combined with getting all .net devs a copy of it); the only place I'm aware of it not matching with what VS does automatically in C# is that it wants to eat the _ in event handlers. It does add default styles for various other things (public/private/etc members); and points out chunks of unneeded code (
this., superfluous casts, unneeded.ToString()calls, the verbose event handler attaching method needed in older versions of .net, etc). Stuff like that is where most of the potential changes would come from. Enforcing a consistent style across code makes it easier to read, and while not entirely what I'd've picked all the defaults are reasonable, but the initial implementation pass can easily result in a checkin with enough changes that automatically diffing past it becomes nearly impossible. One example from a file I'm doing documentation on atm is ~300 suggested changes in ~2500 lines of code (closer to 2000 if refactored to use the autoproperties not available when it was originally written). That's only from the warning (yellow) list, there're a number of lesser code cleanup items (green) but I don't know how to get a total count. Even spread across multiple checkins (remove redundant code, renames, code simplification/cleanup) something like that becomes a real mess in history.3x12=36 2x12=24 1x12=12 0x12=18
-
I hear you. That's why I'm glad I only code for me. I can keep to my standards and not want to strangle the people around me. :laugh: Flynn
A decent chunk of the stuff resharper is picking on in my current code base is stuff that made perfect sense when I was writing in .net 1.1. eg intelisence didn't kick in for members unless you typed
this.; so my old code is blanketed in that verbose idiom. What I'd really like would be a diff tool that would let me flag checkins to be ignored by default and be smart enough to display real code changes when crossing it. I suspect that doing so would be impossible in non-trivial cases however.3x12=36 2x12=24 1x12=12 0x12=18
-
A decent chunk of the stuff resharper is picking on in my current code base is stuff that made perfect sense when I was writing in .net 1.1. eg intelisence didn't kick in for members unless you typed
this.; so my old code is blanketed in that verbose idiom. What I'd really like would be a diff tool that would let me flag checkins to be ignored by default and be smart enough to display real code changes when crossing it. I suspect that doing so would be impossible in non-trivial cases however.3x12=36 2x12=24 1x12=12 0x12=18
Dan Neely wrote:
What I'd really like would be a diff tool that would let me flag checkins to be ignored by default and be smart enough to display real code changes when crossing it. I suspect that doing so would be impossible in non-trivial cases however.
I suspect you may be right. Flynn
-
In a professional setting, you should use your company's coding standards to draw the line. If they don't have one, work with the team to develop one you like. As a hobbyist, I tend to use the SharpDevelop Guidelines (PDF)[^]. About the only thing I change is I always put a brace on a newline instead of only on classes and interfaces. So, my typical if/then statement would be:
if (a + b)
{
// code block;
}instead of:
if (a + b) {
// code block;
}Sometimes I slip back to Hungarian notation for controls and variables, but I usually catch myself early enough to adjust the source accordingly. :-\ Flynn
I have a co-worker who formats all his code like this:
if (a && b) {
// Code goes here
printf("Do something.");
}(Notice the closing brace.) It drives me absolutely batty! :mad:
WE ARE DYSLEXIC OF BORG. Refutance is systile. Your a$$ will be laminated. There are 10 kinds of people in the world: People who know binary and people who don't.
-
I generally don't mess with style unless it is totally messed up. In which case I check-out, reformat the whole document, check in with the comment that all I did was reformat. Then on with code changes. As a general rule though all code should be formatted in the default style within VS. The real formatting issues here are in the SQL stored procedures where we seem to have many distinctly different styles to the point where you can tell who wrote what by the way it's formatted. I've tried to get people to standardize on any of the formats, but have so far received no support on that. If only there was a tool that did as good of a job formatting SQL as VS does with C# code.
Jeremy Hutchinson wrote:
As a general rule though all code should be formatted in the default style within VS.
Amen! :thumbsup:
WE ARE DYSLEXIC OF BORG. Refutance is systile. Your a$$ will be laminated. There are 10 kinds of people in the world: People who know binary and people who don't.
-
If I can read the code and understand it whatever style is fine. I don't care about casing or variable names. I've seen some people blow a gasket over naming conventions and such. To me it's just not that important. What the code is doing matters. How it looks, not so much.
I'll second that; I've been maintaining a puddle of pooh (in VB.net no less) for the last year. I try to only fix bugs, not style.
-
A decent chunk of the stuff resharper is picking on in my current code base is stuff that made perfect sense when I was writing in .net 1.1. eg intelisence didn't kick in for members unless you typed
this.; so my old code is blanketed in that verbose idiom. What I'd really like would be a diff tool that would let me flag checkins to be ignored by default and be smart enough to display real code changes when crossing it. I suspect that doing so would be impossible in non-trivial cases however.3x12=36 2x12=24 1x12=12 0x12=18
Maybe it could compile the code, and run through some variables and expected output for the changed methods. If the outputs change from known input/output set that was ran 'last time' the code was checked in, then the code has changed, otherwise the code just 'looks different'. hmm, an actual reason to bother with unit testing...... ---------------------------------------------------------------------- Get super cheap unlimited backup for 5 computers with this 'special' link: http://www.lushbackup.com?refid=matesrates[^]
-
I have a co-worker who formats all his code like this:
if (a && b) {
// Code goes here
printf("Do something.");
}(Notice the closing brace.) It drives me absolutely batty! :mad:
WE ARE DYSLEXIC OF BORG. Refutance is systile. Your a$$ will be laminated. There are 10 kinds of people in the world: People who know binary and people who don't.
You're lucky. The code I'm facing most often looks like this:
if ((((condition1 || condition2) && condition3) ||
condition4 || (condition5 && condition6 || condition7))
&& (condition8 || condition9)) || condition10)) {
//some code
//some 2473 more if blocks or loops with similarly obfuscated conditions)
// some 12853 more lines of code
}Whether or not the closing brace is aligned correctly does not matter at all, becasue having to scroll over several 1000 lines of code utterly and completely eliminate all purpose from formatting rules.
-
Beyond a certain point you cross over from something being obviously crap, to "I like style X better than style Y"; at which point the main thing you're doing becomes adding noise to your source control diffs? Does it matter if other devs are still working on it, or if it's all you now? Does having a tool like Resharper, whose utility in finding stuff the compiler warnings miss is badly degraded if it's spazzing out over style issues, affect where you draw the line? What if only part of the team has the tool?
3x12=36 2x12=24 1x12=12 0x12=18
When the crap level goes past my knees, I switch from mucking out the stall myself to kicking the ass of the person responsible. If I'm already annoyed that day, it may happen sooner.
Software Zen:
delete this; -
Beyond a certain point you cross over from something being obviously crap, to "I like style X better than style Y"; at which point the main thing you're doing becomes adding noise to your source control diffs? Does it matter if other devs are still working on it, or if it's all you now? Does having a tool like Resharper, whose utility in finding stuff the compiler warnings miss is badly degraded if it's spazzing out over style issues, affect where you draw the line? What if only part of the team has the tool?
3x12=36 2x12=24 1x12=12 0x12=18
Right at the start... Log a project level bug, mention the files that are affected. And the general reason... e.g. not according to coding standard, or whatever.
"Program testing can be used to show the presence of bugs, but never to show their absence." << please vote!! >>
-
Beyond a certain point you cross over from something being obviously crap, to "I like style X better than style Y"; at which point the main thing you're doing becomes adding noise to your source control diffs? Does it matter if other devs are still working on it, or if it's all you now? Does having a tool like Resharper, whose utility in finding stuff the compiler warnings miss is badly degraded if it's spazzing out over style issues, affect where you draw the line? What if only part of the team has the tool?
3x12=36 2x12=24 1x12=12 0x12=18
-
Beyond a certain point you cross over from something being obviously crap, to "I like style X better than style Y"; at which point the main thing you're doing becomes adding noise to your source control diffs? Does it matter if other devs are still working on it, or if it's all you now? Does having a tool like Resharper, whose utility in finding stuff the compiler warnings miss is badly degraded if it's spazzing out over style issues, affect where you draw the line? What if only part of the team has the tool?
3x12=36 2x12=24 1x12=12 0x12=18
If the fix is simple, I generally leave it alone, but if it is not trivial, then I reformat it to make it readable. The previous programmer may have created a house of cards, if you break it, all eyes are on you. So I want the code in a form I can verify for myself that it has a chance of working and be easy to fix if it turns out there was for than madness to the previous programmer's method.
Psychosis at 10 Film at 11
-
Beyond a certain point you cross over from something being obviously crap, to "I like style X better than style Y"; at which point the main thing you're doing becomes adding noise to your source control diffs? Does it matter if other devs are still working on it, or if it's all you now? Does having a tool like Resharper, whose utility in finding stuff the compiler warnings miss is badly degraded if it's spazzing out over style issues, affect where you draw the line? What if only part of the team has the tool?
3x12=36 2x12=24 1x12=12 0x12=18
Stylistic opinions don't warrant code changes unless the entire team agrees. A team of one, or someone with the authority to dictate what they team wants, will find it easy to get agreement. If the team wants some tool to work well, and it has stylistic requirements, then they are agreeing to adhere to its stylistic requirements.
patbob