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) {
...
