The Single Responsibility Principle (SRP) is one of the five SOLID principles. It tells us that a class should have only one responsibility. Obviously this rule can be extended also for methods.
In this post, I’ll walk you through an example, from a project that I worked on, where a method violated SRP. Then I will show you how applying this principle can make the code cleaner, more maintainable, and easier to test.
Method to check if all fields (in a form) are Valid
Below you can see the method allFieldsIsValid() -> Bool. A lot of things stink in this method, including its name 😬. Let’s read the code and then I will break down all the points that need improvement. But first, I will give you some context about this method.
So, basically, every time user taps the button save, it will trigger the method saveButtonDidTap, that calls the method allFieldsIsValid(). If all the fields are valid, it will persist the data. If the fields are not valid, it will throw an alert from the allFieldsIsValid() method.
private func allFieldsIsValid() -> Bool {
var messageTitle = ""
var messageText: String = ""
let requiredFieldsInvalid = self.viewModel.requiredFieldsInvalid()
if requiredFieldsInvalid.count > 0 {
messageTitle = SystemResourceDataManager.instance.getResource(Constants.SystemResourceKey.alertTitleRequiredFields)?.value ?? ""
messageText = Constants.SystemResourceKey.alertBodyRequiredFields.translateResource()
messageText += "\n\n"
messageText += requiredFieldsInvalid.joined(separator: "\n")
}
let valueFieldsInvalid = self.viewModel.valueFieldsInvalid()
if messageText == "" && valueFieldsInvalid.count > 0 {
messageTitle = Constants.SystemResourceKey.alertTitleInvalidFields.translateResource()
messageText = Constants.SystemResourceKey.alertBodyInvalidFields.translateResource()
messageText += "\n\n"
messageText += valueFieldsInvalid.joined(separator: "\n")
}
if messageText != "" {
AlertHelper().errorAlertWithCustom(title: messageTitle, message: messageText, self)
return false
}
return true
}
Identifying the problems
Well, if your eyes didn’t bleed after reading this, let’s start working and make it become a good code. First of all, let’s talk about all the problems in this method:
- It is in the ViewController layer;
- The name of the method:
- Is grammatically incorrect;
- It not describe what it does very well;
- It has too many responsibilities. Let’s break it down:
- It checks if all required fields are filled and store the invalid fields in a variable;
- Then, if there are invalid fields, it sets a message body and title. For each invalid field, it appends the field name to the message body and breaks the line;
- After that, it will validate if all the filled data is valid using specific validators for each kind of data;
- Like in the step 2, it will set the message body and title to the variables;
- At the end, it checks if the message body is empty:
- If it is empty, it returns true;
- If not, it will throw an alert and return false
- There are some lines that are too long;
- There is a last problem in this code. If there is invalid filled data, it overrides the required invalid data 🥲.
Fixing the method
There are some simple ways for fixing this code. I could rename the method to areFieldsValid(), which better reflects its purpose. Next, I would unify the validation logic. It should check for all invalid data in one go. This includes missing required fields and fields that are filled with incorrect data. Since both are considered invalid, there’s no need to separate these checks. Finally, I would implement an early exit, returning false as soon as the first invalid field is detected. This way, we can avoid unnecessary checks. In the place where areFieldsValid() is called, I would handle the response. If the method returns false, I would delegate the task of building and displaying an alert to a separate class.
That sounds as a good solution, but actually it isn’t. At least not in this case. We must throw the alert listing all the invalid fields. Therefore, we need to run 2 loops. One for checking invalid fields, and the other to list them. So, let’s follow another approach.
First of all, let’s create some enums to help us handle the errors. We will implement two enums. The first, ErrorType, to identify what kind of invalid data it is. The other one, ValidationError, that will be thrown if some field is invalid.
enum ValidationError: Error {
case invalidFields(Dictionary<String, ErrorType>)
case emptyFields(Dictionary<String, ErrorType>)
static func getError(_ fields: Dictionary<String, ErrorType>, hasEmptyFields: Bool) -> Self {
return hasEmptyFields ? .emptyFields(fields) : .invalidFields(fields)
}
}
enum ErrorType {
case invalid
case emptyRequired
}
Now let’s create a new class called FormValidator. This class will only handle the validation in the sections and fields for the current form. This class will have a static method called checkForValidationErrors(). It will be responsible for looping through the fields in the sections. The method will return the invalid fields.
class FormValidator {
static func checkForValidationErrors(in sections: Array<FormSection>) -> ValidationError? {
var invalidFields: Dictionary<String, ErrorType> = [:]
var hasEmptyRequiredFields: Bool = false
for section in sections {
for field in section.fields {
if let error = field.checkForValidationError() {
invalidFields[field.name] = error
if !hasEmptyRequiredFields && error == .emptyRequired {
hasEmptyRequiredFields = true
}
}
}
}
guard invalidFields.isEmpty else {
return ValidationError.getError(invalidFields, hasEmptyFields: hasEmptyRequiredFields)
}
return nil
}
}
As you can see, it will run the loop, searching for invalid fields. If it encounters some invalid field, it will append it to the dictionary. It will then return an error or nothing in the end.
Until now we solved the bad name of the method. We also created a class and its method with only one responsibility. The problem of overriding the variables doesn’t happen in our new code. We can easily solve the problem of the Constants.SystemResourceKey using typealias. But we still need to fix one thing: The logic in the ViewController.
For that, let’s create a method in the ViewModel called handlePersistence:
class FormViewModel {
var model: Array<FormSection>
init(model: Array<FormSection>) {
self.model = model
}
func handlePersistence() throws {
if let error = FormValidator.checkForValidationErrors(in: model) {
throw error
}
// persist data...
}
}
You can see that this method can throw an error. I choose to do that because to show the alert we need the Controller, right? so I throw the error here, and handle it in the Controller. This way I don’t need to pass the controller as a parameter to the viewModel or in the method.
Another thing you will notice is that, after checking if all fields are valid, this method will handle the persistence. The logic is here! It is very important to not mix View with business logic. Now let’s check our ViewController.
class FormViewController: UIViewController {
var viewModel: FormViewModel!
override func viewDidLoad() {
super.viewDidLoad()
}
@IBAction func saveTapped() {
do {
try viewModel.handlePersistence()
} catch let error as ValidationError {
// Handle invalid fields error
} catch {
// Handle any other potential errors
}
}
}
As you can see, it is very clean. The saveTapped() method only calls the handlePersistence() method in the viewModel. If it threw some error, we will catch the error and pass it to an AlertHelper handle this. Simple as that.
It looks so much better, but there is still one improvement to do. If we keep the code the way it is, AlertHelper must run an extra loop. This loop will build the string with the invalid fields. To avoid that we can change ValidationError and the checkForValidationErrors() method:
static func checkForValidationErrors(in sections: Array<FormSection>) -> ValidationError? {
var invalidFields: String = String()
var hasEmptyRequiredFields: Bool = false
for section in sections {
for field in section.fields {
if let error = field.checkForValidationError() {
invalidFields += field.name + "\n"
if !hasEmptyRequiredFields && error == .emptyRequired {
hasEmptyRequiredFields = true
}
}
}
}
guard invalidFields.isEmpty else {
return ValidationError.getError(invalidFields, hasEmptyFields: hasEmptyRequiredFields)
}
return nil
}
Takeaways
I hope this post could demonstrate how we change a bad code into a good code. This is possible by simply following some good practices. I choose a real problem from a real product. This way, you guys can see how it is applicable in a real context.
To summarize, after this refactoring section, we:
- Improved the performance by removing unnecessary steps like checking invalid and empty separately;
- We choose names that perfectly describes what the methods and classes does;
- We removed logic from the ViewController layer;
- We applied SRP where we could;
- We make it easier to give maintenance to this code. Since the classes and methods are not multitask;
- It is testable, because all the steps from checking invalid data to throwing the alert are separated. We can write unit tests for each one of those steps to ensure everything works.
Is there room for more improvements in this code? Of course there is! But I am already satisfied with the code we have so far. And I also think that my mission has been accomplished.
And guys, remember: Don’t apply principles and patterns just because it is cool, or because someone said to. Know the result you want to achieve. Keep in mind the problem you want to solve. Then, search for the principles and patterns that will get you there. Otherwise all this good practices will become handcuffs for your creativity.
That’s it for today. If you have some code that you improved using SRP, feel free to share in the comments. I would love to see that.
Coding With Renan 
Leave a reply to renanmaganha Cancel reply