Code Refactor Tips

Just like food and beverages or the any other things which have an expiry date, code also expires and rottens as features keep stacking onto the existing project, which results in a large amount of redundant, ad-hoc, buggy work in the workspace and eventually increases the cost of maintenance for both new feature development and bug fixes. Some of the reasons why code always rots can be:

The current architecture/protocols of the project/specific modules in this project can no longer server the new incoming requirements.
Some of the improper design & codings are mistakenly overlooked during the code review process and they are further reused in the future turns and impacts the whole workspace.
The use cases for some of the code introduced by the original developer are forgotten and as time passed by the code itself became meaningless.

A strict code review process can effectively control the speed of tarnishing of the code quality, but can not simply prevent it from happening. A persistent and periodical overall review on architecture level and corresponding code refactor is the only silver bullet for maintaining a robust, efficient, readable code base.

1. Basic Theories

Some of the basic theories can refer to, from the overall software component design principle to the layers of design patterns.

SOLID principle: a principle describes how software components should be designed in the overall direction. There can be varies design patterns under different layers to serve the goal.
Design patterns: there are layers of design patterns, from those architectural design patterns like: CLEAN (for iOS later it is derived to become VIPER), traditional MVC, MVP and MVVM to those design patterns which focuses on resolving verify specific problems like structural design patterns, behavioral design patterns and creational ones, which are well described by the Gang of Four.

Besides of the above design patterns and design principle, for concrete items and use cases, you also to follow the language-wise best practises and API recommendations. (These varies case by case).

2. Concrete Objectives During Refactors

The following items is the concrete refactoring targets most commonly propsed and likely to be accepted by the code reviewer and the rest of the team.

2.1 Potential Risks

If there is a potential possibility of the current implementation can cause: crash/compatibility issues, it always makes sense to resolve them ahead of production or the issue get exposed.

Example:

if the message optional is nil, the following line crashes.

if message != nil && !message!.isEmpty {
    contentStackView.addArrangedSubview(messageLabel)
}

2.2 Improper Namings & Folder Structures

Improper namings in classes and folders shows the either the poor language skills of the developer or his lack of understanding in design patterns because with the correct knowledge of design patterns, it always will hint you what kind of names you should use for a class. The benefit of good namings and folder structures include:

Provide you and your following developer a easier way to understand the design patterns of the code under relevant modules.
Indicate the guranularity of the object and the position of the object/model under the relevant design patterns.

A good class naming & folder structure is always a essnetial foundation for further architectural refactor or design pattern level refactor in later stage. (To refactor further you need to know first what is the standard and expected ways how the current design should work, otherwise any further bargain or suggestion towards a new design patterns will lack of support on prove for what is the dis

Example:

Here I will give a example on improper class namings and folder structures.

Imaging you have a UserService class under a directory where the rest of the class is used to send backend service request to the server. However the UserService class itself is a user state cache manager, and as the feature aggregates, it also caches wallet information such as balances and transfer limits. In this directory you have another service facade named UserInfoService. It always cost a developer seconds to understand what is the difference between the two classes… The solution is to rename the UserService to something can elaborate the use case for the class and move the class to some other place of the project workspace.
Imaging you have a class named TransactionModel, your fellow developer will be very confused whether it is a model for a transaction list or a single transaction record? so please name it to TransactionListModel/TransactionRecordModel.

2.3 Lack Maintainability

Sometimes code can simply be managed in a more effective manner to reduce total number of lines and also the management cost at the same time.

Example:

the following items are actually synchronized together under the same service object UserInfo. Imaging there are large amount of similar fields in a UserCache, maintaining dozens of such state is a nightmare.

let mobileNo: BehaviorRelay<String> = BehaviorRelay<String>(value: "")
let nickname: BehaviorRelay<String> = BehaviorRelay<String>(value: "")
let avatarUrl: BehaviorRelay<String> = BehaviorRelay<String>(value: "")
let email: BehaviorRelay<String> = BehaviorRelay<String>(value: "")
let emailVerified: BehaviorRelay<Bool> = BehaviorRelay<Bool>(value: false)
let phoneVerified: BehaviorRelay<Bool> = BehaviorRelay<Bool>(value: false)
let biometricEnable: BehaviorRelay<Bool> = BehaviorRelay<Bool>(value: false)

which can simply summarized into a simple codable (which supports data serialization) object of UserInfo:

let userInfo: BehaviorRelay<UserInfoRpcResult?> = BehaviorRelay<UserInfoRpcResult?>(value: nil)

2.4 Conflicts with Current Patterns/SOLID Principle

The SOLID principle specifies that:

A class or modules should have one, and only one, reason to change, and and derived classes must be substitutable for their base classes; (SRP, LSP)
A module should be open for extension, but closed for modification; (OCP)
Module/module relations should be interface based, not concrete implementations; Many client-specific interfaces are better than one general purpose interface (DIP & ISP)

For any implementations goes against the principle, they should be resolved properly under the current design conventions. Here let’s have another example:

Example:

Imaging now we have a profile module, which provides all profile relevant operations, users can enter the my profile sections from multiple entry points from the app. The profile module provides a unified entry points (profile panel) for all its sub-flows - change name, modify phone, verify emails and, etc. Some of the flows need to be also used as a atomic ones, for example, the verify email flow: users need to go to the verify email flow from anywhere of the app and after the email verification, the flow shall go back to the original view from which it starts.

Now there is a code block in the email verification class telling the user to show a hard coded popup after the atomic email verification flow has been done:

func showVerifyEmailSuccess() {
  let title = ${Data Item from another flow: like Login/Registration}.getTitle()
        
  self.responder?
	.showProfileFullScreenSuccess(headline: "Your email address \nhas been verified", 		
		actionTitle: "Back to \(title)", content: "Thank you for taking the time to do it.", 
		image: "iap_universalresult_successscreen", 
		onDone: { [weak self] in
  		guard let self = self else {
    		return
  		}
  		self.backToScreen()      
  })
}

The above code have two question:

It goes against OCP principles, in future if there is any more new use cases, the model on how to retrieve or what the popup message is going to be will highly likely to be changed.
It goes against DIP, the profile model here relies of status of Login/Register module. If in future you want to separate the profile and login/register into different artifacts, problems will occur since there is a coupling dependency.

2.5 Conflicts with Best Practice

Last but not least, if the API usages or code implementation goes against best practices, it is non debatable the fix should be accepted and supported by the rest of the team.

3. References

Previous Next

TOC