• Visitors can check out the Forum FAQ by clicking this link. You have to register before you can post: click the REGISTER link above to proceed. To start viewing messages, select the forum that you want to visit from the selection below. View our Forum Privacy Policy.
  • Want to receive the latest contracting news and advice straight to your inbox? Sign up to the ContractorUK newsletter here. Every sign up will also be entered into a draw to WIN £100 Amazon vouchers!

Code reviews

Collapse
X
  •  
  • Filter
  • Time
  • Show
Clear All
new posts

    Code reviews

    Does anyone else experience dismay from permies code reviewing your code similar tho this?

    I work on a fixing something in their 5 method, 1 thousand line Java source file, and in the process end up splitting it up into 3 or 4 extra classes, with all of the new classes tested and in total now more like 25 - 30 methods so that you can actually read the code in english instead of needing to learn to read the matrix.

    Looks of dismay at code review time - apparently i've made the code more complicated!

    #2
    Just have to argue your case for the changes. Usually a good chance to improve things longer term if you do it well.
    While you're waiting, read the free novel we sent you. It's a Spanish story about a guy named 'Manual.'

    Comment


      #3
      They probably all knew the code inside out. If you redesigned it unnecessarily, I can understand them feeling like you're making their lives more difficult.

      Comment


        #4
        Don't forget that because the code is better in the conventional way does not mean that that is what they wanted. As MS pointed out, they might have liked the code the way it was and found it easy to use etc.
        "He's actually ripped" - Jared Padalecki

        https://youtu.be/l-PUnsCL590?list=PL...dNeCyi9a&t=615

        Comment


          #5
          Always ask permies before dealing with their tulip why their tulip is so bad.

          If two or more of them explain properly why it's so ****ed up leave well alone.
          "You’re just a bad memory who doesn’t know when to go away" JR

          Comment


            #6
            Originally posted by SpontaneousOrder View Post
            Looks of dismay at code review time - apparently i've made the code more complicated!
            If you've used two or three levels of inheritance, you undoubtedly have and they've every reason to be dismayed.

            Anyone who creates layer after layer of smart-ass inheritance should be taken out and shot before they can do any more damage!
            Work in the public sector? Read the IR35 FAQ here

            Comment


              #7
              Originally posted by OwlHoot View Post
              If you've used two or three levels of inheritance, you undoubtedly have and they've every reason to be dismayed.

              Anyone who creates layer after layer of smart-ass inheritance should be taken out and shot before they can do any more damage!
              Are the new classes called from multiple locations or just called once? If called once is there any plausible chance the functions could be reused directly in the future?

              Unless the answer to either question is Yes you are not going to win a popularity context by refactoring working code regardless of how tulip it is...
              merely at clientco for the entertainment

              Comment


                #8
                Also you need to provide us with a bit more context on the task you were asked to perform, was it just a 2/3 line bug fix in which case a complete redesign while you were in there may not have been the best thing to do , what was the test coverage like?, without it those kind of ad-hoc changes often comes back to bite you... I always try to get some buy-in from the stakeholders (and that usually means the permie devs and architect if there is one around) and make my case for such refactoring.

                Comment


                  #9
                  Originally posted by OwlHoot View Post
                  If you've used two or three levels of inheritance, you undoubtedly have and they've every reason to be dismayed.

                  Anyone who creates layer after layer of smart-ass inheritance should be taken out and shot before they can do any more damage!
                  No inheritance in this instance. I generally don't see many occasions where it's useful to use inheritance at all.

                  Just split the code up so that we have several classes doing 1 (or 2 - obviously cleaning up legacy code isn;t always a one-shot fix) thing each, instead of 1 class doing 5 things.

                  Likewise 25 methods doing 1 or 2 things, instead of 5 methods doing 5 things each.

                  Comment


                    #10
                    Originally posted by kal View Post
                    Also you need to provide us with a bit more context on the task you were asked to perform, was it just a 2/3 line bug fix in which case a complete redesign while you were in there may not have been the best thing to do , what was the test coverage like?, without it those kind of ad-hoc changes often comes back to bite you... I always try to get some buy-in from the stakeholders (and that usually means the permie devs and architect if there is one around) and make my case for such refactoring.
                    Yeah.. sorry - i didn't mean to necessarily get a proper analysis. It just feels odd because it's my first contract, and it feels funny trying to throw my weight around when i'm only there for 6 months. I've been a consultant before but in those cases i've had the name of my employer behind me; whereas i know that people are often very suspicious of freelance conractors because there are so many poor ones.

                    In this particular case i have in mind it *could* have been a few lines fix, but those few lines were in tangle of tulipe (a chunk of which turned out to be redundant after i'd cleaned up a little) that needed some deciphering. So by doing the boy-scout thing and leaving it a bit better than I found it the logic is clear and easy to understand (i was able to delete literally dozens of lines of comments), unit tested (which it wasn't before), and more flexible insofar as any future tweaks are concerned (i.e each class now has a single/few reason/s to change). Hopefully when the selenium tests run everything will still look as it should do.

                    Unfortunately they don;t do any pairing at this place. I've found that when pairing it's easier to break down the culture of writing 1000 line classes because you can have a chat as you do it and explain why you think doing something is a better idea. Otherwise when it comes to a code review I think it;s much easier for someone to just think "this isn't what i'd normally do - it must be wrong".

                    Comment

                    Working...
                    X