Think back to the last time you looked at an unfamiliar block of code. Did you immediately understand what it was doing? If not, you’re not alone – many software developers, including myself, find it challenging to grasp unfamiliar code quickly…
I would argue that the validate routines be their own classes; ie UserInputValidator, UserPasswordValidator, etc. They should conform to a common interface with a valid() method that throws when invalid. (I’m on mobile and typed enough already).
“Self-documenting” does not mean “write less code”. In fact, it means the opposite; it means be more verbose. The trick is to find that happy balance where you write just enough code to make it clear what’s going on (that does not mean you write long identifier names (e.g., getUserByEmail(email) vs. getUser(email) or better fetchUser(email)).
Be consistent:
get* and set* should be reserved for working on an instance of an object
is* or has* for Boolean returns
Methods/functions are verbs because they are actionable; e.g., fetchUser(), validate(), create()
Do not repeat identifiers: e.g., UserService.createUser()
Properties/variables are not verbs; they are state: e.g., valid vs isValid
Especially for JavaScript, everything is const unless you absolutely have to reassign its direct value; I.e., objects and arrays should be const unless you use the assignment operator after initialization
All class methods should be private until it’s needed to be public. It’s easier to make an API public, but near impossible to make it private without compromising backward compatibility.
Don’t be afraid to use if {} statements. Short-circuiting is cutesy and all, but it makes code more complex to read.
Delineate unrelated code with new lines. What I mean is that jamming all your code together into one block makes it difficult to follow (like run-on sentences or massive walls of text). Use new lines and/or {} to create small groups of related code. You’re not penalized for the white space because it gets compiled away anyway.
There is so much more, but this should be a good primer.
More or less the same but the user gets passed as a method parameter each time. Validators would be in my opinion a long function inside the service also with named variables like this because it’s just easy to read and there are no surprises. I’d probably refactor it at around 5 conditions or 30 lines of validation logic.
I recommend trying out using the constructor in services for tools such as a database and methods for data such as user. It will be very easy to use everywhere and for many users and whatever
const passwordIsValid = ...
if (!passwordIsValid){
return whatever
}
async function createUser(user) { validateUserInput(user) || throwError(err.userValidationFailed); isPasswordValid(user.password) || throwError(err.invalidPassword); !(await userService.getUserByEmail(user.email)) || throwError(err.userExists); user.password = await hashPassword(user.password); return userService.create(user); }
Or
async function createUser(user) { return await (new UserService(user)) .validate() .create(); } // elsewhere… const UserService = class { #user; constructor(user) { this.user = user; } async validate() { InputValidator.valid(this.user); PasswordValidator.valid(this.user.password); !(await UserUniqueValidator.valid(this.user.email); return this; } async create() { this.user.password = await hashPassword(this.user.password); return userService.create(this.user); } }
I would argue that the validate routines be their own classes; ie
UserInputValidator
,UserPasswordValidator
, etc. They should conform to a common interface with avalid()
method that throws when invalid. (I’m on mobile and typed enough already).“Self-documenting” does not mean “write less code”. In fact, it means the opposite; it means be more verbose. The trick is to find that happy balance where you write just enough code to make it clear what’s going on (that does not mean you write long identifier names (e.g.,
getUserByEmail(email)
vs.getUser(email)
or betterfetchUser(email)
).Be consistent:
get*
andset*
should be reserved for working on an instance of an objectis*
orhas*
for Boolean returnsfetchUser()
,validate()
,create()
UserService.createUser()
valid
vsisValid
const
unless you absolutely have to reassign its direct value; I.e., objects and arrays should beconst
unless you use the assignment operator after initializationif {}
statements. Short-circuiting is cutesy and all, but it makes code more complex to read.{}
to create small groups of related code. You’re not penalized for the white space because it gets compiled away anyway.There is so much more, but this should be a good primer.
I like the service but the constructor parameter is really bad and makes the methods less reusable
That’s fair. How would you go about implementing the service? I always love seeing other people’s perspectives. 😊
More or less the same but the user gets passed as a method parameter each time. Validators would be in my opinion a long function inside the service also with named variables like this because it’s just easy to read and there are no surprises. I’d probably refactor it at around 5 conditions or 30 lines of validation logic.
I recommend trying out using the constructor in services for tools such as a database and methods for data such as user. It will be very easy to use everywhere and for many users and whatever
const passwordIsValid = ... if (!passwordIsValid){ return whatever }
Oof found the Java developer. No thanks.