Where do you draw the line on cleaning up someone elses code
-
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
-
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
What brought you to look at the code? If obfuscation hides functional problems, start refactoring. If I've more time, I work deeper than strictly necessary - often I see problems not apparent when I started. A bigger issue you allude to: "What if only part of the team has the tool?". If you're in the same code base the team needs access to the same tools. Period. Hopefully they all have access to the same training and materials, if need be. Nothing worse than being second class citizen in a project, or worse, having to supplicate the gods of the team, just to get the days tasks done.
-
What brought you to look at the code? If obfuscation hides functional problems, start refactoring. If I've more time, I work deeper than strictly necessary - often I see problems not apparent when I started. A bigger issue you allude to: "What if only part of the team has the tool?". If you're in the same code base the team needs access to the same tools. Period. Hopefully they all have access to the same training and materials, if need be. Nothing worse than being second class citizen in a project, or worse, having to supplicate the gods of the team, just to get the days tasks done.
James W Taylor wrote:
If you're in the same code base the team needs access to the same tools. Period
Agree completely. The problem is my employer has reached the size where they have enough bureaucracy we can't just go to purchasing and say "X Costs $250 and will save me at least 10 hours of work over the course of the project, more than paying for itself", but not enough to have gotten stuff beyond the basics (eg visual studio itself) on the pre-approved software list. :sigh: It took my coworker several months of working the bureaucracy to get me a copy of Resharper. :doh:
3x12=36 2x12=24 1x12=12 0x12=18
-
James W Taylor wrote:
If you're in the same code base the team needs access to the same tools. Period
Agree completely. The problem is my employer has reached the size where they have enough bureaucracy we can't just go to purchasing and say "X Costs $250 and will save me at least 10 hours of work over the course of the project, more than paying for itself", but not enough to have gotten stuff beyond the basics (eg visual studio itself) on the pre-approved software list. :sigh: It took my coworker several months of working the bureaucracy to get me a copy of Resharper. :doh:
3x12=36 2x12=24 1x12=12 0x12=18
The challenge is always the jujitsu of using the bureaucratic mindset for good, i.e. to the bureaucrat there is no difference between the cogs in the machine. So by title or by project if anyone needs a tool, everyone does. For example, define a necessary toolset for the desktop of a developer in the project plan; copy 1 of a new tool is performance evaluated and is added to the list if it pans out, budgeted as "engineering tools, misc.", as needed. I've been on the other end too often. "You *really* need a compiler to do your job?" (as a software developer) - "how 'bout next year?" (for a hot project), or worse, "just use this (old, incompatible) version." Bad enough, from accounting or finance, couple of times I got that from my boss. Clue #1 that I had the wrong position, but that's a different thread.
-
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 one experiences problems with this in more than a trivial number of files then both of the following are probably problems. - Management isn't controlling tasks enough. - The design is flawed. This of course isn't the same as a situation where the person that normally does the database layer code is on vacation and needs to do an update. Nor is the same as taking over the code base that someone else previously owned.
-
If one experiences problems with this in more than a trivial number of files then both of the following are probably problems. - Management isn't controlling tasks enough. - The design is flawed. This of course isn't the same as a situation where the person that normally does the database layer code is on vacation and needs to do an update. Nor is the same as taking over the code base that someone else previously owned.
Of the stuff in the code base I'd like to bash on, I'd only put a minority of the cleanup refactoring down to lack of management control. My best guesses for the two code bases in question would be: 20/40% inconsistent/chaotic class layout (eg methods not in a logical order, excessively large methods, lack of comments, etc) 35/40% making resharper happy 35/10% language improvements 10/10% works better/faster refactoring Only the first category really falls under lack of supervision/control. For the second app most of it is oversize methods and due to the 1.0 code being a strait VBA port and never having received a intern/prototype code removal refactoring pass. The code worked well, it was just slow, hard to maintain/control, and couldn't be properly secured for external use. The app which has lots of room for language level improvements was originally written in .net 1.1, and lacks things like auto-properties which result in some classes being much more verbose than necessary.
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[^]
Not neccesarily. If there is an optimization on the time or memory used, the expected input/output should not change, and the change is useful and important nevertheless.
-
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.
PL/SQL Developer has a "Beautify" option... of course, that client is only for Oracle's PL/SQL databases.