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
—
2012-01-30T23:06:22Z —
#2

rhgiant said:

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.

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.

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
—
2012-01-31T06:02:18Z —
#3

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
—
2012-01-31T19:50:50Z —
#4

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
—
2012-01-31T21:57:25Z —
#5

rhgiant said:

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

`javascript

// 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

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
—
2012-02-02T04:18:30Z —
#10

paul_wilkins said:

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.