Refactor This!




  • [int a loop]
                    if(i==0){i=1;switch=false;}else{i=0;switch=false;}
                    if(i == 0) {
                        control.Text = i.ToString();
                        i++;
                    } else {
                        control.Text = i.ToString();
                        i=0;
                    }
                    switch=true;


    A part of me weeps whenever I see this sort of thing.

    Every time you put a duplicate line of code at the top (or bottom) of each clause in an if statement, an angel gets set on fire.



  • <font face="Courier New">control.Text = i.ToString();
    if(i != 0) i=1;
    switch = true;

    <font face="Times New Roman">I think that's everything...   assuming that's the whole loop (which I doubt) the switch was totally worthless.  what's up with all the line noise?  seriously that looks like it was written just to make it harder to read.</font>
    </font>



  • <font face="Courier New"><font face="Times New Roman">I think that first line should be:

    </font>control.Text = ((i==0)?1:0).ToString();


    <font face="Times New Roman">I think. Talk about convoluted.
    </font></font>



  • I think this does the same as the original and about as easy to read as can be managed. 

    <FONT face="Courier New">if (i == 0)</FONT>

    <FONT face="Courier New">    control.Text = "1";</FONT>

    <FONT face="Courier New">else {</FONT>

    <FONT face="Courier New">    control.Text = "0";</FONT>

    <FONT face="Courier New">    i = 1;</FONT>

    <FONT face="Courier New">} </FONT>

    <FONT face="Courier New">switch = true;</FONT>

     

    Or should it be refactored to use a more service oriented architecture?



  • 0: i = 1 - i;
    1: control.Text = i.ToString();
    2: i = 1 - i;



    Switch

    is always set to True, and is never used in its False state, so I think
    it is forgotten code. Switch is completely redundant, and properly
    refactored it will be declared True before the loop.

    1. Invert i. If it is 0, it will be 1. If it is 1, it will be 0. Oh, and set Switch to False.
    2. Put i into the control. Then invert it [i]right back[/i].




    So this loop continuously inverts the value of the control and even
    number of times, but the control's value will always be the inverted i.



    I'm keeping line 2, because there may be future uses of i. If there are no future uses of i, scrap line 2.

    Hey, perhaps the Switch was meant to switch the loop off? As in, [code]if (switch) break;[/code]? Are we seeing all of the code?



  • PS.
    Even if a post's edit time has expired, you can still delete it and repost. :)

    Yay



  • @dhromed said:

    PS.
    Even if a post's edit time has expired, you can still delete it and repost. :)

    Yay

    Ooooh nifty workaround

    Still, I wonder why there's a frigging edit timeout in the first place...



  • <font face="Times New Roman" size="3">Funnily enough, some of the refactorers<font face="Arial" size="1">[1]</font> haven't quite got it right.



    try:
    <font face="Courier New" size="2">       </font><font size="2"></font><font face="Courier New" size="2">control.Text = (!i).ToString();</font>



    [1] is "refactorers" a word?



    Michael J






    </font>



  • No, that's hardly all the code. It's (naturally) a far more significant beast. The "loop" here is really the ItemDataBound callback for an ASP repeater. The exciting part of this is that "Switch" is an object field, as is "i" [shudders].

    All things considered, I went with


    control.Text = (i==0)?"1":"0";
    Switch=true;


    Incidentally-- the comment for this block of code is "set the alternating color". It's WTF-tastic.




  • @Poultine said:


    All things considered, I went with

    control.Text = (i==0)?"1":"0";
    Switch=true;
    Incidentally-- the comment for this block of code is "set the alternating color". It's WTF-tastic.


    The most expedient way of inverting a [1-0] combination is 1- i, though the speed difference might be negligible unless you do it about 10,000 times.

    I learned that here.

    Whee.

    @Michael J said:
    <font face="Times New Roman" size="3">Funnily enough, some of the refactorers<font face="Arial" size="1">[1]</font> haven't quite got it right.

    try:
    <font face="Courier New" size="2">       </font><font face="Courier New" size="2">control.Text = (!i).ToString();</font>

    </font>




    Not all languages allow you to boolean-invert an integer.



    I say that refactorers is a word.



  • @dhromed said:


    The most expedient way of inverting a [1-0] combination is 1- i, though the speed difference might be negligible unless you do it about 10,000 times.


    Huh? What about i = i^1?

    More importantly, are you counting the integer to string conversion in there as well? I would expect the tertiary (i==0) ? "1" : "0" to run much faster than anything else, though it will soak up a bit more memory for the code and constants.

    And yes, as far as keeping a low refactoring-profile on this, I'm not guaranteeing that if i is not in (0, 1) then it will be set to 1, but... I'm really OK with that.



  • @dhromed said:


    Switch
    is always set to True, and is never used in its False state, so I think
    it is forgotten code. Switch is completely redundant, and properly
    refactored it will be declared True before the loop.

    Hey,
    perhaps the Switch was meant to switch the loop off? As in, [code]if
    (switch) break;[/code]? Are we seeing all of the code?


    I have this uneasy feeling that switch may be a global that controls
    the behavior of ToString or something.  I've seen such things done
    before.



  • @Poultine said:


    [int a loop]
                    if(i==0){i=1;switch=false;}else{i=0;switch=false;}
                    if(i == 0) {
                        control.Text = i.ToString();
                        i++;
                    } else {
                        control.Text = i.ToString();
                        i=0;
                    }
                    switch=true;


    A part of me weeps whenever I see this sort of thing.

    Every time you put a duplicate line of code at the top (or bottom) of each clause in an if statement, an angel gets set on fire.

    This code does something more than control.Text= (!i).ToString(), it also changes i to !!i. (If it was 5, it becomes 1). Also I'd bet that 'switch' is some crazy way of implementing global lock for the i variable so it won't get changed (or maybe even read) during the operation. So my gues is:
    lock(i){
      control.Text= i?"0":"1";
      i=!!i;
    }



  • Oh, no. "Switch" doesn't do anything so clever as all that. There are about five similar blocks of code in this particular class and some of them check Switch:

                    if(Switch==true){
                        //Switch is set on the previous line [poultine: no, it isn't]
                        if(i==0){
                            i=1;Switch=false;
                        }else{
                            i=0;Switch=false;
                        }
                    }
                    control.Text = i.ToString();

    At least that one makes more sense, since i actually changes its value each time through. That duplicated "Switch = false;" makes me want to gnaw my own arm off, though.

    And i never gets anything other than 1 or 0. It just really wants to be a bool.



  • @Poultine said:

    Huh? What about i = i^1?


    Uhm, i^1 = i. Maybe you meant i = (1-i)^1? I don't see how that could be faster than i = 1-i though..



  • XOR, not exponentiation.  And i ^= 1; is even terser, though not necessarily any more efficient.



  • Oh, yeah that makes sense, sorry. :)



  • Doesn't VB implement ^ as exponent?



  • @dhromed said:

    Doesn't VB implement ^ as exponent?


    It's been in every version of VB I ever used.



  • @dhromed said:

    Doesn't VB implement ^ as exponent?

    Probably; I think you have to use Xor instead:


    i = i Xor 1


Log in to reply