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.

"Best practices" are everywhere in our industry. A Google search on "coding best practices" turns up nearly 1.5 million results. The idea seems to bring comfort to many; just follow the instructions, and everything will turn out fine.

When I read about a best practice - for example, I just read through several in Clean Code recently - I get nervous. Does this mean that I should always use this practice? Are there conditions attached? Are there situations where it might not be a good practice? How can I know for sure until I've learned more about the problem?

Several of the practices mentioned in Clean Code did not sit right with me, but I'm honestly not sure if that's because they're potentially bad, or if that's just my personal bias talking. I do know that many prominent people in the tech industry seem to think that there are no best practices, so at least my nagging doubts place me in good company.

The number of best practices I've read about are simply too numerous to list here or ask individual questions about, so I would like to phrase this as a general question:

Which coding practices that are popularly labeled as "best practices" can be sub-optimal or even harmful under certain circumstances? What are those circumstances and why do they make the practice a poor one?

I would prefer to hear about specific examples and experiences.

share

locked by ChrisF Mar 15 '12 at 21:47

This question exists because it has historical significance, but it is not considered a good, on-topic question for this site, so please do not use it as evidence that you can ask similar questions here. This question and its answers are frozen and cannot be changed. More info: help center.

closed as not constructive by Mark Trapp Nov 20 '11 at 17:22

As it currently stands, this question is not a good fit for our Q&A format. We expect answers to be supported by facts, references, or expertise, but this question will likely solicit debate, arguments, polling, or extended discussion. If you feel that this question can be improved and possibly reopened, visit the help center for guidance.If this question can be reworded to fit the rules in the help center, please edit the question.

8  
Which are the practices you disagree with?, just curious. –  Sergio Acosta Oct 28 '10 at 13:17
5  
@Walter: This has been open for months, it is definitely constructive, why close it? –  Orbling Jan 18 '11 at 2:23
2  
Seeing as how my name is being mentioned in here, I suppose I should chip in: I believe that the answers here are valuable, but the question could be reworded into something definitively less poll-like without invalidating any of the answers. Example title: "Which popular 'best practices' can sometimes be harmful, and when/why?" –  Aaronaught Jan 19 '11 at 1:33

45 Answers 45

I think you've hit the nail on the head with this statement

I'd hate to take things at face value and not think about them critically

I ignore almost all Best Practices when it doesn't come with explanation on why it exists

Raymond Chen puts it best in this article when he says

Good advice comes with a rationale so you can tell when it becomes bad advice. If you don't understanding why something should be done, then you've fallen into the trap of cargo cult programming, and you'll keep doing it even when it's no longer necessary or even becomes deleterious.

share
4  
Wonderful quote. –  David Thornley Oct 26 '10 at 20:10
4  
+1 Great quote and I agree 100%. –  Fosco Oct 26 '10 at 20:25
3  
I hope people don't take this as an excuse to not look up what the reasoning behind the best practices are, though. ;) Unfortunately, I have seen developers with this attitude. –  Vetle Oct 27 '10 at 8:23
3  
Rationale is good. Research is better. –  Jon Purdy Oct 27 '10 at 14:26
7  
Absolutely true, and I've said the same before. Perhaps the first standard in any "Standards" document should read, "In the interest of sharing knowledge and providing the information necessary to rescind standards at later times, all standards shall include the reasons for their existence." –  Scott Whitlock Oct 29 '10 at 0:47

Might as well throw this into the ring:

Premature optimization is the root of all evil.

No, it's not.

The complete quote:

"We should forget about small efficiencies, say about 97% of the time: premature optimization is the root of all evil. Yet we should not pass up our opportunities in that critical 3%."

That means that you take advantage of specific, strategic performance enhancements throughout your design process. It means that you use data structures and algorithms that are consistent with performance objectives. It means that you are aware of design considerations that affect performance. But it also means that you do not frivolously optimize when doing so will give you minor gains at the expense of maintainability.

Applications need to be well-architected, so that they don't fall down at the end when you apply a little load on them, and then you wind up rewriting them. The danger with the abbreviated quote is that, all too often, developers use it as an excuse to not think about performance at all until the end, when it might be too late to do anything about it. It's better to build in good performance from the start, provided you're not focusing on minutiae.

Let's say you're building a real-time application on an embedded system. You choose Python as the programming language, because "Premature optimization is the root of all evil." Now I have nothing against Python, but it is an interpreted language. If processing power is limited, and a certain amount of work needs to get done in real-time or near real-time, and you choose a language that requires more processing power for the work than you have, you are royally screwed, because you now have to start over with a capable language.

share
4  
+1 for the strong No, it's not. –  Stephen Oct 26 '10 at 21:26
21  
But if you've already recognized that one particular optimization is within that critical 3%, are you premature in optimizing it? –  John Oct 26 '10 at 21:33
7  
@Robert: Then what is the point of disagreement with the statement that "Premature optimization is the root of all evil"? –  John Oct 26 '10 at 22:20
8  
It is never premature to optimise high level design and technical decisions like language choice. However often it is only after you have mostly completed a design that it's inefficiencies become aparent, which is why Fred Brooks said that most teams write a throw away version whether they intend to or not. Another argument for prototyping. –  DominicMcDonnell Oct 27 '10 at 12:06
8  
@Robert, the Knuth quote was optimized prematurely... –  user1249 Jan 21 '11 at 0:00

One return per function/method.

share
7  
I was going to put this. I love me some early return statements. –  Carson Myers Dec 1 '10 at 3:37
4  
Absolutely! People contrive some pretty interesting program flow to avoid an early return statement. Either deeply nested control structures or continual checks. This can really bloat a method when a if return could really simplify this problem. –  snmcdonald Dec 19 '10 at 22:26
4  
If you need multiple returns in a function (aside from guards) your function is probably too long. –  EricSchaefer Dec 28 '10 at 20:16
18  
There's no point in having a return keyword unless it was intended to appear in multiple places. Return early, return often. It'll only serve to simplify your code further. If people can understand how break/continue statements work, why do they struggle with return? –  Evan Plaice Jan 12 '11 at 16:14
7  
I think this is a very out-dated best practice. I don't think it's a modern best practice. –  Skilldrick Jan 17 '11 at 11:26

Don't reinvent the wheel is a widely mis-used dogma. Its idea is that if a suitable solution exists, use it instead of creating your own; in addition of saving effort, the existing solution is likely better implemented (bug-free, efficient, tested) than what you would come up with initially. So far, so good.

The problem is that a 100 % suitable solution seldom exists. A 80 % suitable solution might exist, and using it is probably fine. But how about 60 % suitable? 40 %? Where do you draw the line? If you don't draw the line, you could end up incorporating a bloated library to your project because you're using 10 % of its features - just because you want to avoid "reinventing the wheel".

If you do reinvent the wheel, you'll get exactly what you want. You'll also learn how to make wheels. Learning by doing shouldn't be underestimated. And in the end, a custom wheel may well be better than off-the-shelf generic wheel.

share
40  
+1 for learn how to make wheels –  Jhonny D. Cano -Leftware- Oct 28 '10 at 13:22
3  
I've had this happen the other way around. I built my own ajax grid component because at the time there were none that did what I wanted, but later on replaced it with Ext JS grids. It helped that I made the assumption from the beginning that the display layer would get replaced. –  Joeri Sebrechts Oct 28 '10 at 19:15
20  
Agreed. If no one ever reinvented the wheel, then we'd all be driving our cars on wooden tires. –  Dr. Wily's Apprentice Oct 29 '10 at 20:41
6  
I always feel like your 10% example when I add Boost to a C++ project. I always need less than 10% of it, directly, but of course the functions I need import other modules which import other modules which... –  romkyns Nov 19 '10 at 13:02
3  
+1: just this week, I reinvented the wheel (e.g. replace the bloated yet popular jquery plugin we used by something that fits our needs yet modular) and it resulted in huge performance gains. Also, there are people whose job is literally to reinvent the wheel: take Michelin for example, they do R&D to improve tires. –  wildpeaks Jan 12 '11 at 18:39

"Unit Test Everything."

I've heard it said often that all code should have unit tests, a point I disagree with. When you have a test for a method, any change to the output or structure of that method must be made twice (once in the code, once in the test).

As such, unit tests should be, in my opinion, proportional to the structural stability of the code. If I'm writing a layered system from the bottom up, my data access layer will have tests out the wazoo; my business logic layer will be pretty well tested, my presentation layer will have some tests, and my views will have little to no testing.

share
7  
I suspect that "Unit Test Everything" has become a cliche, like the "premature optimization" quote. I generally agree with your proportions, and have seen many examples of developers going through monumental effort to mock an application-layer object, effort that might be better spent doing acceptance testing. –  Robert Harvey Oct 26 '10 at 18:23
36  
If a change to the structure of a method causes a change in your tests, you may be doing testing wrong. Unit tests should not verify implementation, only the result. –  Anna Lear Oct 26 '10 at 18:27
7  
@Anna Lear: I think he was talking about doing design/structural changes (refactoring). Since the design is not enough mature, when you find the better way to do it, you might have to modify a lot of test in the way. I agree that when you are more skilled tester you might more easily notice where the test would be a bad idea (because of this reason, and others) but if the design is not really mature, you will still most probably have some tests in the way. –  n1ckp Oct 27 '10 at 0:44
13  
I think this is also why the "do test first" idea do not work. To do the tests first you have to have the design right. But having the design right require that you try things and see how they works so you can improve on them. So you can't really do the tests before you have the design, and getting the design right require you to code and see how it works. Unless you got some really uber-architect I don't really see how that idea will work. –  n1ckp Oct 27 '10 at 1:02
13  
@n1ck TDD is not actually a testing exercise as much as it is a design exercise. The idea is that you evolve your design through tests (as that quickly exposes a reasonable API for your stuff) rather than fit the tests to an existing design (which may be bad/insufficient). So no, you don't have to have the design right to do the tests first. –  Anna Lear Oct 27 '10 at 4:45

Always program to interfaces.

Sometimes there will only be one implementation. If we delay the process of extracting an interface until the time when we see the need for it, we will often find it isn't necessary.

share
4  
Agreed, you program to an interface when you need an interface (i.e. a stable API to work from). –  Robert Harvey Oct 26 '10 at 20:06
45  
In my reading this rule is not about interfaces as language constructs. It means you shouldn't make any assumptions about the inner workings of a class when calling its methods, and should only rely on the API contracts. –  Zsolt Török Oct 26 '10 at 20:52
2  
Ok here's an interesting question - Im primarily a .NET developer so for me my interfaces look like IBusinessManager or IServiceContract. For me this is extremely easy to navigate (and I generally keep my interfaces in another namespace [or even another project]). When I've used Java, I've actually found this confusing (generally interface implementations I've seen are suffixed with .impl - and interfaces have no delineation). So could this be a code standards issue? Of course interfaces in java make the code look cluttered - they look exactly the same as normal classes upon first glance. –  Watson Oct 28 '10 at 14:19
5  
@Watson: one cost is that every time i hit F3 ('jump to declaration') on a method call in Eclipse, i jump to the interface, not the one implementation. I then have to control-T, down-arrow, return to get to the implementation. It also blocks some automatic refactorings - you can't inline a method across an interface definition, for example. –  Tom Anderson Oct 28 '10 at 17:35
4  
@Tom: Well sir, I would gladly engage you in that war, Eclipse vs. Intellij -- however I have a outstanding moral code which prevents me from getting into physical confrontations with someone who has an obvious handicap. BOOM. I'm not saying Eclipse is bad, I'm saying that if the Axis powers had used it to build or design their war machines, WWII would now be known as the "Two-day kerfuffle". Seriously though, I've found that it lacks some polish that I get in off-the-shelf IDEs (Intellij/VS + ReSharper). I have found myself fighting it on more than one occasion -- which is one too many. –  Watson Oct 29 '10 at 14:15

Don't use anything open-source (or non-Microsoft for you .NET developers)

If Microsoft didn't develop it -- we don't use it here. Want to use ORM - EF, want to use IOC - Unity, want to log - enterprise logging application block. So many better libraries exist -- yet I'm always stuck ordering from the dollar menu of the development world. I swear every time I hear Microsoft Best Practices I think "McDonald's Nutritional Guidelines". Sure, you'll probably live if you follow them, but you'll also be malnourished and overweight.

  • Note this might not be your best practice, but it's a common one followed almost everywhere I've worked.
share
13  
Sounds horrible...=( I'm probably too much on the other side, though, I avoid M$ as much as possible. –  Lizzan Oct 28 '10 at 13:23
4  
You won't get fired for buying IBM^wMicrosoft –  Christopher Mahan Oct 29 '10 at 4:30
17  
There is also the mirror-image version, ie Never use anything Microsoft, or never use anything you have to pay for. –  Richard Gadsden Jan 17 '11 at 12:53
5  
I'm lucky enough to work at an organization where this is not a widely held dogma, but in the places where we have adopted commercial solutions, there sure is a great deal of pain. The problem comes in when some part of the commercial solution doesn't quite work. When it's open source, you can look at the source (The ultimate documentation) and find out what's going wrong. With closed source, you have to pay for the privilege of accessing the knowledge of tech support who knows even less about the product that you do. And thats the only available 'fix'. –  TokenMacGuy Jan 29 '11 at 6:15

Object orientation

There is the assumption, just because code is "object-oriented", it's magically good. So people go on squeezing functionality into classes and methods, just to be object-oriented.

share
7  
I can't imagine building a software system of any significant size without taking advantage of the organization that Object Orientation provides. –  Robert Harvey Oct 28 '10 at 20:55
18  
Robert. Unix isn't object oriented, and it certainly qualifies as a software system of significant size. It also seems to be quite popular (think Mac OSX, iPhone, Android phones, etc) –  Christopher Mahan Oct 29 '10 at 4:29
7  
What i meant is that i think we should use the most appropriate methodology. I've seen people use methods and classes where it cumbersome and does not make much sense, just because "it's object-oriented". That's cargo cult. –  LennyProgrammers Oct 29 '10 at 7:49
8  
There is no silver bullet. Functional Programming (Haskell) is quite successfull without being object oriented. At the end of the day, you've got multiple tools and it is your job to pick the best assortment for the task at hand. –  Matthieu M. Nov 22 '10 at 20:46
9  
The funny thing is, that besides using classes, polymorphism and the likes, most of object oriented code is in fact procedural code. –  Oliver Weiler Nov 30 '10 at 0:05

All code should be commented.

No, it shouldn't be. Some time you have obvious code, for example setters should not be commented, until they do something special. Also, why should I comment this:

/** hey you, if didn't get, it's logger. */
private static Logger logger = LoggerFactory.getLogger(MyClass.class);
share
13  
All code should be understandable. Comments are a major tool in that, but far from the only one. –  Trevel Jan 17 '11 at 22:27
2  
Generated documentation. That's what the /** */ format is for instead of the /* */ format comment. Or for .NET it would be /// –  Berin Loritsch Jan 18 '11 at 16:19
10  
Using }//end if, }//end for, }//end while are the best example of wasteful commenting I've ever encountered. I've seen this many times where the opening brace is no more than 2 lines above. IMHO if you need these comments then your code needs re-factoring... or you need to pony up $20 and buy an IDE/Text editor that highlights matching braces. –  scunliffe Jan 21 '11 at 11:54
7  
Code say "how". Comments need to say "why". –  user1249 Jan 27 '11 at 18:03

Methodologies, particularly scrum. I can't keep a straight face when I hear grownups use the phrase "Scrum Master". I get so tired of hearing developers protest that some aspect of Methodology X isn't working for their company, only to be told by Guru So-and-So that the reason it's not working is that they are not, in fact, true practitioners of Methodology X. "Scrum harder, you must, my Padawan learner!"

There are nuggets of wisdom in agile methodologies---lots of them---but they're often couched in so much manure that I can't fight my gag reflex. Take this bit from Wikipedia's scrum page:

A number of roles are defined in Scrum. All roles fall into two distinct groups—pigs and chickens—based on the nature of their involvement in the development process.

Really? Pigs and chickens, you say? Fascinating! Can't wait to pitch this one to my boss...

share
12  
+1...you're right, its hard to take that seriously. <big booming voice> *I AM THE SCRUMMASTER*</voice> –  GrandmasterB Nov 29 '10 at 21:50
2  
And the parables. It reminds me of church sermons, or the kinds of anecdotes that self-help gurus (and comedians) are famous for: "Take my friend Steve. Steve was constantly arguing with his wife Sheryl. Those two would go at it for hours, to the point where their marriage was in real jeopardy. Then, one day..." These kinds of didactic yarns don't bother me in other spheres, but I hate to see them proliferate in the engineering sciences. –  evadeflow Nov 29 '10 at 22:02
2  
What about the Scrum Ninjas? –  Berin Loritsch Jan 12 '11 at 18:20
1  
I don't agree with the "Pig and Chicken" comparison...it flies directly in the face of the Agile Manifesto. Namely "Customer collaboration over contract negotiation". Customers are as vested (if not moreso) in the success of the project as the project team. Calling some roles pigs and other roles chickens sets up an "us versus them" mentality that IMHO is the biggest roadblock to successful projects. –  Mike Brown Jan 21 '11 at 23:58

Object Relational Mapping... http://en.wikipedia.org/wiki/Object-relational_mapping

I don't want to ever be abstracted away from my data, nor do I want to lose that precise control and optimization. My experience with these systems has been extremely poor... The queries generated by these abstraction layers is even worse than what I've seen from off-shoring.

share
19  
Premature optimization is the root of all evil. Slow code is, in real life, only incredibly rarely a problem when compared to unmaintainable code. Use an ORM, then cut through the abstraction only where you need it. –  Fishtoaster Oct 26 '10 at 18:12
28  
ORM's are an 80-20 tool. They are intended to handle the 80% CRUD that becomes so tiresome to write all that endless plumbing code after awhile. The other 20% can be done in more "conventional" ways like using stored procedures and writing ordinary SQL queries. –  Robert Harvey Oct 26 '10 at 18:13
18  
@Fishtoaster: Don't you mean, "We should forget about small efficiencies, say about 97% of the time: premature optimization is the root of all evil. Yet we should not pass up our opportunities in that critical 3%."? –  Robert Harvey Oct 26 '10 at 18:15
5  
@Robert Harcey: There's a reason I didn't use a direct quote. I think that most programmers focus on efficiency way too much- it's a problem few of them really need to solve. Admittedly, there are certain domains where it's more important than others, but maintainability and extensibility are a problem everywhere. Another modified-quote: "Make it work, make it maintainable, make it readable, make it extensible, make it testable, and then, if you have time and it turns out you need it, make it fast." –  Fishtoaster Oct 26 '10 at 18:23
12  
@Craig: How did you not recognize the irony in your statement? Needing a year to learn how to get good performance out of an ORM is an excellent argument against ORMs, as is the need to "control" the SQL produced and inject stored procedures. If you have the knowledge for that, you have the knowledge to bypass the ORM entirely. –  Nicholas Knight Nov 22 '10 at 11:48

Writing function names as if they were English sentences:

Draw_Foo()
Write_Foo()
Create_Foo()

etc. This might look great, but it's a pain when you're learning an API. How much easier is it to search an index for "Everything that starts with Foo"?

Foo_Draw()
Foo_Write()
Foo_Create()

etc.

share
2  
Probably about as easy as typing Foo in TM's function list. –  Josh K Oct 26 '10 at 17:50
68  
Sounds like you'd actually want it to be Foo.Draw(), Foo.Write() and Foo.Create(), so that you could do Foo.methods.sort or Foo.methods.grep([what I seek]).sort –  Inaimathi Oct 26 '10 at 17:59
7  
Starting with 'Get' is another example. –  JeffO Oct 26 '10 at 18:25
1  
I come from the objective-c world, and greatly miss verbose method names and infix notation when I do java (my other life). Since code completion started working I haven't found the extra typing a problem, either. –  user4051 Oct 26 '10 at 18:28
2  
@Scott Whitlock: Not that out of date for some .NET developers, iirc, VS 2008 didn't do that. 2010 does though, and it's fantastic. –  Steve Evers Nov 18 '10 at 21:44

MVC - i often find that shoehorning many web design problems into the MVC approach is more about making a framework (rails etc) happy than about simplicity or structure. MVC is a favorite of "architecture astronauts" who seem to value excessive scaffolding over simplicity. ymmv.

class-based OO - in my opinion encourages complex structures of mutable state. the only compelling cases i have found for class-based OO over the years are the corny "shape->rectangle->square" examples that form chapter 1 of any OO book

share
4  
I've done web development in ASP.NET and ASP.NET MVC and, even though MVC does seem scaffoldy, I much prefer it over ASP.NET, for many reasons: simplicity, maintainability, and exceedingly fine control over the markup. Everything has its place and, while it all seems a bit repetitive, it's a joy to maintain. It's completely customizable, so if you don't like a behavior out of the box, you can change it. –  Robert Harvey Oct 29 '10 at 6:06
1  
As far as OO is concerned, there are good and bad ways to do it. Inheritance is overrated, and used more sparingly in the real world than most books would have you believe; there is currently a trend towards a more functional, immutable development style, even in the OO world. –  Robert Harvey Oct 29 '10 at 6:08
1  
+1 for mentioning MVC. While the concept of MVC (separating data layer logic, presentation logic, and background logic is a good idea) physically separating them into a complex folder hierarchy with a mishmash of files containing code snippets is stupid. I blame this whole phenomena on PHP's lack namespace support and newbie developers glorifying decades-old techniques as 'the newest thing'. It's simple, create a namespace for the database accessors, GUI, and background logic (and sub-namespaces where needed). –  Evan Plaice Nov 19 '10 at 18:50
3  
As for OO, you won't really see it's benefits until your project grows to a size where managing complexity becomes important. Follow the single responsibility principle wherever possible and if your code is accessible publicly exposed (ex. for a .dll) hide the internal implementation of classes/methods/properties wherever possible to make the code more secure and to simplify the API. –  Evan Plaice Nov 19 '10 at 18:55
1  
I personally find the shape->rectangle->square example to be one of the most elegant arguments against oop. for instance, to Square(10).Draw() it is sufficient to Rectangle(10, 10).Draw(). so I guess that means square is a subclass of rectangle. But mySquare.setWidthHeight(5,10) is nonsense (IE, it fails the Liskov substitution principle), a square cannot have different height and width, although a rectangle can, so that implies that rectangle is a subclass of square. This is known in other contexts as the "Circle, Ellipse problem" –  TokenMacGuy Jan 19 '11 at 2:32

YAGNI

(You ain't gonna need it)

This approach has cost me hours and hours when I had to implement features on an existing codebase, where careful planning would have included these features beforehand.

Ideas of mine have often been rejected because of YAGNI, and most of the times someone had to pay for that decision later.

(Of course one could argue that a well-designed codebase would also allow features to be added later on, but reality is different)

share
15  
I agree with YAGNI, but I see what you're getting at. The point of YAGNI is to deal with people who want to plan everything up front down to the slightest detail. Nine times out of ten though, it's used as an excuse to under engineer code or completely skip planning. –  Jason Baker Nov 29 '10 at 19:55
12  
P.Floyd, @Jason Baker: +1 Absolutely right. The old saying applies here "months in the lab can save hours in the library" –  Steve Evers Nov 29 '10 at 21:40
1  
@TokenMacGuy the crucial aspect is the implied by the spec part. That's where opinions differ a lot. –  Sean Patrick Floyd Jan 28 '11 at 14:04

For SQL

  1. Don't use triggers
  2. Always hide tables behind views

In order:

  1. They are a feature that has it's place. You have multiple update paths to a table, or require 100% auditing?

  2. Just why? I would if I was refactoring to maintain a contract, but not when I've read that folk change the view to match any table changes

Edit:

Number 3: Avoiding * with EXISTS. Try 1/0. It works. The column list isn't evaluated as per SQL standard. Page 191

share
3  
#2 is a best practice? –  Hogan Oct 27 '10 at 3:35
2  
1  
@Hogan: A view that simply mirrors a base table adds no security over using the base table directly. If you join to a securityuser table, or mask some columns then fair enough. But SELECT every column from table or view: no difference. Personally, I do use stored procs anyway. –  gbn Oct 27 '10 at 11:20
3  
@Hogan: I know something about SQL Server :-) stackoverflow.com/users/27535/gbn What I mean is GRANT SELECT on table is no different to GRANT SELECT ON VIEW if the view is SELECT * FROM TABLE –  gbn Oct 27 '10 at 11:42
1  
@gbn : I agree. There is no difference there. I think we might be saying the same thing. I guess my original comment ("#2 is a best practice?") was based more on my personal experience that views (like triggers) are more often miss-used than correctly used. Thus such a best practice would only lead to abuse not enhancement. If it is considered a best practice you are 100% right it is a bad one. –  Hogan Oct 27 '10 at 22:53

Design Patterns mostly. They are over used and under utilised.

share
13  
+1 I still don't see how Design Patterns are beautiful or elegant solutions. They are workarounds to language deficiencies, nothing more. –  Oliver Weiler Nov 30 '10 at 0:02
2  
Just see it as an endeavor of eradicating language smell using the language itself. –  Filip Dupanović Jan 18 '11 at 2:10

The Single Responsibility Principle

("every class should have only a single responsibility; in other words, every class should have one, and only one, reason to change")

I disagree. I think a method should have only one reason to change, and all methods in a class should have a logical relationship to one-another, but the class itself might actually do several (related) things.

In my experience, this principle too-often gets applied over-zealously, and you end up with many tiny one-method classes. Both the agile shops I've worked at have done this.

Imagine if the creators of the .Net API had had this sort of mentality: rather than List.Sort(), List.Reverse(), List.Find() etc., we'd have ListSorter, ListReverser, and ListSearcher classes!

Rather than argue anymore against the SRP (which itself isn't terrible in theory), I'll share a few of my long-winded anecdotal experiences:


At one place I worked, I wrote a very simple max flow solver which consisted of five classes: a node, a graph, a graph-creator, a graph-solver, and a class to use the graph-creator/solvers to solve a real-world problem. None were particularly complex or long (the solver was by far the longest at ~150 lines). However, it was decided that the classes had too many "responsibilities," so my co-workers set about refactoring the code. When they were done, my 5 classes had been expanded to 25 classes, whose total lines-of-code were more than triple what they were originally. The flow of the code was no longer obvious, nor was the purpose of the new unit-tests; I now had a hard time figuring out what my own code did.


At the same place, almost every class had only a single method (its only "responsibility"). Following the flow within the program was nearly impossible, and most unit-tests consisted of testing that this class called code from another class, both of whose purpose were equally a mystery to me. There were literally hundreds of classes where there should have been (IMO) only dozens. Each class did only one "thing", but even with naming conventions like "AdminUserCreationAttemptorFactory", it was difficult to tell the relationship between classes.


At another place (which also had the classes-should-have-only-one-method mentality), we were trying to optimize a method which took up 95% of time during a certain operation. After (rather stupidly) optimizing it a bit, I turned my attention towards why it was being called a bajillion times. It was being called in a loop in a class... whose method was being called in a loop in another class.. which was also being called in a loop..

All told, there were five-levels of loops spread out over 13 classes (seriously). What any one class was actually doing was impossible to determine just by looking at it - you had to sketch a mental graph of what methods it called, and what methods those methods called, and so on. If it had all been lumped into one method, it would have only been about 70 lines long with our problem-method nested inside five immediately-obvious levels of loops.

My request to refactor those 13 classes into one class was denied.

share
6  
Sounds like someone got "Pattern Fever" or in this case "Principle Fever" at that job. The list class doesn't violate SRP. All of its functions serve one purpose, manipulating a collection of objects. Having only one function in a class sounds like overkill to me. The principle behind SRP is that a unit of code (be it method, class, or library) should have a single responsibility that can be stated succintly. –  Mike Brown Jan 22 '11 at 0:37
3  
I've started to see this kind of madness from people who find it impossible to write pure plain functional code. Too much educating that every problem in the world can be solved out of a book of patterns. Not enough thought about pragmatism. Like you I've seen some class-based OO code thats so horrible that following it is completely impossible. And its huge and bloated. –  quickly_now Feb 6 '11 at 9:51
3  
2nd comment here. Lots of "principles" are over-applied. There are many things that are good ideas, where doing the exact opposite is sometimes appropriate. GOOD programmers know when the break the rules. Because the rules are not "rules", they are statements of "good practice most of the time except when its a really dumb idea." –  quickly_now Feb 6 '11 at 9:53

Now that you mentioned Clean Code, while it contains some good ideas, I think its obsession to refactor all methods into submethods and those into subsubmethods etc. is taken way too far. Instead of a couple of ten-line methods you're supposed to prefer twenty (supposedly well named) one-liners. Obviously someone thinks it's clean, but to me it seems way worse than the original version.

Also, replacing simple elementary things such as

0 == memberArray.length

within a class with a call to the class's own method such as

isEmpty()

is a questionable "improvement" IMHO. Addition: The difference is that the first check does exactly what it says: checks whether the array length is 0. Ok, isEmpty() might check array length too. But it could also be implemented like this:

return null != memberArray ? 0 == memberArray.length : true;

That is, it includes an implicit null check! This may be fine behavior for a public method - if the array is null, then something is certainly empty - but when when we're talking about class internals, this is not so fine. While encapsulation to outside is a must, class internals must know exactly what's going on within the class. You can't encapsulate the class from itself. Explicit is better than implicit.


This is not to say that breaking down long methods or involved logical comparisons is no good; of course it is, but to which degree to do it -- where the sweet spot is -- is obviously a question of taste. Breaking down a long method results in more methods, and that does not come for free. You have to jump around source code in order to see what's really going on, when you could see it at a glance if all that stuff were in a single method.

I would even go as far as to say that in some cases a 1-line method is too short to deserve being a method.

share
6  
I rarely see this as an issue. Mostly that's because I usually see too much in a single method, not too little. However, according to some studies very low complexity in methods also has a slightly higher bug rate than moderately low complexity. enerjy.com/blog/?p=198 –  MIA Oct 27 '10 at 15:30
1  
I find your second example eminently more readable, and that form (or something similar, like a Length property on the class itself) is a must if you're making it public. –  Robert Harvey Oct 27 '10 at 22:13

"Be liberal with comments"

Comments are definitely a good thing, but too many are just as bad if not worse than not enough. Why? Well, people tend to tune comments out if they see too many unnecessary ones. I'm not saying that you can have perfectly self-documenting code, but it is preferable to code that needs comments for explanation.

share
1  
self-documenting code is definitely nice. Although, I like having comments along-side something like simple calculations (to express what the return-type or returned-value is). However, if your comments need more words than the code itself, it's probably time to rewrite the code. –  sova Nov 29 '10 at 20:11
6  
I have to agree with the way that sova put this one. Clean code is preferable to comments. –  Stargazer712 Nov 29 '10 at 23:47
4  
You still need the "why"'s in there! –  user1249 Jan 21 '11 at 0:04

GoTo Considered Harmful

If you are implementing a state machine a GOTO statement can make more sense (readability and efficient code) than a "structured programming" approach. It really worried some co-workers when the first piece of code I wrote in a new job included not just one but several goto statements. Fortunately they were intelligent enough to realize that it was actually the best solution in this particular case.

Any "best practice" that doesn't allow for sensible and documented exceptions to its rules is just plain scary.

share
16  
I'm going on nine years programming without a single goto statement (including several state machines, as you mentioned). Expand your mind to new ideas. –  Stargazer712 Nov 22 '10 at 4:46
3  
@Mathieu M. - agreed - mixing GOTO with structured control statements isn't sensible. (This was pure C and it wasn't an issue. –  MZB Nov 25 '10 at 0:22
1  
@Stargazer2 - with a simple FSM, it depends whether putting the state in a variable and using it as an index to call a procedure (isn't that the same as a computed GOTO?) gives clearer/faster code than using the program counter as the FSM state. I'm not advocating this as the best solution in most circumstances, just the best solution in some circumstances. Expand your mind to alternative approaches. –  MZB Nov 25 '10 at 0:31
5  
@MZB, wouldn't you agree that a function call is also just a computed GOTO? Same goes for for/while/if/else/switch constructs, among others. Language designers abstract away direct changes to the program counter for a reason. Do not use goto. –  Stargazer712 Nov 29 '10 at 22:37
3  
Directly implementing a statemachine is probably an antipattern. There are lots of ways to have a state machine without expressing the states and transiitions literally. for instance, import re –  TokenMacGuy Jan 19 '11 at 2:37

The sacrifices we make to make code testable

I jump through a lot of hoops to make my code testable, but I don't pretend that I wouldn't if given the choice. However, I often hear people pushing the idea that these are "best-practices." These practices include (written in the language of .Net, but applies to other languages as well):

  • Creating an interface for every class. This doubles the number of classes (files) to deal with, and duplicates code. Yes, interface programming is good, but that is what the public/private specifiers are meant for.
  • Every class not instantiated at startup needs a factory. Clearly, new MyClass() is much simpler than writing a factory, but now the method that creates it cannot be tested in isolation. If not for this fact, I would only make 1/20th the number of factory-classes that I do now.
  • Make every class public, which defeats the purpose of having access specifiers on classes at all. However, non-public classes cannot be accessed (and thus, tested) from other projects, so the only other option is move all the testing code to the same project (and thus release it with the final product).
  • Dependency Injection. Clearly having to give every other class I use a field and a constructor-parameter is significantly more work than just creating them when I need them; but then I can no longer test this class in isolation.
  • The Single Responsibility Principle, which has caused me so many headaches I've moved it to its own answer.

So what could we do to fix this? We'd need a drastic change in the language architecture:

  • We'd need the ability to mock classes
  • We'd need the ability to test private methods of internal classes, from another project (this may seem like a security vulnerability, but I don't see a problem if the testee is forced to name its tester-classes).
  • Dependency injection (or service location), as well as something equivalent to our existing factory pattern, would have to be a core part of the language.

In short, we need a language designed from the ground-up with testability in mind.

share

Separating applications into Tiers; Data Layer, Business Layer, UI Layer

The main reason that I dislike this is that most places that follow this method, use very brittle frameworks for getting it done. I.E. UI Layer is hand coded to deal with business layer objects, business layer objects are hand coded to deal with business rules and database, database is SQL and is already fairly brittle and managed by the "DBA" group who dislike change.

Why is this bad? The most common enhancement request is likely "I need a field on screen X that has Y." Bang! You just have a new feature that affects every single layer, and if you separate layers with different programmers, it just became a big issue involving multiple people and groups, for a very simple change.

Additionally, I don't know how many times I've been in arguments that go something like this. "The name field is limited to a maximum length of 30, is this a UI layer, or business layer, or data layer issue?" And there are a hundred arguments, and no right answer. The answer is the same, it affects all layers, you don't want to make the UI dumb and have to go through all layers, and fail at the database, just so the user finds out his entry is too long. If you change it, it affects all layers, etc.

The "layers" tend to leak as well; If any layer is physically separated by process/machine boundaries (I.E. web UI and business backend logic), the rules get duplicated to make everything work reasonably well. I.e. some business logic ends up in the UI, even though it's a "business rule", because the user needs the UI to be responsive.

If the framework used, or architecture used, is resilient to small changes and leakage, i.e. based on meta data, and dynamically adjusted through all layers, it can be less painful. But most frameworks this is a royal pain, requiring UI changes, business layer changes, and database changes, for every small change, and causing more work and less help than the technique is supposed to produce.

I will probably get slammed for this, but there it is :)

share

JavaBeans

The use of JavaBeans in Java. See my question Why shouldn't I use immutable POJOs instead of JavaBeans? on StackOverflow.

share

User Stories / Use Cases / Personas

 

I understand the need for these when you are programming for an industry that you are not familiar with, but I think that when they are implemented in full force they become too corporate and become a waste of time.

share

80 char/line limits are dumb

I understand that some compromises need to be made to match the pace of the slowest runner on the GUI side (screen resolution limitations, etc) but, why does that rule apply to code formatting?

See... There was this little invention called the horizontal scroll bar that was created to manage virtual screen space outside of the right-most pixel boundary. Why don't developers, who have managed to create great productivity enhancing tools like syntax highlighting and auto-complete use it?

Sure, there are CLI editors religiously used by *nix dinosaurs that follow the tired old limitations of their VT220 terminals but why are the rest of us held to the same standard?

I say, screw the 80 char limit. If the dinosaurs are epic enough to hack on emacs/vim all day, why can't the should be capable of creating an extension that automatically wraps lines or gives horizontal scrolling capabilities to their CLI IDEs?

1920x1080 pixel monitors will eventually become the norm and developers worldwide are still living under the same limitations with no bearing on why they do except, that's what they were told to do by their elders when they were just starting to program.

80 char limits are not a best practice but a niche practice for a very minority of programmers and should be treated as such.

Edit:

It's understandable that many devs don't like the horizontal scrollbar because it requires a mouse gesture so... Why not increase the column width limit to a higher number (than 80) for those of us who use modern displays.

When 800x600 computer monitors became the norm for most users, web developers increased their website widths to accommodate the majority... Why can't developers do the same.

share
1  
@Orbling Nice GWB logic with the ___ is evil angle. So you loathe the hScroll, do you have any valid reason why col-width should be limited to 80 chars? Why not 160 or 256? I think we can all assume that most developers have retired their VT220 terminals and replaced them with puTTY so they're capable of extending the width programmatically anyway. –  Evan Plaice Jan 18 '11 at 1:38
4  
I prefer that we stick to the 80 char limit. As soon as you give me more horizontal space, I'll try to open up additional documents side-by-side with the rest. I'd hate to have to scroll four ways. Also, I've noticed often that I'm force to write more readable code with the 80 char cap. –  Filip Dupanović Jan 18 '11 at 2:17
2  
how would you print it? –  user1249 Jan 21 '11 at 0:11
5  
Sorry - have to disagree on this one. I really hate long long lines - it requires more eye movement, it requires mouse gestures to scroll across, its harder to see subtly little dangly things down the end of a line. In about 99% of cases there are clean ways of making stuff run over several (shorter) lines which is clearer and easier to read. 80 Chars might be arbitrary and "because it was that way in punch card days" but its still a reasonable framework to work inside - most of the time - for the reasons above. –  quickly_now Jan 21 '11 at 1:25
3  
Indent by 2 spaces. And use an editor with an automatic indentation tracker. I've done it that way for years, no big deal. (Emacs with the right modes helps here.) –  quickly_now Feb 6 '11 at 9:47

Measure, Measure, Measure

So fine, measure away, but for isolating performance bugs, measuring works about as well as successive elimination. Here is the method I use.

I've been trying to track down the source of the measure-measure "wisdom". Somebody with a tall enough soapbox said it, and now it travels.

share

My teacher demands I start all my identifiers (not including constants) with lowercase letters, e.g. myVariable.

I know it seems like a minor thing, but many programming languages require variables to start with uppercase letters. I value consistency, so it's a habit of mine to start everything with uppercase letters.

share
9  
I had a teacher that required camelCase because he insisted that was what people use in the realWorld... At the same time I was programming in two different groups at my work - both groups insisted on under_scores. What matters is what your group uses. He could have defined himself as the lead programmer and all would have been fine in my book - we follow his conventions - but he was always giving his opinion as "the way things are done in the real world" as if there was no other valid way. –  xnine Nov 30 '10 at 4:42
5  
Camel case (first letter lowercase) is a pretty common convention as well as Pascal case (first letter of every word capitalized). In most languages, camelCase is used on private/internal variables where PascalCase is used on classes, methods, properties, namespaces, public variables. It's not a bad practice to get used to just be ready for projects that may use a different naming scheme. –  Evan Plaice Jan 12 '11 at 16:18
2  
Just an FYI. Some languages infer meaning from whether the first letter of a variable is capital or not. In one such language, if the first letter is capital the variable is treated as a constant--any attempt to change it will throw an error. –  Berin Loritsch Jan 12 '11 at 18:28
1  
In Go, public methods and members in classes start with an Upper-case letter; private ones with a lower-case letter. –  Jonathan Leffler Jan 18 '11 at 14:39

Use Singletons

When you only should have one instance of something. I cant disagree more. Never use a singleton and just allocate it once and pass around the pointer/object/reference as necessary. There is absolutely no reason not to do this.

share
2  
That's a whole bunch of negatives that has left me rather confused as to your actual stance on singletons. –  Paul Butcher Nov 30 '10 at 11:17
1  
@Paul Butcher: I hate singletons and it should never be used –  acidzombie24 Dec 1 '10 at 7:58
1  
@rwong: Personally i dont think any reason is a legitimate reason. Just write it as a normal class. Really, there is no reason to use a singleton other then lazyness and to promote bad habits or design. –  acidzombie24 Dec 1 '10 at 22:23
6  
Who says that using Singeltons is best practice? –  Phil Mander Dec 10 '10 at 0:29
2  
Singletons do have their place, especially in instances where an operational structure is allocated and filled on startup, then basically becomes read only throughout the run time of the program. In that case, it just becomes a cache on the other side of a pointer, though. –  Tim Post Dec 10 '10 at 8:06

Using unsigned int as iterator

When will they learn that using signed int is much safer and less bug-prone. Why is it sooo important that array index can only be positive number, that everyone is glad to overlook the the fact that 4 - 5 is 4294967295?

share
4  
@Paul Nathan: as far as the buggyness goes here's one example: for(unsigned int i=0; i<10; i++){int crash_here=my_array[max(i-1,0)];} –  AareP Dec 10 '10 at 5:59
1  
@Adam Paynter: yes. This might seem normal for c++ programmers, but let's face it, "unsigned int" is one bad "implementation" of positive-only-numbers. –  AareP Dec 10 '10 at 11:19

Methods shouldn't be longer than a single screen

I agree with the single-responsibility principle completely but why do people perceive it to mean "a function/method can have no more than a single responsibility at the finest level of logical granularity"?

The idea is simple. A function/method should accomplish one task. If part of that function/method can be used elsewhere, chop it out into it's own function/method. If it could be used elsewhere on the project, move it into its own class or a utility class and make it internally accessible.

Having a class that contains 27 helper methods that are only called once in the code is dumb, a waste of space, an unnecessary increase in complexity, and a massive time sink. It sounds more like a good rule for people who want to look busy refactoring code but don't produce much.

Here's my rule...

Write functions/methods to accomplish something

If you find yourself about to copy/paste some code, ask yourself whether it would be better to create a function/method for that code. If a function/method is only called once in another function/method, is there really a point in having it in the first place (will it be called more often in the future). Is it valuable to add more jumps in/out of functions/methods during debugging (Ie, does the added jump make debugging easier or harder)?

I completely agree that functions/methods greater than 200 lines need to be scrutinized but some functions only accomplish one task in as many lines and contain no useful parts that can be abstracted/used on the rest of the project.

I look it at from an API dev perspective... If a new user were to look at the class diagram of your code, how many parts of that diagram would make sense within the greater whole of the project and how many would exist solely as helpers to other parts internal to the project.

If I were to choose between two programmers: the first has a tendency to write functions/methods that try to do too much; the second breaks every part of every function/method to the finest level of granularity; I would choose the first hands down. The first would accomplish more (ie, write more meat of the application), his code would be easier to debug (due to fewer jumps in/out of functions/methods during debugging), and he would waste less time doing busy work perfecting how the code looks vs perfecting how the code works.

Limit unnecessary abstractions and don't pollute the autocomplete.

share
3  
I think one counter argument to this is that refactoring parts of a large method into separate calls can make the larger method easier to read. Even if the method is only called once. –  Jeremy Heiler Jan 17 '11 at 15:33
1  
@Jeremy How? How does, abstracting out a section of code and placing it in its own method make it any more readable than just placing a one line comment at the top of that chunk of code describing what it does? Assuming that block of code is only used once in that section of code. Is it really that difficult for most programmers to decompose the working parts of the code while they read it and/or place a few one-line comments to remind them what it does if they can't? –  Evan Plaice Jan 18 '11 at 1:28
4  
@Evan: Putting pieces of code into functions effectively gives them a name, hopefully well explaining what that piece of code does. Now, wherever that piece of code is called, you can see the name explaining what the code does, instead of having to analyze and understand the algorithm itself. If done well, this can ease reading and understanding code tremendously. –  sbi Jan 19 '11 at 19:00
1  
+1 and I'd give more if I could. There's nothing at all wrong with a lump of C code thats 1000 lines in a single function (eg a parser with a massive switch()) PROVIDED the intent is clear and simple. Busting all the little pieces out and calling them just makes the thing harder to understand. Of course there are limits to this too.... sensible judgement is everything. –  quickly_now Jan 21 '11 at 1:22

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