0
\$\begingroup\$

Throughout my code, I find myself testing if the output of a function is undefined or blank before i use the output. In this case, getQuery checks if the property of an object is either blank or if the object key is not set at all. In both cases, the output can be ignored. But if the object key holds a value, then I want to use this output as the value for my variable.

var filter = ""
if ( typeof getQuery("filter") != 'undefined' && getQuery("filter") != "" ) filter = getQuery("filter")
if ( filter != "" ) createFilterView( filter )

There must be a more optimized way of doing this without the repetition?

I often set the output of the function to a variable first, in order to not have to perform the function several times. A bit faster, I assume, but also not very elegant.

var getQueryFilter_output = getQuery("filter")
if ( getQueryFilter_output != 'undefined' && getQueryFilter_output != "" ) var filter = getQuery_output

What would be a better way of doing this?

Improve this question
\$\endgroup\$
6
  • \$\begingroup\$ I edited your question slightly, because asking for the best or the most [x] seems primarily opinion-based, which would be off-topic \$\endgroup\$ – tim Aug 21 '14 at 8:53
  • \$\begingroup\$ also: 1) sometimes, you call foo("filter") and sometimes foo(). I think it should either be one or the other? 2) some context would help answer this question. For example: what do you do if the output is ""? As is, this question is somewhat off-topic because it contains pseudo-code or example code instead of real code. \$\endgroup\$ – tim Aug 21 '14 at 9:07
  • \$\begingroup\$ I Definitely wouldn't run functions needlessly; so yes, store the return value in a variable. Also, I'd say you should make sure your functions never return more error/failure values than you are interested in differentiating between. For example, if the distinction between "" or 'undefined' is irrelevant then you could change the function to only ever return "" on failure. That way you only have to do a if (foo != "") check. \$\endgroup\$ – Will Aug 21 '14 at 9:56
  • \$\begingroup\$ @tim: You're right - I messed up when "translating" my actual function and variable names "foo"-ish example names. Reinstated the original names and corrected it now. \$\endgroup\$ – Skarven Aug 21 '14 at 10:39
  • \$\begingroup\$ @Skarven this is a lot better. But I would still like to see some context. Why does getQuery sometimes return undefined and sometimes ""? And what happens if it does? Do you exit, or do you use the value filter had before this line? \$\endgroup\$ – tim Aug 21 '14 at 11:05
2
\$\begingroup\$

Indentation

I'm not quite sure if this is correct, but from what I know, I believe it is.

Indentation in JavaScript does not matter as, before your code is run, it is completely minimized - meaning it removes all comments and unnecessary spaces.

That being said, trying to crush your JavaScript into three lines does not make it any faster.

Calling

In your first example, you call the same function three times with the same arguments each time. This is extremely inefficient.

It would be optimal to call it once and save it in a variable.

Therefore, these lines:

var filter = ""
if ( typeof getQuery("filter") != 'undefined' && getQuery("filter") != "" ) filter = getQuery("filter")
if ( filter != "" ) createFilterView( filter )

Can become:

var filter = "";
var ret = getQuery("filter"); // for lack of a better variable name
if(typeof ret != 'undefined' && ret != "") filter = ret;
if(filter != "") createFilterView(filter)

(Yes, you fixed this in the second example)

Conditionals 1

In your first example, you check to see that the return value of getQuery isn't "undefined" and that it isn't "". If that passed, you take the return value and place it in a variable. Then, you go through another conditional to make sure that that variable isn't empty.

Well, since in your first conditional you already check to make sure that the return value isn't empty, why don't you put the body of the second conditional into the first one.

This:

if ( typeof getQuery("filter") != 'undefined' && getQuery("filter") != "" ) filter = getQuery("filter")
if ( filter != "" ) createFilterView( filter )

Becomes:

if(typeof getQuery("filter") != 'undefined' && getQuery("filter") != "") {
    filter = getQuery("filter")
    createFilterView(filter)
}

Conditionals 2

In the conditional statements in both of your examples, you check that the return value of getQuery is not "". You don't have to specifically specify that.

You can just remove the != "" part and it would work the same way.

The further reduce the size of your conditional, you can have your getQuery function actually return the value undefined (null would be better, however) instead of it returning a string of the word "undefined".

Then, you could reduce your conditional to:

if(getQuery("filter"))

Semicolons

In JavaScript, it is a good practice to close statements with a semicolon.

Example:

createFilterView(filter)

Should be:

createFilterView(filter);
                        ^
                        |
                    semicolon

Quotes

This isn't really important, but I just thought I'd point it out.

You are consistent in your code with your quotes. For some strings, you use single quotes and for others, you use double quotes.

It is best to stick to one.

Improve this answer
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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