Wednesday, 17 April, 2024 UTC
Summary
There’s no doubt that code quality plays an important role in any team’s output. Whether you’re weeding out critical issues, improving your skills, or simply establishing standards as a team, it’s paramount to be on the same page when it comes to defining and executing high-quality projects. Having the right tools along the way can also be a huge help.
JetBrains Qodana is a powerful static code analysis platform that enables teams to analyze code quality and find issues before runtime. In this post, we’ll give an in-depth demonstration of how to analyze JavaScript code with Qodana, using a Keystone project as an example.
As you’ll see, this approach enabled us to identify “Critical” and “High-Severity” issues first and then explore any other issues that needed to be addressed. Let’s dive in!
Setup
To start, we used a custom qodana.yaml file, which you can find here: (via the Configuration tab).
In this instance, we used TeamCity to launch the analysis, but you can use whichever CI/CD tool you prefer. You also have the option to use the Qodana Plugin in WebStorm – where we viewed issues and corrected code at times. You’ll see some of this below.
Results
After getting set up, launching, and waiting briefly for the analysis to finish, we examined our results. Let’s take a look at the results of this project, from most to least critical.
In most cases, we ignore problems in generated files and suggest excluding these files from the analysis. Let’s take a look at the most critical issues first.
Critical Issues
Qodana found two issues with “Critical” severity. Both of these were incorrect CSS property names. Take a look here.
It’s possible that the authors wanted to use a CSS property text-decoration-skip-ink, but something went wrong. Luckily, this particular instance has turned out not to be critical, because it only affects underlined text on the error and loading pages, but this won’t always be the case.
Sometimes, problems like these could break the layout of the page. Just imagine if you had typed something like
display: plex instead of display: flex in CSS styles for the root container of your layout. This could easily crash your site layout.In this case, the issue turned out to be an easy fix. Qodana highlighted it, and all we had to do was use the correct property name:
text-decoration-skip-ink: auto;High-severity issues
In this example, there are a lot of “High-severity” issues. Let’s take a closer look at them:
Comma expression
In this case, we’re using WebStorm (The JavaScript and TypeScript IDE by JetBrains) with Qodana.
Qodana says:
“Reports a comma expression. Such expressions are often a sign of overly clever code and may lead to subtle bugs. Comma expressions in the initializer or in the update section of for loops are ignored”.
You can view the result here. This is a real mistake where the author typed a comma instead of a period.
Due to this typo, we won’t see this message the next time prepare-release.js is called:
…because we will get an error, such as Uncaught ReferenceError: push is not defined.
Again, this is an easy fix. We just have to change the comma to a period. Let’s do that immediately.
Unused global symbol
We identified multiple “Unused global symbol” issues. Sometimes, issues like these mean we removed a chunk of code but forgot about interfaces, public class props, etc., that were only used in that chunk of code. These issues can occasionally be false positives, but you should always look into them just to be on the safe side.
For example, let’s say you’re writing the code for the documentation interface. You define headings for H1 to H6 but then only use H1 to H3. In this case, you will get “Unused global symbol” issues, because Qodana can’t find evidence of other heading levels. It’s up to you whether you want to delete the unused headings to make your code clearer or whether you choose to keep it. Both options are fine, but a decision needs to be made.
Unused local symbol
An unused local symbol is much easier to address. This type of issue indicates that you have declared but not used a local variable. In our analysis of these problems, all of them relate to unused function args. You can just remove these unused args or use _arg notation (of course, only if your linter allows it).
Function with inconsistent returns
This is a bit of a tricky one. An issue like this doesn’t always indicate an error in your code. Sometimes, it’s more about clarity and simplicity. But sometimes, it can lead to unexpected and unclear errors.
Of course, from time to time, we use return to simplify code or to return earlier from our function without additional constructions. Regardless, you need to be prepared to deal with scenarios like this, especially when you use these functions in other places.
How do we fix it? Usually, you can rewrite your code to avoid this problem. See the example below:
dir.map(x => {
if (x === '.next') return
return fs.rm(Path.join(projectAdminPath, x), { recursive: true })
})
You can rewrite it as:
dir
.filter(x => x !== '.next')
.map(x => {
return fs.rm(Path.join(projectAdminPath, x), { recursive: true })
})
In some cases, this issue isn’t important, and you may opt not to make any changes to your code. Qodana can highlight these potential issues in your code, but you need to examine the result and decide whether to fix it or not.
Overwritten property
There were a few “Overwritten property” problems highlighted. These can crop up either because CSS properties have literally been overwritten or because some properties have merely been duplicated. Ignoring them can lead to unexpected UI style bugs.
Reserved word used as name
We have three problems here, all of which are caused by using the reserved word
enum as a variable name. In most cases, the code still works perfectly, but some keywords can cause errors. Some developers prefer to avoid situations like this because, at the very least, it will affect code highlighting in the IDE.Unused assignment
In the project we analyzed, Qodana reports this issue in three scenarios:
1. When we try to use a variable before initialization, as was done here. The code in the example will work normally because we have an
undefined value included in the array, and in JS, a construction like [].includes(undefined) generally works like a charm. But in other cases using a variable before initialization can cause bugs.2. Redundant variable initialization, as can be seen here. Language services or static analysis tools can control the variable initialization flow and report an error when you try to use an uninitialized variable (like in p.1). But in this case, it’s mostly about code style. Some developers prefer initialization variables with a default value like
let str = '' If we want to secure a variable type for TS typings, we can use:
let str: string;
if (condition) {
str = 'a';
} else {
str = 'b';
}
3. Redundant value assignment to a variable, as seen here. In this code, we try to get a person. If we’re unsuccessful ( if (!person) ), we try to create one. And here we assign a value to a variable but don’t use this variable anymore.
Exception used for local control-flow
This is an interesting one. In this code, we throw an error in the try block that will always be caught in the catch block.
There’s actually something of a heated debate about this – some people think it’s an anti-pattern (or at least a code smell), but other people just use it with no qualms.
Some developers in our team prefer this way (the article is for C# but works perfectly with other languages) when working with exceptions but still allow themselves to “misbehave”, as in our example.
Single character alternation
Here, we have the same issue in different files. In this code, the following regular expression is used:
<strong>/(>|<)/gi</strong>
What does it mean? We just want to find > or < . Regular expressions have character classes to help us with this, and we can simply rewrite this RegExp (exactly as Qodana suggests).
<strong>/[><]/gi</strong>
This is more readable and should be faster (there may be some fluctuations with different regular expression engines).
Redundant local variable
Here we see cases like this – issues that can be discussed and debated. In this case, it’s mostly about code style. However, we’ve used it both ways – with a redundant variable and without. This is why: If you need to debug code often, creating a variable can be more convenient because it’s easier to work with it in the debugger. In other cases, it’s not as important. Ultimately, it’s up to you and your team.
Unsound type guard check
This is a tricky issue. Here’s a simplified version of the code that will generate this warning:
const func = (value: number) => {
if (typeof value !== "number") {
throw new Error("Oh, no");
}
// do smth
...
}
Example of issue
From the TS point of view, this if statement doesn’t make sense because you can constrain the type for the function arg value – the value can have only the number type, and you can remove this statement to reduce the amount of code. Less code, fewer bugs. Of course, the arg type cannot protect us from something like
func(”string” as any), but that’s truly evil, don’t do that.What if somebody uses your TS code inside a JS codebase? They don’t have type checking, and this kind of if statement can help prevent bugs.
In other words, the decision about this issue should take into account how the code will be used in the future.
Moderate-severity issues
Now let’s look at “Moderate-severity” issues:
Deprecated symbol used
We have about 80 issues like this in the report. In general, these aren’t urgent and don’t need to be fixed right away. But from time to time, we need to deal with them to avoid problems when we want to upgrade the dependencies they affect.
Usually, these are easy to fix, especially if your dependency code has good JSDoc comments like:
/** * @deprecated * * [MDN Reference](https://developer.mozilla.org/docs/Web/API/KeyboardEvent/keyCode) */ readonly keyCode: number;
Of course, sometimes you need to rewrite a bit of your code when changing deprecation usages.
This is another situation where Qodana can help you highlight points where changes are needed and estimate the time you will need to upgrade your code.
Duplicates
Every team has its own tolerance level for duplicate code. Sometimes, duplicate code is okay, but other times, it needs to be addressed, depending on team standards.
Redundant ‘await’ expression
In these cases, we wait for a non-promise result. Something like:
// non-promise
const func = (): string => {
return "a";
}
const asyncFunc = async (): Promise<void> => {
const log = await func(); // redundant "await"
console.log(log);
}
Simply remove these redundant
await and make your code clearer.Redundant nesting in template literal
Here, we have two identical cases:
border: `1px solid ${'var(--border)'}`There may have been some ${variable} here earlier, but that’s no longer the case. We can just rewrite it like this:
border: '1px solid var(--border)' //don't use literal at allRedundant type arguments
A few issues of this type were reported. What are they all about? Let’s take a look and get a general idea.
export interface Foo<T extends string = string> {
prop: T;
}
const variable: Foo<string> = {prop: "a"}
// equals to
// const variable: Foo = {prop: "a"}
In cases like these, we can omit <string> as a solution because it equals the default value.
Expression statement which is not assignment or call
This is the only issue in this category – but it’s a bug. Nothing serious, but important to consider nonetheless. For some reason, this orphaned header ended up alone and didn’t make it into the return block. Let’s fix this!
export default function Index ({ authors }: { authors: Author[] }) {
return (
<>
<h1>Keystone Blog Project - Home</h1>
<ul>
// ... other code
From here, we create a PR again and keep going until we’re happy with our progress.
All told, our Qodana analysis helped us identify issues by severity and address them step by step. Your team can do the same on projects with various license types.
Do you want to try running Qodana analysis for yourself?
If you have any questions or need assistance, submit a ticket to our issue tracker in YouTrack by clicking on New Issue in the top right-hand corner of the screen or let us know in the comments. You can also tag us on Twitter or contact us at [email protected]. We’re always here to help you get set up.
Free 60-day trial!