• (cs)

    Maybe none of the parts have descendents.  So really, it's more efficient to simply return false than search on something you know isn't there.

  • (cs)

    It's an optimization. The compiler can inline the return false which is much faster than doing a Regex.IsMatch.

    Oh, I get it now... he's wasting all the clock cycles concatenating strings. He should wrap that in an if(logLevel == LogLevel.DEBUG) block.

  • (cs)

    Is strAncestorID a member variable, or was it actually supposed to be intAncestorID(the passed parameter)?

    So, we're probably seeing that PartTree is actually a comma delimited list of some sort?  Gotta see more code to see the level of true WTFery.

  • arkon (unregistered)

    ahhhh, it took me some time to understand
    the code snippets in this site are not belonged to bad programmers
    they simply want to insure their job is for a life time :)
    http://thc.org/root/phun/unmaintain.html
    have fun

  • (cs) in reply to Ytram

    Ytram:
    Is strAncestorID a member variable, or was it actually supposed to be intAncestorID(the passed parameter)?

    I was wondering that myself. It doesn't really makes sense to call ToString() on a string variable, so one would assume it was suppose to be a different data type. Although, the passed in variable was intAncesterNum, not intAncesterID. Maybe it's a typo; otherwise, there is some kind of disconnect between the passed in variable and the rest of the function that I don't quite understand.

     

  • Just another WTF (unregistered)

    Well to be honest it looks like a case of someone biting off more than they could chew....

    first try:

    string strAncestor = strAncestorID.ToString();

    if ((regex.ismatch(this.PartTree,strAncestor)....

    ... of course when they ran this with a AncestorID of 1 they got matches for "1", "10","11","21","100","231" and anything else that contained a "1"

    so programmer slaps forehead and further specifies the condition

    second try:

    string strAncestor = ","+strAncestorID.ToString()+",";

    if ((regex.IsMatch(this.PartTree,strAncestor)....

    ... this time they only got matches for partID "1" except that they missed it if partID "1" was a the beginning or the end of the part list.

    so programmer says 'arghhhh!' and starts doing the whole start, end, middle thing but happens to think for a moment and see that now the regex will return matches for ",1," correctly in the middle and "1," correctly at the beginning and ",1" correctly at the end, but will also return matchs for ",201," incorrectly by applying the beginning rule.  The orginial programmer then says 'dang!' comments out the function portion of the code, returns false and forgets about the whole idea.

  • (cs)

    This is completely OT but Buckaroo Bonzai was on this weekend and watched part of it.  Made me think of John Smallberries.  I should have been John Bigboote.

  • Ben Adams (unregistered)

    Eighth!

  • (cs) in reply to Ben Adams

    I'm registered!

  • (cs) in reply to Just another WTF
    Anonymous:

    Well to be honest it looks like a case of someone biting off more than they could chew....

    first try:

    string strAncestor = strAncestorID.ToString();

    if ((regex.ismatch(this.PartTree,strAncestor)....

    ... of course when they ran this with a AncestorID of 1 they got matches for "1", "10","11","21","100","231" and anything else that contained a "1"

    so programmer slaps forehead and further specifies the condition

    second try:

    string strAncestor = ","+strAncestorID.ToString()+",";

    if ((regex.IsMatch(this.PartTree,strAncestor)....

    ... this time they only got matches for partID "1" except that they missed it if partID "1" was a the beginning or the end of the part list.

    so programmer says 'arghhhh!' and starts doing the whole start, end, middle thing but happens to think for a moment and see that now the regex will return matches for ",1," correctly in the middle and "1," correctly at the beginning and ",1" correctly at the end, but will also return matchs for ",201," incorrectly by applying the beginning rule.  The orginial programmer then says 'dang!' comments out the function portion of the code, returns false and forgets about the whole idea.

    Yeah, your theory fits the data very well. Looks like someone in over their head having difficulties. Not nearly as much as a WTF as the for-switch paradigm or the switch with a default case only. We've all had similar troubles and made embarrassing gaffes.

  • (cs) in reply to JohnO
    JohnO:
    This is completely OT but Buckaroo Bonzai was on this weekend and watched part of it.  Made me think of John Smallberries.  I should have been John Bigboote.


    Too late! bwahahaha
  • Bob (unregistered) in reply to Just another WTF

    This is where the hack ("," + this.partTree + ",").indexOf("," + strAncestor + ",") != -1   is useful.

  • (cs)

    I think it's not WTF, programmer just was mathematician. He started to write function and then, in middle of doing he realised there is much better way... probability. He did some research and found out it will return false in most cases, so simplyfied the function by always returning false and added that few lines of strange code to diguise his genial idea. I think we are looking at one of the most effective functions ever written.

  • (cs) in reply to arkon

    Anonymous:
    ahhhh, it took me some time to understand
    the code snippets in this site are not belonged to bad programmers
    they simply want to insure their job is for a life time :)
    http://thc.org/root/phun/unmaintain.html
    have fun

     

    nice read ... very helpful!!!

  • (cs) in reply to Just another WTF
    Just another WTF:

    Well to be honest it looks like a case of someone biting off more than they could chew....

    first try:

    string strAncestor = strAncestorID.ToString();

    if ((regex.ismatch(this.PartTree,strAncestor)....

    ... of course when they ran this with a AncestorID of 1 they got matches for "1", "10","11","21","100","231" and anything else that contained a "1"

    so programmer slaps forehead and further specifies the condition

    second try:

    string strAncestor = ","+strAncestorID.ToString()+",";

    if ((regex.IsMatch(this.PartTree,strAncestor)....

    ... this time they only got matches for partID "1" except that they missed it if partID "1" was a the beginning or the end of the part list.

    so programmer says 'arghhhh!' and starts doing the whole start, end, middle thing but happens to think for a moment and see that now the regex will return matches for ",1," correctly in the middle and "1," correctly at the beginning and ",1" correctly at the end, but will also return matchs for ",201," incorrectly by applying the beginning rule.  The orginial programmer then says 'dang!' comments out the function portion of the code, returns false and forgets about the whole idea.

    I did this once, except I made my comma-delimited list start and end with commas so I could search for ",20," in ",1,2,3,5,20,454,". The on my second day of programming, I learned how to do it right using built-in functions to split the list and search more efficiently.

  • (cs) in reply to Manni
    Manni:

    I did this once, except I made my comma-delimited list start and end with commas so I could search for ",20," in ",1,2,3,5,20,454,". The on my second day of programming, I learned how to do it right using built-in functions to split the list and search more efficiently.



    If you are searching the same list again and again, splitting it into pieces and filling a adaquate container (hashtable or tree) is definitely faster. On the other hand, if you have to search the list only once, or if the list is short, the simple solution (searching for ",20," within the string)  is probably faster.
  • (cs) in reply to Manni

    Manni:
    I did this once, except I made my comma-delimited list start and end with commas so I could search for ",20," in ",1,2,3,5,20,454,". The on my second day of programming, I learned how to do it right using built-in functions to split the list and search more efficiently.

    Define "efficiently".  Your first version probably ran several times faster.

  • (cs) in reply to ammoQ

    ammoQ:
    On the other hand, if you have to search the list only once, or if the list is short, the simple solution (searching for ",20," within the string)  is probably faster.

    Ooops... Should have kept reading before I responded......

  • dasmb (unregistered)

    I can see what the guy was getting at here -- using the matches from start, middle or end to construct an open ended hierarchical tree and quickly identify when we're dealing with the head or the foot.  It's the sort of clever implementation somebody in a Perl/PHP world might come up with -- regular expressions and string handling being the strong suits of these packages.

    But more than anything, it demonstrates poor understanding of software (and probably database) design.  A set of integers implemented as a string array?  In code that is (ostensibly) persisted in a database?  Not only is it slow and wasteful, it's also impossible to maintain and worthless for reporting.  Back to DeVry, guy.

  • (cs) in reply to dasmb

    Anonymous:
    I can see what the guy was getting at here -- using the matches from start, middle or end to construct an open ended hierarchical tree and quickly identify when we're dealing with the head or the foot.  It's the sort of clever implementation somebody in a Perl/PHP world might come up with -- regular expressions and string handling being the strong suits of these packages.

    But more than anything, it demonstrates poor understanding of software (and probably database) design.  A set of integers implemented as a string array?  In code that is (ostensibly) persisted in a database?  Not only is it slow and wasteful, it's also impossible to maintain and worthless for reporting.  Back to DeVry, guy.

    Doing queries from an RDBMs which stores a hierarchy is a somewhat non-trivial task. For example, if you were to provide us a query to display all descendants of a node in a hierarchy modelled by a table which foreign keys into itself, I'm sure many people on this forum would find problems with your solution.

    Someone getting in over their head is a minor WTF compared to the beasts we've seen here. Though the real WTF is the comments people on both sides have been making about the metric system. And to think that I once believed that only church-goers participated in holy wars... [A]

     

  • Will I Am (unregistered) in reply to yaccobb
    yaccobb:
    I think it's not WTF, programmer just was mathematician. He started to write function and then, in middle of doing he realised there is much better way... probability. He did some research and found out it will return false in most cases, so simplyfied the function by always returning false and added that few lines of strange code to diguise his genial idea. I think we are looking at one of the most effective functions ever written.


    I resent that.

    What you meant to say is that the programmer was a statistician.  From Purdue.
  • (cs)

    This should be a TODO, this should be removed completely from the code.   If this is in some DLL/shared library which is must remain for binary compatibility (though it looks like Java to me which I don't think has this problem?) it should throw a fatal exception so that all cases that call this can be fixed.


  • Just Another WTF (unregistered) in reply to dasmb

    Anonymous:
    I can see what the guy was getting at here -- using the matches from start, middle or end to construct an open ended hierarchical tree and quickly identify when we're dealing with the head or the foot.  It's the sort of clever implementation somebody in a Perl/PHP world might come up with -- regular expressions and string handling being the strong suits of these packages.

    But more than anything, it demonstrates poor understanding of software (and probably database) design.  A set of integers implemented as a string array?  In code that is (ostensibly) persisted in a database?  Not only is it slow and wasteful, it's also impossible to maintain and worthless for reporting.  Back to DeVry, guy.

    The really sad part of all this is that the this.PartTree method probably has to walk the structure anyway in order to create its comma seperated list... everytime you call PartTree.  So a function called isDescendant could save a ton of time and effort by simply walking the structure once and returning true when it comes across the integer PartID of the looked for ancestor.  Instead simple core functionality is being implemented by leveraging UI methods... that is they've call PartTree which has to walk the whole tree retrieve the PartId's and build a comma delimited string, then convert the integer parameter into a string and do whatever extra string handling they use to solve their 'partial matches' problem.  Then pass that whole wack of strings into the RegEx engine.  Just imagine all the extra code your carrying, all the extra memory you use, and all those clock cycles that would otherwise have gone to waste.  Its code like this that makes it worthwhile to get the latest and greatest hardware.

  • nickf (unregistered) in reply to Just Another WTF

    this.PartTree doesn't look like it's a method, just a class variable. It was probably constructed as you described, but only once (or once for each time it gets updated).

  • (cs) in reply to John Bigboote

    <font size="2">

    John Bigboote:
    JohnO:
    This is completely OT but Buckaroo Bonzai was on this weekend and watched part of it.  Made me think of John Smallberries.  I should have been John Bigboote.


    Too late! bwahahaha


    There's always John Yaya...
    </font>
  • Krenn (unregistered) in reply to ammoQ
    ammoQ:
    If you are searching the same list again and again, splitting it into pieces and filling a adaquate container (hashtable or tree) is definitely faster. On the other hand, if you have to search the list only once, or if the list is short, the simple solution (searching for ",20," within the string)  is probably faster.


    Right up to the point that 20 is at the front or the end, so it either doesn't have commas leading or trailing.
  • (cs) in reply to Krenn

    Anonymous:
    Right up to the point that 20 is at the front or the end, so it either doesn't have commas leading or trailing.

    AmmoQ is commenting on Manni statement just before.  Read Manni's again....Closely

  • Anonymouse (unregistered) in reply to nickf
    Anonymous:
    this.PartTree doesn't look like it's a method, just a class variable. It was probably constructed as you described, but only once (or once for each time it gets updated).


    If this code is C#, then PartTree could be a property, which in turn means that accessing it could very well be a full-fledged function call (to the getter function defined for it)...
  • (cs) in reply to Manni
    Manni:
    Then on my second day of programming, I learned how to do it right using built-in functions to split the list and search more efficiently.


    ~on the second day of christmas, my true love gave to me...~

    KNOWLEDGE
  • Anonymouse (unregistered) in reply to Krenn
    Krenn:
    ammoQ:
    If you are searching the same list again and again, splitting it into pieces and filling a adaquate container (hashtable or tree) is definitely faster. On the other hand, if you have to search the list only once, or if the list is short, the simple solution (searching for ",20," within the string)  is probably faster.


    Right up to the point that 20 is at the front or the end, so it either doesn't have commas leading or trailing.


    Ever heard the joke about how to catch an elephant? And how the programmer makes sure to place a known instance of an elephant in cape town before iterating over Africa from the north to make sure the algorithm terminates?

    (... which is why you make sure there are commas at the end and beginning of the string you want to search, by putting them there yourself.)


  • Spudley (unregistered)
    ...had a lot of advertised "details" that just didn't seem to work...

    To be honest, knowing how the average sales+marketing team thinks, I'm not surprised they were advertising features that hadn't been written yet; I've seen it happen more times than I can count, and it usually ends up causing trouble for the development team.

    The trick is just not to tell them anything about what you're working on until it has at least passed the first stage of QA. And whatever you do, don't ever show them your mockups or prototypes, because sales people interpret those as completed products, and promptly start trying to sell them.

  • (cs) in reply to Spudley
    Anonymous:
    ...had a lot of advertised "details" that just didn't seem to work...
    To be honest, knowing how the average sales+marketing team thinks, I'm not surprised they were advertising features that hadn't been written yet; I've seen it happen more times than I can count, and it usually ends up causing trouble for the development team. The trick is just not to tell them anything about what you're working on until it has at least passed the first stage of QA. And whatever you do, don't ever show them your mockups or prototypes, because sales people interpret those as completed products, and promptly start trying to sell them.


    My favourite version of the problem:

    Sales team sells stuff yet-to-be-made, with a deadline that couldn't be met even if you had ten times as many people in your team. To make it even worse, the specification of the project is vague so it's difficult to get even complete and accurate specs till the deadline. You start making mockups and prototypes, so at least you have something to show and discuss.
    Then, when the deadline comes close, the project is postponed for external reasons (this happens every single time). Management assigns the team to another (of course also late) project. Six months later, when the new deadline is due, the old project is revitalized and the team has few weeks to turn the mockups and prototypes into the real thing.
  • (cs) in reply to ammoQ

    Dear Recognition,

    Why must you taunt me so?

    Love,
    DH.

  • (cs) in reply to yaccobb
    yaccobb:
    I think it's not WTF, programmer just was mathematician. He started to write function and then, in middle of doing he realised there is much better way... probability. He did some research and found out it will return false in most cases, so simplyfied the function by always returning false and added that few lines of strange code to diguise his genial idea. I think we are looking at one of the most effective functions ever written.


    But what about the bias ???? That's not adressed at all!
  • (cs) in reply to OneFactor
    OneFactor:
    And to think that I once believed that only church-goers participated in holy wars... [A]


    I take it that was before you learned programming.
  • (cs) in reply to OneFactor
    OneFactor:

    For example, if you were to provide us a query to display all descendants of a node in a hierarchy modelled by a table which foreign keys into itself, I'm sure many people on this forum would find problems with your solution.



    Oracle can do exactly that (the following example is copied from the Oracle docs)

    SELECT LPAD(' ',2*(LEVEL-1)) || ename org_chart, empno, mgr, job
    FROM emp
    START WITH job = 'PRESIDENT' 
    CONNECT BY PRIOR empno = mgr;

  • (cs)

    It's part of the new methodology: Design Driven Development. What UAT does not catch is not worth paying any attention to and until the client agrees to a new quote, dont worry!

    <tongue-in-cheek />



  • (cs)
    Alex Papadimoulis:
      string strAncestor = intAncestorNum.ToString();
      string start = strAncestor + ",";
      string end = "," + strAncestor;
      string middle = "," + strAncestor + ",";


    I like how strAncestor is named in (Systems) Hungarian (along with intAncestorNum - but that's a discussion for another day), but this convention is completely dropped for the next three strings, start, middle, and end.  Good going!

  • laka (unregistered) in reply to ammoQ
    ammoQ:
    Sales team sells stuff yet-to-be-made, with a deadline that couldn't be met even if you had ten times as many people in your team. To make it even worse, the specification of the project is vague so it's difficult to get even complete and accurate specs till the deadline. You start making mockups and prototypes, so at least you have something to show and discuss.
    Then, when the deadline comes close, the project is postponed for external reasons (this happens every single time). Management assigns the team to another (of course also late) project. Six months later, when the new deadline is due, the old project is revitalized and the team has few weeks to turn the mockups and prototypes into the real thing.

    Actually, I remember once a guy from the delivery service entering my office and asking about an unknown (by him and me) product he had to send to a customer (who had already paid it). After several hours of research we discovered someone from the sale team just sold to the customer what he asked for, and noone in the dev team even knew about it. The product didn't exist, the design document didn't exist, the features list didn't exist, ... We had to develop the thing in a few days and I've never seen again a soft with that much bugs (we didn't even have the time to develop an interface).

  • Xipher0 (unregistered)
    Alex Papadimoulis:

    Peter S. noticed that his company's flagship product, a parts management system, had a lot of advertised "details" that just didn't seem to work. Being the motivated guy that he is, Pete took it upon himself to fix these, starting with a non-functional search for all container parts given a particular part. Upon looking at the code, he decided that maybe it was just better off left TODO ... 

    /// 
/// returns true if the part is a descendent of the specified part
/// 
public bool IsDescendent(int intAncestorNum)
    {
      string strAncestor = intAncestorNum.ToString();
      string start = strAncestor + ",";
      string end = "," + strAncestor;
      string middle = "," + strAncestor + ",";

      // TODO ...
  //if ((Regex.IsMatch(this.PartTree, strAncestor
  return false;
    }

    (Update: Fixed typo I made in the code)



    Is there any reason that the regex "\D?" + intAncestorNum.ToString() + "\D?" wouldn't have worked? WTF?
  • (cs) in reply to Xipher0
    Anonymous:
    Alex Papadimoulis:

    Peter S. noticed that his company's flagship product, a parts management system, had a lot of advertised "details" that just didn't seem to work. Being the motivated guy that he is, Pete took it upon himself to fix these, starting with a non-functional search for all container parts given a particular part. Upon looking at the code, he decided that maybe it was just better off left TODO ... 

    /// 
/// returns true if the part is a descendent of the specified part
/// 
public bool IsDescendent(int intAncestorNum)
    {
      string strAncestor = intAncestorNum.ToString();
      string start = strAncestor + ",";
      string end = "," + strAncestor;
      string middle = "," + strAncestor + ",";

      // TODO ...
  //if ((Regex.IsMatch(this.PartTree, strAncestor
  return false;
    }

    (Update: Fixed typo I made in the code)



    Is there any reason that the regex "\D?" + intAncestorNum.ToString() + "\D?" wouldn't have worked? WTF?

    Other than the fact that due to the '?' both '\D's are entirely optional and thus would catch every instance of intAncestorNum no matter where it was?
  • (cs) in reply to Casiotone
    Casiotone:
    Anonymous:

    Is there any reason that the regex "\D?" + intAncestorNum.ToString() + "\D?" wouldn't have worked? WTF?

    Other than the fact that due to the '?' both '\D's are entirely optional and thus would catch every instance of intAncestorNum no matter where it was?


    "\b" + intAncestorNum.ToString() + "\b" should work fine though.
  • (cs) in reply to Maurits
    Maurits:
    Casiotone:
    Anonymous:

    Is there any reason that the regex "\D?" + intAncestorNum.ToString() + "\D?" wouldn't have worked? WTF?

    Other than the fact that due to the '?' both '\D's are entirely optional and thus would catch every instance of intAncestorNum no matter where it was?


    "\b" + intAncestorNum.ToString() + "\b" should work fine though.

    As would

    "(^|,)"+intAncestorNum.toString()+"(,|$)"

    (your solution is much cleaner though)

  • test (unregistered) in reply to Sean

    :pizza:

  • test (unregistered) in reply to test

    [pi]

  • (cs) in reply to ammoQ

    Where I work a salesman selling stuff we haven't written yet is a sackable offence.

  • (cs) in reply to masklinn
    masklinn:
    Maurits:
    Casiotone:
    Anonymous:

    Is there any reason that the regex "\D?" + intAncestorNum.ToString() + "\D?" wouldn't have worked? WTF?

    Other than the fact that due to the '?' both '\D's are entirely optional and thus would catch every instance of intAncestorNum no matter where it was?


    "\b" + intAncestorNum.ToString() + "\b" should work fine though.

    As would

    "(^|,)"+intAncestorNum.toString()+"(,|$)"

    (your solution is much cleaner though)



    Yes, in this case they are identical.  The "\b" version is not supported in as broad a selection of regex dialects, so the "(^|,)" version has its merits too.

    A major WTF here is that it seems that the developer was going to do 3 regex searches where one would have been fine.  Regex matching is reasonably expensive.  Doing three searches where one is sufficient is usually a bad thing.

    On the other hand, if the combined regex is much more complex than the individual regexen, it might be worth doing for maintainability.  Speed could be better if the individual regexen were hand tuned or all possibilities didn't need to be checked each time.
  • (cs) in reply to ammoQ
    ammoQ:
    Manni:

    I did this once, except I made my comma-delimited list start and end with commas so I could search for ",20," in ",1,2,3,5,20,454,". The on my second day of programming, I learned how to do it right using built-in functions to split the list and search more efficiently.



    If you are searching the same list again and again, splitting it into pieces and filling a adaquate container (hashtable or tree) is definitely faster. On the other hand, if you have to search the list only once, or if the list is short, the simple solution (searching for ",20," within the string)  is probably faster.

    Yeah in the case where I was using the comma-delimited string, I had to search it repeatedly to map out items that matched up to an ID column in a database. It worked a little better when I started using tables, considering the code now looks through several hundred (or maybe thousand by this point) unique IDs.

  • nickf (unregistered) in reply to Manni

    when i was in my first year of studying IT, we had to learn all about Binary Search Trees and Tries and Hash Tables and 2-3 Trees. Seeing as i'm still studying that degree and don't have that much real world experience, I was just wondering, how often are these used? The seem to be probably a bit more efficient than using strings and regexes, especially when you start getting into the hundreds or thousands of ids.

  • (cs) in reply to nickf
    Anonymous:
    when i was in my first year of studying IT, we had to learn all about Binary Search Trees and Tries and Hash Tables and 2-3 Trees. Seeing as i'm still studying that degree and don't have that much real world experience, I was just wondering, how often are these used? The seem to be probably a bit more efficient than using strings and regexes, especially when you start getting into the hundreds or thousands of ids.


    In the real world, we use these structures too.  However, in the real world we only build them infrequently.  In most cases we either get a generic implementation from a library or we have the data on a database which implements all these things under the hood.

    I'd guess that a lot of good developers have only written an algorithm like that once or twice professionally.

    Comma seperated lists of value are pretty common.  In most cases storing and moving data in some human readable form will simplify debugging and maintenence even if it is less efficient.  So unless the feature is critical to the performance of the application, people will use a very primitive data structure like a comma seperated list.  Also, strings are fairly portable between different kinds of software, so it is common to knit together components which may have been written in different languages using strings.

    A funny thing happened in the IT world.  In the 60s and 70s, lots of research went into making algorithms and processes as fast and efficient as possible.  In that period, hardware was expensive and developers were cheap, and so it was best to optimize for the best use of the expensive resource.In the 90s and this decade, the pendulum has swung.  Hardware is cheap and developers are expensive.  Having a developer spend time trying to make a reasonably fast process faster is usually fool-hardy.  And having a developer implement a custom sort routine when the library has one that is almost as fast and fully tested is lunacy.  Research, in reflection of this, now has moved onto developer productivity.

Leave a comment on “Better Off TODO”

Log In or post as a guest

Replying to comment #:

Log Out

« Return to Article