The Journey To Clean Code
We always want to write code that is more maintainable, more expendable, and more readable. why?
Because these parameters have a major impact on our program's ability to grow in scale, on how easy it is to find and fix bugs, and on how much our team can cooperate on the same code together.
In this -not so short- article, I will mention principles that I find important while having clean code in mind. Each of these topics can be expanded to a blog post of its own, so feel free to search and explore if you feel you would like more details.
Naming
Naming is one of the most important things when talk about clean code. Good naming conventions will significantly rise your code readability.
You should always use descriptive and meaningful names.
Use yes/no questions for booleans (for example isActive), and be consistent, if you started naming all your DB methods with 'find...' keep using this convention.
Comments
Personally, I don't like comments and I try to avoid them as much as I can.
If your naming game is good enough you save yourself the comments hustle most of the time. Although, sometimes the code is a bit too complex or contains a piece that will require your teammates (or your future self) to waste too much time on it. Like when you use Regex for example.
// Regex will match decimal digits
if (entry.getValue().matches("^(\\d*\\.)?\\d+$")){
// some code..
}this comment can also be avoided with a well named variable
String decimalDigitsRegex = "^(\\d*\\.)?\\d+$";
if (entry.getValue().matches(decimalDigitsRegex)){
// some code..
}I also find myself using comments with Todos. Often, I'm in a middle of a task and spot something I would like to change but cannot afford to context switch right now. A Todo reminder will let me come back for it later.
Formatting
Format. Your. Code.
Most IDEs have great built-in tools for auto-formatting your code, use them and you won't join spaces versus tabs debates ever again.
You'll get horizontal formatting for free from the auto-formatter but in addition, you should also make use of vertical formatting.
for example, if you have a method that instantiates variables and then does some operations, I like to separate these parts with a blank line.
public List<Interaction> getUserInteractions(User)
/*
instanciate local variable block
*/
/*
business logic code
*/
return statement
}Functions
You should try and minimize the number of parameters your functions receive.
The "rule" says that is bad to pass more than 3 arguments to a function, but in my experience its not always possible to co-op with it.
But if you see too many arguments passed to one function maybe you can wrap those arguments in an object or a data container.
For example:
// BAD
public String getUserInteractions(Long userId,
String userName, String password, Long connectionid,
String userToken) {
return ripository.
findAllInteractions(userId, userName...)
}
// BETTER
public List<Interaction> getUserInteractions(User user) {
return ripository.
findAllInteractions(user.getId(), user.getUserName()...)
}Functions should also be small and do 1 thing (its not always possible but we should always try). For example, db.findAllInteractions() should not save data to the db or interact with other services.
Avoid mixing levels of abstractions inside functions.
what do i mean by that?
Consider this piece of code:
// MIXED LEVELS OF ABSTRACTION
// getting all users from remote api.
// the response is paginated, each page contains 5 users.
public List<Users> getAllUsers(URI uri, String token ){
List<User> users = new ArrayList<>();
UserResponse userResponse = getForObject(uri, token, UserResponse.class);
int count = userResponse.getCount();
int pageSize = userResponse.getPageSize();
userResponse.getItems().forEach(user ->
users.add(
new User(user.getName(), user.getEmail()));
count = count - pageSize;
while (count > 0){
userResponse = userResponse.getNextResponse();
userResponse.getItems().forEach(user ->
users.add(
new User(user.getName(), user.getEmail()));
count = count - pageSize;
}
return users;
}Notice we are calling high-level methods and doing some low-level calculations simultaneously. This thing is called a mixed level of abstraction. We should try and keep an even level of abstraction in each function. Lower-level abstractions should be extracted to suitable methods or become class functions.
For example, let's improve the code above:
// EVEN LEVELS OF ABSTRACTION
public List<Users> getAllUsers(URI uri, String token ){
List<User> users = new ArrayList<>();
UserResponse userResponse = getForObject(uri, token, UserResponse.class);
int count = userResponse.getCount();
int pageSize = userResponse.getPageSize();
while (userResponse.hasNext()){
userResponse.getItems().forEach(user ->
users.add(
new User(user.getName(), user.getEmail()));
userResponse = userResponse.getNextResponse();
}
return users;
}
public class User {
private final int count;
private final int page;
private final int pageSize;
private final List<User> items;
private boolean hasNextPage;
/...
.../
public boolean hasNextPage(){
return this.hasNextPage;
}
}This way, code is not only shorter. It is significantly readable.
Avoid copy-paste code from one place to the other. If you find yourself copying and pasting, that means you should extract code to a method or a function, or make some abstraction out of it.
Stay DRY! (don't repeat yourself).
Fail Fast
Consider this example bellow
public PlatformUserInfo getUserInfo(String token) {
if (token.isEmpty()){
return null;
}
// do stuff ... This "guard" check token.isEmpty() saves us from wasting time inside the function if we already know that its input is invalid.
We can even improve the above code by throwing the actual error that happened here. Propper error handling will allow us to find and address errors faster.
public PlatformUserInfo getUserInfo(String token) {
if (token.isEmpty()){
throw new TokenIsMissingException();
}
// do stuff ... Don't be afraid to throw errors when you know what state your variables should be in.
"Magic numbers" and Strings
Not much to explain here.
Numbers and Strings belong in constants, config or properties files.
// constansts
private String BASE_URL = "www.prkrdevblog.com";
private int DEFAULT_TIMEOUT = 5000;
Objects and Classes
- Classes should be small.
At the leading edge of OOP are inheritance and polymorphism. That means you can abstract methods and fields among classes, so you can extract a big class to multiple subclasses if one gets too big and hard to maintain. - Classes should focus on a single responsibility.
Consider this example below.
// NON SINGLE RESPONSIBILITY CLASS
public class OnlineTicketShop {
public Ticket searchAvilableTicket(Concert concert, Date date);
public int getTicketPrice(Ticket ticket);
public User loginUser(Credentials credentials);
public boolean chargeUser(User user, int amount);
public List<Concert> getAllConcertsForDateRange(Date start, Date end);
/...
} As you can see the Class OnlineTicketShop deals with tickets, users, and concerts. This violates the single responsibility principle. Instead, you should abstract classes like the one above to multiple subclasses each responsible for its own functionality.
//SINGLE RESPONSIBILITY CLASSES
public class User {
public User login(Credentials credentials);
public User logout();
public boolean charge(int amount);
/...
}
public class Ticket {
public int getPrice();
public boolean isAvailable();
/...
}
public class Concert {
public Ticket getAvailableTicket(Date date);
public boolean isAvailable();
/...
}
public class ConcertService {
public List<Concert> getAllConcertsForDateRange (Date start, Date end);
/...
}
public class OnlineTicketShop {
public List<Concert> getAllConcertsForDateRange (Date start, Date end){
return ConcertService
.getAllConcertsForDateRange(Date start, Date end);
/...
} Cohesion
Cohesion means how much of your class properties are being used by its methods.
Maximum cohesion ⇢ all properties are being used by all methods.
No cohesion ⇢ non of the methods uses any class property.
Achieving maximum cohesion is not a realistic demand, instead, you should tend to write highly cohesive classes where most of your methods use most of your class fields.
"Talk to friends, not to strangers" (Law of Demeter)
public boolean prepareMeal(Cook cook, List<Ingredient> ingredients){
ingredients.forEach(ingredient-> {
if (ingredient.label.details.expirationDate > Instant.now()){
/...
}
})
}Take a look at line 3 above.
We should avoid accessing other object's fields. These fields can be undefined and our method doesn't know anything about them.
Instead, Ingredient class should have getExperationDate() method, that the class should handle and validate before returning the answer.
SOLID
This one should get a post of its own, but in a nutshell, the S and The O affect code cleanliness the most.
Single Responsibility principle:
Classes should have a single responsibility.
It was already mentioned in the Classes section but it is worth mentioning
again.
Open-Closed principle:
"A class should be open for extension but closed for modification."
I remember hearing this for the first time in university.
what??
but actually, once you get it, it's pretty straightforward.
take a look at this example:
Let's say we have some cars in our program
// NON COMPLIENT EXAMPLE
public class Picanto {
private List<Wheel> wheels;
private List<Doors> doors;
private Engine engine;
// constructor...
}
public class BmwM3Sport {
private List<Wheel> wheels;
private List<Doors> doors;
private sportEngine engine;
// constructor...
}
public class Race {
public void race(Car car) {
if (car == null) {
throw new NonPresentCarException("you must show up with a car to the race!");
} // fail fast principle.
if (car instanceof Picanto) {
car.startEngine();
car.spinWheels();
} else if (car instanceof BmwM3Sport) {
car.startEngine();
car.setSportMode();
car.EngineTurbuInject();
car.spinWheels();
}
}
}
This code is fine and will eventually work but it violates the Open-Closed Principle. As you can see, if we would want to add more cars we would have to modify the Race class.
Instead, we can define a Race Interface in which each car class can participate and how it operates will be its own responsibility.
// COMPLIENT EXAMPLE
public interface Car {
void drive();
}
public class Picanto implements Car{
private List<Wheel> wheels;
private List<Doors> doors;
private Engine engine;
// constructor...
@Override
public void drive() {
this.engine.start();
wheels.forEach(Wheel::Spin);
}
}
public class BmwM3Sport implements Car{
private List<Wheel> wheels;
private List<Doors> doors;
private sportEngine engine;
// constructor...
@Override
public void drive() {
this.engine.start();
this.engine.setSportMode();
this.engine.turboInject();
wheels.forEach(Wheel::Spin);
}
}
public class Race {
public void race(Car car) {
if (car == null) {
throw new NonPresentCarException("you must show up with a car to the race!");
} // fail fast principle.
car.drive();
}
}
Now the race class is Open for extension but Closed for modification.
Conclusion
Wow, if you made it here you are a very brave individual.
Keep your code clean!