The top 5 JavaScript issues in all our codebases

All right, thank you very much.

Yes, spying on you all.

sorry about that.

up front.

cool.

That's all right.

That's right.

My name is Phil Nash.

I work at a place called Sonar.

We do products like SonarCube, SonarCloud, and SonarLint, which allow you to help to statically analyze your code, understand what's going wrong, inside your own code base and how to, improve it and improve it over time.

again, my name is Phil Nash.

you can find me on the internet as Phil Nash, in a whole bunch of places, including, what I thought was the greatest URL in the world, which is also just Phil Nash.

Thank you, to the island of St.

Helens, for that.

I'm talking about the top five issues in all our code, in all our applications, all our javaScript And so I thought I'd better start with where the data comes from.

and, so I actually, I picked this out of, Sonar Cloud.

So Sonar Cloud sits in your build pipeline, analyzes your, projects and, and tells you what issues there are in it, and whether your, application is safe to pass through the pipeline.

if you've not made any more mistakes in this particular pull request, which is a favorite problem of mine.

And so I went to the team there and I said, can we get, what are all the issues, like I didn't go and dig into people's code bases.

I wanna let you know that is still private.

But we got the aggregate of all of the kind of issues, listed there.

and I picked out the top five 'cause I thought that would be the most fun or at least the best title for this.

and when I say JavaScript issues, I'm not necessarily talking about stuff that is collapsing, catastrophic, bugs that are, crashing applications in the front or the backend.

I'm merely kind talking about those kind of issues, that we can detect in static analysis, that are going to, perhaps cause you issues over time, cause cruft in your code base, cause, cause a nice code base to some, one day, be described as a legacy code base.

and that's not what we want, right?

We want to stop writing legacy code, stop writing code that gets called legacy code.

We want to write code that we're proud of, and then when we come to maintain and use and ship to production...

That we're happy with.

And so it's those kinds of issues.

we're talking about clean code here in a sense, code that you're happy maintaining, and happy, pushing to production.

I jumped into a couple of quotes about this kind of thing, because I thought that would be a fun way to kick off.

Particularly this one is one of my favorites.

back on the C++ mailing list, John F.

Woods, who is a, yeah, games programmer, said, always code as if the guy who ends up maintaining your code will be a violent psychopath who knows where you live.

And most importantly, code for readability.

I liked this, and this was fascinating actually, because this was just in response to a, quite a simple conversation about, some coding standards in C++ at the time.

those standards, it was a big discussion about whether the comma operator was something that could be, that should be, enforced as a way to simplify conditionals like this, for example.

the idea here being that if you use the comma to do two things on one line, you don't have to put braces around the whole conditional.

that's a useful thing.

And I thought it was interesting as well, because actually this parses as JavaScript too, right?

It's C++ from the original thing, but, it turns out it's the same.

but many other people pointed out in this discussion, aside from comments about violent psychopaths, that actually this comma, if you typo it to a semicolon, for example, you change the meaning of the entire piece of code.

in this case, the, syntax highlighting has just, decided it's a bad idea already.

but we're talking 1991 here.

They, everything was black and white, or green and black, or, you can, but, every JavaScript developer knows, that searching a code base for a semicolon that's there or not there, where it should or shouldn't be, is one of the worst days you'll have.

and so the comma operator, which, it does not differ too much from a semicolon, is not a good way to write clean and readable and maintainable code.

Thus, violent psychopaths.

in a less...

dangerous, quotes.

Raymond Chen on a Microsoft blog said, Design for readability.

Even if you don't intend anybody else to read your code, there's still a very good chance that someone will have to read that code, or stare at your code and figure out what it does, and that person is probably going to be you, in 12 months from now.

And if you know yourself to be a violent psychopath...

Then you, don't just know where you live, you know where you are right now.

in this case, John Wood said code for readability, Raymond Chen says design for readability.

readability, maintainability seems to be, a theme that we can all agree is probably a good idea and is going to make our lives easier, if we go for that.

And ultimately, that's what I mean by this clean code idea.

It's something that runs better in production but is also, better to work with in, in development.

Something you're happy to open up in your editor and work with.

And so these issues that we're trying to avoid are ones that are, ones that are going to make our code less clean, less maintainable.

So what are they?

I'm not going to go through these in order, because it turns out that when you pick the top five most common things, they're really common.

And you've, heard of them all before.

Most of them, I think.

I will start at the bottom, at number five.

And, just to make things a little bit more exciting, I added confetti.

So at number five, we have, unused assignments should be removed.

Unused assignments.

So the fifth biggest issue across all the code bases, that we have in Sonar cloud, and that is hundreds of millions of lines of JavaScript, is that we have unused assignments in our applications.

And by that, it's something as simple as assigning something and then reassigning it to something else without having used it somewhere in the middle.

because it turns out that line one in this case is completely useless.

It might've been useful at one point.

It might've been there, for a reason, but it's gone and been overwritten by something else, and, and is no longer useful.

This means, of course, when we're reading and trying to parse what this thing does in our head, when we're trying to add or change something to a function like this, that we, we have to, handle these things, and, work out what they do.

And if we find out that it I didn't actually do something that causes a break, at least in my mind, when I'm going through things, that's a bad thing.

This was another example of this that I found in open source code, where you have a function that both makes an assignment and returns at the same time.

Now this result...

it's not a local variable.

It's, somewhere outside of the scope of this function.

but this, is confusing to me, right?

You have a function that's going to add things together, but that function is also setting a variable that we're using somewhere else.

and that, is a lot harder to parse and follow the flow of code.

this is why people prefer the idea of pure functions, where if, whatever happens when you put two values in, you always get the same thing out at the other side and no other side effects.

This is a side effect, and, but still actually just an unused assignment.

I say it's unused.

This is how it fails the, this sort of test, although it's probably being used just somewhere else that you don't know when looking at this particular function.

So bad.

so that's number five, relatively straightforward, I reckon.

number four, we don't have to applaud every time.

the, triple equals and not equals, instead of double and not, like who's a fan of just two equals signs?

I didn't think so.

I don't have to explain this one too hard either, but, the best, visualization you might have seen this, is from, you can hit that link at some point, this JavaScript equality table, and this is double equals, where, different values are, down both the X and Y axis, and you can see, the equality of, two things that are the same down the middle, but then you get, fun JavaScript interview questions like, what is, the, result of double equals between true and the string one?

It's true, but the string zero is not, and the string minus one is not.

All sorts of stuff.

and so the only visualization to, to prove the point that, the number four should be changed and updated is very much that this is triple equals, right?

and nothing is equal to anything at the bottom section of that, which is great.

not a number is not, a number.

Sure.

we have isNone for that.

That's fine.

so triple equals, always use triple equals.

We're happy with that.

I told you these were going to be common.

So number three, sections of code should not be commented out.

This one goes down to, I don't even have an example for this kind of goes down to the one I, to the, what I said about, unused, conditional, unused assignments, sorry.

If you're re reading through a function and parsing the code and trying to work out what it does, and, and you see something, or if I see something that's commented out, but also code, I'm also trying to work out what that does, even though it doesn't do anything, because it's behind a comment.

this, to me, is a...

we have a way of doing history in our code, right?

something that says, this code was once doing this job and now we have this code, which is the uncommented stuff.

But we also have version control for that.

and if you put something into version control and then change it and put that into version control, good news is we don't ever lose the previous thing.

and If you find sections of code commented out in your codebases, probably a good reason to just remove them, and, check that in, because you can always recover that later.

No problem.

I thought I'd take time for a TypeScript break.

people in the room using TypeScript as well?

Quite a lot.

Yeah.

All right.

Turns out the top five in TypeScript are not the same as the top five in JavaScript.

and so I thought I'd throw a few, little TypeScript things in here as well.

It's a good time to do that break because it turns out that sections of code should not be commented out is number two in TypeScript.

TypeScript developers apparently very much into commenting code out.

but if we go back to the bottom, we can, I think I dropped the confetti off this cause it was TypeScript and I don't know, track uses of to do tags, which is quite interesting.

TypeScript, people using TypeScript, building TypeScript projects, very much, leaving themselves notes, and I guess not doing them, which is why it turns up as number five.

This, of course, isn't a problem in your code, in fact, it is a good reminder to do something.

But if it keeps coming up, if it turns out the number of to do's in your codebase is ever increasing, then stuff is not getting done.

And don't forget to do the to do's, that's, my takeaway there.

in at number four, is actually a React, issue.

we can describe whether it's an issue or not, or we can debate whether it's an issue or not at another time.

but I thought it was very interesting to find out that in all the TypeScript projects that we have, a potential issue in a React project, came in at number four.

This is the idea of, not using bind or arrow functions inside props, inside JSX.

the idea being that if you are rebinding within the JSX, then, you are creating a new function, every time you render.

It does specifically call out in the old React documentation, that this kind of thing shouldn't be a problem, but it is something that over time can cause performance issues, and so that's why we have a rule for it, in order to avoid potential issues over time.

Ah, it looks like this, and this is a class based component, yeah, don't stick stuff, don't bind stuff in the props, that's, easy.

Just put it in the...

Constructor instead.

Job done.

Or function components, same thing.

Just set a bind in the constructor.

Ooh, and at number one, I forget about the lack of confetti.

Sorry.

Unnecessary imports should be removed.

The top problem in TypeScript, projects is that, everybody's files have a bunch of imports at the top that they're not using.

Not an enormous issue, actually, because of course if you don't use an import inside your file, TypeScript strips it away, doesn't include it anyway.

TypeScript is saving you there.

but unnecessary imports are just clutter, something getting in the way of reading your code and understanding what it's doing.

So I've left space for three in TypeScript and in number, yes, that's an unnecessary import.

Back to JavaScript.

I have space for three in TypeScript and number two in JavaScript and also number one.

so we'll do number one just quickly.

Although actually, have you got any guesses what number one, in JavaScript are the biggest?

There's a, complexity.

Oh, get to come back to that.

It's a good guess though.

Optional chaining, a bit too esoteric, I think, sadly.

Another, I can't hear that, sorry.

Console logs.

Oh, that'd be good, wouldn't it?

It's not, it's none of those.

it turns out the biggest issue, I'm gonna go with it now, because I realize we're also recording this and you can't hear those things.

it wasn't console logs.

It wasn't optional chaining.

It is, using var instead of let and const.

I think we could describe this as legacy, but we have had let and const for eight years now.

so the remaining vars or the reintroduction of vars, is something that, we could consider, legacy.

I think the point here is not that this is going to break anyone's code, or it shouldn't do, especially if you used var in the first place and used it correctly.

But var doesn't tell you much about what you're doing with that variable.

Const and let, on the other hand, communicate more.

And much of, much of these issues, much of the things that are, that linters and other programs like this are going to pick up are ways to actually just communicate to other people better.

Like we've seen things like comments in code and unused variables are just, they're mixing up the communication.

They're making it more difficult to understand.

But in this particular case, var is not communicating and const says we are not going to change this reference and let says, yes, we will.

and that makes it easier to understand the code that follows.

It's a good reason to change it.

And yeah, in at number two, finally, my absolute favorite, and as called out.

Cognitive complexity, a function should not be too high.

I went all out on the confetti on this one.

It's, it'll stop eventually.

cognitive complexity of functions should not be too high.

It turns out it's number three in JavaScript as well.

so we get to keep the confetti rolling on, as people say things like, what's cognitive complexity?

and yes, I will get to that.

What is cognitive complexity?

Cognitive complexity, is best, I think, compared against, a different kind of complexity, which has been a measure that's been around a lot longer, the idea of cyclomatic complexity.

Now, cyclomatic complexity is a score you can give to a function or a piece of code, that, tells you or gives you an idea of how complex it is.

and this is an example of that.

We have a function here that sums up the prime numbers below a max number, a relatively complex thing to do.

Prime numbers are a pain.

and so Cyclomatic Complexity scores this by scoring one for it being a function, which I think is harsh because we don't get a fun without a function, what do we have?

we then score another one for that, first loop.

we get inside that loop.

We have a nested loop that also scores one.

We've got a conditional inside that loop, which scores one.

We drop out of the first loop, sorry, the second loop, for final conditional, and we score five points for this.

That, actually tells us that in order to write enough tests to cover this function, we need to write five tests to cover it fully, and completely, and understand what it's doing.

So it's a useful number, but I've been talking about maintainability and how you can read functions and how you can understand things over time.

this is a bit of a complex function.

Five is not an enormous score, but like a couple of nested loops, a couple of conditionals, has quite a bit going on.

This is another function that scores five points.

It returns a word for, if you give it a number.

So if you give it one, it returns one.

If you give it two, it returns a couple, three, a few, four, many, and five lots.

It's a big switch statement.

But, cyclomatic complexity, because it's talking about the number of pathways through this code, the number of tests you'd need to cover all of this.

Also scores at five, one for being a function and then four more for each case in the switch.

And so I think to me, as a measure of maintainability or understandability, this is a lot more, a lot more understandable than the previous function, yet they are equal in the eyes of cyclomatic complexity.

And so that's where cognitive complexity comes into it.

Cyclomatic complexity, again, measures the number of paths you need to, that you can take through a function.

Whereas Cognitive Complexity is intended to measure how easy it is to read a function.

Now this was a measure that was actually, invented, by a bunch of people at Sonar back in 2016.

so it has nothing to do with me actually.

but I'm very excited, I was very excited to find it as an idea about how you can actually use it to measure code.

The idea of Cognitive Complexity is it's still a score for a function, based on, an algorithm.

and, quite simply the algorithm in this case is we increment the number for a break in flow, much the same as we saw with Cyclomatic Complexity for a loop or a conditional, but we also in the background implement a nesting score.

And, and so when you nest, when you go inside of a conditional or a loop, that nesting score is incremented by one, and then every time we then have a break in flow, we increment by one plus the current nesting score.

And so that's easier to see in an example.

This is our sum of primes, function again, and, you'll notice to start with, we deal away with the idea that we have to score for it being a function.

They're all going to be functions.

We can start at one or zero, but then we hit that first loop and it scores one because the nesting is zero at the time.

And we get inside that, we get to the second loop, and the nesting has score, has been incremented to one.

So our inside loop scores one for being a loop and one for being nested, 2.

The condition inside of that is now nested twice.

So it scores one for being conditional and three points, for, sorry, two points for being nested.

For a total of three, we drop out of the loop, get another conditional that scores two.

And this gives us an ultimate cognitive complexity score of eight.

The nice thing is comparing this to our previous example, our previous other function are, get words.

which scores one, because it's a big conditionally thing.

Cognitive complexity does not take each case into account, when, scoring things, it just says we have one conditional, which has a bunch of options in it, but they're not hard to parse and understand.

and so in terms of just the feel of this, when you see two functions like that next to each other, that scored the same for cog, for cyclomatic complexity, but wildly different for cognitive complexity, it makes sense as a gut check as to this works.

Now, there have been a couple of studies done on cognitive complexity, since the original paper was published, that came up with the, same, response to say, yes, this is a good measure of things being more maintainable.

So third parties have, have checked this over as well, and that's why I like it.

and so the score, sorry, the rule that we actually failed here was that cognitive complexity of functions should not be too high.

So too high implement, implies a threshold, and in JavaScript, that threshold is 15.

I think it's the same for TypeScript.

and that can seem like a bit of an arbitrary kind of number.

I don't think that's how it should be considered.

and I'm going to use an example to demonstrate this.

15 to me is a threshold that says we should look at this function.

And so.

this is a bit of code.

It doesn't really matter what it does.

it implements throwing confetti on my slides.

It's a reveal JS plugin.

and, so on the slide transition end is a, function that we'll run, when a new slide comes onto the page.

and it works out based on whether the current slide has, certain data attributes, inside it, what to do.

And so it gets a couple of things out of some other data attributes here.

And it, then does another conditional.

Another conditional here, and then calls a function.

Or here we do another kind of ternary, getting stuff out of the data attributes, and we then call a request animation frame loop.

got a bunch of stuff going on here.

Here's a big settings object, so that's not too complex, but we've got another conditional, we've got the else, and it does a bunch of stuff.

And in my IDE here, I have sonarlint installed, which is a way, which is one way you can be told, what the cognitive complexity of a function is, and, in this case, if I hover over it, it's going to tell me this should be refactored, to reduce the cognitive complexity from 16 to 15.

What I like about this, is that actually it tells me there's eight locations.

So if I hit quick fix and say, show all the locations, this is actually pointing out to me all the spots where, cognitive complexity, is being increased, within the, within the code base.

And so this kind of ternary here is scoring two because it's nested and it's a conditional.

so what we can do with this is refactor, right?

We've got this kind of warning come up to say, our cognitive complexity has increased, beyond our mildly arbitrary threshold.

I'm sure some people thought about the numbers, but, it doesn't matter what the numbers are and we can fix it, right?

and if I were to add in a quick, function there, that's going to pull an integer out of a data attribute or set a default, we can use that, here in particle count instead of, that, that big kind of, big ternary, the ternary is just moved to another function, but, There we go.

We can change that, and gloriously that fixes everything.

The cognitive complexity is now only 15, and the thing stops complaining about it.

But, yeah, it's not right.

What we're doing is now looking at this function and saying it needs refactoring, and so even though we've just dipped below the threshold what this actually means is we can carry on, and we can make this more readable and maintainable.

and ultimately I'm not going to spend time refactoring this for right now.

if we pulled out, both of the things where I'm getting, different data attributes out of, out of different attributes, out of a data attribute, pulled out the different, functions for doing small bits of confetti or large bits of confetti or medium, bits of confetti.

eventually the actual function to run on the slide transition and just becomes this, get some settings, and choose which function next to run.

And it's much easier.

One, it just fits on one screen now, even when the font size is 24.

but it's much more readable and understandable and you'd be happy to go back to it.

And so 15 might seem arbitrary, but.

It is that point, which I think works to just say, Hey, let's, go break this down and make this a lot easier to understand in the future.

So that's Cognitive Complexity.

It's a measure of how maintainable or understandable your code is.

And I think it's a really useful thing to, to know and understand about your code.

And it turns out it is in fact, yeah, the, second biggest issue in JavaScript projects around the world.

and like I said, it was the third in TypeScript.

Apparently I left that slide off.

Now there are tools that you can use to deal with this.

This is, where, I want to go with this.

Like to understand the things that are going on in your program, like these, issues to find out where they are, there are a bunch of tools for it.

Now, of course, SonarCloud and SonarCube can sit in your, build pipelines and tell you this as it happens.

but, And I saw, you saw me using SonarLint there in my editor.

they're not the only things, of course, ESLint, of course, is, is the open source.

I'm sure there's many ESLint users in the, in the room, that will give you a bunch of rules.

it doesn't give you cognitive complexity, although it will give you probably the other four.

but we, we also publish, an open source plugin for, ESLint, just the SonarJS plugin, which will give you cognitive complexity as a score, as a rule within there as well.

so really useful to, I, I think to understand how maintainable your code base is as you're working on it.

I will publish these slides and, on there, there are some other links, including the Cognitive Complexity paper.

this is the first computer science paper I've read more than once, which I don't think I should be this old and in computing for this long to have to say that, but it was a compelling paper and I really liked it.

so we'll send the link out to that.

It's, it's really interesting.

It is mostly Java based, but there are some affordances for JavaScript within there.

there's also the link to Raymond Chen's, article about, readability, designing for readability.

and, on there, I've got a link to the, all the rules that we, we cover, as part of Sonar, that you can go and have a look at if you're interested.

And finally, the reveal.

js confetti plugin has also been published if you want to do that as well.

so ultimately, we're talking to, we're talking about clean code.

We're talking about, getting rid of those issues in, your, in your application, in your.

In your codebase, I'm not saying you need to drop everything right now and go and refactor everything you've got, because we don't have the time for that, but if you notice these things as you're working on bits of your project, or if you have linters or IDE extensions to, to tell you, some stuff's going on here that you should maybe fix up, and clean your codebase as you work on it, then, we're actually going to find, the quality of your codebase will improve over time, and everybody will be a lot happier.

refactor those complex functions, and don't just bring it down to under a threshold.

Refactor it until that you'll like to read this the next time you come across it.

code for readability, code for yourself in 12 months time, code for the future violent psychopath who may well know where you live.

We can all do that together, we can all make our lives happier, make our codebases happier, remove those issues, and that's all I have for you.

Thank you so much, Web Directions.