98
votes

"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.

14
  • 8
    Which are the practices you disagree with?, just curious. Oct 28, 2010 at 13:17
  • Anyone can write a book, and I do not have to agree with it - it is as simple as that.
    – Job
    Jan 12, 2011 at 18:49
  • One thing I like about the book "Code Complete" by Steve McConnell is that he backs up all his tips with hard evidence and research. Just sayin'
    – JW01
    Jan 17, 2011 at 14:44
  • 5
    @Walter: This has been open for months, it is definitely constructive, why close it?
    – Orbling
    Jan 18, 2011 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, 2011 at 1:33

45 Answers 45

125
votes

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.

7
  • 4
    Wonderful quote. Oct 26, 2010 at 20:10
  • The next paragraph of that Raymond Chen quote is probably describing Hungarian notation! Most companies I see using it with no real good reason that they can explain.
    – Craig
    Oct 27, 2010 at 2:03
  • 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, 2010 at 8:23
  • 3
    Rationale is good. Research is better.
    – Jon Purdy
    Oct 27, 2010 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." Oct 29, 2010 at 0:47
93
votes

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.

20
  • 4
    +1 for the strong No, it's not.
    – Stephen
    Oct 26, 2010 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, 2010 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, 2010 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. Oct 27, 2010 at 12:06
  • 8
    @Robert, the Knuth quote was optimized prematurely...
    – user1249
    Jan 21, 2011 at 0:00
93
votes

One return per function/method.

11
  • 7
    I was going to put this. I love me some early return statements. Dec 1, 2010 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, 2010 at 22:26
  • 4
    If you need multiple returns in a function (aside from guards) your function is probably too long. Dec 28, 2010 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? Jan 12, 2011 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, 2011 at 11:26
87
votes

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.

10
  • 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. Oct 28, 2010 at 19:15
  • 20
    Agreed. If no one ever reinvented the wheel, then we'd all be driving our cars on wooden tires. Oct 29, 2010 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... Nov 19, 2010 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, 2011 at 18:39
  • 2
    @Dr. Wily, those wheels were not reinvented, they were refactored!
    – user1249
    Jan 21, 2011 at 0:01
78
votes

"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.

24
  • 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. Oct 26, 2010 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.
    – Adam Lear
    Oct 26, 2010 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, 2010 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, 2010 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.
    – Adam Lear
    Oct 27, 2010 at 4:45
57
votes

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.

16
  • 4
    Agreed, you program to an interface when you need an interface (i.e. a stable API to work from). Oct 26, 2010 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. Oct 26, 2010 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, 2010 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. Oct 28, 2010 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, 2010 at 14:15
46
votes

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.
8
  • 13
    Sounds horrible...=( I'm probably too much on the other side, though, I avoid M$ as much as possible.
    – Lizzan
    Oct 28, 2010 at 13:23
  • That shouldn't be that way. A library should be picked for its value, not just considering who made it. For instance, I love EF but I had a bad experience with Enterprise Library, and found better tools for validation and logging, such as FluentValidation, log4net and Elmah. Oct 28, 2010 at 13:28
  • 4
    You won't get fired for buying IBM^wMicrosoft Oct 29, 2010 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. Jan 17, 2011 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'. Jan 29, 2011 at 6:15
40
votes

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.

13
  • 7
    I can't imagine building a software system of any significant size without taking advantage of the organization that Object Orientation provides. Oct 28, 2010 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) Oct 29, 2010 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. Oct 29, 2010 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. Nov 22, 2010 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. Nov 30, 2010 at 0:05
35
votes

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);
8
  • 13
    All code should be understandable. Comments are a major tool in that, but far from the only one.
    – Trevel
    Jan 17, 2011 at 22:27
  • Absulutely, code should be understandable. But there is no single reason to write a comment which won't add anything to, for example, method name. If you write /** sets rank. */ void setRank(int rank) { this.rank = rank; } I assume the comment as a stupid one. Why it is written? Jan 17, 2011 at 22:34
  • 2
    Generated documentation. That's what the /** */ format is for instead of the /* */ format comment. Or for .NET it would be /// Jan 18, 2011 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, 2011 at 11:54
  • 7
    Code say "how". Comments need to say "why".
    – user1249
    Jan 27, 2011 at 18:03
32
votes

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...

7
  • Interesting. I agree to an extent. With that last part though: call them what you want, they're mnemonics, that's all. Nov 29, 2010 at 21:38
  • 12
    +1...you're right, its hard to take that seriously. <big booming voice> *I AM THE SCRUMMASTER*</voice> Nov 29, 2010 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, 2010 at 22:02
  • 2
    What about the Scrum Ninjas? Jan 12, 2011 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. Jan 21, 2011 at 23:58
25
votes

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.

16
  • 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. Oct 26, 2010 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. Oct 26, 2010 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%."? Oct 26, 2010 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." Oct 26, 2010 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. Nov 22, 2010 at 11:48
22
votes

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

13
  • 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. Oct 29, 2010 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. Oct 29, 2010 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). Nov 19, 2010 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. Nov 19, 2010 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" Jan 19, 2011 at 2:32
22
votes

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)

4
  • 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. Nov 29, 2010 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" Nov 29, 2010 at 21:40
  • A specification often will (and mostly should) leave most of the implemnentation, and some of the interface open. everything not in the spec directly, but required in order to implement the spec, whether an implementation decision, user viewable interface, or anything else, is also an indirect requirement of the spec. If a feature is not in the spec, and not implied by the spec, then you don't need it. How can this ever be confusing? Jan 28, 2011 at 14:01
  • 1
    @TokenMacGuy the crucial aspect is the implied by the spec part. That's where opinions differ a lot. Jan 28, 2011 at 14:04
21
votes

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.

12
  • 2
    Probably about as easy as typing Foo in TM's function list.
    – Josh K
    Oct 26, 2010 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, 2010 at 17:59
  • 7
    Starting with 'Get' is another example.
    – JeffO
    Oct 26, 2010 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, 2010 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. Nov 18, 2010 at 21:44
20
votes

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

9
  • 3
    #2 is a best practice?
    – Hogan
    Oct 27, 2010 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, 2010 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, 2010 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, 2010 at 22:53
20
votes

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

2
  • 13
    +1 I still don't see how Design Patterns are beautiful or elegant solutions. They are workarounds to language deficiencies, nothing more. Nov 30, 2010 at 0:02
  • 2
    Just see it as an endeavor of eradicating language smell using the language itself. Jan 18, 2011 at 2:10
14
votes

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.

8
  • 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, 2010 at 15:30
  • Yeah, this is definitely an issue in Clean Code only. In real life methods tend to be too long, as you said. But it's interesting to see that curve! Indeed, things should be made as simple as possible, but no simpler. Oct 27, 2010 at 16:01
  • 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. Oct 27, 2010 at 22:13
  • @Robert Harvey: The second example is a fine public method, but calling it from within the class itself is questionable, because you don't know exactly what it does before you look how it's implemented. It could check for null, for example. See my addition above. Oct 28, 2010 at 5:58
  • @Joonas: Fair enough. Oct 28, 2010 at 21:08
14
votes

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.

4
  • 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. Jan 22, 2011 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. Feb 6, 2011 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." Feb 6, 2011 at 9:53
  • "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!". This is exactly what is done in C++, and it is wonderful. The algorithms are separated from the data structures, so if I write my own container, all of the algorithms that work with the standard library just work with my new container. It must be horrible in .Net land, writing a new sorting function for every new container that you want to sort.
    – Mankarse
    Oct 26, 2011 at 12:14
13
votes

"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.

4
  • 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, 2010 at 20:11
  • 6
    I have to agree with the way that sova put this one. Clean code is preferable to comments.
    – riwalk
    Nov 29, 2010 at 23:47
  • 4
    You still need the "why"'s in there!
    – user1249
    Jan 21, 2011 at 0:04
  • I'd rather have comments explaining why. It means I have to THINK less in reverse engineering the code intent when I come to look at it. Jan 21, 2011 at 1:17
12
votes

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.

3
  • I'm guessing you've never heard of TypeMock? It allows mocking classes, privates, statics (factory), anything. Jan 20, 2011 at 23:16
  • @Allon: I have, but it is far from free, making it not an option for most people. Jan 20, 2011 at 23:30
  • If you have to write lots of factory classes then you're doing something wrong. Smart DI libraries (e.g. Autofac for C#) can utilize Func<T>, Lazy<T>, Delegates etc. for factories, without writing any boilerplate yourself.
    – gix
    Feb 22, 2011 at 15:01
11
votes

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.

15
  • 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.
    – riwalk
    Nov 22, 2010 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, 2010 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, 2010 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.
    – riwalk
    Nov 29, 2010 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 Jan 19, 2011 at 2:37
10
votes

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 :)

2
  • +1, sounds like my last place of employement to the most minute detail! I respect tiered applications on principle however too many people treat it like a silver bullet when it doesn't make sense. Most business software has a staggerringly low amount of business logic and what it does have is relatively simple. This can make layering business logic a nightmare of boilerplate code. Many times the lines can get blurred between data access and business logic because the query IS the business logic.
    – maple_shaft
    Jun 23, 2011 at 12:42
  • ... further, most shops absolutely fail at recognizing UI logic or Presentation logic. Because they don't understand just how little business logic exists in a typical CRUD application, they feel like they must be doing something wrong when the majority of their logic resides in the presentation layer as presentation logic. It gets falsely identified as business logic and then people push it to the server for yet another server call. A Thin Client can and should have Presentation logic, Eg. (Hide textField2 if option1 is selected in dropDown3).
    – maple_shaft
    Jun 23, 2011 at 12:46
9
votes

JavaBeans

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

7
votes

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.

7
votes

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.

17
  • 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. Jan 18, 2011 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. Jan 18, 2011 at 2:17
  • 2
    how would you print it?
    – user1249
    Jan 21, 2011 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. Jan 21, 2011 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.) Feb 6, 2011 at 9:47
5
votes

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.

5
votes

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.

7
  • 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, 2010 at 4:42
  • @xnine I don't have enough rep on this site yet to rate your comment, so I'll just reply with this comment of approval.
    – Maxpm
    Nov 30, 2010 at 4:44
  • 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. Jan 12, 2011 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. Jan 12, 2011 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. Jan 18, 2011 at 14:39
5
votes

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.

16
  • 2
    That's a whole bunch of negatives that has left me rather confused as to your actual stance on singletons. Nov 30, 2010 at 11:17
  • 1
    @Paul Butcher: I hate singletons and it should never be used
    – user2528
    Dec 1, 2010 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.
    – user2528
    Dec 1, 2010 at 22:23
  • 6
    Who says that using Singeltons is best practice? Dec 10, 2010 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.
    – user131
    Dec 10, 2010 at 8:06
5
votes

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?

7
  • Okay, now I'm curious- why do you say that? I'm feeling a little dumb - can you provide some code examples to back up your statements? Dec 10, 2010 at 3:22
  • 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, 2010 at 5:59
  • @AareP: To be sure - I presume you're referencing the fact that when an unsigned int 0 is decremented by 1, you actually end up with the maximum positive value that an unsigned int may store? Dec 10, 2010 at 10:11
  • 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, 2010 at 11:19
  • Not a good idea on small embedded machines - frequently unsigned ints will generate smaller, faster code. Depends on the compiler and processor. Jan 21, 2011 at 1:19
4
votes

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.

7
  • This. I once refactored some long function into several, only to realize that almost all of them needed almost all of the parameters of the original code. The parameter handling was such a pain that it was easier to just go back to the old code.
    – l0b0
    Jan 17, 2011 at 14:59
  • 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
    Jan 17, 2011 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? Jan 18, 2011 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, 2011 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. Jan 21, 2011 at 1:22

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