2
\$\begingroup\$

I'm a self-taught programmer and I've recently started working as a junior developer. Unfortunately I'm also the most experienced member of the team due to my employers unwillingness to pay senior developer salaries. I've recently created a tool for our work which works but I'm worried its not as efficient as it could be. Most of the main batch class is below, I've left in comments to colleagues in case anyone has any advice on them.

public class Batch
{
    ///////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////
    //                                                                                                                                   //
    //      For each new project class added:                                                                                            //
    //                                                                                                                                   //
    //      * Add a new entry to the dictionary for the project                                                                          //
    //                                                                                                                                   //
    //      * Call the key(string) the project name in CAPITALS and add an underscore at the beginning                                   //
    //                                                                                                                                   //
    //      * Make sure you declare the ICallable interface as a NEW instance                                                            //
    //                                                                                                                                   //
    ///////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////

    //-----Dictionary containing project classes-----
    private Dictionary<string, ICallable> classList =
        new Dictionary<string, ICallable>
        {
            {"_ProjectA", new ProjectA()},
            {"_ProjectB", new ProjectB()}
        };

    //----------Code below this line should not be edited when adding a standard project----------

    //Instance variables
    private int _pageCount, _printAmount;
    private XElement _currentProj;
    private readonly XDocument _xml;
    private Dictionary<Label, ComboBox> _dropDowns;
    private MainWindow _main;

    /// <summary>
    /// Class constructor
    /// </summary>
    /// <param name="projectName">Name of selected project to create document for</param>
    public Batch(string projectName)
    {
        _currentProj = _xml.Descendants("Project").First(item => item.Attribute("Name").Value == projectName);  //set current project to found project
        _main = Application.Current.Windows.Cast<Window>().FirstOrDefault(window => window is MainWindow) as MainWindow;
    }

    /// <summary>
    /// Class constructor
    /// </summary>
    /// <param name="dropDowns">Dictionary of labels and comboboxes used in the main window</param>
    /// <param name="xml">XML file containig project information</param>
    /// <param name="projectName">Name of selected project to create document for</param>
    public Batch(Dictionary<Label, ComboBox> dropDowns, XDocument xml, string projectName)
    {
        _dropDowns = dropDowns;
        _xml = xml;
        _currentProj = _xml.Descendants("Project").First(item => item.Attribute("Name").Value == projectName);  //set current project to found project
        _main = Application.Current.Windows.Cast<Window>().FirstOrDefault(window => window is MainWindow) as MainWindow;
    }

    /// <summary>
    /// Populate the labels and comboboxes dependant on the information retireved from the Project XML file
    /// </summary>
    public void LoadLists()
    {
        IEnumerable<XElement> batchFields = _currentProj.Element("Header").Element("Header_Fields").Descendants("BCode");   //collection of BCode nodes
        Dictionary<string, List<string>> batchInfo = new Dictionary<string, List<string>>();                                

        //for each bcode node add the name and the list of options to the dictionary
        foreach (XElement field in batchFields)
        {
            if (field.Attribute("Name").Value == "ID") continue;

            //Compile options into a list
            List<string> options = new List<string>();
            foreach (XElement option in field.Elements().Where(x => x.Name == "Option"))
            {
                if (option.Value != "")
                {
                    options.Add(option.Value);
                }
            }

            batchInfo.Add(field.Attribute("Name").Value, options);
        }

        int i = 0;
        //add each option into the drop down list for that field
        foreach (KeyValuePair<string, List<string>> entry in batchInfo)
        {
            _dropDowns.ElementAt(i).Key.Content = entry.Key;
            _dropDowns.ElementAt(i).Value.IsEnabled = true;
            _dropDowns.ElementAt(i).Value.Items.Add("Select...");
            foreach (string item in entry.Value)
            {
                _dropDowns.ElementAt(i).Value.Items.Add(item.Replace("\n", ""));
            }
            _dropDowns.ElementAt(i).Value.SelectedIndex = 0;
            i++;
        }
    }

    /// <summary>
    /// Retrieve unique numbers from specified database using an encrypted connection string and strored procedures
    /// </summary>
    /// <param name="isBatchHeader">If the document to be produced is a batch header</param>
    /// <returns></returns>
    public List<string> GetDecodedNum(bool isBatchHeader)
    {
        string storedProcCode;
        string storedProcName;
        int padding;
        XElement bCode;
        int numOfIterations;

        //select stored procedure, padding amount and items per page depending on if batch header or separator
        if (isBatchHeader)
        {
            bCode = _currentProj.Element("Header").Element("Header_Fields").Descendants("BCode").First(x => x.Attribute("Name").Value == "ID");
            storedProcName = "spNextBatchNumber";
            padding = 10;
            numOfIterations = 1;
        }
        else
        {
            bCode = _currentProj.Element("Separator").Element("Separator_Fields").Descendants("BCode").First(x => x.Attribute("Name").Value == "ID");
            storedProcName = "spNextSeparatorNumber";
            padding = Convert.ToInt32(bCode.Attribute("Padding").Value);
            numOfIterations = (classList["_" + _currentProj.Attribute("Name").Value]._SeparatorAmount);
        }

        //add prefix if one present
        if (bCode.Attribute("Prefix").Value != "")
        {
            storedProcCode = bCode.Element("Stored_Proc").Value;
        }
        else
        {
            storedProcCode = bCode.Element("Stored_Proc").Value;
        }

        //Redacted code using company decryption library goes here 

        List<string> decodedNumbers = new List<string>();
        //try to open connection to database and get required amount fo numbers
        try
        {
            SqlConnection con = new SqlConnection(dbCon);
            SqlCommand cmd = new SqlCommand(storedProcName, con);
            cmd.CommandType = CommandType.StoredProcedure;
            con.Open();

            for (int i = 0; i < numOfIterations; i++)
            {
                decodedNumbers.Add(bCode.Attribute("Prefix").Value + cmd.ExecuteScalar().ToString().PadLeft(padding, '0'));
            }
            return decodedNumbers;
        }
        catch
        {
            MessageBox.Show("Unable to get number from database");
            return decodedNumbers;
        }
    }

    /// <summary>
    /// Set up printer settings and print to user selected printer
    /// </summary>
    /// <param name="Amount">Amount of pages to print</param>
    public void PrintDoc(TextBox Amount)
    {
        //make sure all fields have an option selected
        foreach (KeyValuePair<Label, ComboBox> item in _dropDowns)
        {
            if ((item.Value.Items.Count > 0) && (item.Value.SelectedIndex == 0))
            {
                MessageBox.Show("Please select an option from each field.");
                return;
            }
        }

        PrintDocument pDoc = new PrintDocument();

        string batchOrSeparator;
        if (Amount.Text != "")
        {
            _printAmount = int.Parse(Amount.Text);
        }
        else
        {
            _printAmount = 1;
        }

        //setup depending on if batch header or separator
        if (_main.rdb_Batch.IsChecked == true)
        {
            pDoc.PrintPage += PrintBatchHeader;
            batchOrSeparator = "Batch";
        }
        else
        {
            pDoc.PrintPage += PrintSeparator;
            pDoc.DefaultPageSettings.Landscape = classList["_" + _currentProj.Attribute("Name").Value]._Landscape;
            batchOrSeparator = "Separator";
        }


        PrintDialog pDialog = new PrintDialog();

        //if user clicks ok on printer dialong then print
        if (pDialog.ShowDialog() == true)
        {
            _pageCount = 0;
            pDoc.PrinterSettings.PrinterName = pDialog.PrintQueue.FullName;
            pDoc.Print();
        }
    }

    /// <summary>
    /// Create batch header pages ready to be printed
    /// </summary>
    private void PrintBatchHeader(object o, PrintPageEventArgs e)
    {
        List<string> decodedNums = GetDecodedNum(true);
        e.Graphics.ScaleTransform(0.48f, 0.48f);
        e.Graphics.DrawImage(classList["_" + _currentProj.Attribute("Name").Value].CreateBatchHeader(decodedNums.First()), 0, 0);
        _pageCount++;
        e.HasMorePages = (_pageCount < _printAmount);
    }

    /// <summary>
    /// Create separator pages ready to be printed
    /// </summary>
    private void PrintSeparator(object o, PrintPageEventArgs e)
    {
        List<string> decodedNums = GetDecodedNum(false);
        e.Graphics.ScaleTransform(0.48f, 0.48f);
        e.Graphics.DrawImage(classList["_" + _currentProj.Attribute("Name").Value].CreateSeparator(decodedNums), 0, 0);
        _pageCount++;
        e.HasMorePages = (_pageCount < _printAmount);
    }

This class needs to dynamically call other classes by name depending on which options the user chooses. The other classes will contain the information needed to draw the page layout correctly. The project information is read in from an XML.

Are there better ways to call a specific class at runtime that won't be detrimental to performance? Are there more efficient ways/a standard way of reading XML data? Does anyone have any pointers for me to improve my code? Thanks in advance.

\$\endgroup\$
  • \$\begingroup\$ You should edit the title to tell us what the code does rather than what it is. \$\endgroup\$ – Der Kommissar Aug 4 '17 at 14:03
  • \$\begingroup\$ Changed it to include "page creator". \$\endgroup\$ – N1ghtwolf89 Aug 4 '17 at 14:06
3
\$\begingroup\$

This

_main = Application.Current.Windows.Cast<Window>().FirstOrDefault(window => window is MainWindow) as MainWindow;

can be simplified to

_main = Application.Current.Windows.OfType<MainWindow>().FirstOrDefault();

or

_main = Application.Current.MainWindow;

This

Dictionary<string, List<string>> batchInfo = new Dictionary<string, List<string>>();                                

//for each bcode node add the name and the list of options to the dictionary
foreach (XElement field in batchFields)
{
    if (field.Attribute("Name").Value == "ID") continue;

    //Compile options into a list
    List<string> options = new List<string>();
    foreach (XElement option in field.Elements().Where(x => x.Name == "Option"))
    {
        if (option.Value != "")
        {
            options.Add(option.Value);
        }
    }

    batchInfo.Add(field.Attribute("Name").Value, options);
}

can be simplified to

var batchInfo = batchFields.Where(f => f.Attribute("Name").Value != "ID")
                           .ToDictionary(f => f.Attribute("Name").Value,
                                         f => f.Elements()
                                               .Where(x => x.Name == "Option")
                                               .Select(x => x.Value)
                                               .Where(v => !string.IsNullOrEmpty(v))
                                               .ToList());

Always use string.Empty instead of "", Environment.NewLine instead of "\n" and don't repeate code. This fragment

foreach (KeyValuePair<string, List<string>> entry in batchInfo)
{
    _dropDowns.ElementAt(i).Key.Content = entry.Key;
    _dropDowns.ElementAt(i).Value.IsEnabled = true;
    _dropDowns.ElementAt(i).Value.Items.Add("Select...");
    foreach (string item in entry.Value)
    {
        _dropDowns.ElementAt(i).Value.Items.Add(item.Replace("\n", ""));
    }
    _dropDowns.ElementAt(i).Value.SelectedIndex = 0;
    i++;
}

can be rewritten as

foreach (var entry in batchInfo)
{
    var dropDownsElement = _dropDowns.ElementAt(i);
    var label = dropDownsElement.Key;
    var comboBox = dropDownsElement.Value;
    var comboBoxItems = comboBox.Items;

    label.Content = entry.Key;
    comboBox.IsEnabled = true;
    comboBoxItems.Add("Select...");

    foreach (var item in entry.Value)
    {
        comboBoxItems.Add(item.Replace(Environment.NewLine, string.Empty));
    }

    comboBox.SelectedIndex = 0;
}

As it is WPF code you should use MVVM and shouldn't populate Items through code-behind. Use ItemsSource of CombBox'es with binding in XAML.


If a field is not supposed to be changed it should be defined as readonly like this:

private readonly MainWindow _main;

Other people will write you some another mistakes.

\$\endgroup\$
  • \$\begingroup\$ Thanks, I've changed some of the code. The Environment.NewLine doesn't work for me so I've had to revert to using the string representation. The string.Empty does work though so I've changed them over. \$\endgroup\$ – N1ghtwolf89 Aug 7 '17 at 6:58
0
\$\begingroup\$

I think _xml can be null here

public Batch(string projectName)
{
    _currentProj = _xml
\$\endgroup\$
  • \$\begingroup\$ The xml is checked in a window that opens at the start of the program before it gets this far, it only gets this far if the xml is readable. \$\endgroup\$ – N1ghtwolf89 Aug 4 '17 at 16:20
  • \$\begingroup\$ In the code it appear it could be null as opposed to should not be null \$\endgroup\$ – paparazzo Aug 4 '17 at 17:01
  • \$\begingroup\$ This isn't all of the code. It's checked in case of null in the part of the code I haven't posted. \$\endgroup\$ – N1ghtwolf89 Aug 4 '17 at 18:15
  • \$\begingroup\$ Again should not be null is not the same and can not be null. \$\endgroup\$ – paparazzo Aug 4 '17 at 18:35
  • \$\begingroup\$ Ah, I think I see what you mean, _xml doesn't have chance to be set before the constructor calls it. I haven't had any problems with running the code so I'll need to check on Monday if that constructor is now actually redundant and is never used. \$\endgroup\$ – N1ghtwolf89 Aug 4 '17 at 19:08

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.