Is this code as weird as I think it is?

Question

Combing through legacy code, I found this:

public static void SetupSiteDeptBox(string siteNo, System.Windows.Forms.ComboBox cbo, ref Hashtable DeptHash)
{
    DeptHash.Clear();
    if (siteNo != "")
    {
        cbo.Items.Clear();
        . . .
        cbo.Items.Add(row);
    . . .

This method is called from one place, another form (this code is in the main form). Okay, calling the main form may be alright, because it can be counted on to be in existence, but why pass the comboBox from one form to the other? Why not return an ObjectCollection (or ??) instead of void, rather than pass a control from one form to another?

Answer 1

This code looks crappy on a number of levels.

First, I see no reason whatsoever to pass the Hashtable with ref.

Then, as you said, why pass in a ComboBox UI element? Seems like a poor separation of concerns.

Finally, Hashtable should (likely) be replaced with a modern generically-typed Dictionary<K,V>.

Your intuitions are correct.