Is there any reason not to refactor this, making it simpler?

Question

I ran across this in the legacy codebase:

string[] deptNo = UPC.getDept().Split(new Char[] {'.'});

...and thought this would be more straightforward:

string[] deptNo = UPC.getDept().Split('.');

...but don't want to change it if there's a possibility that characterization of the dot does something "magic" that will cause the whole spaghetti house to come slithering down if I make this change.

UPDATE

In response to the cautions given: Yes, I know what you mean about the need to proceed with utmost caution whilst thrashing about in years-old spaghetti. A seemingly innocuous change to this code:

private void comboVendor_SelectedValueChanged(object sender, EventArgs e)
{
    try
    {
        if ((comboVendor.SelectedIndex >= 0) && (cmbVendor.Text != comboVendor.Text))
        {
            string t = comboVendor.Text; 
            t = t.Substring(0,maxVendorChar);
            t = t.Substring(0,substrLen);
            t = t.Trim();
            cmbVendor.Text = t;
            cmbVendor.Focus();
        }
    }
    catch (Exception ex)
    {
        Duckbill.ExceptionHandler(ex, "frmInv.comboVendor.SelectedValueChanged");
    }               
}

...namely, appending "Trim()" to the instances of ".Text",caused the err msg "Specified argument was out of the range of valid values." because "maxVendorChar" was not larger than the value of the string being substringed. So I had to change it to this to get it to work:

private void comboVendor_SelectedValueChanged(object sender, EventArgs e)
{
    int substrLen;
    try
    {
        if ((comboVendor.SelectedIndex >= 0) && (cmbVendor.Text.Trim() != comboVendor.Text.Trim()))
        {
            string t = comboVendor.Text.Trim(); 
            substrLen = GetLowerInt(t.Length, maxVendorChar);
            t = t.Substring(0,substrLen);
            t = t.Trim();
            cmbVendor.Text = t;
            cmbVendor.Focus();
        }
    }
    catch (Exception ex)
    {
        Platypus.ExceptionHandler(ex, "frmInv.comboVendor.SelectedValueChanged");
    }               
}

private int GetLowerInt(int first, int second)
{
    return first < second ? first : second;
}

Off in the distance, I heard a raven croaking "nevermore," but I'm not sure what he was talking about.

UPDATE 2

Whenever people note just how starchy-stringy-slippery this project is, they almost invariably recommend that I refactor it; however, as noted above, sometimes a little refactoring sets off a series of deafening explosions that makes me wish I had left ill enough alone. The real solution is to completely rewrite this project, with better methodologies and at least less-ancient technologies. That's my vision; for now, though, maintenance mode seems to be the order of the day.

Those 2 calling conventions are identical in C# when using `params` keyword, which `Split()` does. — lukegravitt, Sep 18 '13 at 21:44

score 3 · Answer 1 · answered Sep 18 '13 at 21:35

3

If you are dealing with "a big ball of mud", then it might be best to leave well enough alone (assuming it behaves correctly).

answered Sep 18 '13 at 21:35

Eric

1,737
1
13
17

"Big ball of mud" is an understatement; this code was written by several people over several years, all with different styles and at different skill levels. It's as if Justin Bieber, Lady Gaga, The Bay City Rollers, The Troggs, John Coltrane, and J.S. Bach were all jamming together with no rehearsal. – B. Clay Shannon-B. Crow Raven Sep 18 '13 at 21:38

score 2 · Accepted Answer · edited May 23 '17 at 12:20

2

Depends on whether you are looking for a philosophical reason or a technical reason.

There is no technical reason. There is no magic, and the two statements are the same. The method parameter for split uses the params keyword to allow the variable number of arguments syntax:

public string[] Split(params char[] separator)

See this discussion on the purpose of adding the params keyword to your method parameters: Why use the params keyword?

Philosophically, there might not be any real benefit to refactoring it. As a general rule, you probably don't want to clean up just because you can, especially without tests.

edited May 23 '17 at 12:20

Community

1
1

answered Sep 18 '13 at 21:57

Steve Clanton

4,064
3
32
38

1

Great idea about unit testing (every time I make a change to this code, I cover my head, dive underneath my desk, close my eyes, and think about Winnie the Pooh). However: I can't run this app on the desktop - I must compile it in XP Mode, copy it (and its ancillary DLL from the same solution) to a folder my Windows 7 drive, copy it from there to the handheld device, and run it there. I can't add more stuff to the handheld device because it's already packed fuller than Takeru Kobayashi after a hot dog eating contest. Any suggestions on how I can implement testing in my predicament? – B. Clay Shannon-B. Crow Raven Sep 19 '13 at 17:19
1

That's one of those really hard questions--one that doesn't have a general solution. The goal for automated test is to do them in your development environment. There are techniques lots of techniques, but I'd try posting another question giving some details with the the right tags to get the best answer. It's way too big a question for a comment. – Steve Clanton Sep 19 '13 at 17:58

score 1 · Answer 3 · answered Sep 18 '13 at 21:25

1

Sounds like you need some unit tests to give you the required confidence in your code to refactor. The best practice would be to add them before refactoring.

We can't see the method signature for the Split() method, but if it uses varargs then your refactor is theoretically safe.

answered Sep 18 '13 at 21:25

Asaph

159,146
25
197
199

What "varargs" are? is it var args maybe? – Yair Nevet Sep 18 '13 at 21:31

mcy · Answer 4 · 2013-09-18T21:48:06.507

1

Assuming that getDept() returns a string, the first approach has the advantage of including new chars as splitter easier. You just add them to the char array. The second approach fixes the delimiting character. In the second approach, '.' is also a char, unlike ".", which is a string. If the method getDept() returns a custom class which has a method of Split(char[] splitters), you will be in trouble unless the method has an overload of Split(char splitter). On a totally non-related but important point, it might be a good idea to implement a version control system so that when something fails, you can just 're-roll' the changes (i.e. return to a previous 'commit').

edited Sep 18 '13 at 21:48

answered Sep 18 '13 at 21:39

mcy

1,209
6
25
37

We are using version control, although I can't say it works all that well (IOW: I don't trust it, and often save off text files and name them frmEntry_20130918.txt and such. Oftentimes I've checked out a file when my cohort already had it checked out, but the system didn't tell me that (TFS in XP Mode, Visual Studio 2003). – B. Clay Shannon-B. Crow Raven Sep 18 '13 at 21:41
I see. then using your own ways are always a better option – mcy Sep 18 '13 at 21:44

score 1 · Answer 5 · answered Sep 18 '13 at 21:53

1

I see no value in re-factoring this code since I can't see any added value here for that work.

Moreover, I would have leave the current code also because it would allowed me to add more delimiters in the futures.

answered Sep 18 '13 at 21:53

Yair Nevet

12,725
14
66
108

Is there any reason not to refactor this, making it simpler?

UPDATE

UPDATE 2

5 Answers5