Always avoid
Cryptic names.
This applies to variables, functions, classes, types, filenames, everything! This means no single letter variables, no abbreviations, nothing cute. It’s better to have names that are long but clear than names that are short and confusing.
Writing unit tests to prove nothing.
Unit tests are most effective when they’re asserting something that’s not obvious. Think of a unit test as a comment on steroids, comments explain something that doesn’t make sense, unit tests explain the code AND assert that it doesn’t get changed. Furthermore, if it turns out we can simplify or we change that code, the test MUST be changed whereas comments are content to just float around misleading people.
Most ternaries, especially nested ones.
This one is pretty straight forward, ternaries work best for simple assignments. const url = isAbsolute(path) ? "https://" + path : path. When you’re doing more than simple assignment, ternaries just make your code harder to read.
Magic strings, magic numbers, magic in general.
This is another of many rules with the same goal, simple to understand code. When we come across functionality that is driven by very specific strings/numbers, it’s hard to know the significance. Did the developer choose 42 because that’s the answer to all of our problems? Or is 43 okay too? Does getProduct(20) mean we’re going to get product by id 20? or maybe getting 20 products back in an array? Magic might feel nifty, time-saving, or cleaver as an author but I promise everyone else who has to work with it will be worse off.
Try to avoid
Abusing utilities.
Dumping logic into a “util” in the pursuit of “DRY” code. For a service to be classified as a “utility” it should be applicable to ANY project not just Polly. So if you’re about to add a new utility it should be something that would be useful to poptarts.com, puginarug.com, etc.
Clever solutions.
Try to choose simple solutions over fancy ones. This tends to create more maintainable code bases, especially with junior devs on the team. Often times more senior devs get bored and over engineer solutions that add unnecessary complexity. Sometimes we trick ourselves into thinking that complexity is justified because of something like performance, but it’s almost never the case. Don’t trade easily understood for a few milliseconds of faster page load.
Comments in code.
Comments should be thought of as a necessary evil. They are a tool that serves a purpose, but wielding that tool should be done with great tact. Robert Martin mentions this in Clean Code perfectly.
"Usually they are crutches or excuses for poor code or justifications for insufficient decisions, amounting to little more than the programming talking to himself."
Code should be readable and understandable without comments. If you feel like you need to add a comment to explain yourself, be sure you can’t accomplish the same ends with better naming.
// products that cost $4
const products = await getProducts(4)
// if products is loaded and not errored
if(products.results && !products.loading && !products.errored) {
// update products to include category
return mapProducts(products.results)
}
const desiredPriceRange = 4
const productsInPriceRange = await getProductsByPrice(desiredPriceRange)
const productsAreLoadedAndNotErrored = products.results && !products.loading && !products.errored
if(productsAreLoadedAndNotErrored) {
return setCategoryOnProducts(products.results)
}
Even with readable code, sometimes we’re tempted to use comments to tell future devs why certain logic was used. Maybe this is business logic, or a lesson hard learned, but the solution should probably be a unit test, not a comment.
Using javascript to solve css problems.
This is primarily about separation of concern, but also is almost always better for performance. CSS Is really good at CSS things. Just because you could solve it in JS doesn’t mean you should!
Nesting.
Aka pyramid of doom. Every time you nest something, you’re asking the reader to jump into a rabbit hole. We as a reader must retain the context of what’s happening above while continuing through this nested context. If you need an if statement in your function, make sure whats nested inside is ridiculously simple or have it call another function that we can dig into when we’re done with this function.
Using an else also usually just creates more unnecessary nesting. It’s preferable to return early.
This applies to both conditions in TS as well as DOM nodes.
Enums in Typescript
Enums do not provide the same level of type safety as other TypeScript constructs like union types. They allow implicit type coercion, which can lead to subtle bugs. It’s safer and a better developer experience to just turn your enum into a string union. type Color = 'red' | 'blue' | 'green'
Potential code smell
Using ts-expect-error, eslint-ignore.
In general, if you have to circumvent the tools that are in place to ensure code quality and consistency that’s almost certainly a bad idea. There are times when these things are actually acceptable (eslint-ignore more so than ts-expect-error). However, the presence of these is definitely a code smell and reason to dig deeper as a code reviewer.
If you ever do intend to submit one of these into production code, it’s helpful to include your reasoning in the comment.
Adding new dependency.
Adding dependencies to a project can seem like a quick solution to many problems, but it’s important to consider the long-term impacts. Dependencies come with overhead, keeping them up-to-date, learning their syntax, and working within their dev cycle to get bugs resolved. Using external dependencies often limits our ability to provide consistent developer experience and opens up our app to security vulnerabilities and overall bloat.
Type casting.
When comparing Typescript to Javascript it’s often said "TypeScript makes easy things hard and hard things any". Typescript is hard, it requires us to define our logic twice. Often times developers who are new to Typescript will only focus on the runtime logic, neglecting the mirrored logic for types. The lazy solution to this is to just cast your type from whatever Typescript says it is to what you wanted it to be. Sometimes it’s necessary, but Typescript is often trying to tell you something useful. Be sure to heed this advice from time to time and not use casting indiscriminately.
Committing dead or commented out code.
We use git, if you need to preserve a previous iteration of what you’re working on, commit it and it will continue to be available to you until you squash and merge. It’s too easy to forget to remove your commented out code.
If you come across dead or commented out code you should
Complicated, flakey tests.
If your test is hard to write, the underlying code is probably too complicated and responsible for too much. Tests should be just as readable and maintainable as our production code. Which is quite a daunting task, but one that we should take seriously. Tests that are too complex to understand provide little value.
Often times complex tests are also flakey tests. When any number of tests are flakey, it jeopardizes the faith developers will have in the entire test suite.
It’s better to have more tests that are concise and simple than it is to have fewer tests that are large and hard to maintain.
Try to encourage
Breaking things out.
Breaking out functions, services, components to keep things small and concise. Every function/service/component should have a SINGLE purpose, if you’re adding a block of logic that’s doing something else, it should be pulled out into a separate dependency. Also whenever you’re inside a loop (for-loop, or v-for), the contents should probably be a separate function/component.
Encapsulate complexity.
A common example of this is conditions. Rather than putting a condition in an if statement and expecting the reader to understand what we’re checking, move it into an easily understood const. For example, if(user.age > 16 && user.ownsCar && user.hasLicense) { ... } is much more easily understood as
const userCanDrive = user.age > 16 && user.ownsCar && use.hasLicense
if(userCanDrive){ ... }
Avoid extra noise.
Skip semi-colons, avoid excess DOM nodes, don’t be repetitive in variable names, use self-closing vue components. The less boilerplate, the faster we can all get to the meaningful parts. Be sure that the noise you’re removing is actually noise, and not helpful context. Don’t take this rule to mean you should be playing code golf.
Always encourage
Consistency!
It’s always better to be consistent than accurate. Even if you have a more “correct” syntax, if the “less correct” syntax is use all over you should use the “less correct” syntax. Take your great idea to the team and open a separate PR to update all of the places to use your new “correct” syntax.
Boy scout’s rule.
Leave the code cleaner than you found it. Even if it wasn’t you’re fault that there’s a mess in a file, it’s all of our responsibilities to get the codebase as clean as possible.
If you open a file and it’s a hot mess, don’t feel like you have to fix EVERYTHING. The goals says “cleaner” not “perfect”.
Semantic HTML.
This is a skill that’s deeply underrated. Take some time to learn some of the nuance of semantic HTML, this makes a bigger difference than you might think. Like so many things, Chrome tends to hide a lot of our mistakes.
Top comments (0)