Hi all, I recently did a simple test where I should export an API but I didn’t pass because the recruiter said I should break classes better and that I wasn’t following SOLID principles. I don’t get it, could you please help me to check if there’s something wrong with the design of my classes?
const val PASSWORD_LENGTH = 9
class ValidacaoService {
val logger = LoggerFactory.getLogger(ValidacaoService::class.java)
val passwordPattern: String =
"^(?=.*[0-9])(?!(.*(.)\\1+))(?=.*[a-z])(?=.*[A-Z])(?=.*[@!#\$%^&()*+=])(?=\\S+\$).{4,}\$"
fun isValid(password: String?): RetornoValidacao {
val pattern: Pattern = Pattern.compile(passwordPattern);
var isValid = false
if (password != null && password!!.length >= PASSWORD_LENGTH) {
val matcher: Matcher = pattern.matcher(password);
isValid = matcher.matches()
}
if(isValid) {
val isDup = thereIsDuplicateChars(password!!)
if(isDup) {
isValid = false
}
}
logger.info("isValid: ${password} - ${isValid}")
return RetornoValidacao(isValid)
}
private fun thereIsDuplicateChars(password: String): (Boolean) {
var isDuplicate = false
val password = password.toLowerCase()
val map = HashMap<Char, Int>()
for (i in password.toCharArray()) {
if (map.keys.contains(i)) {
isDuplicate = true
break
} else {
map[i] = 1
}
}
return isDuplicate
}
}
class ValidacaoController(val service: ValidacaoService) {
val logger = LoggerFactory.getLogger(ValidacaoController::class.java)
fun validatePassword(ctx: Context): RetornoValidacao {
val password = ctx.header("password")
logger.info("password: ${password}")
return service.isValid(password)
}
}
If one works in idiomatic kotlin and forget about C++ design principles, one does not need classes at all for that.
// specialized free value-class to separate password from other strings
@JvmInline
value class Password(val passwordString: String)
// encapsulated value, not visible outside the file
private val passwordPattern = "..."
fun Password.isValid(): Boolean {...} // the logic needs to be reworked as well
Now if you need a service with more than one possible implementation, you need to abstract it away you can use function-types. like this:
I wouldn’t be quite so quick to dismiss SOLID principles. While they do hark back to C++ days, they are relevant beyond C++ and even beyond OO code.
@darksnake’s refactoring suggestion is excellent and (somewhat ironically) is much more aligned with SOLID principles that the OP’s submission.
Breaking out a PasswordValidator is an example of the open closed principle, in that the code is open to extension without requiring the code to be modified.
It also puts the Liskov substitution principle to work in that Password.validateWith(…) will allow any validator matching the signature to be substituted.
Dependency Inversion is there, too. Password.validateWith(…) depends on an interface, which validation implementation code depends on.
The use of extension functions, rather than member functions, for Password.validateWith() and/or Password.isValid() allows client code to depend on (import) Password, without being forced to depend on those methods if it doesn’t use them.
As for single responsibility, while I probably wouldn’t change much about @darksnake’s code in a real project, (it is already very simple,) if this is for a test, you might want to consider breaking PasswordValidator down further into a collection of PasswordRule, allowing each rule to be responsible for just a single validation, like length, or duplicate chars, etc.
I would also suggest that the concepts of service, controller and context in the OP’s code are not useful in the domain of password validation. They may be useful in the context of implementing some kind of service that allows the password validation code to be invoked, but fitting into some kind of framework is a separate concern to the actual domain logic, so violates single responsibility.
Answering your question specifically I would say your code isn’t really bad regarding SOLID principles. If I had to guess what did recruiter mean, then:
No interface for ValidacaoService - this is actually quite important, because it means ValidacaoController can only work with this specific validator service and you can’t replace it with another one. In practice, I often ignore this problem and extract an interface only when it is needed, but it depends on the specific case.
thereIsDuplicateChars() isn’t really related to the logic of ValidacaoService - it could/should be extracted as a generic utility for handling strings.
isValid() could be split into separate functions or a list of validation rules that check the length, regex and duplicate chars. For this simple case I think it would be an overengineering. If description of the task included information that there may be new validation requirements in the future, it would make sense.
ValidacaoController.service should be private. This is more related to the encapsulation than SOLID, but anyway this is a bad OOP design.
Additionally, I see quite a lot of “code smell” here:
(arguably) it is a bad design to create a mutable var and modify it several times in some branched code. Such code is very hard to read and understand. In most cases it is cleaner to use immutable variables (val). Kotlin makes it much easier with its “almost everything is an expression” principle.
Isn’t it just cleaner? Similarly in thereIsDuplicateChars() you create isDuplicate variable, but why not to just return true when found duplicate and return false after the loop?
You should use a set, not a map in thereIsDuplicateChars().
You should compile the regex pattern once, not each time isValid() is invoked.
There are also places that don’t seem very kotlin-ish or they look like you don’t have too much experience with it:
Kotlin has its own regex support with Regex (but there is nothing wrong in using Java variant).
You don’t need to use !! in password!!.length, because password was smart-casted to String. What is interesting here is that you still has to use it (I believe) in thereIsDuplicateChars(password!!), but note that in my refactored code I could remove it here as well. This is exactly what I mean: you overcomplicated the code flow and the compiler can’t understand that password can’t be null at this point.
You use HashMap directly instead of mutableMapOf().
You sometimes use parentheses where they are not needed (and probebly IDE suggest removing them): (Boolean), "password: ${password}", etc.
If we only look at the code you wrote, then the only serious problem is that you are compiling the pattern every time you check a password. That is a big red flag for me, and when I interview candidates, I always check to see if they have a mental model of how long their code takes to run.
To understand the design problem, however, we probably need to know more about the context. What is the purpose of this code in detail and what are all the requirements you were given?
You gave us a clue by mentioning that this is an API, and yes the interface you provided is inappropriate for a validity-checking service API, because the rules for what is and isn’t a valid password are arbitrary and subject to change. Your design should anticipate that these rules will change, and that at some point you will need different rules for different clients. Those are the sorts of problems that you would apply the SOLID principles to address.