Minor foreach wtf



  • Yeah, I know this is nitpicking, and all that, but it still pushes my buttons.

    My coworker produced a staggering 12 lines of code last week, and  10 of them was initializing a pair of date format conversion objects.
    The objective was to go through about 20.000 items of Excel-imported data, read one of the columns, convert it to a sane date format, and pass the data on.

    A whole lot of other stuff is going on in this "filter" at the same time, but that was his part of it.

    What he did was pretty good code, except the creation of said object pair (one for reading the date as-is, one for returning a sane format) was created inside the foreach loop.
    I'm not sure what the object creation and destruction overheads in Perl are, but if they are more than zero this is silly, so I moved the creation outside the loop, left a friendly little comment about it and committed back in with a vague "Slight object creation optimization" commit message.

    Am I being an ass for RAGEing over this kind of silly little stuff?
    Is my RAGE better focused on team-members producing 12 lines of code in a week?



  • @Chewbacca said:

    My coworker produced a staggering 12 lines of code last week,
     

     I have occasional weeks where I don't write any code at all.

    That doesn't mean I'm not being productive, though.

     



  • Well, of course there are other things to do than code, but he was specifically assigned an already designed and thought-through part of the application, to implement a specific fix/workaround (the 12 lines), and it took all week.

    **shrug**

     

    Edit:
    To be fair, he only works three days of the week, so make that "..., and it took three whole days".
    All this, of course, is not the point.  Am I being an ass for nitpicking about where/when objects are being constructed?



  • @Chewbacca said:

    Am I being an ass for nitpicking about where/when objects are being constructed?
     

    This depends.  If you are still building the application, you should focus on functionality rather than performance.

    If, on the other hand, you are finishing up or maintaining the code (and the note about implementing a fix/workaround suggests this), then you should be concerned about this.  I recall a case in C++ code where a function call that passed a super-simple object (two integers representing an IPv4 address and a TCP/UDP port number) by value rather than by reference cost the application 10-15% of its CPU charge (due to this call happening *a lot*, and calling the copy constructor and the destructor every time).

     



  • Yep, this is a workaround for a problem cought at the very end of QA tests, where the data would very rarely have a different date format.
    Rather than testing every line for this deviation, we pull everything through this pair of "conform to this format, dammit" objects.

    Just did some benchmarking on our set of test-data, and creating/destroying the object seems to be optimized away by the enterpreter anyway.

    Just some more irrational raging from me, then ^__^;



  • @Chewbacca said:

    What he did was pretty good code, except the creation of said object pair (one for reading the date as-is, one for returning a sane format) was created inside the foreach loop.
    If I am to believe a main contributor of the Perl MongoDB library in her claim that Perl's date objects are very heavy (in contrast to the Perl version being the fastest of the MDB libs) this point would be something else you could point out. (I'd like it very much if someone who actually writes Perl could confirm this).@Chewbacca said:
    Am I being an ass for RAGEing over this kind of silly little stuff?
    If you indeed go into a rage, yes. However, it looks like you handled it well.



  • @Chewbacca said:

    except the creation of said object pair was created inside the foreach loop.
    Yeah, I see that here too and it bugs me. That and initialising stuff inside an if and then returning it at the end of the function. We use PHP which is pretty lax about that sort of thing, until a function that was supposed to return an array, returns null instead, which ends up inside a foreach which vomits all over the page. 

    I'd love to watch my colleagues try to use a statically typed language.



  • Are you creating a new pair of dates during each iteration anyway?  If so, this would be a case of proper scoping.  I'm not a Perl programmer, but I know there is a school of thought in other languages that objects should be scoped as narrowly as possible and creation should be deferred as long as possible.  Scott Meyers is a proponent of this.



  • @Chewbacca said:

    Just did some benchmarking on our set of test-data, and creating/destroying the object seems to be optimized away by the enterpreter anyway.

    It really depends on whether you expect this programmer to only ever work with a compiler that cleans up after them. There will be others that do not take care of it in this way. If your dev makes that assumption in another language, or even similar situations in the same language where the compiler cannot make that optimisation, it could cause performance problems in a bottleneck situation.

    Sure, premature optimisations can be a problem too, but in this case it is not premature, given that it is already end-of-initial-development. It's probably a good thing to point out the better practice to the developer, to show the better approach. Otherwise they may make that mistake in other situations where it does cause a problem.

    @frits said:
    objects should be scoped as narrowly as possible

    In Perl, you should be able to get the best of both worlds, by putting a scoping block {...} around the outside of your loop, and creating the variables inside that block. They will then be created and destroyed once, while being limited to existing only while the loop is executing. Some other languages offer the same feature (in JS, you do it with an anonymous function call.)



  • I'm not sure I understand the scenario. Do you mean something like this?

    
    foreach($value in $collection) {
        $dateReader = new DateReader($value)
        $dateWriter = new DateWriter($dateReader.readDate())
    }
    
    

    Because I don't see what's wrong with that. I'm sure I'm missing something...



  • @Chewbacca said:

    Am I being an ass for RAGEing over this kind of silly little stuff?
    Is my RAGE better focused on team-members producing 12 lines of code in a week?

    If you're doing it for performance reasons, yes. Run your Perl code through NYTProf under normal operating conditions and see where the real hotspots are, and optimize those. Otherwise, optimize for style / readability / consistency / effective scoping, that your code be maintainable.

    Of course, if you're creating the exact same elements repeatedly, then that's a perfectly valid style / "code is stupid" issue.



  • @toth said:

    I'm not sure I understand the scenario. Do you mean something like this?


    foreach($value in $collection) {
    $dateReader = new DateReader($value)
    $dateWriter = new DateWriter($dateReader.readDate())
    }

    Because I don't see what's wrong with that. I'm sure I'm missing something...

     

     More like...

     


    foreach my $element (@vast_array){
    my $dateReader = new DateReader(Foo => 'Bar',Boink => 'Oif');
    my $dateWriter = new DateWriter(Foo => 'Oif',Boink => 'Bar');
    $dateReader->read($element->[219]);
    $element->[219] = $dateWriter->write($dateReader);
    }

    Which I changed to ...

     


    my $dateReader = new DateReader(Foo => 'Bar',Boink => 'Oif');
    my $dateWriter = new DateWriter(Foo => 'Oif',Boink => 'Bar');
    foreach my $element (@vast_array){
    $dateReader->read($element->[219]);
    $element->[219] = $dateWriter->write($dateReader);
    }

    Don't get me started on the "two objects?!" thing. I don't have time to write that rant this century. It basically comes down to $dateWriter reading $dateReader's UNIX time representation.

     

    [edit]  I can't believe I'm debugging non-existant code.  Yes, I'm a pedant.



  • Why bother getting upset over it? Fix it, move on, and stop being petty. You probably spent twice the amount of time complaining as it took you to fix the issue.



  • @Chewbacca said:

    What he did was pretty good code, except the creation of said object pair (one for reading the date as-is, one for returning a sane format) was created inside the foreach loop.
    I'm not sure what the object creation and destruction overheads in Perl are, but if they are more than zero this is silly, so I moved the creation outside the loop, left a friendly little comment about it and committed back in with a vague "Slight object creation optimization" commit message.

    Honest question: did you benchmark it? Did your optimization help?

    Edit: Sorry skimmed the thread; I see you already benchmarked it, and your "optimization" didn't do crap. So... case closed.



  • @svm_invictvs said:

    Why bother getting upset over it? Fix it, move on, and stop being petty. You probably spent twice the amount of time complaining as it took you to fix the issue.

    Well, that's what I thought, too. I just wondered what other people think about this sort of thing, hence the thread.
    I would probably have already forgotten about the thread if it wasn't for the friendly e-mails from thedailywtf.com ^__^



  • @Chewbacca said:

    @svm_invictvs said:
    Why bother getting upset over it? Fix it, move on, and stop being petty. You probably spent twice the amount of time complaining as it took you to fix the issue.

    Well, that's what I thought, too. I just wondered what other people think about this sort of thing, hence the thread.
    I would probably have already forgotten about the thread if it wasn't for the friendly e-mails from thedailywtf.com ^__^

    So you go off into a rage over a non-issue, then forget about it moments later? Time to take the mood stabilizers...



  •  I've seen where object initialization in a loop was something like 50% of a given function when I profiled the code. It was a pretty frequently called function so fixing it had a marked improvement in speed. It's an easy optimization to miss so a heads up to whoever did it isn't complaining imo.



  • @blakeyrat said:

    Time to take the mood stabilizers...

    ...or I got over it really quickly because I have you nice folks to vent to.



  • @blakeyrat said:

    So you go off into a rage over a non-issue, then forget about it moments later? Time to take the mood stabilizers...

    @Han Solo said:

    That's 'cause droids don't pull people's arms out of their sockets when they lose.



  • I have to agree with TarquinWJ on this one. Not all languages/compilers/interpreters can optimize this kind of thing away, so I'd personally always move as much code as possible out of a loop. Even if the language/compiler/interpreter does optimize properly, by moving the code myself, I can be sure to avoid any 'unfortunate' optimizations. It's also the way I was taught to write code (in the dys BEFORE optimizing compilers/interpreters), so a certain amount of 'old habits' in my case. :)



  • Or you could just know how the language you're using works.



  • I'm not a developer but I did a fair bit of coding before and during my degree course.  Back then I always tried to declare stuff in the right place, the right place being apparent by understanding what was going on in the code.  Again, I am not a developer, but my concern would be that someone who either didn't care enough or know enough to get the small things right would be likely to get bigger things wrong as well.

    But I concede I may be reading way to much into this.   



  •  You are not being nitpicky.  My developers would be ridiculed for doing the same.  If on top of that they were unproductive, they would be fired.



  • @hoodaticus said:

    You are not being nitpicky.  My developers would be ridiculed for doing the same.  If on top of that they were unproductive, they would be fired.

    "Optimizing" the code without benchmarking it first-- I think that's the bigger offense here. In any case, it counters his "screw-up" if you make an equal-but-opposite screw-up while trying to fix it.



  • @Chewbacca said:

    @svm_invictvs said:
    Why bother getting upset over it? Fix it, move on, and stop being petty. You probably spent twice the amount of time complaining as it took you to fix the issue.

    Well, that's what I thought, too. I just wondered what other people think about this sort of thing, hence the thread.
    I would probably have already forgotten about the thread if it wasn't for the friendly e-mails from thedailywtf.com ^__^

    I know nothing about optimisation, but to me this is about good programming practice accurately representing what you're doing. If something's inside a loop, it's something that is being changed during the loop. If it's not changing, it shouldn't be in the loop.



  • @davedavenotdavemaybedave said:

    @Chewbacca said:
    @svm_invictvs said:
    Why bother getting upset over it? Fix it, move on, and stop being petty. You probably spent twice the amount of time complaining as it took you to fix the issue.

    Well, that's what I thought, too. I just wondered what other people think about this sort of thing, hence the thread.
    I would probably have already forgotten about the thread if it wasn't for the friendly e-mails from thedailywtf.com ^__^

    I know nothing about optimisation, but to me this is about good programming practice accurately representing what you're doing. If something's inside a loop, it's something that is being changed during the loop. If it's not changing, it shouldn't be in the loop.

    +1



  • @superjer said:

    @davedavenotdavemaybedave said:
    I know nothing about optimisation, but to me this is about good programming practice accurately representing what you're doing. If something's inside a loop, it's something that is being changed during the loop. If it's not changing, it shouldn't be in the loop.

    +1

     

    +2



  • @dhromed said:

    @superjer said:

    @davedavenotdavemaybedave said:
    I know nothing about optimisation, but to me this is about good programming practice accurately representing what you're doing. If something's inside a loop, it's something that is being changed during the loop. If it's not changing, it shouldn't be in the loop.

    +1

     

    +2

     

    +3



  • @Someone You Know said:

    @dhromed said:

    @superjer said:

    @davedavenotdavemaybedave said:
    I know nothing about optimisation, but to me this is about good programming practice accurately representing what you're doing. If something's inside a loop, it's something that is being changed during the loop. If it's not changing, it shouldn't be in the loop.

    +1

     

    +2

     

    +3

     

    ++



  • @hoodaticus said:

    @Someone You Know said:

    @dhromed said:

    @superjer said:

    @davedavenotdavemaybedave said:
    I know nothing about optimisation, but to me this is about good programming practice accurately representing what you're doing. If something's inside a loop, it's something that is being changed during the loop. If it's not changing, it shouldn't be in the loop.

    +1

     

    +2

     

    +3

     

    ++



  • @blakeyrat said:

    So you go off into a rage over a non-issue, then forget about it moments later? Time to take the mood stabilizers...

    Can I quote you on that?


Log in to reply