Hello and welcome back to the channel. In this video I wanted to review several examples of bad code that people send me to show what techniques I'm using to refactor and the way of thinking.
So let's jump right into it.

Code #1

(this.router.state['view']['data'] || {})['menuOpen'] && this.router.navigateBack();

The first step to rewrite code is to understand it on 100%. You can't rewrite the code correctly if you don't fully understand the logic. The most important point here that even 5 lines of code can be super tricky to understand and to refactor. So always take your time.

While trying to understand code I always start with global picture without deep dive into specific implementation and it doesn't matter if I'm working with a project or a function.

So here the global picture is that we have the left side which is used like a boolean and the right side which is executed when expression on the left returns true.

My recommendation is to avoid using such notation because it makes things difficult to read and programmers are not that used to see such notation.

So better way is simple to write if condition here.

if ((this.router.state['view']['data'] || {})['menuopen']) {
  this.router.navigateback()
}

As you can see it's already better because we splitted a single liner in several lines.

The next step would be to move this calculation to boolean without refactoring it. In this case we avoid reading and analysing the whole construction and we must read only variable name.

But for this we must understand what's going on inside. So actually we check here that menu is opened. So let's move it to variable.

const isMenuOpened = (this.router.state['view']['data'] || {})['menuopen']
if (isMenuOpened) {
  this.router.navigateback()
}

As you can see now we understand what we check without need to read code because the name of our variable is so clear. Prefix "is" helps to understand that it's boolean. MenuOpened is human readable to understand that menu is alread opened. It's not isOpen or isMenuOpen.

The next step is we improve the code that checks menu. Here we have lot's of unnecessary property selection and we also see that view data can see undefined or null. And as we want to get menuopen property we default it to empty object.

First of all let's remove this array notation.

const isMenuOpened = (this.router.state.view.data || {}).menuopen

This doesn't change anything but is easier to read. Now actually I don't like || {} notation because it makes code less readable and it's better to move it to additional property.

So I see 3 ways to solve it.

If you are writing Typescript or Javascript with babel where you can use elvis operator. Elvis operator is just a question mark which doesn't fail if property is undefined.

const isMenuOpened = this.router.state.view.data?.menuopen || false

So here I used elvis operator to say that data can be undefined and this line will simply return undefined then and not throw the error. As we want to get boolean back I wrote or false at the end so it will fallback to false.

This is the shortest code.

Second possibility is leave code like it was but just create additional property to make it readable..

const viewData = this.router.state.view.data || {}
const isMenuOpened = viewData.menuopen

And if you don't want to create additional variable you can just to inline ternary operator.

const isMenuOpened = this.router.state.view.data ? this.router.state.view.data.menuopen : false

This is okayish but a bit verbose because we duplicate our code a bit.

And here is bonus example if you use additional library for data manipulation like Lodash or Ramda.

const isMenuOpened = R.pathOr(false, ['state', 'view', 'data'], this.router)

As a first parameter we set a default value and then we have an array which is our path to get a property. In this case it will never fail at any step of object selection.

If you are interested I have course both about Ramda and Lodash libraries and I will link them in the description below.

So as you can see it's quite a lot of things how we can improve even a single line of code.

Code #2

function IsNumeric(sText) {
    var ValidChars = "0123456789.";
    var IsNumber = true;
    var Char;
    for (i = 0; i < sText.length && IsNumber == true; i++) {
        Char = sText.charAt(i);
        if (ValidChars.indexOf(Char) == -1) {
            IsNumber = false;
        }
    }
    return IsNumber;
}

Here is our second example. So somebody wrote a function IsNumeric which gets a string and returns true or false if it's a valid number which is written inside string. So here we have validChars string with all numbers and . So it looks like it should returns true for ints and floats.

Then we loop thorough our text and read every symbol with charAt function. Then we check if this symbol is inside our string of Valid chars.

One more problem that I see here that I won't give back the correct result if we type inside 3 dots for example.

So the first thought that should come to mind is if we are reinventing a wheel. Becuase isNumeric function sounds like something super basic which is already implemented in any language. So instead of improving this code it's better to think how we can solve it with standard functions of Javascript.

Actually inside Javascript we have a Number where inside we can throw whatever and it will try to convert it to valid number. What is also important is that it works with floats also. If the number is not valid then we get NaN.

So actually we can change the whole function is a single liner.

const isNumeric = text => !isNaN(Number(text))

So here we first tried to convert text to Number. We either get back a valid number or NaN. This is why I used here isNaN to check if we have back a NaN. Also we need exclamation mark to invert the logic.

As you can see we simplified our function a lot just by using native Javascript so always try to solve problems with existing functions.

Also if you want to improve your programming skill I have lots of full courses regarding different web technologies.