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.

Answer 1

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.

Answer 2

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

Answer 3

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.

Answer 4

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').

Answer 5

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.