Take the 2-minute tour ×
Programmers Stack Exchange is a question and answer site for professional programmers interested in conceptual questions about software development. It's 100% free, no registration required.

I've started working on my first open source project on Codeplex and came across some terrible code. (I did learn that C# still has the "goto" statement) I started adding features that the "owner" wanted and after exploring the code base and seeing what a mess it was (e.g using "goto") I wanted to clean it up a bit. But I am a bit concerned and that is why I'm turning to you all: is it proper etiquette for me to "fix" the "bad code" or should I let it be and work on new features? Like I said before, I'm new to the whole OSS scene and working on a team in general so I would like to not mess it up.

share|improve this question
11  
goto is not necessarily a mess. There are many cases when its use is perfectly justified. –  SK-logic Mar 24 '11 at 13:01
1  
@SK-Logic - while I don't have the source in front of me, it seemed like a situation where it would make more sense (and be much clearer) to use a method instead of goto. However, my development experience is limited so I could be wrong :) –  Jetti Mar 24 '11 at 13:29
1  
have you contacted the initial author and asked him what he thinks about? –  k3b Mar 24 '11 at 13:33
    
@k3b - I haven't yet. I will definitely do that tonight and see what he thinks. –  Jetti Mar 24 '11 at 13:47

6 Answers 6

up vote 14 down vote accepted

It's ok to do this, if you're modest about it and it doesn't break anything. You can't go around reformatting code and introducing bugs. Does it have good unit tests? If not, I'd start contributing by adding unit tests, and then go fix the structure later.

share|improve this answer
1  
I agree. Good documentation and tests are more valuable than ugly code which already happens to work. –  Filip Dupanović Mar 24 '11 at 13:11
    
no unit tests. No classes to really test. All code is currently in the UI code. (e.g button1_click() { // all code }) –  Jetti Mar 24 '11 at 13:27
3  
@Jetti - adding unit tests and then migrating the code out of the GUI code-behind would be a valuable contribution then. After that, you could refactor to your heart's content. –  Scott Whitlock Mar 24 '11 at 13:30
    
+1 for don't break anything –  Ryan Hayes Mar 25 '11 at 17:18

The purpose of open source is to get more eyes on a project and improve it. That includes making the code better. That said, it is good form to advertise on the list what you intend to do. You may get some push back, or you may get a bunch of +1's. Those goto statements just might be there because the original author couldn't think of a better way to get the job done. If you get push back, it's good to enter a dialog to find out where the pressure is coming from. Try not to let it get personal, and try to address the concerns.

Bottom line, unit tests speak louder than dogma. If you can prove that the code will malfunction in certain cases the way it is now, you'll either get the thumbs up or the original author will go in and fix things.

Remember, in open source, community is more important than code. If there is no community (both of users and of developers), then there is no project.

share|improve this answer
    
+1 for community is more important than code. Lots of people for get this. –  Wyatt Barnett Mar 24 '11 at 13:18

People who are jealously defensive about their code generally don't post it for the world to scrutinize, and if they do, the community around it doesn't last very long. Be tactful, but don't fret that you will hurt feelings.

Just tell them what you want to do before you invest too much time into it. Sometimes there are historical reasons for things that are non-obvious. The reason gotos are avoided is they can produce unanticipated paths through the code. Accordingly, the danger of removing gotos is that you don't notice one of the beneficial paths, and accidentally omit it from the refactor.

On the other hand, maybe the original author just couldn't think of a cleaner way to write it at the time. This is where code speaks louder than words, because they may not believe it can be done cleaner until you show them. One of my first open source contributions was an undo stack fix that significantly improved performance, but that some of the core developers said sounded too complex when I first described it. A short code sample brought them on board.

If it turns out there really are good reasons to leave it in, I would push at least for a comment explaining those reasons.

share|improve this answer

Speaking from experience...

The first Open Source project I contributed to, I was all full of piss and vinegar when I first started too.

What I immediately did was go through a bunch of source files and start stylizing things to my personal preferences, created a massive patch, and submitted it.

If you're working with somebody who is 'good' (like I was) (s)he'll immediately reject the patch. Mostly because, when you contribute to an open source project, you're expected to break down your fixes into bite size chunks that address a single issue. 'Removed all gotos' isn't a good example of an atomic commit. Even if you do break it down into smaller, well-documented commits first it might still be rejected.

The reason is, because the code is worked on by multiple people (with different styles) over time it isn't really feasible to accept changes over the whole library to suit one developer's style. If changing style for style's sake were feasible then every open source project would never move forward because the code would constantly be edited to suit different devs styles.

Refactoring code and adding functionality (or removing deprecated cruft) usually takes precedence over 'cleaning' code.

The most difficult and most rewarding part of working on an open source project is, you will be asked why you are proposing to make the changes you're submitting. If you can give a good reason there's a better chance that your patch will be submitted.

My advice is to do a few of these changes on one source file to give a taste of what you're trying to do first. If the changes are well justified and accepted, ask if more changes like it would improve the quality of the project. That way you won't waste a lot of effort for nothing if your patches are rejected in the future.

Developing open source is more than writing code. You're working to build a trust relationship because the gatekeepers (devs who control push access) will do what they have to to protect the integrity of the project. As you submit more patches the gatekeeper will get a better feel for your style and you won't have to justify your changes as much.

It's a process that takes time but it's very rewarding. Not only will you learn a lot from being able to look at, and critiquing someone elses' code but you will be critiqued on your own style.

Before you waste a lot of time trying to 'correct the injustice of the errors of other's coding style' ask yourself this:

Are the changes you're proposing based on adding value to the project or are they based on your own internal stylistic religion.

There is a lot of religion on Stack Overflow (and related Stack Exchange sites). I mean a lot. People think and talk about style endlessly, as if the more you talk about it, the closer you get to the 'perfect, ideal, indestructable, infallible' coding style. I talk about it a lot too mostly because it's fun.

In the Open Source world, style isn't so important. Function is.

Note: All of this advice assumes that your gatekeeper is a reasonable and talented programmer. If (s)he is, count yourself lucky that you didn't get stuck with one of the whiny b@&#&es whose sole concern is protecting their 'baby'. They do exist out in the wild, so don't be surprised if you encounter one.

share|improve this answer

Quality > Etiquette

In my opinion you should not worry about editing other people's code as soon as you discover it has poor quality. In order to achieve a good software quality you simply have to care for clean code. I don't see any problem with committing improvements to other people's code other people should be aware and grateful that there are coders who work on their code.

share|improve this answer

If you could figure out a better way to solve the problem without using "goto", then I suggest go for it. A bit effort in making the code better today may save you much more effort in the future.

Communicating with the original author is also a good idea.

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.