Open Source Etiquette

Question

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.

Answer 1

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.

Answer 2

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.

Answer 3

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.

Answer 4

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.

Answer 5

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.

Answer 6

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.

Answer 7

There is nothing implictly wrong with goto. Look at assembly code - lots of gotos (jumps and branches) all over the place.

The reason why goto has a bad name these days is because of the Dijkstra paper Go To statement considered harmful which pinpointed the goto statement as a very bad thing.

Note that that was 50 years ago where software engineering wasn't even named yet, and most programming languages were essentially abstractions of the underlying machine so as the machine language contained goto, so did they. You can try programming some in Microsoft Basic (the original one, on the Apple ][ or Commodore 64) to get an idea of how that mindset was.

What Dijkstra was arguing was that to keep things simple do not jump all over the place, but instead keep a simpler program path with a common ending. E.g. a return from a method. In other words - only local jumps, not global ones.

This was a step towards bringing in things like method calls with arguments, modularization of code, packages etc which all have been introduced to tame the complexity of software development. The goto statement was just the first bystander in that war.