current community

your communities

more stack exchange communities

company blog
Stack Exchange Inbox Reputation and Badges
tour help
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. Join them; it only takes a minute:

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top
up vote 1 down vote favorite

This is what my viewObject looks like:

{
    description: "GOOG AAPL TSLA"
    id: 42
    ticker_1: "GOOG"
    ticker_2: "AAPL"
    ticker_3: "TSLA"
    timespan: "1mo"
}

Here is the function use to check the object. In a very dirty fashion, it creates a string to be used to replace the URL with:

function createUrl(viewObj) {
    var urlString  = '',
        ticker1    = '',
        ticker2    = '',
        ticker3    = '',
        time       = '';

    if (viewObj.ticker_1) {
        ticker1 = '&ticker_1='+viewObj.ticker_1;
        ulrString = ulrString+ticker1;
    }
    if (viewObj.ticker_2) {
        ticker2 = '&ticker_2='+viewObj.ticker_2;
        ulrString = ulrString+ticker2;
    }
    if (viewObj.ticker_3) {
        ticker3 = '&ticker_3='+viewObj.ticker_3;
        ulrString = ulrString+ticker3;
    }

    if (viewObj.timespan) {
        time = '&timespan='+viewObj.timespan;
        ulrString = ulrString+time;
    }

    console.log('/dashboard'+ulrString);
    return '/dashboard'+ulrString;
}

I have multiple if/else statements and code that can be repeated. How would you accomplish this?

share|improve this question
    
Have you taken a look at stackoverflow.com/questions/316781/… ? – Roman Susi Feb 13 at 19:12
    
Thanks for the link, however that solution is definitely not what I want... hmm I will see if there is a lodash solution for this – Leon Gaban Feb 13 at 19:26
1  
Is this in a Node environment or in the browser? – Dan Pantry Feb 13 at 19:36
    
Browser, this is a small function in my Angular app – Leon Gaban Feb 13 at 19:37
1  
Any reason you aren't using the params options in $http for this? $http.get('/dashboard', { ticker_1: ticker_1 }) would resolve to /dashboard?ticker_1=foo if ticker_1 is foo or just /dashboard otherwise. It's also a service - $httpParamsSerializer – Dan Pantry Feb 13 at 19:39
up vote 2 down vote accepted

You can try this:

function createUrl(viewObj) {
    var urlParams = ['ticker_1', 'ticker_2', 'ticker_3', 'timespan'],
        urlString = '',
        current;
    for (var i = 0; i < urlParams.length; i++) {
        current = urlParams[i];
        if (viewObj[current]) urlString += '&' + current + '=' + viewObj[current];
    }
    console.log('/dashboard' + urlString);
    return '/dashboard' + urlString;
}

The algorithm explained:

As the treatment you need for each of the URL parameters is the same, it's enough with storing all of them in an array (urlParams) and then iterating over each that array.

At each iteration, current will be one of them. E.g.: at the first iteration, current will be 'ticker_1'. That way,

if (viewObj[current])

replaces

if (viewObj.ticker_1)

Besides, it's not necessary to store the new string to a variable (as you do with ticker1, ticker2, etc.), before appending it to urlString, so I just removed those lines.

Finally, for string concatenation, the += operator can be used, so

urlString = urlString + something;

can be replaced with

urlString += something;
share|improve this answer
    
Thanks! I love how clean and simple this is, I get it too... I know what params to look for, I set current to the next param it finds, if it finds it add it to the string. The loop does however has to run for each possible param, I wonder if there is a way to optimize that? Maybe a _.find or something with lodash? – Leon Gaban Feb 13 at 20:01
up vote 3 down vote

Do you mean write code that would repeat the multiple if/else statements? cause that would be the right idea and make your code more DRY.

The first thing to take a look at is your data structure. As it is, you have hard-coded a handful of ticker properties that would make iteration difficult. You could use a for/in loop to go through the properties of your object, but we know we don't want the description or id in the string. Better to put all tickers in an array. That way it'll be easy to create a string with as many (or little) tickers as you want.

{
    description: "GOOG AAPL TSLA"
    id: 42
    tickers: ["GOOG", "AAPL", "TSLA"]
    timespan: "1mo"
}

It's now easy to iterate over the tickers and construct a string out of them. Since we always start with ticker_<n> as the beginning, we can construct that inside our loop, using the index to count which ticker # we are on.

For the function:

function createUrl(viewObject){
    // use .map to add "ticker_<n>=<value>" to each of the tickers,
    // then join with "&" for the query.
    var urlString = viewObject.tickers
        .map(function(ticker, index){
             // prepend the first query with its own '&'
            var and = index === 0 ? "&" : "";

            //create a 1-indexed count of the tickers.
            var tickerNum = index + 1; 
            return and + "ticker_" + tickerNum + "=" + ticker;
        })
        .join('&'); //<--separate each query with "&"

    // for timespan, you don't need to make a seperate `time` variable,
    // this can be done in one line.
    if(viewObject.timespan) urlString += "&timespan=" + viewObject.timespan;

    console.log('/dashboard'+urlString);
    return '/dashboard'+urlString;
}
share|improve this answer
    
Welcome to Code Review! Good job on your first answer. – SirPython Feb 14 at 2:45

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.