When you know it's bad code, but can't articulate WHY



  • You're trying to decipher WTF this 5000 lines code is supposed to do, without benefit of documentation, comments, or even sensible variable names. There is a slew of single-character variable names and methods calling other methods of the same name in different classes with similar names.. And management wants to know why you are tearing your hair out...

    How do you articulate that??  Did I just do it? I'm paralyzed by the sheer burden of deciphering this code, much less attempting to maintain it.


  • Trolleybus Mechanic

    @amyb said:

    How do you articulate that??
     

    Is your management even slightly literate with code?  Pick a particular CodeSOD-worthy block, show it to them.  Let them know there's 1000x more you're plowing through.


  • Considered Harmful

    @Lorne Kates said:

    @amyb said:

    How do you articulate that??
     

    Is your management even slightly literate with code?  Pick a particular CodeSOD-worthy block, show it to them.  Let them know there's 1000x more you're plowing through.


    We have some 10000+ LOC outsourced, which has a bugs-to-lines ratio greater than 1.0. I've been able to show management lines code like:
    			string fileNameWithoutExtn =
    fileNameWithExtnFromXML.Substring( 0, fileNameWithExtnFromXML.LastIndexOf( "." ) ).Replace( ",", "" ).Replace( ",", "" ).Replace( "'", "" ).Replace( ".", " " ).Replace( "™", "" ).Replace( "(", "" ).Replace( ")", "" ).Replace( "®", "" ).Replace( "&", "" ).Replace( "!", "" );

    		fileNameWithoutExtn = Regex.Replace( fileNameWithoutExtn, "\\s", "spacespace" );
    		fileNameWithoutExtn = Regex.Replace( fileNameWithoutExtn, "\\W", "" ).Replace( "spacespace", " " ).Trim();
    
    		string sanitizedName = string.Concat( fileNameWithoutExtn.Trim().Split( invalidcharacters ) );
    
    		return sanitizedName;
    

    and they seemed to understand how bad that was.

    Yes, that is actual, real, non-anonymized code.



  • Go to your local puzzle store and buy some jigsaw puzzles.

    Put one of those single-color 5,000-piece monsters on the conference table and say, "this is the code we have".

    Then get one with the same number of pieces, but has a good photo on it and say, "this is code with variable names and comments".

    Then get a 250-piece puzzle and say, "this is code with reasonable function sizes, variable names, and comments".

    Then grab one of those 25-piece puzzles for infants and say, "here's what it would be if we had documentation also".

    Then say, "every feature request we get, I have to solve the puzzle before I can even start working on it".

    Then expense the puzzles.



  • @blakeyrat said:

    Then expense the puzzles.
     

    Then do the puzzles, because it's rather relaxing, really.



  • @dhromed said:

    Then do the puzzles, because it's rather relaxing, really.

    And bill every single hour to "training".



  •  I've been a developer for fourteen years and I still can't find the right way to articulate what is wrong with everything. Saying it is "inefficient" isn't good enough because the first response is "well, does it work?" Sure, it works. It just doesn't work...well.



  • @blakeyrat said:

    Go to your local puzzle store and buy some jigsaw puzzles...
    I like this. I really, really like it, although I might be inclined to start at the simple -- this is what good code is like; take away the documentation and it's like this. I think this may just be the best post of blakey's that I've read.

     



  • @blakeyrat said:

    (using jigsaw puzzles to demonstrate a point)
     

    Brillant!

    I'd probably present the first puzzle without the photo, just to illustrate the frequently-mistaken notion of "you can understand what the program does by looking at the source".

    But other than that... love the concept. This your idea originally?

     


  • Considered Harmful

    @dhromed said:

    @blakeyrat said:

    Then expense the puzzles.
     

    Then do the puzzles, because it's rather relaxing, really.


    We had some puzzles in the breakroom, it was noticed that the UI designers would sort and match by color, while developers would sort and match by shape. I'm not sure what implications if any this has.



  • @Darsithis said:

    "well, does it work?"
     

    "yes, it does work. But without documentation, I've no idea how it works. And with the way it's been designed, I've no idea how to fix it when it stops working. And the way it's been built, I can't guarantee it won't stop working as soon as I change something.

    So should we just leave it alone until it breaks then throw it away in puzzlement... or should we invest spend some time now in decyphering and understanding it better, so that later on we are able to modify/maintain/fix it?"



  • @joe.edwards said:

    it was noticed that the UI designers would sort and match by color, while developers would sort and match by shape. I'm not sure what implications if any this has.
     

    you could spot who was in the wrong job...



  • @Cassidy said:

    This your idea originally?

    As far as I know.



  • @blakeyrat said:

    @Cassidy said:
    This your idea originally?

    As far as I know.

     

    It's a good one.

    Mind if I quote it to stress the importance of maintainability and documentation when teaching programming courses?



  • You have to pay me a BILLION DOLLARS IN ROYALTIES!

    Yeah of course use it, I don't care.



  • @blakeyrat said:

    You have to pay me a BILLION DOLLARS IN ROYALTIES!
     

    Done! Just awaiting the wife of a Nicaraguan president to release some funds to me. Should be any day now...

    @blakeyrat said:

    Yeah of course use it, I don't care.

    Cheers, ta.

     



  • Hahaha! Genius :) 

     

    @blakeyrat said:

    Go to your local puzzle store and buy some jigsaw puzzles.

    Put one of those single-color 5,000-piece monsters on the conference table and say, "this is the code we have".

    Then get one with the same number of pieces, but has a good photo on it and say, "this is code with variable names and comments".

    Then get a 250-piece puzzle and say, "this is code with reasonable function sizes, variable names, and comments".

    Then grab one of those 25-piece puzzles for infants and say, "here's what it would be if we had documentation also".

    Then say, "every feature request we get, I have to solve the puzzle before I can even start working on it".

    Then expense the puzzles.


     



  • Here is an example: it's from a Zend Framework-driven site. This is from an action helper set up to be called from the page controller in order to display a series of 'widgets'

    There are about 50,000 more lines of code like this, and the codebase is growing exponentially as they duplicate entire blocks of code, rewrite processes that are already handled elsewhere, and generally reinvent the wheel.

    I've changed variable and function names to sort-of protect the identity of the guilty party. 

     The data is being pulled from a MySQL database.

     

    /**
      *  Widgets Helper Class
      *
      *  provides Widget functions to controllers and related helper classes
      *
      *  @copyright:  xxxxxxxxxxxxxxxxx
      *  @version:  $Id:$
      *  @package:  xxxxxxxxxxxxxxx
      *  @subpackage:  xxxxxxxxxxxxxx
      *  @author:  xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx
      */
      class Somenamespace_Helper_Widgets extends Zend_Controller_Action_Helper_Abstract {

        public $_pc;

        /**
        *  class contstruct function
        *
        *  set up main private|public variables/resources
        *
        *  @param:  none
        *  @return:  none
        */
        public function __construct() {
          //  load common plugin
          $this->pc = new Somenamespace_Plugin_Common();
        }

        /**
        *  ???
        *
        *  ???
        *
        *  @param:  none
        *  @return:  none
        */
        public function formatWidgetData($WidgetData, $imageMapType, $timThumbParams, $WidgetAreaId) {

          $singleImageBlock = <<<HTML
            <div class="Widget-content">
              <div class="Widget-img-bg">%s</div>
            </div>
    HTML;
          $singleImageWContentBlock = <<<HTML
            <div class="Widget-content">
              <div class="Widget-img-bg">%s</div>
              <h5>%s</h5>
              <p>%s</p>
            </div>
    HTML;
          $sliderImageBlock = <<<HTML
            <li>%s</li>
    HTML;
          $imageSource = <<<HTML
            <img usemap="#image_map_%s" src="%s" />
    HTML;
          $mapSource = <<<HTML
            <map name="image_map_%s">%s</map>
    HTML;
          $areaSource = <<<HTML
            <area coords="%s" shape="rect" title="%s" href="%s" />
    HTML;
          $WidgetsHtml = '';
          for ($i = 0; $i < count($WidgetData); $i++) {
            $eachWidget = $WidgetData[$i];
            $eachWidgetLinks = $eachWidget['image_links'];
            $areaHtml = '';
            for ($j = 0; $j < count($eachWidgetLinks); $j++) {
              $eachLink = $eachWidgetLinks[$j];
              //  handle area coordinates
              if ((trim($eachLink['x']) != '') && (trim($eachLink['y']) != '') &&
                  (trim($eachLink['x2']) != '') && (trim($eachLink['y2']) != '')) {
                $areaCoords =
                  implode(',',
                    array($eachLink['x'], $eachLink['y'], $eachLink['x2'], $eachLink['y2'])
                    );
              } else {  //  no hotspot
                $areaCoords =
                  implode(',',
                    array(0, 0, 0, 0)
                    );
              }
              //  handle area links
              if (!empty($eachLink['link_url'])) {
                $areaLink = $eachLink['link_url'];
              } else {  //  no link
                $areaLink = 'javascript:void(0);';
              }
              $areaHtml .=
                sprintf($areaSource,
                  $areaCoords,
                  $eachLink['title_caption'],
                  $areaLink
                  );
            }
            switch($imageMapType) {
              case 'slider-images':
                $WidgetsHtml .=
                  sprintf($sliderImageBlock,
                    sprintf($imageSource,
                      implode('_', array(str_replace('-', '_', $WidgetAreaId), $i)),
                      $timThumbParams . $eachWidget['image_file']
                      ) .
                    sprintf($mapSource,
                      implode('_', array(str_replace('-', '_', $WidgetAreaId), $i)),
                      $areaHtml
                      )
                  );
                break;
              case 'single-image':
                $WidgetsHtml .=
                  sprintf($singleImageBlock,
                    sprintf($imageSource,
                      implode('_', array(str_replace('-', '_', $WidgetAreaId), $i)),
                      $timThumbParams . $eachWidget['image_file']
                      ) .
                    sprintf($mapSource,
                      implode('_', array(str_replace('-', '_', $WidgetAreaId), $i)),
                      $areaHtml
                      )
                  );
                break;
              case 'single-image-w-content':
                $WidgetsHtml .=
                  sprintf($singleImageWContentBlock,
                    sprintf($imageSource,
                      implode('_', array(str_replace('-', '_', $WidgetAreaId), $i)),
                      $timThumbParams . $eachWidget['image_file']
                      ) .
                    sprintf($mapSource,
                      implode('_', array(str_replace('-', '_', $WidgetAreaId), $i)),
                      $areaHtml
                      ),
                    $this->pc->decodedHtml($eachWidget['content_title']),
                    $this->pc->decodedHtml($eachWidget['content_text'])
                  );
                break;
            }
          }
          return $WidgetsHtml;
        }


        /**
         *  ???
         *
         *  ???
         *
         *  @param:  none
         *  @return:  none
         */
        public function getWidgetData($WidgetAreaId) {

          $WidgetMapsModel = new Global_Model_SiteWidgettionalMaps();
          $WidgetMapSettings = $WidgetMapsModel->getMappedWidgettionalSettings(SITE_ID, $WidgetAreaId);
          $imageMapType = $WidgetMapSettings['image_map_type'];

          //  have to put sliders types in specific order
          $setDisplayOrder = $imageMapType == 'slider-images' ? true : false;

          //  get content image constraint info
          $timThumbParams = $this->pc->timThumbImageParams(
            $WidgetMapSettings['image_setting_max_width'],
            $WidgetMapSettings['image_setting_max_height']
            );

          $WidgetsModel = new Global_Model_SiteWidgettionals();
          $WidgetSettings = $WidgetsModel->getWidgettionalSettings(SITE_ID, $WidgetAreaId);

          if (is_array($WidgetSettings) &&
              count($WidgetSettings) == 1) {
            $currentTimeStamp = time();
            $filteredWidgetData = array();
            $WidgetData = unserialize($WidgetSettings[0]['settings']);
            if ($setDisplayOrder) {
              $WidgetData = $this->pc->subKeySort($WidgetData['settings'], 'display_order');
            } else {
              $WidgetData = $WidgetData['settings'];
            }
            for ($i = 0; $i < count($WidgetData); $i++) {
              $eachWidget = $WidgetData[$i];
              if ($eachWidget['status'] == 1) {  //  status is enabled
                if (!empty($eachWidget['start_date']) &&
                    !empty($eachWidget['end_date'])) {  //  has date settings
                  if (($currentTimeStamp >= (int) $eachWidget['start_date']) &&
                      ($currentTimeStamp <= (int) $eachWidget['end_date'])) {  //  has date settings, and is current
                    $filteredWidgetData[] = $eachWidget;
                  }
                } else { //  does not have time settings
                  $filteredWidgetData[] = $eachWidget;
                }
              }
            }
            if ($setDisplayOrder) {
              $orderedData = array();
              $randomizedData = array();
              for ($i = 0; $i < count($filteredWidgetData); $i++) {
                $eachDataSet = $filteredWidgetData[$i];
                if ($eachDataSet['display_order'] == 0) {
                  $randomizedData[] = $eachDataSet;
                } else {
                  $orderedData[] = $eachDataSet;
                }
              }  //  end for
              shuffle($randomizedData);
              $filteredWidgetData = array_merge($orderedData, $randomizedData);
            }
            return array($filteredWidgetData, $imageMapType, $timThumbParams);
          } else {
            return array(false, false, false);
          }

        }

      }



  • We had some puzzles in the breakroom, it was noticed that the UI designers would sort and match by color, while developers would sort and match by shape. I'm not sure what implications if any this has.

    We have puzzles in the breakroom too :) I sort and match by color, then shape. ;)

     

     



  • We have some 10000+ LOC outsourced, which has a bugs-to-lines ratio greater than 1.0. I've been able to show management lines code like:
    			string fileNameWithoutExtn =
    				fileNameWithExtnFromXML.Substring( 0, fileNameWithExtnFromXML.LastIndexOf( "." ) ).Replace( ",", "" ).Replace( ",", "" ).Replace( "'", "" ).Replace( ".", " " ).Replace( "™", "" ).Replace( "(", "" ).Replace( ")", "" ).Replace( "®", "" ).Replace( "&", "" ).Replace( "!", "" );
    
    		fileNameWithoutExtn = Regex.Replace( fileNameWithoutExtn, "\\s", "spacespace" );
    		fileNameWithoutExtn = Regex.Replace( fileNameWithoutExtn, "\\W", "" ).Replace( "spacespace", " " ).Trim();
    
    		string sanitizedName = string.Concat( fileNameWithoutExtn.Trim().Split( invalidcharacters ) );
    
    		return sanitizedName;
    

    and they seemed to understand how bad that was.

    Yes, that is actual, real, non-anonymized code.

     

     

    My dear God... My poor eyes.

     



  • This is great -  although a realization hit me this morning after watching the latest checkins scroll by. To more accurately present my situation, I would need to buy 4 identical single-color puzzles, then dump them all on the conference table and mix them together.

    Except each of these puzzles would have ONE piece shaped differently, just to make sure they were 'unique' identical puzzles.

     

    @blakeyrat said:

    Go to your local puzzle store and buy some jigsaw puzzles.

    Put one of those single-color 5,000-piece monsters on the conference table and say, "this is the code we have".

    Then get one with the same number of pieces, but has a good photo on it and say, "this is code with variable names and comments".

    Then get a 250-piece puzzle and say, "this is code with reasonable function sizes, variable names, and comments".

    Then grab one of those 25-piece puzzles for infants and say, "here's what it would be if we had documentation also".

    Then say, "every feature request we get, I have to solve the puzzle before I can even start working on it".

    Then expense the puzzles.

     

     



  • Possibly relevant to this thread's interests:

    World's Most Frustrating Puzzle Has No Edges and 5 Extra Pieces


  • Considered Harmful

    Our first workplace puzzle was my contribution of Escher's Relativity. The disorienting perspectives and monochrome contributed to the fun.



  • @blakeyrat said:

    Possibly relevant to this thread's interests:

    World's Most Frustrating Puzzle Has No Edges and 5 Extra Pieces

     

    I'm game.

     


  • Considered Harmful

    What about a 2-sided puzzle, so there are now eight ways a piece can be oriented instead of four?



  • @joe.edwards said:

    What about a 2-sided puzzle, so there are now eight ways a piece can be oriented instead of four?
    if you've ever seen one of those then you'd know that it's easy which side should be on top (just look at how the edges where cut; the "top" has rounded edges while the bottom has sharp edges)



  • Or one of those puzzles where every piece is the exact same shape and you only have the image to go by.



  •  A puzzle that has been atomized into pigments and all you have is this finely pointed tool to put it together.

     

    Oh wait.



  • A puzzle that is made solely of raw potato chunks, and if you don't assemble it within four days it becomes an oozing pile of putrid liquid covered in worms and fruit flies.



  • @mott555 said:

    fruit flies

    Joke's on them.


  • Considered Harmful

    @Ben L. said:

    @mott555 said:
    fruit flies

    Joke's on them.


    Man, those flies sure are stupid!



  • @mott555 said:

    A puzzle that is made solely of raw potato chunks, and if you don't assemble it within four days it becomes an oozing pile of putrid liquid covered in worms and fruit flies.
    Or you could ferment it and turn it into vodka.



  • @Douglasac said:

    @mott555 said:
    A puzzle that is made solely of raw potato chunks, and if you don't assemble it within four days it becomes an oozing pile of putrid liquid covered in worms and fruit flies.
    Or you could ferment it and turn it into vodka.
    and then you'll get arrested for moonshining



  •  it sounds like the OP does not have a coding standard, or if they do it is not explicit enough.  Additionally this sort of thing should be trapped at code review.... " does not seem maintainable, lack of clear concise comments as well as variable naming means we would ask to rework parts and perhaps refactor"

     




  • @Helix said:

     it sounds like the OP does not have a coding standard, or if they do it is not explicit enough.  Additionally this sort of thing should be trapped at code review.... " does not seem maintainable, lack of clear concise comments as well as variable naming means we would ask to rework parts and perhaps refactor"

     


    It's funny how in real life when a conversation has been over for over a week, people have no clue what you're talking about because the conversation is already over.

    As opposed to here, where people have no idea what you're talking about because you wrote this document.



  • @Ben L. said:

    It's funny how in real life when a conversation has been over for over a
    week, people have no clue what you're talking about because the
    conversation is already over.


    As opposed to here, where people have no idea what you're talking about because you wrote this document.



    OK - Since you have been eating retard sandwiches, let me make it clear for you

     

    @amyb said:

    You're trying to decipher WTF this 5000
    lines code is supposed to do, without benefit of documentation,
    comments, or even sensible variable names. There is a slew of
    single-character variable names and methods calling other methods of the
    same name in different classes with similar names.. And management
    wants to know why you are tearing your hair out...

    How do you
    articulate that??  Did I just do it? I'm paralyzed by the sheer burden
    of deciphering this code, much less attempting to maintain it.


     

    @Helix said:

     it sounds like the OP does not have a coding standard, or if they do it is not explicit enough.  Additionally this sort of thing should be trapped at code review.... " does not seem maintainable, lack of clear concise comments as well as variable naming means we would ask to rework parts and perhaps refactor"

     


     


  • Considered Harmful

    @Ben L. said:

    As opposed to here, where people have no idea what you're talking about because you wrote this document.

    You should probably proofread that sometime.



  • @joe.edwards said:

    @Ben L. said:
    As opposed to here, where people have no idea what you're talking about because you wrote this document.

    You should probably proofread that sometime.

    psst



  • @Ben L. said:

    @joe.edwards said:
    @Ben L. said:
    As opposed to here, where people have no idea what you're talking about because you wrote this document.
    You should probably proofread that sometime.
    psst
     

     

    *Sigh* I miss blakeyrat


  • Considered Harmful

    @Ben L. said:

    @joe.edwards said:
    @Ben L. said:
    As opposed to here, where people have no idea what you're talking about because you wrote this document.

    You should probably proofread that sometime.

    psst

    That's a relief. For a minute I just thought you were batshit fucking crazy.



  • @joe.edwards said:

    @Ben L. said:
    @joe.edwards said:
    @Ben L. said:
    As opposed to here, where people have no idea what you're talking about because you wrote this document.

    You should probably proofread that sometime.

    psst

    That's a relief. For a minute I just thought you were batshit fucking crazy.

    No, he is. SciGEN is actually just Ben L. He writes all those so-called "auto-generated" papers by hand. Then he takes a picture on a wooden-table and runs it through some OCR software written in Go on his Chromebook.


Log in to reply