Refactor / Best Practice

[rhgiant]

Hello,

This is how my code looks now:

//var car already retrieved
//var bus already retrieved
if ( typeof(car) != "undefined" ) {
	
	var year = process(car);
	if( year === false ) {
		//do something
	} else {
		//do something else
	}	
	
}else{
	
	if (typeof( bus ) != "undefined") {

		var year = process(bus);
		if (year === false) {
			//do something
		} else {
			//something else	
		}
	
	} else {
		//do something
	}

}

I know it sucks. It could be wrapped in a function, it could be an object, it could many things that it is not. It's working, but it's lacking elegancy, and completely missing the point good-practice-wise.

I would like to learn how such a simple snippet could be improved.

a) I look if a car variable.
b) if it doesn exist, I look for a bus variable (in the future I will look at other types of vehicles).

Can someone help me refactor this, put on the path of well-written code?

[paul_wilkins]

Can someone help me refactor this, put on the path of well-written code?

Sure thing. First we'll run the code through jslint.com to deal with existing obvious issues, such as declaring variables, and using !== in preference to !=, and typeof not being a function, and spacing.

JavaScript doesn't have block scope, so declaring a variable inside of an if statement does not restrict that variable to within that area. There is only function scope. As such, all variables should be declared at the start of the function in one comma separated var statement, so that you do not run the risk of misunderstanding what is declared and where.

That gives us a starting point of:

//var car already retrieved
//var bus already retrieved
var year;
if (typeof (car) !== 'undefined') {
    year = process(car);
    if (year === false) {
        //do something
    } else {
        //do something else
    }
} else {
    if (typeof bus !== 'undefined') {
        year = process(bus);
        if (year === false) {
            //do something
        } else {
            //something else	
        }
    } else {
        //do something
    }
}

The first step here is to reduce the duplication. We have car and bus, and you say that other types of vehicles will be used too, so we can put those in a single array and loop through them.

Assuming that the only falsy value that the process function returns is false, we can just use (!year) as the condition instead.

It also seems from your code that only one vehicle will be acted on at any one time. So if both a car and a bus are both defined, that only the car will be acted on. For the bus to be acted on there must not be a car involved in things.

So assuming that you only want one thing dealt with, we can break out of he loop once it finds a vehicle that can be processed. We can also add a flag to help us determine if nothing has been processed. Typically such flags are best prefixed with has or is, such as hasApples or isFixed, which helps you to understand that they contain a boolean value.

var vehicles = [car, bus],
    i,
    vehicle,
    year,
    hasProcessed = false;
for (i = 0; i < vehicles.length; i += 1) {
    vehicle = vehicles[i];
    if (typeof vehicle !== 'undefined') {
        year = process(vehicle);
        if (!year) {
            //do something
        } else {
            //do something else
        }
        hasProcessed = true;
        break;
    }
}
if (!hasProcessed) {
    // no vehicles were found, so do something
}

Taking the other assumption that you want all vehicles to be processed, we can just remove the break statement.

var vehicles = [car, bus],
    i,
    vehicle,
    year,
    hasProcessed = false;
for (i = 0; i < vehicles.length; i += 1) {
    vehicle = vehicles[i];
    if (typeof vehicle !== 'undefined') {
        year = process(vehicle);
        if (!year) {
            //do something
        } else {
            //do something else
        }
        hasProcessed = true;
    }
}
if (!hasProcessed) {
    // no vehicles were found, so do something
}

Lastly, we can place the code in a function, so that the vehicles can be passed to it.

function processVehicles(vehicles) {
    var i,
        vehicle,
        year,
        hasProcessed = false;
    for (i = 0; i < vehicles.length; i += 1) {
        vehicle = vehicles[i];
        if (typeof vehicle !== 'undefined') {
            year = process(vehicle);
            if (!year) {
                //do something
            } else {
                //do something else
            }
            hasProcessed = true;
        }
    }
    if (!hasProcessed) {
        // no vehicles were found, so do something
    }
}
processVehicles([car, bus]);

It's also tempting to remove the year and place the process part in the condition itself:

if (process(vehicle)) {
    ...

But it's not a good idea to place a function that also does something inside of a condition. Leaving it assigned to the year and then checking the year afterwards is a good compromise here.

year = process(vehicle);
if (!year) {
    ...

[rhgiant]

That's wonderful Paul. I feel like I'm making giant steps (at my own small level )

Will work on that, process your post, and be back

[rhgiant]

Ok, another pattern I have noticed occurs quite often is that one:

->we check for a variable
->if that variable is undefined, trigger another scenario

It's pretty much like cars/buses, except that cars and buses were on the same level (and logically, in the same array).

I have taken an example with shelves and books: only if we find shelves will we look at books.

// we're looking for a color, we don't care if it comes from a shelf or a book
// all we know is that we want the color of a shelf first
// and ONLY if a shelf hasn't been found will we dig deeper and look for books
 
// have we found a shelf?
if ( typeof(shelf) != "undefined" ) {
	color = findColor(shelf);	
} else {
//no shelf, look for a book	
        //have we found a book?
	if (typeof( book ) != "undefined") {
		color = findColor(book);
        //nothing, no color available
	} else {
		color = false;
	}
}

I know it sounds dumb as it's a purely theoretical example, but it's less abstract than my actual code.

[paul_wilkins]

I have taken an example with shelves and books: only if we find shelves will we look at books.

// we're looking for a color, we don't care if it comes from a shelf or a book
// all we know is that we want the color of a shelf first
// and ONLY if a shelf hasn't been found will we dig deeper and look for books
 
// have we found a shelf?
if ( typeof(shelf) != "undefined" ) {
	color = findColor(shelf);	
} else {
//no shelf, look for a book	
        //have we found a book?
	if (typeof( book ) != "undefined") {
		color = findColor(book);
        //nothing, no color available
	} else {
		color = false;
	}
}

Given that sort of situation, you can achieve the same task with only one line, using the OR operator.

color = findColor(shelf) || findColor(book);

That way, if a non falsy value is found for the shelf, that will be used. However if it's a falsy value, the color of the book is looked for instead.

[rhgiant]

Given that sort of situation, you can achieve the same task with only one line, using the OR operator.

color = findColor(shelf) || findColor(book);

That way, if a non falsy value is found for the shelf, that will be used. However if it's a falsy value, the color of the book is looked for instead.

Very interesting and clever.
What if I need to do something specific if shelf is found, and something specific in the case it's a book?

Could there be an approach such as:

color = findColor(shelf)(function specificToShelves(){...})() || findColor(book)(function specificToBooks(){...})();

[paul_wilkins]

What if I need to do something specific if shelf is found, and something specific in the case it's a book?

Could there be an approach such as:

color = findColor(shelf)(function specificToShelves(){...})() || findColor(book)(function specificToBooks(){...})();

That gets messy quite quickly.

There's no reason though why you can't do something like this though:

color = findColor(shelf) || findColor(book);
if (shelf) {
    ...
}
if (book) {
    ...
}

Although, it would be simpler if the objects contained a consistent interface, so that you could then work with them without needing to know specifics about them.

function doStuffWith(item) {
    var color = item.getColor();
    item.doSpecificStuff();
};

[rhgiant]

Oh, sounds like a great approach.

How should an object look to follow that path?

Once again, I really want to thank you for your time and patience with me.

[paul_wilkins]

Oh, sounds like a great approach.

How should an object look to follow that path?

Well, you could do it using an old-style technique such as:

 
var Book = function () {
    this.doSpecificStuff = function () {
        // ...
    };
    this.doStuff = function () {
        var color = this.color;
        this.doSpecificStuff();
        // plus more stuff
    };
};
var book = new Book();
book.title = 'The Wind In The Willows';
book.format = 'hardcover';
book.isbn = '978-0805002133';
book.color = 'red';
book.pages = 634;
book.bookmark = 300;
book.rating = 8;

That way, when the book is passed to a function, the function can do stuff with it, without needing to know the specific details.

function doStuffWith(item) {
    item.doStuff();
}
 
doStuffWith(book);

[paul_wilkins]

Well, you could do it using an old-style technique

That comment leads to the question of what are newer ways to do this, such as by using ES5 techniques.

What I'd like to ask of other people though is if there are cleaner ways without using the new constructor.
A quick test resulted in the following, which requires compatibility code from Object.create on older web browsers.

var Book = function () {};
Book.prototype.doSomething = function () {
    alert('Book color is ' + this.color);
};
Book.prototype.doStuff = function () {
    this.doSomething();
    // plus more stuff
};
var book = Object.create(Book.prototype, {
    title: {value: 'The Wind In The Willows'},
    format: {value: 'hardcover'},
    isbn: {value: '978-0805002133'},
    color: {value: 'red'},
    pages: {value: 634},
    bookmark: {value: 300},
    rating: {value: 7}
});
 
book.doStuff();

[rhgiant]

Perfect! This thread made me make tremendous progress. Thank you so much Paul.