Useless if statements, and when true really means false in javascript



  • The following code is from Super Enterprisey Accounting Software Company. SEASC has their Flagship Application written to run on WPF. While trying to get it to run in Firefox under WPF (which, so far, it won't, even though I have the right version of .NET installed), I found the following javascript. I haven't written JS in a long time, but I'm pretty sure that appName isn't going to change here before it's tested.

        Initialize();
    
    appName = "Application 6.1";
    if (appName.indexOf("Other_App_Gov") >= 0) brandingImage.src="images/oagbranding.png" mce_src="images/oagbranding.png";
    if (appName.indexOf("Other_App") >= 0) brandingImage.src="images/oabranding.png" mce_src="images/oabranding.png"
    if (appName.indexOf("Application") >= 0) brandingImage.src="images/applicationbranding.png" mce_src="images/applicationbranding.png"
    brandingImage.style.visibility = "inherit";
    
    openTimeout = window.setTimeout("openVision()", 3000)
    

    Moving on to when true really means false; this is more of an annoyance of mine about how people name functions than a real WTF, but when you name something "HasRuntimeVersion", I expect that when it returns boolean true that it HAS what you are looking for, not the other way around. Testing for !HasRuntimeVersion would have made this readable instead of confusing</rant> I also haven't yet figured out why GetVersion is matching numbers case-insensitive.

        runtimeVersion = "3.5.30729";
        fwNotInstalled = true;
        function Initialize() {
            if (HasRuntimeVersion(runtimeVersion)) {
                fwNotInstalled = false;
                hiddenLink.href = directLink;
            }
        }
        function HasRuntimeVersion(v) {
            var va = GetVersion(v);
            var i;
            var a = navigator.userAgent.match(/\.NET CLR [0-9.]+/g);
            if (a != null)
                for (i = 0; i < a.length; ++i)
                if (CompareVersions(va, GetVersion(a[i])) <= 0)
                return true;
            return false;
        }
        function GetVersion(v) {
            var a = v.match(/([0-9]+)\.([0-9]+)\.([0-9]+)/i);
            return a.slice(1);
        }
        function CompareVersions(v1, v2) {
            for (i = 0; i < v1.length; ++i) {
                var n1 = new Number(v1[i]);
                var n2 = new Number(v2[i]);
                if (n1 < n2)
                    return -1;
                if (n1 > n2)
                    return 1;
            }
            return 0;
        }
    


  • Check the code again: <font face="courier new,courier">HasRuntimeVersion()</font> returns <font face="courier new,courier">true</font> if <font face="courier new,courier">navigator.userAgent.match(/.NET CLR [0-9.]+/g)</font> contains a version => expected version; in which case <font face="courier new,courier">fwNotInstalled</font> is set to <font face="courier new,courier">false</font>, because it is installed.



  • @Zecc said:

    Check the code again: <font face="courier new,courier">HasRuntimeVersion()</font> returns <font face="courier new,courier">true</font> if <font face="courier new,courier">navigator.userAgent.match(/.NET CLR [0-9.]+/g)</font> contains a version => expected version; in which case <font face="courier new,courier">fwNotInstalled</font> is set to <font face="courier new,courier">false</font>, because it is installed.

    It does demonstrate how naming your boolean "Not(something" is confusing as hell.

    (Glad you posted, I thought the code looked correct too, but I wasn't willing to put my brains on the line... heh.)



  • @Zecc said:

    Check the code again: <FONT face="courier new,courier">HasRuntimeVersion()</FONT> returns <FONT face="courier new,courier">true</FONT> if <FONT face="courier new,courier">navigator.userAgent.match(/\.NET CLR [0-9.]+/g)</FONT> contains a version => expected version; in which case <FONT face="courier new,courier">fwNotInstalled</FONT> is set to <FONT face="courier new,courier">false</FONT>, because it is installed.

    *FACEPALM*

     



  • @rad131304 said:

    The following code is from Super Enterprisey Accounting Software Company. SEASC has their Flagship Application written to run on WPF. While trying to get it to run in Firefox under WPF (which, so far, it won't, even though I have the right version of .NET installed), I found the following javascript. I haven't written JS in a long time, but I'm pretty sure that appName isn't going to change here before it's tested.

        Initialize();
        
        appName = "Application 6.1";
        if (appName.indexOf("Other_App_Gov") >= 0) brandingImage.src="images/oagbranding.png" mce_src="images/oagbranding.png";
        if (appName.indexOf("Other_App") >= 0) brandingImage.src="images/oabranding.png" mce_src="images/oabranding.png"
        if (appName.indexOf("Application") >= 0) brandingImage.src="images/applicationbranding.png" mce_src="images/applicationbranding.png"
        brandingImage.style.visibility = "inherit";
        
        openTimeout = window.setTimeout("openVision()", 3000)
    




    I'd wager appName is written dynamically server-side... so, depending on which URL you access the app from (or whatever), you get a different image...



  • @rad131304 said:

    @Zecc said:

    Check the code again: <font face="courier new,courier">HasRuntimeVersion()</font> returns <font face="courier new,courier">true</font> if <font face="courier new,courier">navigator.userAgent.match(/\.NET CLR [0-9.]+/g)</font> contains a version => expected version; in which case <font face="courier new,courier">fwNotInstalled</font> is set to <font face="courier new,courier">false</font>, because it is installed.

    *FACEPALM*

    In your defense, the code is smelly.

    <font face="courier new,courier">GetVersion()</font> returns an array with the version components, skipping the first one. That's... weird.

    What is <font face="courier new,courier">fwNotInstalled</font> used for?

     



  • @Zecc said:

    <font face="courier new,courier">GetVersion()</font> returns an array with the version components, skipping the first one. That's... weird.
     

    a[0] would have contained "1.2.3" (as in what the entire regexp matched) before a.slice(1). Of course passing a string with only one dot to GetVersion would return null.



  • @rad131304 said:


    Initialize();

    appName = "Application 6.1";
    if (appName.indexOf("Other_App_Gov") &gt;= 0) <span style="color:red;">{</span> brandingImage.src="images/oagbranding.png"<span style="color:red;">;</span> mce_src="images/oagbranding.png"; <span style="color:red;">}</span>
    <span style="color:red;">else</span> if (appName.indexOf("Other_App") &gt;= 0) <span style="color:red;">{</span> brandingImage.src="images/oabranding.png"<span style="color:red;">;</span> mce_src="images/oabranding.png"<span style="color:red;">; }</span>
    <span style="color:red;">else</span> if (appName.indexOf("Application") &gt;= 0) <span style="color:red;">{</span> brandingImage.src="images/applicationbranding.png"<span style="color:red;">;</span> mce_src="images/applicationbranding.png"<span style="color:red;">; }</span>
    brandingImage.style.visibility = "inherit";
    
    openTimeout = window.setTimeout("openVision()", 3000)<span style="color:red;">;</span>
    

    FTFY

    If appName always has the value above, you would never know that there is something wrong with those if statements.



  • @Zecc said:

    @rad131304 said:

    @Zecc said:

    Check the code again: <FONT face="courier new,courier">HasRuntimeVersion()</FONT> returns <FONT face="courier new,courier">true</FONT> if <FONT face="courier new,courier">navigator.userAgent.match(/\.NET CLR [0-9.]+/g)</FONT> contains a version => expected version; in which case <FONT face="courier new,courier">fwNotInstalled</FONT> is set to <FONT face="courier new,courier">false</FONT>, because it is installed.

    *FACEPALM*

    In your defense, the code is smelly.

    <FONT face="courier new,courier">GetVersion()</FONT> returns an array with the version components, skipping the first one. That's... weird.

    What is <FONT face="courier new,courier">fwNotInstalled</FONT> used for?

    It throws an error indicating you need .NET 3.5 SP1 or higher if the test fails - I think what it's really looking for is a base level for WPF, but that's more of a SWAG than anything else.




  •     appName = "Application 6.1";
    if (appName.indexOf("Other_App_Gov") >= 0) brandingImage.src="images/oagbranding.png";
    if (appName.indexOf("Other_App") >= 0) brandingImage.src="images/oabranding.png"
    if (appName.indexOf("Application") >= 0) brandingImage.src="images/applicationbranding.png"
    brandingImage.style.visibility = "inherit";

    openTimeout = window.setTimeout("openVision()", 3000)
     
    We have a number of cases where the server generates Javascript and ends up with code like this.  
    appName would change based on some server-side prefs, but the rest is always there because it's 
    easier to write those if statements in javascript than a javascript-writing nVelocity template.
    Still a WTF, mind you, but just trying to shed some light on how it could have possibly gotten there. 

Log in to reply