15

The quality of the code in our software is indeed lacking. When you change a line of code in one component it usually breaks code in other components.

Our software architect blames this on collective code ownership. He believes that if every developer was responsible for his own piece of code then he would tidy it up all the time because he would feel ownership over the component.

In the current situation, developers change code in multiple components and they break other components either because they don't understand how the component works (because they work with so many components) or because they don't care about how their changes will affect the other clients of the component (as long as they are not affected).

How can I address the concerns of the software architect but still maintain collective code ownership?

Tulains Córdova
  • 39,201
  • 12
  • 97
  • 154
Eugene
  • 1,943
  • 1
  • 21
  • 35
  • 12
    Out-of-interest, do you have code reviews? – matt freake Feb 25 '14 at 13:17
  • 25
    Out of more interest, how good are the unit tests? – Brendan Feb 25 '14 at 13:18
  • Not entirely invalid, large teams probably require more ownership and specialization. However this can devolve into turf wars and politics with l the wrong culture. – Doug T. Feb 25 '14 at 13:49
  • 2
    I think a fitting name for the criticized aspect is `collective code disownership`. – Patrick Feb 25 '14 at 14:27
  • 2
    Your software architect should work towards a better design where components are more loosely coupled. They need to take ownership of this before they expect developers to take ownership of the individual components. – Bernard Feb 25 '14 at 14:42
  • 1
    What if you aimed for `Weak Code Ownership`, as explained in [this answer](http://programmers.stackexchange.com/a/186686/4127) – Eric King Feb 25 '14 at 14:55
  • We tried both ownership ways and they both failed. We discovered that unit tests and proper code reviews were much much more important. – Songo Feb 26 '14 at 13:17
  • @Songo it isn't. You need both tests, code reviews, and people taking responsibility for design decisions (and strong coupling like that is definitely a design flaw in the architecture). – jwenting Feb 28 '14 at 08:59

11 Answers11

24

When you change a line of code in one component it usually breaks code in other components. ...if every developer was responsible for his own piece of code then he would tidy it up all the time because he would feel ownership over the component.

So hang on, if the problem is that updating your component breaks someone else's component (due to a dodgy architecture that doesn't have any decent decoupling), how is it that making a component be owned by a developer suddenly stops someone else's component magically decouple so it doesn't break on updating the first!?!

The only answer is to properly organise the components so that are no longer as intertwined as they currently are. That, and documenting the interfaces and data contracts between them.

I do agree with component ownership as it can help with quality of a lot more than just code, but a substitute is code reviews - someone after all needs to know a lot about each component in order to enforce the API and correct behaviour of it. In this case, I think code reviews are just a way of implementing ownership anyway.

gbjbaanb
  • 48,354
  • 6
  • 102
  • 172
  • 5
    If only everyone ignored the parts of the plan that won't work. – JeffO Feb 25 '14 at 14:01
  • I agree to the main part of you answer, but "code reviews are just a way of implementing ownership"? You don't believe that seriously, don't you? – Doc Brown Feb 26 '14 at 12:29
  • @DocBrown in that you'd end up with the same level of understanding as you would if you had code ownership in the first place. Without a named individual per component, the team (or part of the team) would take ownership instead. Still ownership. – gbjbaanb Feb 26 '14 at 13:16
  • "The only answer is to properly organise the components so that are no longer as intertwined as they currently are." Which would actually be the task of the mentioned software architect.... – Johannes S. Feb 28 '14 at 10:14
20

Just three things: code reviews, code reviews, code reviews - they will help you improve the code quality as well as the feeling of responsibility for "the shared code".

Doc Brown
  • 199,015
  • 33
  • 367
  • 565
  • 28
    A good list, but I think code reviews could be added to it. – hyde Feb 25 '14 at 13:29
  • Let's many programmers a,b,c,d,... made components A,B,C,D.... Now the programmer 'a' wants to make a change in his component A that will cause a problem in the component B. Programmer 'a' doesn't know about it. I his code would be checked by the programmer 'b', everything is OK, the problems will be found. But the code checker was chozen arbitrary. So, with the great probability it won't help. How this could help the QA with his problem? – Gangnus Feb 25 '14 at 14:29
  • @Gangnus The point is to use code reviews to improve code quality *instead* of siloing. Have everyone know all of the code. Siloed developers lead to huge problems when employees inevitably go on vacation or get sick or quit. – Plutor Feb 25 '14 at 14:38
  • If developer c (who doesn't know about component A or component B) does the review, he is then exposed to both and ideally learns about both. As Plutor says this helps prevent siloing. – matt freake Feb 25 '14 at 14:42
  • @Plutor You want every piece of code to be checked by every programmer in the group? If yes, it is impossible, if no, where is the profit? – Gangnus Feb 25 '14 at 14:42
  • @Disco3 The connection A-B could be easily found by 'b' only. Who will tell the programmer 'c' that he should check THESE TWO components? – Gangnus Feb 25 '14 at 14:44
  • @Gangnus: I'm not saying that every developer should do every code review. I'm saying that *any* developer should be *able* to do it. Every developer on a reasonably-sized group should be relatively familiar with all of the code in their project. (Good unit test coverage also helps, in fact might help more in terms of breakiness.) – Plutor Feb 25 '14 at 15:02
  • @Gangnus: I guess there is high chance that b tells a "other components depend on A, so before changing it lets check if it has any impact on the other parts of the system". And in a good code review, the team members will also discuss about taking the right measures of automatic and/or manual testing for a change in code. You would be surprised how many quality problems can be solved just by working as a team. – Doc Brown Feb 25 '14 at 15:08
  • I agree this may be enough in a normal project. But if the project already HAS bad architecture so that components are too often dependent on each other? And in this case IMHO, we have to choose carefully who will do which code check. – Gangnus Feb 25 '14 at 15:49
  • @Gangnus: Even with the worst architected project, code reviews can only help move it in the right direction. If the reviewers are chosen randomly a given developer may not be the ideal person to catch a particular problem, but over time, all developers would become more familiar with all aspects of the project and become better at identifying problems. Not an immediate fix, but with large tangled messes there usually are none. Also, how do you carefully choose reviewers? If you don't know the problems a change may cause in the first place, how do you pick people to look for those problems? – Mr.Mindor Feb 25 '14 at 18:32
  • 2
    @Mr.Mindor "...code reviews can only help..." Without any doubt. But they are not enough. And this post insists that they are ALL we need. – Gangnus Feb 25 '14 at 18:40
  • @Gangnus: I agree they are not enough. Here we minimally should also have unit tests and some level of automated integration testing, and long term probably re-architecting to reduce/remove the interdependency. I read your previous comments not as _'worthwhile, but not enough on its own'_, but as _'Not enough, so no point'_ – Mr.Mindor Feb 25 '14 at 18:51
  • @Mr.Mindor I would also add the automated testing for UI. It is very demanding and covers really many problems. But I am afraid that when a group is guided by a person, who obviously tries to move his own guilt to somebody else, it needs special treatment – Gangnus Feb 25 '14 at 19:41
  • @Gangnus: while a, b, c and d made A, B, C and D, their code was being reviewed by the others. So everybody should have a reasonable idea of what A, B, C and D do by the time they're done. – RemcoGerlich Feb 25 '14 at 20:51
  • 1
    @Gangnus: while not denying the importance of unit tests there are problems which cannot be detected by it (races, undefined behaviours, ... - probably not problems meant by OP but still) even in 'innocent' change. So answering the original question - don't ask only one person - send it to group. Then someone might notice and ask b to take a look. – Maciej Piechotka Feb 25 '14 at 22:10
  • @MaciejPiechotka But that would work only if they already know B a bit, to find that A-B connection while analyzing A, and know that b is the best specialist in B. – Gangnus Feb 25 '14 at 22:13
  • 1
    @Gangnus If it is 4 people project then sending to all is an option. If it is not then there is A/B/C/D team. People working on interaction between A and B subscribe to both lists so if review is sent to list A then b received a copy. To miss it (s)he would need to miss it and everyone else who knows that b works with A-B interaction. (Of course ideally is if tests would catch it before sending review saving everyone's time - but after review b knows what has changed as well) – Maciej Piechotka Feb 25 '14 at 22:32
  • 1
    @Plutor "Have everyone know all of the code" - but then, if everyone knew all of the code... you wouldn't need code reviews as each individual would be able to make the correct changes to all components that were required. – gbjbaanb Feb 25 '14 at 22:53
  • @gbjbaanb - code reviews is one way how you achieve 'have everyone know all the code' and keep it that way. Knowing all the code won't happen by itself. – Peteris Feb 25 '14 at 23:06
  • @MaciejPiechotka If it is a 4-men project, they all know their scopes of work. I am talking about a large project, with tens of programmers. And not knowing who is better in what could be fatal. – Gangnus Feb 26 '14 at 08:21
  • @Peteris but if many people don't know the code, you have to have ownership to determine who does. Code reviews are a tool to spread that knowledge. Initial knowledge transfer... documentation? – gbjbaanb Feb 26 '14 at 08:30
  • @Gangnus - for that case you have "if not" and majority of my comment. – Maciej Piechotka Feb 26 '14 at 17:44
  • @MaciejPiechotka I had said several times that we have many programmers and one component for a programmer. So your second part is also for some other case, sorry. I understand, that defining processes for small projects is fine and simple, but we should solve tasks that are set, not other ones that are simple. – Gangnus Feb 26 '14 at 19:31
  • @Gangnus it's getting really off-topic but as an example of larger company which do use reviews see [Google](http://scientopia.org/blogs/goodmath/2011/07/06/things-everyone-should-do-code-review/) - they seems to mention also similar arguments. – Maciej Piechotka Feb 26 '14 at 20:40
14

When you change a line of code in one component it usually breaks code in other components.

Ideal world scenario: You should have your APIs formalized and the API invariants tested in unit tests. If this is covered, when a developer makes a change that touches the invariants, tests will fail. When a developer makes a change that breaks unit tests, they should consider either updating the unit test, or updating the documentation of the API (and removing the unit test).

Our software architect blames this on collective code ownership.

No. Just no. As I understand it, it is more of a problem with everyone modifying code ad-hoc, across multiple components at the same time. This is not an issue with code ownership, but with the methodology of your team not formalizing/respecting API contracts and invariants.

He believes that if every developer was responsible for his own piece of code then he would tidy it up all the time because he would feel ownership over the component.

That's not realistic. If I wrote a piece of code and some other developer breaks it, I will not drop what I am doing to fix bugs introduced by said developer (as I have my own tasks and deadlines).

In current situation developers change code in multiple components and they break other components either because they don't understand how the component works (because they work with so many components) or because they don't care about how their changes will affect the other clients of the component (as long as they are not affected).

Developers changing multiple components (and breaking random stuff) to make their current code work, means lack of clear/formal definitions for your APIs (i.e. at this point there is no clear image of what the component should do), and lack of proper documentation for your APIs (i.e. developers entering functions to see what the implementation does, instead of reading the documentation for it).

What do you believe we should do to address the concerns of the software architect but still maintain collective code ownership?

  • Formalize your interfaces and document them. Ensure that the documentation of an API is always accurate (if your function states that a null value yields a MyException thrown, then you better have a unit test that calls the API with a null and catches the exception).

  • Give up cowboy coding and "five-minutes" fixes to any defect. There is no "quick fix" to anything. A defect fix should cover all of these:

    • API redesign (this is done on documentation, then API declaration)
    • unit test completion (add unit test for new behavior, remove and edit unit tests for modified behavior, only remote unit tests for behavior that is no longer actual)
    • API implementation changes
    • all tests passing
  • ensure clean code and clean diffs (no commented dead code, no code that is weirdly arranged, etc).

Most importantly, DO NOT ACCEPT public commits with failing tests or that have not gone through two rounds of code review (if a change causes failing tests, it is not a fix, it is one or more introduced defects). Two rounds of code review mean:

  • reviewer reviews changes and provides feedback to developer
  • developer performs changes according to review feedback
  • reviewer reviews changes again and gives approval/rejection
  • developer commits changes (after approval)

Ideally, each fixed defect will have a new unit test added to the system that checks exactly the behavior described in the defect.

When done like this, a one-liner "five minutes fix" becomes a "30 minutes or more" fix. Ultimately, this is not wasted time, it is time you will spend on implementing new features, instead of time spend in the debugger.

utnapistim
  • 5,285
  • 16
  • 25
  • 2
    In my experience it helps the team to accept tests if you make them visible to everyone. We've put a 'tests'-display in our office so that everyone sees the red light and cares to fix. **Make the metrics that you care about easy to see for your devs** – tessi Feb 26 '14 at 06:55
5

This is more of a supplement/commentary to Doc Brown's answer.

IMHO - the software architect (SA) is looking for a way to pinpoint blame on someone else. I'm not hearing whether or not everyone is following the plan. Some companies may rely on a team lead or the team as a whole to be responsible for following the plan, so the architect is only involved when there is a question about the plan itself. Others may rely on the SA to decide if the plan is being followed.

Some plans are better than others. Imagine a head chef who has a very poor recipe for chicken and relies on one cook who is particularly good at it (basically, he has his own recipe). Some nights another cook has to step in and gets blamed for the poor quality, because he attempted to follow a bad recipe (no seasoning). Often there aren't enough good coders to bail-out a poor design or follow a plan if left on their own.

As Doc Brown indicated in his answer, a code review is the best way to make sure this is happening along with the following of other standards. There are those that think people will take responsibility for code if they have to fix their own bugs. The bottom line is, someone has to make sure code is shippable and if there is no review, you're just taking your chances.

Peter Mortensen
  • 1,050
  • 2
  • 12
  • 14
JeffO
  • 36,816
  • 2
  • 57
  • 124
5

Ownership, collective or individual, isn't going to change the fact that changing things breaks code in other components.

Start adding unit tests for everything that gets touched. If possible, add tests for some (or all) of the callers of whatever you touch. This will (eventually) make it really hard for people to break other code without knowing it, which is probably what's going on right now (I'm assuming your team isn't deliberately committing broken code).

They'll also probably point out that there's a lot of weird inter-twining of your code, which is probably at least a part of the problem.

Michael Kohne
  • 10,038
  • 1
  • 36
  • 45
  • 7
    "Unit tests" aren't technically what he wants. What he needs are "automated integration tests", which will probably get ran on an xUnit style framework. FWIW. – DougM Feb 25 '14 at 14:25
4

How can I address the concerns of the software architect but still maintain collective code ownership?

I'm probably bikeshedding, but that's not your problem. The company owns the code, not a developer or an architect. Everyone works to better the whole. You have problems in your engineering process, and it's resulting in a blame game later.

You need to determine the cause of the failure (failed self test and broken components), and put policies and procedures in place to address the gaps. Once the policies and procedures are in place, there's no one to blame because "things just work".


In the current situation, developers change code in multiple components and they break other components either because they don't understand how the component works (because they work with so many components) or because they don't care about how their changes will affect the other clients of the component (as long as they are not affected).

Perhaps there's a third: the developers don't know they've nudged or broken code in other components.

That begs the question: what is in your engineering process to address the gaps? Are there policies and procedures in place to catch the problems early?

Process One

I can say for certain that liberal use of ASSERTs (as opposed to POSIX's useless assert) will ensure a developer knows about nearly every unexpected condition at development time. I call it "self-debugging" code, and I spend nearly no time under a debugger because the code tells me where the problems are.

When I say "liberal use of ASSERTs", I mean everything. You assert all parameters are expected when entering a function. You assert all program state used in a function. And you assert all return values from system calls. If there's an if to validate something, then there has to be a matching ASSERT. If there's an ASSERT to validate something, then there has to be a matching if. The code will not be able to fart or sneeze without a developer knowing about it.

Another way to look at it: why should you debug my code, and why should I debug your code? Let the code debug itself and tell you where the problems are.

Another way to look at it: why should you spend time debugging? Your time is better spent on adding features. Let the code debug itself and tell you where the problems are.

If you have not studied how to ASSERT, then it would a treat to read John Robbins. He is a master bug slayer. The book I learned from was Debugging Applications. His blog is at http://www.wintellect.com/blogs/jrobbins.

Process Two

There must be complete self tests for components. I don't waste a lot time on the positive self tests. I can hire a kid from India or Pakistan for US$10 a day to copy/paste code that works as expected under benign conditions. Rent-a-Coder is full of them.

I focus on the negative self test. I want to know how the program fails and ensure it recovers or fails gracefully. And when I say testing, I mean testing everything - even protected and private interfaces in a C++ class.

I don't buy into the arguments that "only public interfaces need testing". Hogwash until someone produces a program without bugs. I've got code in the field that goes years between bug reports because my engineering process requires 100% coverage with negative test cases.

Process Three

Continuous integration into a staging area. Every commit kicks off a build and self tests. Reject the check-in if it breaks a build.

The Real Gem

The real gem in the three processes above is you don't need expert coders. You can hire interns and junior developers to instrument code with ASSERTs and write test cases. I would even argue you need fewer expert programmers because the code is doing the heavy lift by debugging itself. That should make management happy.

And your expert developers will get pissed off when a junior starts breaking their code in obvious ways. I can't tell you how many times a guys with 10 or 15 years of experience complained to me about all the damn asserts that are firing. It's amazing what you find when you start looking for problems in code you thought was solid.

Peter Mortensen
  • 1,050
  • 2
  • 12
  • 14
  • 3
    "only public interfaces need testing" - what do you define as a public interface? I find that at the class level test on non-public members very brittle as you will refactor. Good testing IMO tests public interfaces at each level - the public API of a class, the public API of a module, the public interface of an application. These tests won't change every time you modify the code, but still assert the behavior is correct. – Michael K Feb 25 '14 at 18:23
  • 2
    +1 for the ASSERT technique. It is very much underappreciated. – Peter Mortensen Feb 25 '14 at 19:22
3

There is validity to his analysis, generally a programmer will 'load' a problem into their head and work on it as a whole.

Holding programs in one's head - pg essay

Don't have multiple people editing the same piece of code. You never understand other people's code as well as your own.

Some real life circumstances will dictate that programmers work with each other's code, but it should not be the standard operating procedure.

Incidentally, this is a big reason why TDD and code quality are popular. They catch bugs when they are the cheapest to fix. It costs a lot more to fix problems later than as the code is written and the information starts to fade away.


Part of the software architect's role should be to help migrate the architecture in this direction.

The software architect should be actively assisting in this process. Since I assume they have a deeper knowledge of the architecture, they should be able to help determine the specifics of this plan, what can be broken out into modules? how are dependencies managed? Who should start to take ownership of which part? If the software architect is simply saying 'take ownership' and not helping to hash out all the details, they are a part of this problem as well.

One approach is to find the parts with the least amount of dependencies, and refactoring them. Don't refactor just for the sake of it, but more often than not each module will have a feature request or two that would make it worthwhile to refactor. Don't make it a rewrite!

If testing and code reviews are added to this process, the programmers will actually be better equipped to work with the modules other people create, because they will be writing more similar code that should be more easily tested and more debuggable, but code reviews and code ownership should be part of a plan to improve the software, not the entirety of the plan.

FMJaguar
  • 3,039
  • 18
  • 14
2

I personally think your architect is correct. I've never bought into this 'code ownership is bad' notion. We're craftsmen. Coders need to be responsible for what they create. And if they create a mess, they need to be the ones who clean it up. Otherwise you end up with programmers making bugs, others fixing them, and the original coder never learning from their mistake. Or worse, you end up with certain poor coders always being the ones making mistakes, and others always being stuck fixing them, carrying their weight.

Yes, code reviews will help that. And its always good to have people familiar with other modules for the proverbial run over by a bus situation. But nothing helps a coder code responsibly and defensively more than learning the hard way that if they screw something up, they'll be the ones stuck fixing it.

That said, with a dainty code base that breaks easily such as the one you describe, I doubt the lack of code ownership is the only problem in play here. That indicates to me a lack of clear thought moving forward, and probably a lot of short term thinking on the part of everyone involved.

GrandmasterB
  • 37,990
  • 7
  • 78
  • 131
  • This team sounds so chaotic, they may need to constrict sections of the code to a single dev just to get some semblance of control. It doesn't seem like they're ready to handle too many moving parts. – JeffO Feb 25 '14 at 19:45
1

If changes in components influence other components too often, it means the bad architecture. So, every such error has the cause in the architect himself.

After he acknowledges it, you could agree with the info on personal authorship, for easier finding WHO KNOWS the component better, not for any blaming. Together with code reviews made by owners of the neighbouring components, it could work.

This way no bad atmosphere is created.

Gangnus
  • 2,805
  • 4
  • 21
  • 31
1

In the current situation, developers change code in multiple components and they break other components either because they don't understand how the component works (because they work with so many components) or because they don't care about how their changes will affect the other clients of the component (as long as they are not affected).

This sounds like a management problem really - that the developers have not been given the time to learn the skills they need to own the code. Other answers offer practical solutions to this, yet I do not think they solve the root problem.

I'd recommend looking into the 5 whys and applying it to your group to find out why the skills are not being learned - this would be an example of it.

  • Why don't developers know how the components work?
    • Maybe because they need training in them?
  • Why haven't they gotten training in them?
    • Maybe because they don't have time to do training?
  • Why don't they have time to do the training?
    • Maybe because they have too much work to do?
  • Why do they have too much work to do?
    • Maybe because their manager prioritized work over training

At this point, you need to convince your manager that the benefit of investing in this training now will allow true code ownership and hopefully improved quality.

Peter Mortensen
  • 1,050
  • 2
  • 12
  • 14
John D
  • 161
  • 9
1

It's a shared responsability.

Each programmer should be responsable for their own code, and, at the same time, the architect should check every programmer's code.

In order to implement that "shared responsability", your company should also have some policies about code. They should be "close" enough to avoid bugs, and, "open" enough to allow programmers to apply their skills, without feeling too much restricted.

An example of a policy could be to install version control software, and every developer should store their last changes to the code every day before leaving.

Another example of a policy could be how to write an "if-else" sentence, using a "C"-like language. In a company I worked in, we always use brackets, even if we only use a single instruction.

So, this:

if ()
    DoSomething();
else
    DoSomethingElse();

Became this:

if ()
{
    DoSomething();
}
else
{
    DoSomethingElse();
}

When a programmer is hired, he should be informed about the coding policies. And, sign a contract. But, the policies should be explained as something that helps do the job, with examples or training.

You may allow a developer to skip the rules sometimes, too much workload, or an unexpected scenario. Sometimes the broken rule becomes the one rule.

But, if a developer breaks them, frequently, with a non-significant reason, you should fire him, because he may spoil the other coworkers.

If your company doesn't have formal policies, or those policies require to be updated, check them together with the programmers, not just the architect or project manager.

Peter Mortensen
  • 1,050
  • 2
  • 12
  • 14
umlcat
  • 2,146
  • 11
  • 16