SitePoint Sponsor

User Tag List

Results 1 to 11 of 11
  1. #1
    SitePoint Zealot
    Join Date
    Jul 2011
    Posts
    199
    Mentioned
    0 Post(s)
    Tagged
    0 Thread(s)

    Refactor / Best Practice

    Hello,

    This is how my code looks now:

    Code:
    //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?


  2. #2
    Unobtrusively zen silver trophybronze trophy
    paul_wilkins's Avatar
    Join Date
    Jan 2007
    Location
    Christchurch, New Zealand
    Posts
    14,696
    Mentioned
    101 Post(s)
    Tagged
    4 Thread(s)
    Quote Originally Posted by rhgiant View Post
    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:

    Code javascript:
    //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.

    Code javascript:
    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.

    Code javascript:
    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.

    Code javascript:
    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:

    Code:
    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.

    Code javascript:
    year = process(vehicle);
    if (!year) {
        ...
    Programming Group Advisor
    Reference: JavaScript, Quirksmode Validate: HTML Validation, JSLint
    Car is to Carpet as Java is to JavaScript

  3. #3
    SitePoint Zealot
    Join Date
    Jul 2011
    Posts
    199
    Mentioned
    0 Post(s)
    Tagged
    0 Thread(s)
    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

  4. #4
    SitePoint Zealot
    Join Date
    Jul 2011
    Posts
    199
    Mentioned
    0 Post(s)
    Tagged
    0 Thread(s)
    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.

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


  5. #5
    Unobtrusively zen silver trophybronze trophy
    paul_wilkins's Avatar
    Join Date
    Jan 2007
    Location
    Christchurch, New Zealand
    Posts
    14,696
    Mentioned
    101 Post(s)
    Tagged
    4 Thread(s)
    Quote Originally Posted by rhgiant View Post
    I have taken an example with shelves and books: only if we find shelves will we look at books.

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

    Code javascript:
    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.
    Programming Group Advisor
    Reference: JavaScript, Quirksmode Validate: HTML Validation, JSLint
    Car is to Carpet as Java is to JavaScript

  6. #6
    SitePoint Zealot
    Join Date
    Jul 2011
    Posts
    199
    Mentioned
    0 Post(s)
    Tagged
    0 Thread(s)
    Quote Originally Posted by paul_wilkins View Post
    Given that sort of situation, you can achieve the same task with only one line, using the OR operator.

    Code javascript:
    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:

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

  7. #7
    Unobtrusively zen silver trophybronze trophy
    paul_wilkins's Avatar
    Join Date
    Jan 2007
    Location
    Christchurch, New Zealand
    Posts
    14,696
    Mentioned
    101 Post(s)
    Tagged
    4 Thread(s)
    Quote Originally Posted by rhgiant View Post
    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:

    Code javascript:
    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:

    Code javascript:
    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.

    Code javascript:
    function doStuffWith(item) {
        var color = item.getColor();
        item.doSpecificStuff();
    };
    Programming Group Advisor
    Reference: JavaScript, Quirksmode Validate: HTML Validation, JSLint
    Car is to Carpet as Java is to JavaScript

  8. #8
    SitePoint Zealot
    Join Date
    Jul 2011
    Posts
    199
    Mentioned
    0 Post(s)
    Tagged
    0 Thread(s)
    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.

  9. #9
    Unobtrusively zen silver trophybronze trophy
    paul_wilkins's Avatar
    Join Date
    Jan 2007
    Location
    Christchurch, New Zealand
    Posts
    14,696
    Mentioned
    101 Post(s)
    Tagged
    4 Thread(s)
    Quote Originally Posted by rhgiant View Post
    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:

    Code javascript:
     
    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.

    Code javascript:
    function doStuffWith(item) {
        item.doStuff();
    }
     
    doStuffWith(book);
    Programming Group Advisor
    Reference: JavaScript, Quirksmode Validate: HTML Validation, JSLint
    Car is to Carpet as Java is to JavaScript

  10. #10
    Unobtrusively zen silver trophybronze trophy
    paul_wilkins's Avatar
    Join Date
    Jan 2007
    Location
    Christchurch, New Zealand
    Posts
    14,696
    Mentioned
    101 Post(s)
    Tagged
    4 Thread(s)
    Quote Originally Posted by paul_wilkins View Post
    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.

    Code javascript:
    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();
    Programming Group Advisor
    Reference: JavaScript, Quirksmode Validate: HTML Validation, JSLint
    Car is to Carpet as Java is to JavaScript

  11. #11
    SitePoint Zealot
    Join Date
    Jul 2011
    Posts
    199
    Mentioned
    0 Post(s)
    Tagged
    0 Thread(s)
    Perfect! This thread made me make tremendous progress. Thank you so much Paul.


Bookmarks

Posting Permissions

  • You may not post new threads
  • You may not post replies
  • You may not post attachments
  • You may not edit your posts
  •