What popular "best practices" are not always best, and why? [closed]

Question

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

Answer 1

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.

Answer 2

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.

Answer 3

One return per function/method.

Answer 4

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.

Answer 5

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

Answer 6

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.

Answer 7

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.

Answer 8

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.

Answer 9

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

Answer 10

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

Answer 11

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.

Answer 12

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

Answer 13

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)

Answer 14

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.

Answer 15

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

Answer 16

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

Answer 17

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.

Answer 18

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.

Answer 19

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

Answer 20

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.

Answer 21

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.

Answer 22

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

Answer 23

JavaBeans

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

Answer 24

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.

Answer 25

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.

Answer 26

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.

Answer 27

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.

Answer 28

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.

Answer 29

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?

Answer 30

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.