Two weeks ago I started to work in a new project where some code was already written. However, there wasn't any best practices to follow. Something that matter when you start a new project is to get together to define the foundations and best practices/guidelines the team will follow to do the best code: Maintainable, readable, easy to understand.
I'm going to describe 5 scenarios that I saw in the project and how those can be improved.
Keyword for this is: Consistency
1. Order of import modules
Arranging your ES6 modules in an organized way will save you some time while trying to find any missing/not needed modules.
Before
import { DatePicker } from '../../components'
import axios from 'axios'
import { IUser } from '../../models/User'
import React from 'react'
import { toCamelCase } from '../utils'
import { Button } from '@material-ui/core'
After
// node_modules
import React from 'react'
import { Button } from '@material-ui/core'
import axios from 'axios'
// Local modules
import { DatePicker } from '../../components'
import { toCamelCase } from '../utils'
// Types + Interfaces
import { IUser } from '../../models/User'
In the Before we can see that the packages are unordered, probably for one file wouldn't make too much noise, but when you open a ton of files and try to look for a specific package is really hard to do it. Something that the team has agreed on is to group the imports in the After way, separating each module by an empty line. The comments can be removed since the files will be consistent.
2. Using destructuring whenever is possible
Another important thing is to prevent unnecessary nesting and repetition. In most of the cases this will improve readability a lot.
Before
const UserProfile = props => (<div>
<span>{props.firstName}</span>
<span>{props.lastName}</span>
<img src={props.profilePhoto}/>
</div>)
After
const UserProfile = ({ firstName, lastName, profilePhoto }) =>
(<div>
<span>{firstName}</span>
<span>{lastName}</span>
<img src={profilePhoto}/>
</div>)
3. Naming convention for variables and methods
Something important about code is to know what a method will return or also easily read what a variable represents just by its name, for example:
Before
let User = {}
User.car = true
User.admin = true
function NewUser() {
return User
}
function add_photo(photo) {
user.photo = photo
}
After
let user = {}
user.hasCar = true
user.isAdmin = true
function getUser() {
return user
}
function setUserPhoto(photoUrl) {
user.photoUrl = photoUrl
}
In After we are keeping consistency on how to name variables and methods, being consistent in:
- For booleans use: is, has, should prefixes
- For methods use get/set prefix if are for props
- Overall use camelCase for methods and variables
4. Make your components prepared for common props
Before
const UserProfile = props => {
const { firstName, lastName, profilePhoto } = props
return (<div>
<span>{firstName}</span>
<span>{lastName}</span>
<img src={profilePhoto}/>
</div>)
}
After
const UserProfile = props => {
const { firstName, lastName, profilePhoto, ...rest} = props
return (<div {...rest}>
<span>{firstName}</span>
<span>{lastName}</span>
<img src={profilePhoto}/>
</div>)
}
In the After, the component is prepared to inject common React properties such as: style, className, key, etc. Using the spread operator you are grouping all common props and passing them to the container.
5. Really dumb components will make your life easier
Creating dumb components and following the Single Responsibility Principle allows you to create and contribute in an easily way and keep a clean codebase.
Before:
import axios from 'axios'
const UserProfile = props => {
const [user, setUser] = React.useState(null);
React.useEffect(() => {
getUser();
}, []);
async function getUser() {
try {
const user = await axios.get('/user/25')
} catch(error) {
console.error(error)
}
if(user.country === "DE") {
user.flag = "/de-flag.png"
} else if(user.country === "MX") {
user.flag = "/mx-flag.png"
}
setUser(user);
}
const { firstName, lastName, profilePhoto, userFlag} = user
return (<div>
<span>{firstName}</span>
<span>{lastName}</span>
<img src={profilePhoto}/>
<img src={userFlag}>
</div>)
}
After:
What can cause issues?
Adding Business Logic (BL) inside a component can make it hard to maintain, debug and test. My recommendation is to keep your component as presentational component. In this way you isolate the BL and you can focus on testing that part independent. Previously everything was mixed. Now we have separated each responsibility, which makes it easy to test and debug.
// UserProfilePage.jsx
// Does everything related to the UserProfilePage, adding any additional props or BL
import { fetchUser } from '../api'
const UserProfilePage = props => {
const [user, setUser] = React.useState(null);
React.useEffect(() => {
getUser();
}, []);
async function getUser() {
const user = fetchUser(error => console.error(error))
if(user.country === "DE") {
user.flag = "/de-flag.png"
} else if(user.country === "MX") {
user.flag = "/mx-flag.png"
}
setUser(user);
}
return <UserProfile {...user}/>
}
// API.js
// Fetches the data and handles errors on that. That's it
export const fetchUser = async (errorHandler) => {
try {
const user = await axios.get('/user/25')
} catch(error) {
errorHandler(error)
}
}
// UserProfile.jsx
// Displays the UserProfile and that's it
const UserProfile = props => {
const { firstName, lastName, profilePhoto, ...rest} = props
return (<div {...rest}>
<span>{firstName}</span>
<span>{lastName}</span>
<img src={profilePhoto}/>
</div>)
}
Bonus: If you are using a type-checker, make it count.
In case your team chooses to use a type-checker it's really important that you become strict and use it to ensure it's covering and serving for the purpose it was decided to use it.
Before:
const UserProfile = (props: any) => {
const { firstName, lastName, profilePhoto, shouldShowPhoto } = props
return (<div>
<span>{firstName}</span>
<span>{lastName}</span>
<img src={profilePhoto}/>
</div>)
}
After:
interface IUserProfile {
firstName: string
lastName: string
profilePhoto: string
shouldShowPhoto?: boolean
}
const UserProfile = (props: IUserProfile) => {
const { firstName, lastName, profilePhoto, shouldShowPhoto } = props
return (<div>
<span>{firstName}</span>
<span>{lastName}</span>
{shouldShowPhoto && <img src={profilePhoto}/>}
</div>)
}
I'm not saying these rules apply for all the projects but your team should be able to define them and agree on that.
Which best practices/guidelines do you use?
Top comments (14)
For the first Tip
manually order the imports is tedious
I'm using
import-sortwithhuskypre-commit hookThat makes my life much easier, I never need to care about the ordering of it
Just use auto import and commit + push my code, really simple
github.com/renke/import-sort
Thanks for sharing!
This is so cool Mark. What a nice way to do it; automating it!
Thanks for sharing!
These are great. Another simple tip is to always format your code (and ideally, to one of the widely accept standards, e.g. 2 spaces for indents). Nothing annoys me more than seeing unformatted code - it only takes 2-3 keypresses to fix!
This is a really good tip! Totally agree with you.
In #5, you're manually setting user.flag before passing it to setUser. Does that cause any issues with immutablity? Normally I would do something like setUser({...user, flag}), but if I could save myself that step I would like to.
Hello, user.flag is not a state
User is. So isn't user.flag? It's on the user object created with usestate
Hello，In #5 API.js
export const fetchUser = async (errorHandler) => {
try {
const user = await axios.get('/user/25')
} catch(error) {
errorHandler(error)
}
}
Is there need to return ‘user ’？
Thaks a ton
A better refactor of #5 would be to extract the logic to a custom hook.
That way it won't be coupled to the UserProfile component
Hi Idan, in this case the UserProfilePage component has the logic decoupled from the User component. The User component is a presentational component while your UserProfilePage holds the Business Logic, this follows Atomic Design. bradfrost.com/blog/post/atomic-web...
Please let me know if you think otherwise. :)
The logic is decoupled, however not reusable.
So if you need the same logic in, say, Comments component, you'd need to write a CommentsPage.
You can avoid that by changing UserProfilePage to useUserProfile and have it return the user instead of JSX.
Small change, with large benefits
Thanks for the 4 point, very valuable!