What to do with WTF coders ? Am I being too demanding ?



  • I have a few developers under me that write the type of code that reminds me of the stuff I see posted here.

    Here's an example of some recent code that I changed.

    We have a method to calculate the commission for items in an order using two prices (price in the database, price after discounts), a boolean to determine which margin to use, and two margins.



    My changes break out the logic to determine which margin to use.



    I'm wondering if I'm being too demanding in asking my developers to "see" that the AFTER is better than the BEFORE.



    Anybody here a team leader or in a similar position ? How do you approach your developers when you think there code could be a lot better, but they just "don't get it" ?



    Am I being too anal about good code ?



    http://pastie.textmate.org/private/ca78pi2qtyimfakeoeiqq


  • Discourse touched me in a no-no place

    @chrisp said:

    I have a few developers under me that write the type of code that reminds me of the stuff I see posted here.
    Here's an example of some recent code that I changed.
    We have a method to calculate the commission for items in an order using two prices (price in the database, price after discounts), a boolean to determine which margin to use, and two margins.

    My changes break out the logic to determine which margin to use.

    I'm wondering if I'm being too demanding in asking my developers to "see" that the AFTER is better than the BEFORE.

    Anybody here a team leader or in a similar position ? How do you approach your developers when you think there code could be a lot better, but they just "don't get it" ?

    Am I being too anal about good code ?

    http://pastie.textmate.org/private/ca78pi2qtyimfakeoeiqq

    For posterity:

     

    BEFORE:

    Function calculateCommision(base_price, adjusted_price, useMargin1, margin1, margin2)
    if (useMargin1) then
    if (adjusted_price > base_price) then
    commission = (base_price * margin1)
    commission = commission + (adjusted_price - base_price)
    else
    commission = (adjusted_price * margin1)
    end if
    else
    if (adjusted_price > base_price) then
    commission = (base_price * margin2)
    commission = commission + (adjusted_price - base_price)
    else
    commission = (adjusted_price * margin2)
    end if
    end if
    End Function

    AFTER:

    Function calculateCommision(base_price, adjusted_price, useMargin1, margin1, margin2)
    if (useMargin1) then
    marginToUse = margin1
    else
    marginToUse = margin2
    end if

    if (adjusted_price > base_price) then
    commission = (base_price * marginToUse)
    commission = commission + (adjusted_price - base_price)
    else
    commission = (adjusted_price * marginToUse)
    end if
    End Function


    The idea of that function is broken to begin with. What happens if you have more than 2 margins?

    You should have a function to calculate the commision based on one margin, then have a different function to calculate the margin and call that function. Or have one function do both.

    Neither the Before or After are much use.



  • On the basic point, yes when possible, removing unnecissary nesting is better, yet I also agree with PJH that giving the function two margins and which margin to use is a bit silly. Now I didn't get what language that was, but for stuff like margins I would probebly prefer them to be statically defined as a constant and set in some some config file.

    Also why not just write 

    [code]commission = (base_price * marginToUse) + (adjusted_price - base_price) [/code]

    Instead of spending two lines on it. There also seems a definite lack of comments. But it could be that you removed those for the example.



  • @chrisp said:

    Anybody here a team leader or in a similar position ? How do you approach your developers when you think there code could be a lot better, but they just "don't get it" ?

    First of all I'd like to say that it's extremely annoying when an OP here makes a point and a bunch of people will go off on tangents to bitch about some irrelevant issue. This is why we can't have nice things, so to speak.

    On to the OP's question... it really depends on who you're dealing with. If it's under-achievining  low-expectation code monkeys, then you could try just laying down the law. For example "next time I see copy-pasted code, I'm assigning all bugs in the bug database to that person". Of course like the saying goes, you can't polish a turd so you might be stuck with crap code because the people writting it just don't care and never will. Try to do as much damage control as possible and see if you can somehow get veto power for the next hire.

    If these people can improve then you really need to do some mentoring. Set up code review, show them what's wrong, explain about the general rule they broke (ie, no copy-pasted code) and most important of all give real life examples of how this mistake will make their lives harder in the future. "If we need to fix a bug we, and by we I mean you, will have to apply the fix in several places instead of one. If you enjoy that kind of thing, then keep duplicating code". No matter what though, do not express the rage. No matter how much you're fuming, be nice about it. Otherwise they'll instinctively get defensive and you wont get through to them.

    Also know that this takes patience. They will repeat mistakes even after you explain what's wrong. Keep at it. I know that they're there to work, not go through a friggin comp sci course, but as we all know, management will usually fuck it all up by hiring the first guy with the right acronyms in his resume and then someone has to try and fix the mess. On the bright side you are the team leader so they're expecting to be led. You could have been like me and have to deal with colleagues you have no power over .



  •  That kind of code (the same code, duplicated for each condition) always pisses me off, and I fight tooth and nail in code reviews for it to be refactored the way you did.  


    That being said, I agree that the method in general is fucked up.  If you really need that method to stay arround then refactor to:

    # This mofo be deprecated bitch
    Function calculateCommision(base_price, adjusted_price, useMargin1, margin1, margin2)
    if (useMargin1) then
    marginToUse = margin1
    else
    marginToUse = margin2
    end if
    commission = calculateCommision(base_price, adjusted_price, margenToUse)
    End Function
    Function calculateCommision(base_price, adjusted_price, margin)
    if (adjusted_price > base_price) then
    commission = (base_price * marginToUse) +
    (adjusted_price - base_price)
    else
    commission = (adjusted_price * marginToUse)
    end if
    End Function

     


  • Garbage Person

    VB detected. The second you choose to develop in that language, you throw away your right to demand quality.


  • Discourse touched me in a no-no place

    @DOA said:

    @chrisp said:

    Anybody here a team leader or in a similar position ? How do you approach your developers when you think there code could be a lot better, but they just "don't get it" ?

    First of all I'd like to say that it's extremely annoying when an OP here makes a point and a bunch of people will go off on tangents to bitch about some irrelevant issue.

    Considering what we were 'bitching' about was how the "code [presented] could be a lot better, but they just don't "get it" ", I fail to see how they were off-tangent.



  • @DOA said:

    No matter what though, do not express the rage. No matter how much you're fuming, be nice about it. Otherwise they'll instinctively get defensive and you wont get through to them.
     

    +1 important



  • @dhromed said:

    @DOA said:

    No matter what though, do not express the rage. No matter how much you're fuming, be nice about it. Otherwise they'll instinctively get defensive and you wont get through to them.
     

    +1 important

    Awww, no Gorden Ramsey style management. I wonder if he's willing to do Hell's Cubicle type series.

    "You fuckingly call that fucking refactoring, you fucking good for nothing piece of shit. Ah man, how can I even present that to the fucking customer, you really have no idea of fucking standards do you."



  • Besides the rest of the problems, that function returns an empty variant (worse than null imo, since 1 + calculateCommision() == 1).

    If it's supposed to be a sub, make it a sub. Someone might try to use it as a function, or "fix" it by changing commision to calculateCommision.


Log in to reply