Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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?

share|improve this question
4  
Yes, there are many bad programmers out there. I would suggest getting a copy of Reshaper, running StyleCop, and if after that you still feel like the smartest person in the room, then you are in the wrong room. –  Leonid Feb 26 '13 at 1:05

1 Answer 1

up vote 5 down vote accepted

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.

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.