3
\$\begingroup\$

I have an application that has multiple functions. Two in particular will query a remote computer using WMI, and return the installed printers in a datagridview. The other is a new method that will query the remote computer using WMI, and return information about the RAM.

I don't want to repeat the code that populates the datagrideview each time I need to display info from a WMI call. To accommodate this, I changed the way the datagrideview is populated. It is now in a separate class.

I'm just wanting to know if this is the best solution.

Here is one of the methods that implements the new class I made. This method below will get the installed printers on the target machine:

 class remote_Info
 {

 public Form getPrinters(string target, Form print_Form) 
    {
    /**This will query the remote computer for the installed printers. 
     * It does this by using WMI and looking in Win32_Printer. 
     * 
     * */
    // Check to make sure no null arguments were passed 
    if (target == null | print_Form == null)
        throw new ArgumentNullException();

    // Define the dataTable and DataColumns 
    // The columns will server as the header in the datagrid 
    DataTable dt = new DataTable();
    DataColumn colName = new DataColumn("Name");
    DataColumn col_Driver = new DataColumn("Driver");
    DataColumn colLocation = new DataColumn("Location");
    dt.Columns.Add(colName);
    dt.Columns.Add(col_Driver);
    dt.Columns.Add(colLocation);          

    ManagementScope scope = new ManagementScope("\\\\" + target + "\\root\\cimv2");
    SelectQuery query = new SelectQuery("SELECT * FROM Win32_Printer");
    ManagementObjectSearcher mos = new ManagementObjectSearcher(scope, query);

    try
        {


        using (ManagementObjectCollection moc = mos.Get())
            {

            // Fire off a foreach loop. First creating a new data
            foreach (ManagementObject mo in moc)
                {
                try
                    {
                    DataRow row1 = dt.NewRow();
                    row1[colName] = mo["Name"] ?? "N/A";
                    row1[col_Driver] = mo["DriverName"] ?? "N/A";
                    row1[colLocation] = mo["Location"] ?? "N/A";
                    dt.Rows.Add(row1);
                    }
                finally
                    {
                    if (mo != null)
                        mo.Dispose();
                    }
                }
            }
        }
    catch(Exception ex)
        {
        writeLog(ex);
        MessageBox.Show("There was an error while processing your request. View the errorlog for more information");
        }

    display_Forms displayInstance = new display_Forms();            
    return  displayInstance.wmi_FillGrid("Printers", dt, print_Form);    

    }
 }

Now here is the class and method I made to fill the datagrideview. My idea is to use this each time I need to fill data from WMI into a datagrideview.

 class display_Forms
{
public Form wmi_FillGrid(string title, DataTable dt, Form print_Form)
    {
    // Set the title of the form. 
    print_Form.Text = (title);
    print_Form.AutoSize = true;

    // Declare the datagrid view             
    DataGridView dataGrid = new DataGridView();
    // Datagrid Properties 
    dataGrid.BorderStyle = BorderStyle.None;
    dataGrid.BackgroundColor = SystemColors.GradientInactiveCaption;
    dataGrid.AutoSize = true;
    dataGrid.Width = 0;
    dataGrid.Height = 0;
    print_Form.Width = dataGrid.Width;
    print_Form.Height = dataGrid.Height;
    dataGrid.AutoSizeColumnsMode = DataGridViewAutoSizeColumnsMode.AllCells;
    // Set the datasource to the Datatable
    dataGrid.DataSource = dt;

     // Add the datagrid to the form
    print_Form.Controls.Add(dataGrid);

    return print_Form;
    }

}

Just to show a full example, here is the calling method:

private void getInstalledPrinterListAtTargetToolStripMenuItem_Click(object sender, EventArgs e)
    {
    // This will query the list of installed printers on the target machine. 
    // Using WMI, it will query win32_Printers and loop through the returned list. 
    // This is all done on a seperate thread, finally returning the form in which the data is dispalyed. 
    // The form is called with begininvoke and displayed on the GUI thread. 

    string target = null;
    DialogResult results = custom_Forms.supperInputBox("Computer Name", "Enter Computer Name", ref target);
    if (results == System.Windows.Forms.DialogResult.Cancel)
        return;
    if (validation.validate(target) == false | validation.alphaNum(target) == false)
        return;
    // Declare the local objects 
    remote_Info info = new remote_Info();      
    Form print_Form = new Form(); 

    // Execute the query on a seperate thread. Wrapping in a try/catch/finally block
    // The finally statement will display the returned Form from the getPrinters method.

    (new System.Threading.Thread(() =>
    {
    try
        {
        print_Form = info.getPrinters(target, print_Form);
        }
    catch (Exception ex)
        {
        writeLog(ex);
        return;
        }
    finally
        {

        this.BeginInvoke((Action)delegate
            {
            if(print_Form != null)
            print_Form.Show();
            });

        }

    })).Start();


    print_Form = info.getPrinters(target, print_Form);
    print_Form.Show();

    }
\$\endgroup\$
2
  • 2
    \$\begingroup\$ Please run StyleCop on this first. You are not even following the right naming conventions ... \$\endgroup\$ – Leonid Apr 20 '12 at 15:45
  • 3
    \$\begingroup\$ Stylecop is usually a little too picky for my taste. There's a large number of rule that were developed for Microsoft internally and do not really come from the naming and layout conventions for .NET source code in general. So when using Stylecop, go through the list of rules to disable any rule which is too strict. \$\endgroup\$ – jessehouwing Apr 20 '12 at 18:27
2
\$\begingroup\$

First, please do what Leonid mentions in his comment. The code is woefully out of standard convention for C# and .NET. Is this a Java port perhaps? Secondly, check your spelling in your comments. Nothing makes a developer wince more than looking at someone else's code and starting off by seeing egregious spelling errors. It does not speak well for the rest of the code.

That bit out of the way, I'm going to tackle your display_Forms class as an example of where to start to clean up. A big part of what I did what separate concerns into separate methods so that future changes are more easily found and dealt with. Note I made the class static as you're storing no state (also take note that the remote_Info class is the same way) and I'm using an extension method on the Form so it can be used a bit more fluently.

Best of luck.

internal static class DisplayForms
{
    public static Form WmiFillGrid(this Form printForm, string title, DataTable dt)
    {
        return printForm == null ? null : UpdatePrintForm(printForm, title, CreateDataGridView(dt));
    }

    private static Control CreateDataGridView(DataTable dt)
    {
        // Declare the data grid view
        return new DataGridView
        {
            // Data grid Properties
            BorderStyle = BorderStyle.None,
            BackgroundColor = SystemColors.GradientInactiveCaption,
            AutoSize = true,
            Width = 0,
            Height = 0,
            AutoSizeColumnsMode = DataGridViewAutoSizeColumnsMode.AllCells,

            // Set the data source to the Data table
            DataSource = dt
        };
    }

    private static Form UpdatePrintForm(Form printForm, string title, Control dataGrid)
    {
        // Set the title of the form.
        printForm.Text = title;
        printForm.AutoSize = true;

        printForm.Width = dataGrid.Width;
        printForm.Height = dataGrid.Height;

        // Add the data grid to the form
        printForm.Controls.Add(dataGrid);

        return printForm;
    }
}
\$\endgroup\$
3
  • \$\begingroup\$ Thanks for you input. I like the idea of making displayForms static. Also, no this ilsnt a java port. I have had no formal c# training, and everything is self taught. No excuses, I need to adopt conventional standards. Also yes, I am a very bad speller. Maybe I can find a spell check add-in for VS =) \$\endgroup\$ – Anon Apr 20 '12 at 16:47
  • 2
    \$\begingroup\$ Spell Check Add-on: visualstudiogallery.msdn.microsoft.com/… \$\endgroup\$ – jessehouwing Apr 20 '12 at 18:24
  • 3
    \$\begingroup\$ Naming Conventions: msdn.microsoft.com/en-us/library/ms229002.aspx \$\endgroup\$ – jessehouwing Apr 20 '12 at 18:25

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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