Something to note: A lot of this is situational, and depends on the platform you are working and the accepted practices for that platform/language.
- The goal when writing clean code is readability, not efficiency, not less lines.. only readability.
-
Meaningful/Searchable Names
int d; // elapsed time in days vs int elapsedTimeInDays; -
Minimal encoding of type in a variable name is helpful (a.k.a. Hungarian notation)
btnLogin or buttonLogin
-
Functions should be short.
-
Functions should do one thing.
-
Function arguments should be reduced by grouping (easier to read)
Circle makeCircle(double x, double y, double radius); Circle makeCircle(Point center, double radius); -
No Side effects!
Side effects are lies. Your function promises to do one thing, but it also does other hidden things. Your function should either do something or answer something. Having a function change the state of an object and return information is confusing. In the case of writing a side effect for a getter/setter. Avoid it if possible.. if not.. add a comment or change the name of the getter/setter so it stands out.. like setNameAndDoStuff(string name); as a bandaid until you can fix it. -
Avoid modifying data passed in as an argument
Thats what the return mechanism is for. Granted there are rare cases where it can't be helped due to the limiting nature of the language.
-
Classes that are 1000+ lines are difficult to read.
-
One trick to see if you're breaking SRP (single responsibility principle) is to look at the class variables, and in which functions they are used in.
If you have a group of variables only used in 3 out of 20 functions then that code has potiental to become its own class -
Use nouns to name the class. Avoid verbs. Avoid Managers, ect.
-
Singletons can be used when implemented against a hard dependency where only 1 instance of that dependency is possible or worth the effort implementing.
This also may be difficult to explain. Lets say logging for example, not entirely bad to use a singleton. However, you lose the ability to have customized logging instances, disabling specific logs (without macros), direct different logs to different outputs. or like a game engine for example, normally you have only 1 instance of the engine running per process. Having a singleton is ok in this instance. It would not be okay if you would want to run a game within a game (usually not worth the effort implementing. convenience vs cost) Another thing to note, Singletons is the _merging_ of data+functionality with other data+functionality. The key is: when your class depends on a singleton they are now no longer separate... (long pause here) Keeping data seperate is what allows us to isolate bits of code, so we can debug and reuse it easily. Somewhere down the line, the benefit of convenience out weighes the cost.
-
Avoid repeating yourself
-
Avoid repeating your plumbing/wiring.
This one is a bit harder to explain. Lets say you have 2 delegates that provide info for a table/picker that basically shows a list of strings. You have 2 classes that provided data to this table and picker. One for users and one for filters. Users is a structure with many members. Filters is actually just a array of strings. In this case you can convert Users to an array of strings then pass it to a single delegate class. -
Horizontal alignment is nice but not necessary.
public Socket socket; private InputStream input; private OutputStream output; private Request request; private Response response; protected FitNesseContext context; -
One line, one instruction. (is easier to read)
"No function should be longer than what can be printed on a single sheet of paper in a standard reference format with one line per statement and one line per declaration" - http://pixelscommander.com/wp-content/uploads/2014/12/P10.pdf
-
Comment as you see fit..
-
Then go back and refactor your code to remove your comments
// Check to see if the employee is eligible for full benefits if ((employee.flags & HOURLY_FLAG) && (employee.age > 65)) vs if (employee.isEligibleForFullBenefits()) vs bool isEligibleForFullBenefits = ((employee.flags & HOURLY_FLAG) && (employee.age > 65)); The goal here is to keep comments in the code minimal. Comments are generally not reliable for a few reasons. They lie, become outdated, more effort to maintain, clutter code, and if you have to write a comment its usually a code smell like the one shown in the example. -
Delete commented out code if it's already committed somewhere in the repo.
Everyone is always too afraid to delete commented out code because they don't know how important to the code is.. so eventually your code base gets cluttered if you leave these suckers alone for too long.
-
Don't repeat yourself
Class User { /// Sets the user's name void setName(string name); }
Rationale: Provides no value..
-
Comment as you see fit..
Also comment a description of *what* the class does not *how* it does it. How can easily change. Also, If you notice that your description of a class uses the word 'and' a lot. Then your class is probably breaking the single responsibility principle. We avoid commenting everything. For example, "as useful as javadocs are for public APIs, they are anathema to code that is not intended for public consumption. Generating javadoc pages for the classes and functions inside a system is not generally useful, and the extra formality of the javadoc comments amounts to little more than cruft and distraction." - Clean Code - Robert Martin
- Document everything, no matter how dumb. (example: The java-docs for the standard Java library)
-
Avoid over abstracting.
In some cases you can easily be repeating your plumbing/wiring. This can be difficult to judge. *If you have no experience in what you're working in* a good rule to follow is to avoid abstracting anything unless you're adhering to the 'Don't repeat yourself' rule. This way, when you notice you have or you are going to write repeat code. You can extract that code out into its own class. Same thing with plumbing/wiring. -
Concrete plain old data (horizontal complexity) vs Abstractions (vertical complexity)
I like the visualize this by imagining a file system. Imagine you have 100 files (High horizontal complexity) they are difficult to manage because there is no grouping of items, and all items are visible. Lets say we organize these files into folders, but .. a lot of folders.. some structures are 10 folders deep. You want to find a file.. but now you need to dig through many folders (High vertical complexity). Not only that.. things are confusing because some files makes sense to be in multiple different folders. The same applies to abstractions. The best ones offer a balance between horizontal and vertical complexity. Something to note: Horizontal complexity naturally has a lower cost compared to vertical comlexity. -
Abstractions can reduce the complexity of your code but they come at a cost
In many cases, hiding complexity allows us to easily digest code. Many people can drive a car without knowing how the internals work. However, in the same illustration we see an obvious disadvantage. If you don't know how a car works then how are you going to fix it when it breaks? Luckly, we can find a local expert (mechanic) to fix our car. Here we see another advantage. Grouping similar concepts of ideas together allows to have a mental 'context'. In most cases, when we go bug fixing we only need to worry about the car and how the car is used. One might say that this pattern is super useful.. then decide to use it for everything! An abstraction provides a layer of indirection, but abstractions can leak. TCP is an abstraction over IP. TCP is 'gaurenteed' to sned and receive packets... but its not. Lets say you unplug your computer from the router. TCP will fail. The more abstractions there are the more layers there are.. the more layers the more digging you need to do. -
Prefer non-member functions to avoid clutter within a class.
http://www.drdobbs.com/cpp/how-non-member-functions-improve-encapsu/184401197 Lets say you have a string class. The amount of methods you can write for it are nearly infinite. All you need to write is the bare minimal public methods to utilize the class. Then if you wish to add additional functionality you can create a new file with functions that take a string as a parameter. string UrlEncode(string source); string TrimWhiteSpace(string source); Some languages makes this much cleaner to do: obj-c categories or c# extensions Lastly, with a bare minimal interface your string class is less prone to changes. -
Breaking SRP (not considered bad or good is some cases)
Lets say our app uses only 1 api call in the entire app. Making a whole api class would not provided any value. Granted separation does provide value. However, it comes at a cost of more time and an extra class cluttering the project. So its not good and its not bad. -
Prefer explicit over implicit
Even implicit casting can be dangerous.. depending on the situation you can loose much needed precision casting a float to an int. Even more dangerous is using an implicit cast to truncate precision which can easily be looked over. Lets look at the definition of implicit: "implied though not plainly expressed." implied requires who ever is reading your code to know what is supposed to happen ahead of time. They also make the code less expressive which reduces readability, and the intent of the developer is lost. Was this implicit behavior intentional? -
Prefer composition over polymorphism.
This way we treat reusable components more like reusable components. -
Polymorphism (virtual methods) should be avoided.
Unless it is providing a simple solution. Using other methods such as DI, delegates, callbacks may introduce more mechanism and less clarity than polymorphism in specific situations. However, those solutions are perferred over polymorphism. -
When using polymorphism always 'extend' behavior or provide no default behavior when replacing (like an abstract class; Open/closed principle).
This is what I mean by extend: //Parent virtual void viewDidLoad() { addImage(); addText(); ImplementationSpecificBehavior(); } virtual void ImplementationSpecificBehavior() { //Feel Free to replace this in any subclass } //Subclass override void viewDidLoad() { base.viewDidLoad() // <- extending addMoreImages(); base.viewDidLoad() // <- not extending } by extending behavior you keep the original behavior intact which keep the behavior consistent between all subclasses. And by using ImplementationSpecificBehavior() we minimize repeat code. -
In an OOP architecture, focus on telling an object what to do. Instead of utilizing an object to do something.
categoryLabel.backgroundColor = schoolSubject.colorForBackground; categoryLabel.textColor = schoolSubject.colorForText; vs schoolSubject.customizeLabel(categoryLabel); Lets say we drop 'colorForBackground' we would have to refactor every place that uses this code.