• 0

How do you feel about others refactoring your code?


Question

Until now, all the team projects I've worked on had no coding conventions whatsoever, whether at school or at my internship. Now, when I was at my internship, I tried to stick with the de facto conventions present in the code, but there are times where I'd do my way instead.

For instance, I think in a C-based language, if-else statements should be written like so:

if (condition) {
	statements
}
else {
	statements
}

instead of the more traditional

if (condition) {
	statements
} else {
	statements
}

And that one-line if-statements are unacceptable. I've too often seen this:

if (condition)
	statement 1
	statement 2
statement 3

Where indentation suggests statement 2 is conditional, when it's not. This is incredibly misleading, and yes, I've seen it cause bugs. So I always refactor that code as:

if (condition) {
	statement 1
}
statement 2
statement 3

I also think predicates should be named isSomething(), and other such conventions.

Now, I tend to take it pretty personal when someone in the team changes function names, layout etc in my code, without discussing it with me first. However, even if we discuss it, there's no easy way to reach common ground on these matters. For instance, my school teachers think all methods should be preceded by such monstrosities:

/*************************************************
*   function name : add						  *
*   arguments :								  *
*	   int number1 : the first number to add	*
*	   int number2 : the second number to add   *
*   returns : the sum of number1 and number2	 *
*   author : Dr_Asik							 *
*   last modified : 6/01/09					  *
*   author's blood group : AB+				   *
*   more crap									*
*   even more crap							   *
*												*
*************************************************/
int add(int number1, int number2) {
	return number1 + number2;
}

...which are such a pure GIMMICK and an utter WASTE OF TIME, it makes me wanna hit someone.

How do you cope with projects where there are no established conventions? Above all else I think maintaining good, open relations with my team members is the most important, so I'm a bit embarassed when it comes to coding conventions. Either we adopt mine or someone else's, and it becomes a matter of who's able to prove his authority over the team better. I don't like it, especially if we're equal team members on the project.

Edited by Dr_Asik
Link to comment
Share on other sites

Recommended Posts

  • 0

If we are talking about formatting conventions, I dont mind someone else making a change, so long as that isnt the sole purpose of modifying that code (assuming there is no set standard). If they modify the code simply for the purpose that they believe their way is better Ill be ****ed.

If we are talking about refactoring to make the overall code more efficient, I strongly encourage it.

Link to comment
Share on other sites

  • 0
I disagree. Most of the work can be eliminated by a good IDE. The tests would depend on what you're trying to accomplish. One could argue that you would need the tests regardless of how the code is actually broken down.
I've created low-res screenshots of his code example in the chapter on functions, just so that you can get a glance over the supposed improvement (and so that the text is illegible and I'm not accused of copyright infringement).

Here's the original method:

cleancodemessyoriginalt.png

And the refactored version :

cleancoderefactoredj.png

He's replaced a single method by a class with 18 methods and 5 fields. Sure, it's cleaner. But it's also an incredible amount of noise. Each method has a return type, arguments with their types, whether it throws or not; and none is even documented at all yet. That class looks much more complex than it is : it only has one public method after all (2 if you count the overload).

I think there must be some middle ground. It must be possible to refactor that messy method into maybe 2 or 3 ones; the use of a class is probably warranted, but the number of methods seems abusive. There's nothing wrong with 10-30-lines methods as long as their purpose is well defined and the code is fairly intuitive.

Edited by Dr_Asik
Link to comment
Share on other sites

  • 0

What matters most isn't which coding convention you decide to use, it's that you all agree to which one you're all going to follow.

At my place of work, we've all decided on something, then we set up Visual Studio's editor as appropriate. Then we exported the editor settings, and re-imported the settings file on all other dev machines so we know we all use the same thing.

Link to comment
Share on other sites

  • 0

from the blurred look - the refactred version seem better ;)

to me you have too much logic in one method...

I read someone saying 10 lines per method ... I tend to say a max of 30 lines - of code (comments dont count)

Programming code is art not quite a science - so sometime going over is ok :)

Link to comment
Share on other sites

  • 0
I've created low-res screenshots of his code example in the chapter on functions, just so that you can get a glance over the supposed improvement (and so that the text is illegible and I'm not accused of copyright infringement).

Here's the original method:

cleancodemessyoriginalt.png

And the refactored version :

cleancoderefactoredj.png

He's replaced a single method by a class with 18 methods and 5 fields. Sure, it's cleaner. But it's also an incredible amount of noise. Each method has a return type, arguments with their types, whether it throws or not; and none is even documented at all yet. That class looks much more complex than it is : it only has one public method after all (2 if you count the overload).

I think there must be some middle ground. It must be possible to refactor that messy method into maybe 2 or 3 ones; the use of a class is probably warranted, but the number of methods seems abusive. There's nothing wrong with 10-30-lines methods as long as their purpose is well defined and the code is fairly intuitive.

I find the second one much easier to read. Just like a class, a method should also follow the single responsibility principle. If you have a 20-30 line method, you have to wonder how many tasks that method is accomplishing.

The thing is, he's not going to document it. Unless he has to explain why he's doing something, he's not doing to document it. A lot of developers would create their 30 line method and use comments to separate various pieces. Instead, he's using separate methods with informative names to relay intent.

Link to comment
Share on other sites

  • 0
My thought : Not a single line of comment in my source files. I always begin designing a very solid model with a solid documentation detailing the different classes, methods, properties, functions etc ... before starting the implementation. In other words, I make comments before writing code. If I have to clarify something I go back to my model and clarify it there.

I think that is scary - no code comments - sure having a models and design is great to begin with, but designing every function etc isnt very practical to me.

Advantage of documentation in java code is being able to generate the API and diagrams from code.

with out code comments you make maintence very very hard - maintaing an application is what costs the most!!!!!

BTW:

I have a huge maintence nightmare.... functions which are err... 200 lines long! it is just crazy!!!!

I'm breaking the rules a little in that I'm changing existing non broken code so I can read it and understand it.

scary stuff - but important as having huge functions is just a nightmare to maintain.

All fun... :s

Edited by _kane81
Link to comment
Share on other sites

  • 0
I think that is scary - no code comments - sure having a models and design is great to begin with, but designing every function etc isnt very practical to me.

Advantage of documentation in java code is being able to generate the API and diagrams from code.

with out code comments you make maintence very very hard - maintaing an application is what costs the most!!!!!

I just think we need to change why we write comments. The majority of comments that I've seen are redundant. They either specified something that the code already implied.

int numberOfBooks;	// Number of books.

Or, the comment was used in place of better cod equality.

int iBks; // Number of books

The same thing applies with API documentation. If the documentation of the method provides as much information as the method name itself, then it's pointless and redundant.

/// <summary>
/// Creates a new book
/// </summary>
/// <param name="bookName">Name of the book</param>
public Book createNewBook(string bookName)
{
	 .....

}

Comments are useful, no doubt. I just wish people thought about the reasoning behind their comments. Instead of writing a comment because something isn't clear, they should say, "Could I make this clearer by refactoring this code?" before they do it.

Link to comment
Share on other sites

  • 0

I don't like it when people mess with my code without just cause. I much prefer it when there is an open forum for discussion before changes are made because they may be a very good reason for doing something the way you have done.

Link to comment
Share on other sites

  • 0
BTW:

I have a huge maintence nightmare.... functions which are err... 200 lines long! it is just crazy!!!!

I'm breaking the rules a little in that I'm changing existing non broken code so I can read it and understand it.

scary stuff - but important as having huge functions is just a nightmare to maintain.

All fun... :s

That sounds like hell.

About a year or so ago, I acquired a company's asp.net web application after the primary developer quit. They seemed concerned as they weren't sure what the quality was like. I said, no problem, we'll take a look at it.

Oh my. Outside of the normal database/presentation pollution, we had random items stuffed into session, and viewstate. At this point, I'm thinking, "Yeah. Great." I then started to look into the pages. Each page was around 2500+ lines of code, not including HTML. Why were the pages so long in an average workflow application? Glad you asked. Each page contained about 1000 lines of code that were duplicated across every page. Therefore, technically, every page could approve a record, deny a record, create a record, and finalize a workflow. Even user management pages that had nothing to do with the process at all had this code. That was fun.

Link to comment
Share on other sites

  • 0

^^

yes, those sort of comments are useless, and are not needed!!!

Descriptive names / self documenting / readable code all the way :)

^

ouch - yeah did u say dont touch it! / it be faster to rewrite ;)

Link to comment
Share on other sites

This topic is now closed to further replies.
  • Recently Browsing   0 members

    • No registered users viewing this page.