We don't do code reviews..



  • "We don't do code reviews" said my new boss when I started. "We've got developers of very mixed abilities here and I don't want the juniors to feel bad about their work. I don't particularly mind how people do their work as long as it gets done and the quality is good".

    Not doing code reviews is precisely how quality shite like this makes it into production:

    Last line of a 150-line ASP.net Page_Load handler: this.PreRender += new EventHandler(Detail_PreRender);

    Can somebody tell me why?

    Exception handling is as follows:

    catch (Exception exp)
    {
        exp.ToString();
    }

    Nice.

    In the same page there is a 200-line method that builds the UI. No, the UI isn't in the markup but first implemented as an xml file that bears a passing resemblence to html. This xml file is then transformed with an xslt and the method loops over the resultant transformed xml, dynamically creating controls and adding them to the page. Nobody customises the original xml file so you can't even argue that it's actually dynamic.

    Code reviews, coming to all junior developers near me, every day until you can be trusted. Which will be never. 



  • @JimLahey said:

    "We don't do code reviews" said my new boss when I started. "We've got developers of very mixed abilities here and I don't want the juniors to feel bad about their work. I don't particularly mind how people do their work as long as it gets done and the quality is good".

    The "and the quality is good" is the key here. Without code reviews, nobody knows what the quality actually is. If he really wants quality, he needs to do code reviews to find out. If he doesn't want the juniors to feel bad about their work, then just do the code reviews all alone in your little room -- and maybe create a quality statistics to show your boss where the quality standards are (not).

     



  • I'm so glad that at least one other person on the planet agrees with me.

    I think he was implying that quality can be deduced from the products being released on time and passing through QA.Granted, it's one metric but QA should start with the developers and quality should also include things like readability, maintanability and adhering to some sort of common standard. It's ASP.net, not WTF.net.



  • @JimLahey said:

    I don't want the juniors to feel bad about their work
     

    TOUGH COOKIES, BUDDY.


  • ♿ (Parody)

    @dhromed said:

    @JimLahey said:
    I don't want the juniors to feel bad about their work

    TOUGH COOKIES, BUDDY.

    Indeed. But it's often more important to make the seniors feel bad about their work, since they're often more difficult to correct, and the juniors are liable to follow the example of the mistakes thinking that they're doing the right thing. And we all need correction at times.

    We don't have formal code reviews. There are some developers whose code I never trust, although sometimes I'm just too scared to look closely. Others I peek in on only occasionally .



  • @JimLahey said:

    It's ASP.net, not WTF.net.
    Ahem. So different indeed.



  • @dhromed said:

    @JimLahey said:
    I don't want the juniors to feel bad about their work

    TOUGH COOKIES, BUDDY.

    I'm more curious about how a code review is supposed to make a junior feel bad. I mean "Hey you could use this thingy here instead and it would make your life easier" makes people feel bad? How?



  • Nice one. :)


  • ♿ (Parody)

    @locallunatic said:

    I'm more curious about how a code review is supposed to make a junior feel bad. I mean "Hey you could use this thingy here instead and it would make your life easier" makes people feel bad? How?

    You're obviously not up on the state of the art in self esteem creation and maintenance. Please do not disturb the precious snowflakes.



  • Me and the development manager just had an extended chat about it and he knows of several developers here who don't react well to criticism and are pretty stuck in their ways. He also supports my suggestion that a designated person on each product team should assume the role of reviewer so I'm good to go on my plan. Now I'm code reviewer, scrum master and the only guy who's ever heard of sketching something out in Visio before I start work on a product feature that will be in production for a number of years. How does the saying go again, speak softly but carry a big stick? Does it matter that the stick is a junior's arm that I've just torn off?


  • ♿ (Parody)

    @JimLahey said:

    How does the saying go again, speak softly but carry a big stick?

    This reminded me of an instant classic from Sheriff Joe



  • I think I've just reached the very zenith of WFTery today. I challenge anyone to produce something quite so pointless, obsolete, retarded and awful as this:

    string pagging = string.Empty;
    double pages = jobAmount / double.Parse(ViewState["pageSize"].ToString());
    bool setBeginTag = true, setEndTag = true;
    const int amountOfLinksVisible = 15;
    int currentPage = int.Parse(ViewState["pageIndex"].ToString());

    for (int i = currentPage - (amountOfLinksVisible / 2); i < (currentPage + (amountOfLinksVisible / 2)) + pages - ((int)pages); i++)
    {
        if (i < 0) setBeginTag = false;
        else if (i >= pages)
        {
            setEndTag = false;
            break;
        }
        else if (i == currentPage) pagging += "<i> " + (i + 1) + " </i>";
        else pagging += "<a href=javascript:__doPostBack('Pagging','" + i + "')> " + (i + 1) + " </a>";
    }
    pagging = (setBeginTag
                  ? "<a href=javascript:__doPostBack('Pagging','" + 0 + "')> << </a>.."
                  : string.Empty) + pagging + (setEndTag
                        ? "..<a href=javascript:__doPostBack('Pagging','" + (int)(Math.Ceiling(pages) - 1) + "')> >> </a>"
                        : string.Empty);

    // get amount of cells for colspan
    int cellAmount = cells.Count;
    cells.Clear();
    cells.Add(new TableCell { Text = "<td align=center colspan=" + cellAmount + ">" + pagging + "</td>" });

    I had wondered why no amount of styling would change the appearance of the pager on all the GridViews in our application. The original author had implemented all the paging himself in the FooterTemplate instead of using the built in functionality that has been part of the ASP.net GridView (DataGrid for all you fellow lifers out there) since ASP.net was released over a decade ago. Be aware that this code is copypasta on every page that has a paged table. The ratio of copypasta to GridViews is 1-1, so on our page with two GridViews, this code is present twice.

    I'm going back to the judge and I'm going to ask for the death penalty instead if I am going to have to put up with this for the next 30 years.



  • @JimLahey said:

    I think I've just reached the very zenith of WFTery today. I challenge anyone to produce something quite so pointless, obsolete, retarded and awful as this:

    snip unformatted code

    I had wondered why no amount of styling would change the appearance of the pagger on all the GridViews in our application. The original author had implemented all the pagging himself in the FooterTemplate instead of using the built in functionality that has been part of the ASP.net GridView (DataGrid for all you fellow lifers out there) since ASP.net was released over a decade ago. Be aware that this code is copypasta on every pagg that has a pagged table. The ratio of copypasta to GridViews is 1-1, so on our pagg with two GridViews, this code is present twice.

    I'm going back to the judge and I'm going to ask for the death penalty instead if I am going to have to put up with this for the next 30 years.

    FTFY



  • @JimLahey said:

    Exception handling is as follows:

    catch (Exception exp)
    {
        exp.ToString();
    }

    They wanted to get rid of that annoying "variable 'exp' is declared but never used" warning.



  • @pkmnfrk said:

    @JimLahey said:

    I think I've just reached the very zenith of WFTery today. I challenge anyone to produce something quite so pointless, obsolete, retarded and awful as this:

    snip unformatted code

    I had wondered why no amount of styling would change the appearance of the pagger on all the GridViews in our application. The original author had implemented all the pagging himself in the FooterTemplate instead of using the built in functionality that has been part of the ASP.net GridView (DataGrid for all you fellow lifers out there) since ASP.net was released over a decade ago. Be aware that this code is copypasta on every pagg that has a pagged table. The ratio of copypasta to GridViews is 1-1, so on our pagg with two GridViews, this code is present twice.

    I'm going back to the judge and I'm going to ask for the death penalty instead if I am going to have to put up with this for the next 30 years.

    FTFY

    Damn straight... If you're going to be wrong, be wrong consistently!



  • @dhromed said:

    @JimLahey said:

    I don't want the juniors to feel bad about their work
     

    TOUGH COOKIES, BUDDY.

    Hey, I'm self-aware enough to know that I am not the smartest coder in the world.

    Therefore, there are people smarter than me, who can teach me useful things. Which, in turn, gives me more experience and makes me even smarter.

    Therefore, I welcome constructive cricitism from my peers and from other developers. More experienced developers can teach me things. Less experienced developers who ask "why did you do it that way?" give me a chance to explain myself, teach others, and everybody benefits.

    If an employee -- at ANY level, not just juniors -- doesn't want his work peer reviewed because he might "feel bad" then that is a huge red flag.

     



  • @WhiskeyJack said:

    If an employee -- at ANY level, not just juniors -- doesn't want his work peer reviewed because he might "feel bad" then that is a huge red flag.

    What if you're so much smarter than everyone else that having lesser coders review your work makes no sense, like having an ant try to comprehend the Sistine Chapel?



  • @morbiuswilters said:

    @WhiskeyJack said:
    If an employee -- at ANY level, not just juniors -- doesn't want his work peer reviewed because he might "feel bad" then that is a huge red flag.

    What if you're so much smarter than everyone else that having lesser coders review your work makes no sense, like having an ant try to comprehend the Sistine Chapel?

     

     

    I have others review my code so that even if they can understand a little bit of it, they will be that much better for it. There is always something for them to learn.



  • @JoeCool said:

    @morbiuswilters said:

    @WhiskeyJack said:
    If an employee -- at ANY level, not just juniors -- doesn't want his work peer reviewed because he might "feel bad" then that is a huge red flag.

    What if you're so much smarter than everyone else that having lesser coders review your work makes no sense, like having an ant try to comprehend the Sistine Chapel?

     

    I have others review my code so that even if they can understand a little bit of it, they will be that much better for it. There is always something for them to learn.

    Agreed... Plus even the smartest people make bone headed mistakes.... Code review is a MUST.

     (...and Morb's...I am willing to bet that I can even make a claim for one of the biggest boneheaded mistakes on this forum....anyone else ever slam the main gun of a warship into the deck with sufficient force to crack the cradle?)


  • ♿ (Parody)

    @morbiuswilters said:

    @WhiskeyJack said:
    If an employee -- at ANY level, not just juniors -- doesn't want his work peer reviewed because he might "feel bad" then that is a huge red flag.

    What if you're so much smarter than everyone else that having lesser coders review your work makes no sense, like having an ant try to comprehend the Sistine Chapel?

    The Senior has no Bugs! Which is better than that he have no clothes.



  • @boomzilla said:

    @morbiuswilters said:
    @WhiskeyJack said:
    If an employee -- at ANY level, not just juniors -- doesn't want his work peer reviewed because he might "feel bad" then that is a huge red flag.

    What if you're so much smarter than everyone else that having lesser coders review your work makes no sense, like having an ant try to comprehend the Sistine Chapel?

    The Senior has no Bugs! Which is better than that he have no clothes.

    But when any team members have bugs crawling on their clothes, you know it is time to RUN



  •  I once worked with a guy who got filthier as the week went by, but was clean every Monday. I'm guessing he showered every Sunday night whether he needed it or not.



  • @TheCPUWizard said:

    I am willing to bet that I can even make a claim for one of the biggest boneheaded mistakes on this forum

    There are two kinds of people in this world: sharks and sheep. Obviously, I am a shark. You just outed yourself as a sheep. A sheep who doesn't comprehend the Sistine Chapel that this shark has painted.

    @TheCPUWizard said:

    anyone else ever slam the main gun of a warship into the deck with sufficient force to crack the cradle?

    Yes, but it was a "Hold my beer and watch this" moment, so it doesn't count.



  • @JoeCool said:

     I once worked with a guy who got filthier as the week went by, but was clean every Monday. I'm guessing he showered every Sunday night whether he needed it or not.

    I worked with a guy who had a progressively worse BO problem. Ironically, he was an OSX user, not a Linux user like you'd expect (although he had switched to OSX from Linux, so..) He was one of these people who insisted OSX was super-duper superior to Windows in every way and that Windows was made by retarded monkeys.

    One time he used one of our Windows testing workstations to fix a bug in his Javascript. After two days, the thing was wrecked: temp files scattered across the desktop, in no pattern, with many of the icons overlapping one another; managed to install a bunch of malware while trying to test our internal web app; hosed the Registry after editing a bunch of settings because "that's what you have to do to get Windows to even be usable". Luckily we had disk images we could recover from, but I realized then and there that some people aren't cut out for using Windows.

    I wanted to fire the guy because he kept taking down all of our production sites by pushing out "fixes" he had never tested, but management wouldn't let me. I finally took away his commit access and he had to submit patches through me, since that was the only way I could be sure they were tested.

    He quit a few months later to get his PhD in CS (already had his Masters)..


  • ♿ (Parody)

    @morbiuswilters said:

    @TheCPUWizard said:
    I am willing to bet that I can even make a claim for one of the biggest boneheaded mistakes on this forum

    There are two kinds of people in this world: sharks and sheep. Obviously, I am a shark. You just outed yourself as a sheep. A sheep who doesn't comprehend the Sistine Chapel that this shark has painted.

    You forgot about the people who are the killer whales that drown the sharks. Anyways, sheep don't really have to worry about sharks. I'd rather be a sheep. All that swimming just sounds tiresome, and I can't hold my breath for very long any more.



  • @boomzilla said:

    I'd rather be a sheep. All that swimming just sounds tiresome, and I can't hold my breath for very long any more.

    Gutsy statement. You're a shark. Sharks are winners. And they don't look back because they don't have necks. Necks are for sheep. I'm proud to be the shepherd to this flock of sharks.



  • @JimLahey said:

    Me and the development manager just had an extended chat about it and he knows of several developers here who don't react well to criticism and are pretty stuck in their ways.
     

    That's the red flag for me. I'd lay money (a small amount, anyway) that those developers' code is *not* quality.

    One way to deal with it is to never give them a raise. If they are not willing to change how they do their job then why should their emploery change how they renumerate them? *evil grin*

     



  • @morbiuswilters said:

    @TheCPUWizard said:
    I am willing to bet that I can even make a claim for one of the biggest boneheaded mistakes on this forum

    There are two kinds of people in this world: sharks and sheep. Obviously, I am a shark. You just outed yourself as a sheep. A sheep who doesn't comprehend the Sistine Chapel that this shark has painted.

    @TheCPUWizard said:

    anyone else ever slam the main gun of a warship into the deck with sufficient force to crack the cradle?

    Yes, but it was a "Hold my beer and watch this" moment, so it doesn't count.

    Dadgum, Morbs, are you from the South too??   Well hush my puppies!


  • ♿ (Parody)

    @morbiuswilters said:

    @boomzilla said:
    I'd rather be a sheep. All that swimming just sounds tiresome, and I can't hold my breath for very long any more.

    Gutsy statement. You're a shark. Sharks are winners. And they don't look back because they don't have necks. Necks are for sheep. I'm proud to be the shepherd to this flock of sharks.

    I suppose I could make this shark thing work...

    CANDYGRAM



  • @morbiuswilters said:

    @boomzilla said:
    I'd rather be a sheep. All that swimming just sounds tiresome, and I can't hold my breath for very long any more.

    Gutsy statement. You're a shark. Sharks are winners. And they don't look back because they don't have necks. Necks are for sheep. I'm proud to be the shepherd to this flock of sharks.

    I'm in Arizona.  The sheep here live a heck of a lot longer than the sharks.



  • @rstinejr said:

    Dadgum, Morbs, are you from the South too??   Well hush my puppies!

    Southern-ish midwest. I was joking about damaging a warship but I grew up hearing "hold my beer and watch this" (or as kids "hold my cigarette and watch this").



  • @da Doctah said:

    @morbiuswilters said:

    @boomzilla said:
    I'd rather be a sheep. All that swimming just sounds tiresome, and I can't hold my breath for very long any more.

    Gutsy statement. You're a shark. Sharks are winners. And they don't look back because they don't have necks. Necks are for sheep. I'm proud to be the shepherd to this flock of sharks.

    I'm in Arizona.  The sheep here live a heck of a lot longer than the sharks.

    The sheep/shark thing was a reference to Futurama. I can't find a video because Fox is made up of assholes who can only think in the short-term and don't realize that allowing clips of a show to stay up on Youtube might make people want to give them money.



  • At my last gig, I made a modification to stored procedure. Mind you, this stored procedure was pretty messed up before I got to it and managment knew it (trwtf is we had decent managment). So anyway, the manager asked the DBA to review my changes. He came back saying the entire sproc was messed up and needed to be rewritten. I reminded him that he only needed to review my changes.

    Once again he came back with problems in the sql that I had nothing to do with. So I went back to the manager and she came with me to his desk and reminded him to ONLY REVIEW MY CHANGES.

    He came back the third time, again not reviewing the changes. Apparently he did not know how to do a diff. The manager asked me I could show him. I replied, "If I have to show him how to do a diff, why is he the one reviewing my code.

    My code went into production without further review :) 



  • @this_code_sucks said:

    My code went into production without further review :) 

    That's pretty goddamn dysfunctional. Maybe better than being pestered by the inept DBA but "my company can't even manage to do code reviews" isn't something I'd smile over.



  • @this_code_sucks said:

    Apparently he did not know how to do a diff. The manager asked me I could show him. I replied, "If I have to show him how to do a diff, why is he the one reviewing my code.

    It does make sense to have a SQL expert review SQL code, even if he is not an expert in source control. And the guy is also right to review the whole stored procedure, and not just your changes, because it is a unit of code, it won't get partially executed in production. *You* may feel that your changes are not impacting something else, but there is a reason why *he* is the DBA.

    Your manager is guilty in this situation because that was a bad call; what he did was to oil the wheel that was making the more noise (i.e.: you) instead of thinking about the big picture.



  • @morbiuswilters said:

    @this_code_sucks said:
    My code went into production without further review :) 

    That's pretty goddamn dysfunctional. Maybe better than being pestered by the inept DBA but "my company can't even manage to do code reviews" isn't something I'd smile over.

    Not really, it actually just got reviewed by another DBA



  • @this_code_sucks said:

    @morbiuswilters said:

    @this_code_sucks said:
    My code went into production without further review :) 

    That's pretty goddamn dysfunctional. Maybe better than being pestered by the inept DBA but "my company can't even manage to do code reviews" isn't something I'd smile over.

    Not really, it actually just got reviewed by another DBA

    Either it went into production without review or it didn't. If the former, I'd say that's a problem.



  • @Speakerphone Dude said:

    @this_code_sucks said:
    Apparently he did not know how to do a diff. The manager asked me I could show him. I replied, "If I have to show him how to do a diff, why is he the one reviewing my code.

    It does make sense to have a SQL expert review SQL code, even if he is not an expert in source control. And the guy is also right to review the whole stored procedure, and not just your changes, because it is a unit of code, it won't get partially executed in production. *You* may feel that your changes are not impacting something else, but there is a reason why *he* is the DBA.

    Your manager is guilty in this situation because that was a bad call; what he did was to oil the wheel that was making the more noise (i.e.: you) instead of thinking about the big picture.

    It was a very, very, very large stored procedure. The recommendation had been to rewrite it; however, we did not have time. The orginal author had quit (dramatically too) after his code came under reveiw, but by then he had added too much code to rewrite by our deadline. Even though the IT department was generally pretty good, the company as a whole was pretty messed up.The DBA had been told that a rewrite was scheduled but for now we did not have time (plus it was just a report).

    Also, my changes were just to correct some spelling mistakes in the aliases

    BTW, this dba accidently deleted my DB (for a new application under development) a few times. Luckily, I had the foresight to backup



  • @morbiuswilters said:

    @this_code_sucks said:

    @morbiuswilters said:

    @this_code_sucks said:
    My code went into production without further review :) 

    That's pretty goddamn dysfunctional. Maybe better than being pestered by the inept DBA but "my company can't even manage to do code reviews" isn't something I'd smile over.

    Not really, it actually just got reviewed by another DBA

    Either it went into production without review or it didn't. If the former, I'd say that's a problem.

    It got reviewed, I lied for dramatic effect in my first post



  • @this_code_sucks said:

    @morbiuswilters said:
    @this_code_sucks said:

    @morbiuswilters said:

    @this_code_sucks said:
    My code went into production without further review :) 

    That's pretty goddamn dysfunctional. Maybe better than being pestered by the inept DBA but "my company can't even manage to do code reviews" isn't something I'd smile over.

    Not really, it actually just got reviewed by another DBA

    Either it went into production without review or it didn't. If the former, I'd say that's a problem.

    It got reviewed, I lied for dramatic effect in my first post

    gasp Lies, on my TDWTF!? For shame!



  • @morbiuswilters said:

    @this_code_sucks said:
    @morbiuswilters said:
    @this_code_sucks said:

    @morbiuswilters said:

    @this_code_sucks said:
    My code went into production without further review :) 

    That's pretty goddamn dysfunctional. Maybe better than being pestered by the inept DBA but "my company can't even manage to do code reviews" isn't something I'd smile over.

    Not really, it actually just got reviewed by another DBA

    Either it went into production without review or it didn't. If the former, I'd say that's a problem.

    It got reviewed, I lied for dramatic effect in my first post

    gasp Lies, on my TDWTF!? For shame!

    If I told you about the WTFs I face at my current job you would think code going into prod without review was best practice. I could make like any featured article or even snoofle's posts seem like no big deal :-/



  • @this_code_sucks said:

    If I told you about the WTFs I face at my current job you would think code going into prod without review was best practice. I could make like any featured article or even snoofle's posts seem like no big deal :-/

    But I can't trust a word out of your keyboard now. [turns away to peer out the window] Just... just go..



  • @Speakerphone Dude said:

    It does make sense to have a SQL expert review SQL code, even if he is not an expert in source control. And the guy is also right to review the whole stored procedure, and not just your changes, because it is a unit of code, it won't get partially executed in production. *You* may feel that your changes are not impacting something else, but there is a reason why *he* is the DBA.

    I'm of the opinion that the DBA shouldn't be responsible for reviewing SQL code by default; your senior developers ought to have enough SQL expertise to write and review sprocs.  The DBA ought to be busy doing SQL Servery stuff like maintaining server health, rebuilding indexes, performing backups, and trying to get the other half of the company off of MySql already, dammit.



  • @JimLahey said:

    "We don't do code reviews" said my new boss when I started. "We've got developers of very mixed abilities here and I don't want the juniors to feel bad about their work. I don't particularly mind how people do their work as long as it gets done and the quality is good".
    Your boss is obviously a "people's person", who cares about the sensetivities of his people and the harmony in his team. He also should be taken out and shot, because he and his people are judged on the quality of the output.

    We have a very similar situation here, where there's never any time to do code reviews. Nominally,  I should be doing that, but you know how it is... always more work than time.

    Now the previous manager has buggered off, and some developers have either followed suit or taken on a new role, and I have had to go through some of the code to make some changes or fix bugs.

    Oh. My. Goodness.

    My new manager agrees that we need to start doing code reviews, and I've decided that I'm going to do that. After all, I've written a document with a bunch of non-functional specifications when it comes to development, such as mandatory use of the PMD tool, but this document is largely ignored.

    Put it simply, you cannot trust the code of the people you work with.

     



  • No code review is one reason for failure of project.
    No testing is another.

    But # 1 reason for failure of project is client not ready to sing off on on deliveries that is previously agreed up on by him..



  • @JimLahey said:

    "...I don't want the juniors to feel bad about their work."

    @JimLahey said:

    Does it matter that the stick is a junior's arm that I've just torn off?

    There [i]are[/i] other ways...


  • Discourse touched me in a no-no place

    @Nagesh said:

    But # 1 reason for failure of project is client not ready to sing off on on deliveries that is previously agreed up on by him..
    What if they're terrible at karaoke?



  • @Severity One said:

    Put it simply, you cannot trust the code of the people you work with.



    +1

    Although, code reviews go a long way to increasing quality; there are some problems. First of all, often people doing the reviewing are now qualified to be in front of a key board. In my opinion a shity coder reviewing a mentally challeged coder doesn't go that far.

    Also, the biggest problems in code bases are not usually indvidual wtfy funtions, but rather all encompasing archetechures and design patterns that were agreed upon early in the project. For example, we all know it, we all love it, THE DOUBLE DATA ACCESS LAYER PATERN. One data access layer, one business layer with no business logic that mirrors the entities exactly. <sarcasm> Now that's how you build an xml parser :) </sarcasm>



  • @PJH said:

    @Nagesh said:
    But # 1 reason for failure of project is client not ready to sing off on on deliveries that is previously agreed up on by him..
    What if they're terrible at karaoke?
     

    Don't feed the troll.



  • @JoeCool said:

     I once worked with a guy who got filthier as the week went by, but was clean every Monday. I'm guessing he showered every Sunday night whether he needed it or not.

    Maybe his wife loved smell of his sweat?

Log in to reply