3
\$\begingroup\$

I have a C# program that I would like to have reviewed. I am new to programming and want to know how it could be improved. It is a Windows Forms project that generates a PDF using the iTextSharp dll. I have tried to leave off the repetitive parts (multiple textboxes, etc) as well as the part that uses iTextSharp.

namespace InvoiceGenerator
{
public partial class InvoiceGenerator : Form
{
    List<string> state = new List<string>();
    public InvoiceGenerator()
    {
        InitializeComponent();

        string[] states = new string[] { "Alabama", "Alaska", "Arizona", "Arkansas", "California", "Colorado",
        "Connecticut", "Delaware", "Florida", "Georgia", "Hawaii", "Idaho", "Illinois", "Indiana", "Iowa", "Kansas", 
        "Kentucky", "Louisiana", "Maine", "Maryland", "Massachusetts", "Michigan", "Minnesota", "Mississippi", 
        "Missouri", "Montana", "Nebraska", "Nevada", "New Hampshire", "New Jersey", "New Mexico", "New York",
        "North Carolina", "North Dakota", "Ohio", "Oklahoma", "Oregon", "Pennsylvania", "Rhode Island", "South Carolina",
        "South Dakota", "Tennessee", "Texas", "Utah", "Vermont", "Virginia", "Washington", "West Virginia", "Wisconsin",
        "Wyoming" };
        comboBoxState.Items.AddRange(states);           
    }

    private void InvoiceGenerator_Load(object sender, EventArgs e)
    {
        maskedTextBoxDate.Text = DateTime.Today.ToString("MM/dd/yyyy");
        maskedTextBoxDate.ValidatingType = typeof(System.DateTime);
        lblTotalPrice.Text = "$0.00";
    } 

    public static bool IsFileLocked(FileInfo file)
    {
        FileStream stream = null;
        try
        {
            stream = file.Open(FileMode.Open, FileAccess.ReadWrite, FileShare.None);
        }
        catch (IOException)
        {
            return true;
        }
        finally
        {
            if (stream != null)
                stream.Close();
        }
        return false;
    }

    private void btnGenerate_Click(object sender, EventArgs e)
    {
        FillForm();

        maskedTextBoxDate.Text = DateTime.Today.ToString("MM/dd/yyyy");
        txtName.Text = "";
        txtAddress.Text = "";
        txtCity.Text = "";
        comboBoxState.SelectedIndex = -1;
        txtZipCode.Text = "";
        txtAttn.Text = "";
        checkBoxFull.Checked = false;
        checkBoxFirst.Checked = false;
        checkBoxNext.Enabled = false;
        lblTotalPrice.Text = "";
        textBox1.Text = "";

    }

    private void FillForm()
    {
        string pdfTemplate = @"\\server\files\Invoice_Generator\invoice.pdf";
        string newFile = @"\\server\files\Invoice_Generator\completed_invoice.pdf";
        FileInfo fileStatus = new FileInfo(newFile);
        if (IsFileLocked(fileStatus))
        {
            MessageBox.Show("An invoice is currently open and needs to be closed before another one can be created.", 
                "File currently in use.", MessageBoxButtons.OK, MessageBoxIcon.Warning);
        }
        else
        {

            //iTextSharp code

            Process.Start(newFile);
        }
    }

    private void checkBoxFull_CheckedChanged(object sender, EventArgs e)
    {
        if (checkBoxFull.Checked)
        {
            lblFullPrice.Text = "$1,000.00";
            numericUpDownFull.Enabled = true;
        }
        else
        {
            lblFullPrice.Text = "";
            lblTotalFull.Text = "";
            numericUpDownFull.Value = 0;
            numericUpDownFull.Enabled = false;
        }
    }

    string invoiceFull = String.Empty;
    private void numericUpDownFull_ValueChanged(object sender, EventArgs e)
    {
        if (numericUpDownFull.Value == 0)
        {
            lblTotalFull.Text = "";
            decimal total = (((decimal)numericUpDownFull.Value * 1000) + ((decimal)numericUpDownFirst.Value * 65)
                    + ((decimal)numericUpDownNext.Value * 40));
            lblTotalPrice.Text = total.ToString("C");
            invoiceFull = "";
        }
        else
        {
            decimal Value = numericUpDownFull.Value;
            if (numericUpDownFull.Value > 0)
            {
                Value = Value * 1000;
                string Label = Value.ToString("C");
                lblTotalFull.Text = Label;
                decimal total = (((decimal)numericUpDownFull.Value * 1000) + ((decimal)numericUpDownFirst.Value * 65)
                    + ((decimal)numericUpDownNext.Value * 40));
                lblTotalPrice.Text = total.ToString("C");
                invoiceFull = numericUpDownFull.Value.ToString();
            } 

        }
    }

    private void maskedTextBoxDate_TypeValidationCompleted(object sender, TypeValidationEventArgs e)
    {
        if (!e.IsValidInput)
        {
            MessageBox.Show("Please enter a valid date.", "Invalid Date");
            maskedTextBoxDate.Text = DateTime.Today.ToString("MM/dd/yyyy");
            maskedTextBoxDate.Focus();
        }
        else
        {
            //Now that the type has passed basic type validation, enforce more specific type rules.
            DateTime userDate = (DateTime)e.ReturnValue;
            if (userDate > DateTime.Now)
            {
                MessageBox.Show("Please enter a valid date.", "Invalid Date");
                maskedTextBoxDate.Text = DateTime.Today.ToString("MM/dd/yyyy");
                maskedTextBoxDate.Focus();
            }
            else
            {
                DateTime beginTime = new DateTime(1900, 1, 1);
                if (userDate < beginTime)
                {
                    MessageBox.Show("Please enter a valid date.", "Invalid Date");
                    maskedTextBoxDate.Text = DateTime.Today.ToString("MM/dd/yyyy");
                    maskedTextBoxDate.Focus();
                }
            }
        }

    }

    private void TextEntry(object sender, KeyPressEventArgs e)
    {
        if (!char.IsLetter(e.KeyChar) &&  !char.IsWhiteSpace(e.KeyChar) && !char.IsControl(e.KeyChar))
        {
            e.Handled = true;
        }
    }

    private void txtName_KeyPress(object sender, KeyPressEventArgs e)
    {
        TextEntry(txtName, e);
    }

    string invoiceName = String.Empty;
    private void txtName_TextChanged(object sender, EventArgs e)
    {
        if (!string.IsNullOrEmpty(txtName.Text))
        {
            invoiceName = txtName.Text;
        }
    }
}
}
\$\endgroup\$
3
\$\begingroup\$
  1. Moving the states list to some sort of data store (could be a text / xml file, or database) is preferable; this way you won't have to re-compile if you want to use your application in countries other than the US.
  2. (A positive point) I see that IsFileLocked is copied from a SO answer about file locking. using proven and tested code (i.e from a good source online) is a very good practice. however, why is this method public?
  3. In FillForm, if the file is locked, you can just return from the function after displaying the message to the user; there's no need for the else clause.
    Personally, I always advocate verification at the beginning, and returning immediately if it fails. This saves levels of nesting. (this comment also applies to checkBoxFull_CheckedChanged and numericUpDownFull_ValueChanged and maskedTextBoxDate_TypeValidationCompleted)
  4. Is invoiceFull a class member? if so, I would recommend naming it as such (either with a _ or m_ prefix). It's also recommended to declare all members at the top of the class.
    Also, I fail to see where it's used.
  5. If numericUpDownFull.Value == 0 , then total will also will always be 0. no need to calculate it.
  6. The member invoiceName is never used for reading...
|improve this answer|||||
\$\endgroup\$
  • \$\begingroup\$ thanks. as for # 2:because that is how I found it off of SO. Didn't think to make it private since I'm not calling it from elsewhere. #3:If I don't have an else statement the code for the pdf creation will try to run and I'll get an error since the pdf file is open. #4:invoiceFull is just a string variable that I use in the pdf creation (iTextSharp) part( pdfFormFields.SetField("QtyFull", invoiceFull);. #5:good point! #6: its used in the pdf creation part. \$\endgroup\$ – KFP Apr 17 '13 at 20:16
  • \$\begingroup\$ #2- if you're not using it from anywhere else- it should be private. #3- please note I suggested you return from the function; therefore no further code would be executed. #4- my suggestions for naming and regioning still stand, though. \$\endgroup\$ – J. Ed Apr 17 '13 at 21:55
1
\$\begingroup\$

(This is in addition to the review from sJhonny.)

These three lines are repeated three times in maskedTextBoxDate_TypeValidationCompleted:

MessageBox.Show("Please enter a valid date.", "Invalid Date");
maskedTextBoxDate.Text = DateTime.Today.ToString("MM/dd/yyyy");
maskedTextBoxDate.Focus();

Instead of copy-pasting them, move them to a separate method and call that.

The same is true for this:

decimal total = (((decimal)numericUpDownFull.Value * 1000) 
    + ((decimal)numericUpDownFirst.Value * 65)
    + ((decimal)numericUpDownNext.Value * 40));
lblTotalPrice.Text = total.ToString("C");

Repeated at least twice. Imagine you need to change this logic: now you need to do it in two places instead of one.


I'm not a fan of this:

if (!e.IsValidInput)
{}
else
{}

I prefer a "positive check":

if (e.IsValidInput)
{}
else
{}

Note that local variables should be camelCase:

decimal Value = numericUpDownFull.Value; string Label = Value.ToString("C");

Moreover, "Value" is not good enough a name, ditto "Label". I don't see any need for "Label" anyway, since it is only used once: lblTotalFull.Text = Label;.


I note that several of your fields don't have an explicit access modifier:

List<string> state = new List<string>();
string invoiceFull = String.Empty;
string invoiceName = String.Empty;

I'd always set those explicitly.


Most importantly though I'd urge you to separate your business logic from your display. IMHO assignments to e.g. lblTotalFull.Text should probably be encapsulated into a method; perhaps the business logic could be moved to a separate class even. Same for the iTextSharp code in FillForm() (is that even a correct name for the functionality of that method?): move that to a separate class, along with the IsFileLocked method.

|improve this answer|||||
\$\endgroup\$

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.