1
\$\begingroup\$

I have a code which checks if any of the 4 textfields on our server contains text. If even one of the four textboxes is not empty, the generated table won´t skip that row and will draw it, otherwise the row will be completely skipped and not shown in the table.

For that I declared the variables for the words and the Boolean, so I can later use it to hide the table row. What the code does is it checks all the possible combinations of the wordboxes and if even one on the wordboxes contains something, the checkRow variable will be true.

This "if" statement method that I use works, however it is very hard to follow and I think it could be better done. I have documented myself but all the alternatives involve jquery - which is not supported on our webclient. Does anyone have a suggestion how to better this code?

                var checkRow=false;
                var word1='{{WORD1}}';
                var word2 ='{{WORD2}}';
                var word3 ='{{WORD3}}';
                var word4 ='{{WORD4}}';
                if ((word1 !=''&& word2 !=''&& word3 !=''&& word4 != '') ||(word1 ==''&& word2 ==''&& word3 !='' && word4!='') || (word1 ==''&& word2 ==''&& word3 =='' && word4!='')
                || (word1 ==''&& word2 !=''&& word3 !='' && word4!='') || (word1 !=''&& word2 !=''&& word3 =='' && word4=='') || (word1 !=''&& word2 ==''&& word3 =='' && word4!='')
                || (word1 !=''&& word2 ==''&& word3 !='' && word4 =='') || (word1 ==''&& word2 !=''&& word3 !='' && word4=='') || (word1 ==''&& word2 !=''&& word3 =='' && word4!='')
                || (word1 !=''&& word2 ==''&& word3 =='' && word4 =='') || (word1 ==''&& word2 !=''&& word3 =='' && word4=='') || (word1 ==''&& word2 ==''&& word3 !='' && word4!='')) {
                    var checkRow= true;
                } else  
                var checkRow= false;
\$\endgroup\$
1
  • \$\begingroup\$ As we all want to make our code more efficient or improve it in one way or another, try to write a title that summarizes what your code does, not what you want to get out of a review. Please see How to get the best value out of Code Review - Asking Questions for guidance on writing good question titles. \$\endgroup\$
    – BCdotWEB
    Commented Mar 18, 2016 at 9:31

3 Answers 3

3
\$\begingroup\$

you could just say

var checkRow = false;
if(word1 != '' || word2 != '' || word3 != '' || word4 != '') {
    checkRow = true;

or you could concat the single words and check if the length is greater than 0


EDIT: since some people favored the string-concat solution: actually that was kind of a joke in the sense of https://en.wikipedia.org/wiki/Barometer_question. if the strings are not large it doesn't make much of a difference, but from a performance-point of view that would not be a good solution.

\$\endgroup\$
2
  • 1
    \$\begingroup\$ Yes, the first solution was so easy, I don´t know why I just went ahead and wrote that sausage code up there instead. As for the second suggestion: I love it, I am going to use that. Thanks a lot. \$\endgroup\$
    – Daria M
    Commented Mar 18, 2016 at 9:36
  • \$\begingroup\$ I would go for the concat solution :] \$\endgroup\$ Commented Mar 30, 2016 at 5:21
2
\$\begingroup\$

You can assign to checkRow directly:

var checkRow = !!(word1 + word2 + word3 + word4);

The !! casts to a boolean. It's not required since an empty string is falsy, but it clarifies the intent.

\$\endgroup\$
3
  • \$\begingroup\$ I like your take n it but I do not know how to use it. Later in my code where I need to hide the row do i simply say if(checkRow==false) --> do something ? I can use is this var as a normal boolean ? \$\endgroup\$
    – Daria M
    Commented Mar 18, 2016 at 15:16
  • \$\begingroup\$ Hi @DariaM... The var is a normal boolean, so yes, you can use it as a normal boolean. Basically, you shouldn't need to change any of the code that uses the var. It should work without modifications. \$\endgroup\$
    – tiffon
    Commented Mar 20, 2016 at 18:47
  • \$\begingroup\$ Ok, thanks for the tip, will def try it. I learnt something new. Thank you \$\endgroup\$
    – Daria M
    Commented Mar 21, 2016 at 7:52
1
\$\begingroup\$
if ((1!&& 2!&& 3!&& 4!) ||(1&& 2&& 3! && 4!) || (1&& 2&& 3 && 4!)
|| (1&& 2!&& 3! && 4!) || (1!&& 2!&& 3 && 4) || (1!&& 2&& 3 && 4!)
|| (1!&& 2&& 3! && 4) || (1&& 2!&& 3! && 4) || (1&& 2!&& 3 && 4!)
|| (1!&& 2&& 3 && 4) || (1&& 2!&& 3 && 4) || (1&& 2&& 3! && 4!)) {

since they're all in order of "word1, word2, word3, word4", lets reduce them to bits

0000 1100 1110
1000 0011 0110
0101 1001 1010
0111 1011 1100

All cases for 4 bits are this:

0000 0001 0010
0011 0100 0101
0110 0111 1000
1001 1010 1011
1100 1101 1110
1111

So at least 4 cases aren't covered, because you've got 12 cases and the total amount of cases you can have is 16.

Let's cross off the matched cases...

1100 (duplicate?)

0001 0010 0100 1101 1111

You could shorten in down to those 5 cases if you wanted to maintain original functionality.

We can cut it down further; 1 AND 2 AND 4 will be able to replace 1101 and 1111. 00 + (3 XOR 4) will be able to replace 0001 and 0010.

So, to preserve original functionality:

if (
  (word1 !=''&& word2 !=''&& ((word3 == '') != (word4 == ''))) //00 3 XOR 4
||(word1 !=''&& word2 ==''&& word3 !='' && word4!='') //0100
|| (word1 ==''&& word2 ==''&& word4==''))//11 3 XOR 4
)

Will do the trick.

If that's not what you wanted; such are the dangers of writing out every case; you haven't got them all until you've written down (where the amount of conditions you're checking is \$n\$) \$2^n -1\$ cases. So 1, 3, 7, 15, 31... etc. (The last one is caught via the else).


I guess this is my way of saying that your code as is contains bugs for the cases

'' '' '' 'bug'
'' '' 'bug' ''
'' 'bug' '' ''
'bug' 'bug' '' 'bug'
\$\endgroup\$
0

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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