Skip to content
  • Categories
  • Recent
  • Tags
  • Popular
  • World
  • Users
  • Groups
Skins
  • Light
  • Cerulean
  • Cosmo
  • Flatly
  • Journal
  • Litera
  • Lumen
  • Lux
  • Materia
  • Minty
  • Morph
  • Pulse
  • Sandstone
  • Simplex
  • Sketchy
  • Spacelab
  • United
  • Yeti
  • Zephyr
  • Dark
  • Cyborg
  • Darkly
  • Quartz
  • Slate
  • Solar
  • Superhero
  • Vapor

  • Default (No Skin)
  • No Skin
Collapse
Code Project
  1. Home
  2. Other Discussions
  3. The Weird and The Wonderful
  4. That's what you get when you don't perform code reviews

That's what you get when you don't perform code reviews

Scheduled Pinned Locked Moved The Weird and The Wonderful
csstutorial
20 Posts 8 Posters 1 Views 1 Watching
  • Oldest to Newest
  • Newest to Oldest
  • Most Votes
Reply
  • Reply as topic
Log in to reply
This topic has been deleted. Only users with topic management privileges can see it.
  • L Lost User

    I'm no longer allowed to pass a Tuple-parameter after my last review :-\ So I now create a specific object:

    class MyList: List<Tuple<int, string, string>> { }

    -- Joking aside, the [ProcessStartInfo](https://msdn.microsoft.com/nl-nl/library/system.diagnostics.processstartinfo%28v=vs.110%29.aspx)[[^](https://msdn.microsoft.com/nl-nl/library/system.diagnostics.processstartinfo%28v=vs.110%29.aspx "New Window")] class would be a nice example of limiting the amount of parameters required, while still keeping it readable.

    Bastard Programmer from Hell :suss: If you can't read my code, try converting it here[^][](X-Clacks-Overhead: GNU Terry Pratchett)

    S Offline
    S Offline
    Sascha Lefevre
    wrote on last edited by
    #8

    :laugh:

    If the brain were so simple we could understand it, we would be so simple we couldn't. — Lyall Watson

    1 Reply Last reply
    0
    • M mav northwind

      A former employee of ours left some really wonderful smells in our code, for example a class with no less than 8 constructor parameters. And right now I stumbled across a method named TryGetUsergroupByNameOrTostringDss(). Thanks BH, you know who you are... I guess it's my fault as well - now I can see how important code reviews for every code would have been. But it's too late to mourn now and I have to clean out the manure on my own :(

      Regards, mav -- Black holes are the places where God divided by 0...

      K Offline
      K Offline
      KarstenK
      wrote on last edited by
      #9

      he wrote a lot. Compacter is:

      //Usergroups or dss
      Users()

      As master Yoda says: "Make it or not. Dont: try" :rolleyes:

      Press F1 for help or google it. Greetings from Germany

      1 Reply Last reply
      0
      • L Lost User

        ..though I would be recommending to encapsulate them in a specific parameter-class and pass a single object :)

        Bastard Programmer from Hell :suss: If you can't read my code, try converting it here[^][](X-Clacks-Overhead: GNU Terry Pratchett)

        realJSOPR Offline
        realJSOPR Offline
        realJSOP
        wrote on last edited by
        #10

        I really think it depends on the requirements of the object and how it fits into the existing code's style.

        ".45 ACP - because shooting twice is just silly" - JSOP, 2010
        -----
        You can never have too much ammo - unless you're swimming, or on fire. - JSOP, 2010
        -----
        When you pry the gun from my cold dead hands, be careful - the barrel will be very hot. - JSOP, 2013

        L 1 Reply Last reply
        0
        • M mav northwind

          It's the only constructor and none of the parameters are optional... Oh, and did I mention that the whole class has exactly 0 comments?

          Regards, mav -- Black holes are the places where God divided by 0...

          F Offline
          F Offline
          Foothill
          wrote on last edited by
          #11

          I've got a class with 12 constructor parameters but the class represents a single row of data from a database so it is a discrete object. I also did full xml comments on everything so I don't forget why I wrote it. Would that be flagged in a code review?

          if (Object.DividedByZero == true) { Universe.Implode(); } Meus ratio ex fortis machina. Simplicitatis de formae ac munus. -Foothill, 2016

          realJSOPR 1 Reply Last reply
          0
          • realJSOPR realJSOP

            I really think it depends on the requirements of the object and how it fits into the existing code's style.

            ".45 ACP - because shooting twice is just silly" - JSOP, 2010
            -----
            You can never have too much ammo - unless you're swimming, or on fire. - JSOP, 2010
            -----
            When you pry the gun from my cold dead hands, be careful - the barrel will be very hot. - JSOP, 2013

            L Offline
            L Offline
            Lost User
            wrote on last edited by
            #12

            John Simmons / outlaw programmer wrote:

            I really think it depends on the requirements of the object and how it fits into the existing code's style.

            I think it depends on the amount of switches. Easy example in the ProcessStartInfo[^] class. It does not depend on how it fits into the "style". Show me a more readable version of the same using a constructor and a param for each switch, then I'll accept it may depend on requirements.

            Bastard Programmer from Hell :suss: If you can't read my code, try converting it here[^][](X-Clacks-Overhead: GNU Terry Pratchett)

            realJSOPR 1 Reply Last reply
            0
            • L Lost User

              John Simmons / outlaw programmer wrote:

              I really think it depends on the requirements of the object and how it fits into the existing code's style.

              I think it depends on the amount of switches. Easy example in the ProcessStartInfo[^] class. It does not depend on how it fits into the "style". Show me a more readable version of the same using a constructor and a param for each switch, then I'll accept it may depend on requirements.

              Bastard Programmer from Hell :suss: If you can't read my code, try converting it here[^][](X-Clacks-Overhead: GNU Terry Pratchett)

              realJSOPR Offline
              realJSOPR Offline
              realJSOP
              wrote on last edited by
              #13

              Matching the current coding style goes a long way towards "readability". Besides, I'm not here to argue semantics. I merely stated that there's nothing necessarily wrong with a constructor with eight parameters. There are often many more considerations than "I think this code sucks". I'm not really interested enough to list all the ones I can think of right off the top of my head.

              ".45 ACP - because shooting twice is just silly" - JSOP, 2010
              -----
              You can never have too much ammo - unless you're swimming, or on fire. - JSOP, 2010
              -----
              When you pry the gun from my cold dead hands, be careful - the barrel will be very hot. - JSOP, 2013

              L 1 Reply Last reply
              0
              • D Duncan Edwards Jones

                unless the OP means 8 overloaded constructors with different parameter signatures? (in which case defaults could definitely help)

                realJSOPR Offline
                realJSOPR Offline
                realJSOP
                wrote on last edited by
                #14

                He never said anything about overloads, so I think it's safe to assume there is one constructor with eight parameters.

                ".45 ACP - because shooting twice is just silly" - JSOP, 2010
                -----
                You can never have too much ammo - unless you're swimming, or on fire. - JSOP, 2010
                -----
                When you pry the gun from my cold dead hands, be careful - the barrel will be very hot. - JSOP, 2013

                1 Reply Last reply
                0
                • F Foothill

                  I've got a class with 12 constructor parameters but the class represents a single row of data from a database so it is a discrete object. I also did full xml comments on everything so I don't forget why I wrote it. Would that be flagged in a code review?

                  if (Object.DividedByZero == true) { Universe.Implode(); } Meus ratio ex fortis machina. Simplicitatis de formae ac munus. -Foothill, 2016

                  realJSOPR Offline
                  realJSOPR Offline
                  realJSOP
                  wrote on last edited by
                  #15

                  Foothill wrote:

                  I've got a class with 12 constructor parameters but the class represents a single row of data from a database

                  I don't do that with column data anymore unless I need the object to be initialized a certain way. I just use object initializers.

                  ".45 ACP - because shooting twice is just silly" - JSOP, 2010
                  -----
                  You can never have too much ammo - unless you're swimming, or on fire. - JSOP, 2010
                  -----
                  When you pry the gun from my cold dead hands, be careful - the barrel will be very hot. - JSOP, 2013

                  1 Reply Last reply
                  0
                  • realJSOPR realJSOP

                    Matching the current coding style goes a long way towards "readability". Besides, I'm not here to argue semantics. I merely stated that there's nothing necessarily wrong with a constructor with eight parameters. There are often many more considerations than "I think this code sucks". I'm not really interested enough to list all the ones I can think of right off the top of my head.

                    ".45 ACP - because shooting twice is just silly" - JSOP, 2010
                    -----
                    You can never have too much ammo - unless you're swimming, or on fire. - JSOP, 2010
                    -----
                    When you pry the gun from my cold dead hands, be careful - the barrel will be very hot. - JSOP, 2013

                    L Offline
                    L Offline
                    Lost User
                    wrote on last edited by
                    #16

                    John Simmons / outlaw programmer wrote:

                    Matching the current coding style goes a long way towards "readability".

                    So you'd prefer to implement that same class as a constructor with 16 switches? Show me a "current coding style" where that is more readable than a single class as described on MSDN :)

                    John Simmons / outlaw programmer wrote:

                    I'm not really interested enough to list all the ones I can think of right off the top of my head.

                    I did not ask you to.

                    Bastard Programmer from Hell :suss: If you can't read my code, try converting it here[^][](X-Clacks-Overhead: GNU Terry Pratchett)

                    1 Reply Last reply
                    0
                    • L Lost User

                      ..though I would be recommending to encapsulate them in a specific parameter-class and pass a single object :)

                      Bastard Programmer from Hell :suss: If you can't read my code, try converting it here[^][](X-Clacks-Overhead: GNU Terry Pratchett)

                      L Offline
                      L Offline
                      Lost User
                      wrote on last edited by
                      #17

                      Yes! And you can try to provide alternative constructors with fewer parameters. Do you really always need 8 parameters?

                      Get me coffee and no one gets hurt!

                      L 1 Reply Last reply
                      0
                      • M mav northwind

                        A former employee of ours left some really wonderful smells in our code, for example a class with no less than 8 constructor parameters. And right now I stumbled across a method named TryGetUsergroupByNameOrTostringDss(). Thanks BH, you know who you are... I guess it's my fault as well - now I can see how important code reviews for every code would have been. But it's too late to mourn now and I have to clean out the manure on my own :(

                        Regards, mav -- Black holes are the places where God divided by 0...

                        L Offline
                        L Offline
                        Lost User
                        wrote on last edited by
                        #18

                        Not to make fun of your signature:

                        Quote:

                        Black holes are the places where God divided by 0

                        But: Yes! And the minds of some of the sunshines in Q&A are where He multiplied by zero!

                        Get me coffee and no one gets hurt!

                        1 Reply Last reply
                        0
                        • L Lost User

                          Yes! And you can try to provide alternative constructors with fewer parameters. Do you really always need 8 parameters?

                          Get me coffee and no one gets hurt!

                          L Offline
                          L Offline
                          Lost User
                          wrote on last edited by
                          #19

                          Taking the same ProcessStartInfo class as an example. Yes, I need those 16 switches, and no, it would not become more readable if you provided an overloaded version of each possible combination.

                          Bastard Programmer from Hell :suss: If you can't read my code, try converting it here[^][](X-Clacks-Overhead: GNU Terry Pratchett)

                          1 Reply Last reply
                          0
                          • M mav northwind

                            It's the only constructor and none of the parameters are optional... Oh, and did I mention that the whole class has exactly 0 comments?

                            Regards, mav -- Black holes are the places where God divided by 0...

                            Sander RosselS Offline
                            Sander RosselS Offline
                            Sander Rossel
                            wrote on last edited by
                            #20

                            mav.northwind wrote:

                            the whole class has exactly 0 comments

                            That's exactly how much comments code should have! :D Great code requires no comments. Most comments are bad anyway. I'm so allergic to comments I even wrote a tip on why you shouldn't comment (and the rare case you should): Write comments that matter[^]

                            Visit my blog at Sander's bits - Writing the code you need. Or read my articles at my CodeProject profile.

                            Simplicity is prerequisite for reliability. — Edsger W. Dijkstra

                            Regards, Sander

                            1 Reply Last reply
                            0
                            Reply
                            • Reply as topic
                            Log in to reply
                            • Oldest to Newest
                            • Newest to Oldest
                            • Most Votes


                            • Login

                            • Don't have an account? Register

                            • Login or register to search.
                            • First post
                              Last post
                            0
                            • Categories
                            • Recent
                            • Tags
                            • Popular
                            • World
                            • Users
                            • Groups