in reply to Refactoring technique?

Hello BrowserUk,

I look at the subroutine as providing a specific function, so I would name the input parameters as how they relate to the specific function. It really doesn't matter where the input values come from, but what is the purpose of the subroutine. So if the sub is going to build a box, then I would design the sub as: 

# my $graphic = buildbox( $x, $y, $z );
sub buildbox
{   my $length = shift; my $height = shift; my width = shift;
    ...
}

[download]
To the maintainer, it should be obvious what the subroutine does and not how it gets called! YMMV.

Regards...Ed

"Well done is better than well said." - Benjamin Franklin

Replies are listed 'Best First'.
Re^2: Refactoring technique?
by BrowserUk (Patriarch) on Apr 24, 2015 at 16:05 UTC
    It really doesn't matter where the input values come from, but what is the purpose of the subroutine.

    You're right! Until I can give doStuff( ... ): a proper name, the parameter names are always going to be a compromise.

    The problem is that the 6 blocks of code that will be replaced by the calls to doStuff(), do an aweful lot of stuff; so understanding all the stuff they do sufficiently to give it an appropriate name is quite a challenge. More so as I'm working in a language I'm only barely familiar with, and the author has made some ... what you might term politely .. dubious choices.

    Eg. The code is littered with calls to a function called _OAV(), which is annotated with //Object, Attribute, Value. It looks like this:

    function _OAV(oo,aa,vv) //Object, Attribute, Value
{ if (useSVG)
  { if (aa=="from")
    { with (oo) 
      { setAttribute("x1",vv.split(",")[0]);
        setAttribute("y1",vv.split(",")[1]);
      }
      return;
    }
    if (aa=="to")
    { with (oo) 
      { setAttribute("x2",vv.split(",")[0]);
        setAttribute("y2",vv.split(",")[1]);
      }
      return;      
    }
    if (aa=="color")
    { with (oo) setAttribute("fill",vv);
      return;
    }
    if (aa=="innerText") 
    { oo.firstChild.replaceData(0,108,vv);
      return;
    }    
    with (oo) setAttribute(aa,vv);
  }
  else
  { if (aa=="fill") oo.fillcolor=vv;
    if (aa=="stroke") oo.strokecolor=vv;
    if (aa=="stroke-width") oo.strokeweight=vv;
    if (aa=="visibility") oo.style.visibility=vv;
    if (aa=="from") oo.from=vv;
    if (aa=="to") oo.to=vv;
    if (aa=="x") oo.style.left=vv;
    if (aa=="y") oo.style.top=vv;  
    if (aa=="d") oo.path=vv;
    if (aa=="color") oo.style.color=vv;   
    if (aa=="innerText") oo.innerText=vv;    
  }      
}

    [download]

    Which is just horrible!

    With the rise and rise of 'Social' network sites: 'Computers are making people easier to use everyday'
    Examine what is said, not who speaks -- Silence betokens consent -- Love the truth but pardon error.
    "Science is about questioning the status quo. Questioning authority". I'm with torvalds on this
    In the absence of evidence, opinion is indistinguishable from prejudice. Agile (and TDD) debunked
[reply]
[d/l]
[select]

      Which is just horrible!
      Ewww. Agreed.

      It seems to me that the purpose of this is to dispatch the setting of attributes. Perhaps setAttrDispatcher( obj, attr, value ). Still ick. Longer (of course, that is hard not to be), and still confusing (where does useSVG come from, for example).

      update: small bit of cleanup and clarification

      --MidLifeXis

[reply]
        (where does useSVG come from, for example)

        Ah. That bit's gone from my version.

        The library supports both SVG and VML, and useSVG is a global that is set either manually at the topmost level or by detecting the useragaent. I'm not interested in supporting old IEs, so I've already stripped all the useSVG and VML stuff out.

        I've also separated out all the branches of the sub and at this point that sub reduces to:

        function _OAV( oo,aa,vv ) { //Object, Attribute, Value
    oo.setAttribute( aa,vv );
}   

function fromOV( oo, vv ) {
    oo.setAttribute( "x1", vv.split( "," )[0] );
    oo.setAttribute( "y1", vv.split( "," )[1] );
}

function toOV( oo, vv ) {
    oo.setAttribute( "x2", vv.split( "," )[0] );
    oo.setAttribute( "y2", vv.split( "," )[1] );
}

function colorOV( oo, vv ) {
    oo.setAttribute( "fill", vv );
}

function innerTextOV( oo, vv ) {
    oo.firstChild.replaceData( 0, 108, vv );
}

        [download]

        Most of those will be inlined back into the main code; with only the fromOV() & toOV() which set two attributes left to deal with. The current practice of concatenating the two values together in the calling code and then splitting them in the functions will go. Not sure beyond that.

        With the rise and rise of 'Social' network sites: 'Computers are making people easier to use everyday'
        Examine what is said, not who speaks -- Silence betokens consent -- Love the truth but pardon error.
        "Science is about questioning the status quo. Questioning authority". I'm with torvalds on this
        In the absence of evidence, opinion is indistinguishable from prejudice. Agile (and TDD) debunked
[reply]
[d/l]

      BrowserUk,

      WOW! Now I understand!

      So the new sub would be: 

      # my $WhoKnows = SeeOriginalAuthorForMoreInformation( $oo, $aa, $vv );
+ 
sub SeeOriginalAuthorForMoreInformation
{   my $Object = shift; my $Attribute = shift; my $Value = shift;
    ...
}

      [download]
      I guess the author wants you to be a miracle worker :-)

      It wouldn't be so bad if you could move 'to' and 'from' to a different subroutine. Good Luck!

      Regards...Ed

      "Well done is better than well said." - Benjamin Franklin

[reply]
[d/l]

In Section Seekers of Perl Wisdom