When granularity goes too far



  • I've found some "functions" in my project that went a little too far, IMO, to try and granularize common functions:


    function checkForQuoteSymbol(string) {
      return (string.indexOf("'") != -1);
    }

    function checkForSpace(string) {
      return (string.indexOf(" ") != -1);
    }

    function roundNumber(value, opt_precision) {
      var precision = (opt_precision != null) ? parseInt(opt_precision) : 0;

      return Number(value).toFixed(precision);
    }
    function isVowel(ch){
        var s = "" + ch;
        ch = s.toLowerCase()[0];
        return ch == 'a'
            || ch == 'e'
            || ch == 'i'
            || ch == 'o'
            || ch == 'u';
    }


    They didn't account for "and sometimes Y"



  • [quote user="treefrog"]

    They didn't account for "and sometimes Y"

    [/quote]

    function isMostlyVowel(ch){ ... } 



  • boolean isMostlyWTF(...)  {...}



  • I don't think isVowel() is too bad, at least for someone who apparently doesn't know that JavaScript has regular expressions. ;-)



  • checkForQuoteSymbol and checkForSpace are at least somewhat excusable on the grounds of readability.

     



  • [quote user="iwpg"]I don't think isVowel() is too bad, at least for someone who apparently doesn't know that JavaScript has regular expressions. ;-)[/quote]

    Even without regular expressions, how about:

    1. if ("aeiouAEIOU".indexOf(ch) > -1) { // is vowel }

    2. or if you are willing to trade a little space for zippier performance, how about a singleton Hashtable, pre loaded with Character's: a, e, i, o, u, A, E, I, O and U:

        if (theVowelHash.get(ch) != null) { // is vowel }

     



  • [quote user="emurphy"]checkForQuoteSymbol and checkForSpace are at least somewhat excusable on the grounds of readability.[/quote]

    True - you know what the person is doing when calling the function... 



  • [quote user="emurphy"]checkForQuoteSymbol and checkForSpace are at least somewhat excusable on the grounds of readability.[/quote]I'd've thunk the correct thing to do would be:

    function stringContains (haystack, needle) {
        return haystack.indexOf(needle) >= 0;
    }

    Otherwise, you'll end up copy-and-paste-ing the function for each new character you want to check for. Also, it's more specific -- is a "QuoteSymbol" a ['] or a ["]?



  • [quote user="snoofle"]

    [quote user="iwpg"]I don't think isVowel() is too bad, at least for someone who apparently doesn't know that JavaScript has regular expressions. ;-)[/quote]

    Even without regular expressions, how about:

    1. if ("aeiouAEIOU".indexOf(ch) > -1) { // is vowel }

    2. or if you are willing to trade a little space for zippier performance, how about a singleton Hashtable, pre loaded with Character's: a, e, i, o, u, A, E, I, O and U:

        if (theVowelHash.get(ch) != null) { // is vowel }

     

    [/quote]

    So much for internationalization.

     

     



  • [quote user="danielpitts"]So much for internationalization.[/quote]

    That's a good point, but is there any i18n-safe way to check whether something's a vowel without hardcoding a list of vowels for each language you want to support (not sure if even that would count)?  Bonus points if it works in JavaScript, since that's the language used here, but I'd be interested in ways to do it in other languages too.  I didn't find anything with a quick Google, but I'm not sure what kind of search terms to use for something like this.



  • Why do you think these went too far?  These functions clearly communicate their intent in a non-abstract manner, although I would prefer a "Contains" function to replace the first two.



  • I think there comes a point when creating a wrapper function for every feasible usage of the indexOf command or replacing the Math.round and/or toFixed methods is just too much. It's a waste of time and it doesn't really add any value. 

    If I'm reading the code and I see 'num.toFixed(2)', I'm pretty certain I know what's going to happen.  Is it really necessary to type roundNumber(num,2)?  If another developer has to inherit the code (which is where I'm at), and there's an error with that function, I'm going to have to hunt it down and figure out what's going on.  If the intent is to round to the specified digits - just do it. 

    If I'm checking for an instance of a space or an apostrophe, why not just use the builtin indexOf method inherent in the string?  Do I really need to pollute the namespace, add overhead, and allocate memory for a function that's doing what a built-in method already provides?

    My best comparison would be buying a brand new car - but installing a hand-crank starter rather than turning the key in the ignition.  It would be pretty obvious that I was starting the car whenever I used the hand-crank - but it would be kind of dumb when there's a perfectly good built-in starting method already...

     



  • thunk???

    isn't the word - thought.
     



  • [quote user="treefrog"]I think there comes a point when creating a wrapper function for every feasible usage of the indexOf command or replacing the Math.round and/or toFixed methods is just too much. It's a waste of time and it doesn't really add any value.

    If I'm reading the code and I see 'num.toFixed(2)', I'm pretty certain I know what's going to happen.  Is it really necessary to type roundNumber(num,2)?  If another developer has to inherit the code (which is where I'm at), and there's an error with that function, I'm going to have to hunt it down and figure out what's going on.  If the intent is to round to the specified digits - just do it.[/quote]
    It does something different than num.toFixed, though.  While the current function is not ideal, its problems do not arise from its granularity.  There are actually a few idioms encapsulated there, such as the type conversions and fallback values.  I would have made the method even more granular by encapsulating the type conversion with fallback to a default value idiom as a function as well.

    [quote user="treefrog"]If I'm checking for an instance of a space or an apostrophe, why not just use the builtin indexOf method inherent in the string?[/quote]
    if (foo.Contains("x")) is clearer than if (foo.indexOf("x") != -1).  Though not by much, of course.

    [quote user="treefrog"]Do I really need to pollute the namespace, add overhead, and allocate memory for a function that's doing what a built-in method already provides?[/quote]
    The overhead and memory here is trivial.  Namespace pollution is not a very convincing argument either, because JavaScript has some very flexible structuring mechanisms - you can even place your extension methods directly on the built-in classes you extend, which is great.  Contains, isVowel and roundNumber are clearly doing more than the built-in methods already provide.



  • [quote user="Chris F"][quote user="treefrog"]If I'm reading the code and I see 'num.toFixed(2)', I'm pretty certain I know what's going to happen.  Is it really necessary to type roundNumber(num,2)?  If another developer has to inherit the code (which is where I'm at), and there's an error with that function, I'm going to have to hunt it down and figure out what's going on.  If the intent is to round to the specified digits - just do it.[/quote]
    It does something different than num.toFixed, though.  While the current function is not ideal, its problems do not arise from its granularity.  There are actually a few idioms encapsulated there, such as the type conversions and fallback values.  I would have made the method even more granular by encapsulating the type conversion with fallback to a default value idiom as a function as well.[/quote]
    The additional work that's being done here such as the type conversion and the fallback are unnecessary if the Number object's toFixed method is used...because you're already operating on a number and it has a default of 0 if you don't supply an argument.
    [quote user="Chris F"]The overhead and memory here is trivial.  Namespace pollution is not a very convincing argument either, because JavaScript has some very flexible structuring mechanisms - you can even place your extension methods directly on the built-in classes you extend, which is great.  Contains, isVowel and roundNumber are clearly doing more than the built-in methods already provide.[/quote]

    I probably should have given more context to my WTF - What I didn't share is that this is SSJS - not Client-side.  Some of these files have so many "includes" that I've seen processor usage spike to 25%+ just to load a simple form.  While I agree that one or two functions that wrap some built-in methods are probably fine; there are hundreds of "extra" functions like this scattered throughout the application.  The WTF, to me, is that these type of one-liner functions (except maybe isVowel) that give a different name to built-in object methods are a waste to type, use, and track down when something goes wrong.



  • [quote user="iwpg"]... is there any i18n-safe way to check whether something's a vowel without hardcoding a list of vowels for each language ... I didn't find anything with a quick Google, but I'm not sure what kind of search terms to use for something like this.[/quote]

    Search for "linguistics" (or buy a book - it's good reading anyway).  Only languages derived from the Latin alphabet actually have individual characters (not particles) that can be labelled "vowels".

    A function called "IsVowel" (that operates on a whole character) implies Latin/Germanic derision - all of Asia (including Russia & India), parts of the Middle East, all of Africa and even parts of Europe; native languages cannot be expressed this way.

     


Log in to reply