Forem: Evan Sutherland

Unsolicited Wisdom: My Take on Clean Code in the Frontend

Evan Sutherland — Mon, 01 Jul 2024 16:38:00 +0000

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.

The case against comments

Evan Sutherland — Fri, 28 Jun 2024 00:54:54 +0000

Most developers starting out assume comments are like sprinkles. They're something everyone likes and the more you use it, the prettier your donut. One of my favorite authors Robert C. Martin, author of "Clean Code", has this to say about comments.

"Usually they are crutches or excuses for poor code or justifications for insufficient decisions, amounting to little more than the programer talking to himself."

Given this context, it's not surprising that junior devs write so many comments, since their code tends to be pretty rough around the edges. Even veteran developers who can truly write beautiful code sometimes think they need to reach for comments.

I'd like to make a case against this impulse.

How comments are used

To explain what is already obvious

There are plenty of times I've seen developers write perfectly simple code only to litter it with comments that explain what was already obvious.

export async function checkUserPermission(permission: Permission): boolean {
  // if user is not authenticated, throw error
  if(!auth.isAuthenticated){
    throw new UserNotAuthenticatedError()
  }

  // get current user role
  const currentUserRole = await userService.getUserRole(auth.currentUser)

  // check if current user role includes permission
  return currentUserRole.hasPermission(permission)
}

To hold on to dead code

Many devs can't help but comment out code they're afraid to delete.

// const hasPermission = await checkUserPermission(permissions.posts.edit)

// if(!hasPermission){
//   router.reject('insufficient access')
// }

To explain strange business logic

Even the most eloquent engineer might struggle to explain particularly complex or misleading business logic.

function calculateLoyaltyMemberDiscount(customer:Customer) number {
  if (customer.isLoyaltyMember) {
    // loyalty members discount is 1% per month before merger, max 5%
    return Math.min(monthsInInterval({start: new Date(2010, 1, 1), end: customer.startDate}) * 0.01, 0.05)
  }
}

To document code

Especially in situations like projects that use vanilla JS, tools like JSDoc can provide some seriously useful context that happens to be provided in the form of comments.

/**
 * Calculates the loyalty discount for a customer based on their membership duration.
 * 
 * - Loyalty members receive a 1% discount for each month they were members before the merger.
 * - The maximum discount is capped at 5%.
 * 
 * @param {Customer} customer - The customer object.
 * @param {boolean} customer.isLoyaltyMember - Indicates if the customer is a loyalty member.
 * @param {Date} customer.startDate - The start date of the customer's loyalty membership.
 * @returns {number} The calculated loyalty discount.
 */
function calculateLoyaltyMemberDiscount(customer) {
  ...
}

Why comments are evil

Most comments are added in an effort to improve readability, but what's evil about comments is they inevitably do the exact opposite. Even the best, most clearly written comment will eventually become evil, serving the demise of the code it was sworn to protect.

Comments can move

Just like in real estate, comments are all about location. As blocks of code get moved, lines added, lines removed, the location of a comment might get moved away from the block it was referring to.

Comments can be outdated

Codebases are constantly changing. Functions get renamed, conditions get added, APIs get deprecated, bugs are removed, bugs are added. How much do you trust other engineers, or yourself in 6 months, to make sure to refactor comments alongside the code that the comments refer to?

Comments can be misleading

Now imagine that a refactor happens and nobody catches the outdated comment in code review. That comment is now not only wrong, it's misleading. Future engineers will spend more time trying to figure out how the code ever worked than they would if there was no comment at all.

How to stop using comments

Fortunately, the problem of explaining our poor code has better solutions than adding comments. Most of the time code is capable of explaining itself, it just takes a little practice.

Give your variables, parameters, functions clear names

Avoid single letter variables, acronyms, or shortening in general. There is no good reason in modern development to not be verbose.

function exec(cb) {
  let t;
  return (...args) => {
    clearTimeout(t);
    t = setTimeout(() => cb(args), 2000);
  };
}

function debounce(callback) {
  let timer;
  return (...args) => {
    clearTimeout(timer);
    timer = setTimeout(() => callback(args), 2000);
  };
}

Break things out

Some developers seem to think that you should only break out a function, component, file, if you think it's in pursuit of DRY code. Or in other words, only if it's likely to be reused. That's more of a convenient side-effect of the primary goal, keeping blocks of code small and concise!

Encapsulate primitives and conditions with clear names

Whenever you see a primitive, a ternary, an if statement, or any other condition, see if breaking out a variable would make the intent clearer.

// 42 is the offset of the header
scrollTo(42)

// if animal is a dog
if('sound' in animal && animal.sound === 'bark'){
  ...
}

const headerOffset = 42
scrollTo(headerOffset)

const animalIsDog = 'sound' in animal && animal.sound === 'bark'
if(animalIsDog){
  ...
}

Use unit tests

Even with all of the tips above, some bits of business logic are too weird to explain easily in code. Complexity like this is a great candidate for a unit test. The title of your unit test can literally be the comment you were going to write, but a good test lets you assert the behavior doesn't change without mucking up the production code itself.

Example

Let's look an example of some seriously ugly code and clean it up using the suggestions above.

Before

type Usr = {
  id: string,
  perm: number[],
}

// validates user has API access and active session
const v = async (x?:Usr) => {
  if(!x) return false
  // user must have API access
  if(!x.perm.includes(1)) return false

  // check session is active
  return await fetch(`/usr/${x.id}/valid`).then((rsp) => {
    const valid = rsp.json()

    return valid
  }).then(valid => {
    return valid.valid
  })
}

After

const USER_PERMISSIONS = {
  API: 1,
  ADMIN: 2,
} as const

type UserPermission = typeof USER_PERMISSIONS[keyof typeof USER_PERMISSIONS]

type User = {
  id: string,
  permissions: UserPermission[],
}

const validateUserHasActiveApiSession = async (user?:User) => {
  if(!user) {
    return false
  }

  const userNotSignedUpForApi = !checkUserHasApiAccess(user)
  if(userNotSignedUpForApi) {
    return false
  }

  return await checkSessionStillActive(user)
}

const checkUserHasApiAccess = (user: User) => {
  return user.permissions.includes(USER_PERMISSIONS.API)
}

const checkSessionStillActive = (user: User) => {
  return UserSessionApi.checkUserSession(user.id).then(({valid}) => {
    return valid
  })
}

class UserSessionApi {
  public static checkUserSession(id: string): Promise<{valid: boolean}> {
    return fetch(`/usr/${id}/valid`).then((response) => {
      const sessionValidityStatus = response.json()

      return sessionValidityStatus
    })
  }
}

This rewrite might be longer, but each block is tiny and significantly easier to understand and as a result maintain. Best of all, as the code evolves there are no comments that will get moved out of place, outdated, or misleading.

Comments in code is a tool, you should use all the tools at your disposal to make the cleanest code base possible. However, it's important to understand that all tools have their downsides. So, next time you think your code needs a comment, see if you can fix it without a comment.

Is mitt dead? 🥊

Evan Sutherland — Wed, 19 Jun 2024 02:36:31 +0000

Mitt was first released 7 years ago. Today, it has open issues going back to 2021, issues waiting to be triaged, open PRs completely ignored, and no commits since mid 2023. Are the issues not worth doing? Is the library just avoiding exceeding it's 200 byte "microscopic" constraint? Maybe the author is busy with new projects and is waiting on new contributors? It's hard to say, but it makes me wonder if it's just dead.

What is a meaningful aim for a project like this?

Mitt is clearly still successful. It's easily one of the most popular Typescript event emitter libraries coming up on ~7M downloads on npm. Having an audience makes it easy to assume it's good as is, but would it be even more popular if it were more actively maintained? Or would feature bloat kill it entirely? Does keeping that 200 byte zip size actually matter?

Here's the past 5 years of downloads according to npm.

Despite a flat-lined commit history.

When does it make sense to stop?

Is it normal for projects to just stop? With the pace of change, especially as it relates to Typescript, if you're not changing it must mean you're dying. Is an event emitter just so simple that it can actually be "done"?

Taking it personally

In our case we needed a once function. It's clear that mitt is not interested in adding this feature any time soon. The implication was that this is a problem for users to solve, though we couldn't get the types to pass. So we built our own

Kitbag Events

https://events.kitbag.dev/

Type safe events and payload
Api includes on, off, once, emit, and clear
Support global Handlers
Supports broadcast channel

Kitbag Events is a fresh take on an old concept. Like Mitt it's tiny with zero dependencies, though not trying to be "microscopic". My bet is that developers would prefer a modern library that's actively maintained, even it it comes with some extra bytes. Come check it out, drop a star ⭐️, and let us know if this was a good bet.

Happy engineering!

Introducing a new router for Vue

Evan Sutherland — Fri, 24 May 2024 03:18:10 +0000

Brief history of my journey with vue-router

Virtually every developer using Vue.js is currently using or has only ever used vue-router. Many of those developers, myself included, are mostly happy with it. The biggest complaint I hear is the lack of strongly typed routes, which is usually solved with a bit of Typescript wrapping your routes.

// router/routes.ts
import { RouteLocationRaw, RouteRecordRaw } from 'vue-router'

type RouteFunction = (...args: any[]) => RouteLocationRaw

export const routes = {
  home: () => ({ name: 'home' }) as const,
  club: (clubId: string) => ({ name: 'clubs.view', params: { clubId } }) as const,
  profile: (userId?: string) => ({ name: 'profile.view', query: { userId } }) as const,
} satisfies Record<string, RouteFunction>

Then routes becomes your source of truth, so navigation should always use a route from routes to send proper names to vue-router.

<router-link :to="routes.club('club-123')">go to club</router-link>

The syntax of defining routes is a bit cumbersome but honestly works fairly well. However, because I’m a serial starter-of-new-projects and love a challenge, I teamed up with Pleek91 to see if we could build our own.

Introducing Kitbag Router

https://router.kitbag.dev

It’s intentionally different

We made the decision to start the project from scratch; solving problems from first principles rather than taking the path of so many others and try to build on top of the relatively old vue-router project. This means Kitbag Router is NOT a drop in replacement for projects using vue-router. Therefore, this freedom also means we can have a relentless pursuit of the best possible developer experience possible, not weighed down by having to have feature parity with vue-router.

First and foremost, it’s type safe!

Kitbag Router uses a createRoutes utility that returns routes with the name, path, query, all preserved in the Route type. This means the routes you provide to createRouter will maintain type safety when accessing the current route or performing navigation.

Type safety in routing means auto complete when assigning to on <router-link>, easy discovery of available routes when defining a beforeRouteEnter hook, and ensuring that Typescript will error if the expected params for a route change without updating the router.push() call.

Params are way more powerful

Not only does Kitbag Router know what params are expected for a given route, it can also support param types beyond String.

Kitbag Router ships with built in support for params of typeString, Number, Boolean, Date, RegExp, JSON, as well as any custom param type you define. Not only is this type useful when navigating, but it also means a route that expects a number for an “id” param won’t be considered a match if the value provided in the URL doesn’t satisfy the number constraint. Furthermore, your Vue component that gets rendered for the route doesn’t have to worry about whether the param will be present or not or having to convert it from string to the type we know it’s supposed to be.

Support for the query

Params aren’t relegated to only the path. Kitbag Router allows you to configure your expected params in the query the same way you’re used to defining params in other parts of the URL.

Handling rejections

Inevitably when defining your routes, you’ll want to add a “catch all” or “not found” route. While Kitbag Router fully supports defining plain ol’ Regex in your path/query, it also ships with even better support for handling NotFound. We call this a “rejection”, which is customizable and can be extended to any custom rejection you need, like perhaps NotAuthorized. When a rejection is triggered, usually from within one of your hooks, Kitbag Router will automatically render the rejection component you assigned for the given rejection type.

And more…

These are the key features we think really make Kitbag Router compelling but there is so much more. Kitbag router is built for Vue3 and support async components, route hooks, editable params from useRoute, components for RouterLink and RouterView, and more.

Is it ready?

Kitbag Router is still super green and currently maintained by a team of 2. It has not yet had a stable release (on 0.2.0 as of the time I’m writing this), so things are still likely to change. That being said, I hope you’ll give it a try, maybe give it a star. We’re excited to hear from the community on what features we should focus on next.

Happy engineering!