4
\$\begingroup\$

Just looking for reviews to improve my coding and my structure of C#. It feels I probably have some unnecessary code that is not needed, been following some YouTube videos to accomplish some tasks.

Also, I am not sure if the actual structure is okay of my program. Would like to hear some opinions.

I am working on a project that sends a form to MySQL database. With a an extra bit of touch - Autofill most of the Text-boxes, from a button click and a given Sales Order Number found in SQL Server database.

If an Order Number is given then text boxes such as Customer Name, Telephone, Email, Products and Pack size is able to be auto-filled.

The Products is a combo-box value, so it'll be a list of ordered products and the user is able to selected a value.

This is the form:

enter image description here

Code:

ComplaintModel.cs

public class ComplaintModel
{
    public int Id { get; set; }
    public int UserId { get; set; }
    public DateTime Date { get; set; }
    public string OrderNumber { get; set; }
    public int IsCustomerSelected { get; set; }
    public string CustomerName { get; set; }
    public string CustomerContactName { get; set; }
    public string Telephone { get; set; }
    public string Email { get; set; }
    public string CustomerReference { get; set; }
    public int IsProductSelected { get; set; }
    public string Product { get; set; }
    public string PackSize { get; set; }
    public string BatchNumber { get; set; }
    public int IsBestBefore { get; set; }
    public DateTime BestBeforeDate { get; set; }
    public string QuantityInvolved { get; set; }
    public string Details { get; set; }
    public string Comments { get; set; }

}

ComplaintForm.cs

public partial class Complaints_Form : Form
{
    BindingList<string> errors = new BindingList<string>();

    List<ComplaintModel> mjmOrderNumber = new List<ComplaintModel>();

    List<ComplaintModel> customerNames = new List<ComplaintModel>();

    List<ComplaintModel> productNames = new List<ComplaintModel>();

    public Complaints_Form()
    {
        InitializeComponent();

        loadCustomers();
        loadProducts();
    }

    private void loadCustomers()
    {
        DataAccess db = new DataAccess();
        customerNames = db.LoadCustomers();
        customer_name.DataSource = customerNames;
        customer_name.DisplayMember = "CustomerName";
        customer_name.SelectedIndex = -1;
    }

    private void loadProducts()
    {
        DataAccess db = new DataAccess();
        productNames = db.LoadProducts();
        product.DataSource = productNames;
        product.DisplayMember = "Product";
        product.SelectedIndex = -1;

        pack_size.DataBindings.Add("Text", productNames, "PackSize", false, DataSourceUpdateMode.OnPropertyChanged);
    }


    public void clearBindings()
    {
        customer_name.DataBindings.Clear();
        email.DataBindings.Clear();
        telephone.DataBindings.Clear();
        product.DataBindings.Clear();
        product.DataSource = null;
        product.Items.Clear();
        pack_size.DataBindings.Clear();
    }

    public void searchButton_Click(object sender, EventArgs e)
    {
        clearBindings();

        DataAccess db = new DataAccess();

        mjmOrderNumber = db.FindOrderNumber(orderNumber.Text);

        // Binding values from Sales Order Number here:
        customer_name.DataBindings.Add("Text", mjmOrderNumber, "CustomerName", false, DataSourceUpdateMode.OnPropertyChanged);
        email.DataBindings.Add("Text", mjmOrderNumber, "Email", false, DataSourceUpdateMode.OnPropertyChanged);
        telephone.DataBindings.Add("Text", mjmOrderNumber, "Telephone", false, DataSourceUpdateMode.OnPropertyChanged);
        

        product.DataSource = mjmOrderNumber;
        product.DisplayMember = "Product";

        pack_size.DataBindings.Add("Text", mjmOrderNumber, "PackSize", false, DataSourceUpdateMode.OnPropertyChanged);

    }

    private void FormValidation()
    {
        errors.Clear();

        ComplaintModel complaint = new ComplaintModel();
        complaint.Date = date.Value;
        complaint.IsCustomerSelected = customer_name.SelectedIndex;
        complaint.CustomerContactName = customer_contact.Text;
        complaint.Telephone = telephone.Text;
        complaint.Email = email.Text;
        complaint.CustomerReference = customer_ref.Text;
        complaint.IsProductSelected = product.SelectedIndex;
        complaint.PackSize = pack_size.Text;
        complaint.BatchNumber = batch.Text;
        complaint.IsBestBefore = bestBeforeQuestion.SelectedIndex;
        complaint.BestBeforeDate = best_before.Value;
        complaint.QuantityInvolved = quantity.Text;
        complaint.Details = details.Text;
        complaint.Comments = comments.Text;

        ComplaintsValidator validator = new ComplaintsValidator();

        ValidationResult results = validator.Validate(complaint);

        if (results.IsValid == false)
        {
            foreach (ValidationFailure failure in results.Errors)
            {
                errors.Add($"- {failure.ErrorMessage}");
            }

            string message = string.Join(Environment.NewLine, errors);
            MessageBox.Show(message, "Empty Fields", MessageBoxButtons.OK, MessageBoxIcon.Warning);
        }
        else
        {
            insertComplaintForm();
        }
    }

    private void insertComplaintForm()
    {
        var confirm = MessageBox.Show("Are you sure the details you have entered are Correct?", "Confirmation", MessageBoxButtons.YesNo, MessageBoxIcon.Question);

        if (confirm == DialogResult.Yes)
        {
            DataAccess db = new DataAccess();

            db.InsertComplaint(UserDetails.userId, date.Value, orderNumber.Text, customer_name.Text, customer_contact.Text, telephone.Text, email.Text, customer_ref.Text, product.Text, pack_size.Text, batch.Text, best_before.Value, quantity.Text, details.Text, comments.Text);

            clearFields();
        }
    }

    void clearFields()
    {
        clearBindings();
        customer_name.SelectedIndex = -1;
        customer_name.ResetText();
        date.Value = DateTime.Today;
        customer_contact.Clear();
        telephone.Clear();
        email.Clear();
        customer_ref.Clear();
        
        batch.Clear();
        orderNumber.Clear();
        best_before.ResetText();
        quantity.Clear();
        details.Clear();
        comments.Clear();
        bestBeforeQuestion.SelectedIndex = -1;

        loadProducts();

        product.SelectedIndex = -1;
        product.ResetText();
        pack_size.Clear();
    }

    private void saveBtn_Click(object sender, EventArgs e)
    {
        FormValidation();
    }

    private void clearBtn_Click(object sender, EventArgs e)
    {
        clearFields();
    }


}

DataAccess.cs

public class DataAccess
{

    public List<ComplaintModel> GetComplaint(string _OrderNumber)
    {
        throw new NotImplementedException();
    }


    // Used in "ComplaintForm" to get list of Product and Customer Name from MJM Database.
    public List<ComplaintModel> FindOrderNumber(string orderNumber)
    {
        using(SqlConnection conn = new SqlConnection(ConnectionString.MJMconnString))
        {
            var output = conn.Query<ComplaintModel>($@"
                    SELECT 
                    TRIM(p.description) as Product,
                    p.searchRef1 as PackSize,
                    c.name AS CustomerName,
                    c.email as Email,
                    c.phone as Telephone
                    FROM SalesOrder so JOIN SalesOrderLine sol ON
                    so.id = sol.salesOrderID JOIN Customer c ON
                    so.customerID = c.id JOIN Product p ON
                    sol.productID = p.id WHERE so.number = '{orderNumber}'").ToList();
            return output;
        }
    }

    public List<ComplaintModel> LoadCustomers()
    {
        using(SqlConnection conn = new SqlConnection(ConnectionString.MJMconnString))
        {
            var output = conn.Query<ComplaintModel>($@"SELECT name AS CustomerName FROM Customer WHERE dormant = 0;").ToList();
            return output;
        }
    }

    public List<ComplaintModel> LoadProducts()
    {
        using (SqlConnection conn = new SqlConnection(ConnectionString.MJMconnString))
        {
            var output = conn.Query<ComplaintModel>($@"SELECT CONCAT(description, ' - ', searchRef1) AS Product, searchRef1 AS PackSize FROM product WHERE dormant = 0").ToList();
            return output;
        }
    }

    // This is used to send Complaint details into MySQL database for table "CustomerComplaints"
    public void InsertComplaint(
        int userId,
        DateTime date,
        string orderNumber,
        string customerName,
        string customerContactName,
        string telephone,
        string email,
        string customerReference,
        string product,
        string packSize,
        string batchNumber,
        DateTime bestBeforeDate,
        string quantityInvolved,
        string details, 
        string comments)
    {
        using(MySqlConnection conn = new MySqlConnection(ConnectionString.ConnString))
        {
            List<ComplaintModel> complaint = new List<ComplaintModel>();

            complaint.Add(new ComplaintModel {
                UserId = userId,
                Date = date,
                OrderNumber = orderNumber,
                CustomerName = customerName,
                CustomerContactName = customerContactName,
                Telephone = telephone,
                Email = email,
                CustomerReference = customerReference,
                Product = product,
                PackSize = packSize,
                BatchNumber = batchNumber,
                BestBeforeDate = bestBeforeDate,
                QuantityInvolved = quantityInvolved,
                Details = details,
                Comments = comments});

            conn.Execute(
                @"INSERT INTO CustomerComplaints
                (UserId, Date, OrderNumber, CustomerName, CustomerContactName, Telephone, Email, CustomerReference, Product, PackSize, BatchNumber,
                BestBeforeDate, QuantityInvolved, Details, Comments) 
                VALUES 
                (@UserId, @Date, @OrderNumber, @CustomerName, @CustomerContactName, @Telephone, @Email, @CustomerReference, @Product, @PackSize, @BatchNumber,
                @BestBeforeDate, @QuantityInvolved, @Details, @Comments)", complaint);
        }
    }


}

ComplaintsValidator.cs

public class ComplaintsValidator : AbstractValidator<ComplaintModel>
{
    public ComplaintsValidator()
    {
        RuleFor(p => p.IsCustomerSelected)
            .Must(DropDownSelected).WithMessage("'Customer Name' must be valid");

        RuleFor(p => p.CustomerContactName)
            .Cascade(CascadeMode.Stop)
            .NotEmpty()
            .Length(1, 100);

        RuleFor(p => p.Telephone)
            .Cascade(CascadeMode.Stop)
            .NotEmpty()
            .Length(1, 40)
            .When(p => string.IsNullOrEmpty(p.Email));

        RuleFor(p => p.Email)
            .Cascade(CascadeMode.Stop)
            .NotEmpty()
            .Length(1, 40)
            .When(p => string.IsNullOrEmpty(p.Telephone));

        RuleFor(p => p.CustomerReference)
            .Cascade(CascadeMode.Stop)
            .Length(0, 255);

        RuleFor(p => p.IsProductSelected)
            .Must(DropDownSelected).WithMessage("'Product Name' must be valid");

        RuleFor(p => p.PackSize)
            .Cascade(CascadeMode.Stop)
            .NotEmpty()
            .Length(1, 40);

        RuleFor(p => p.BatchNumber)
            .Cascade(CascadeMode.Stop)
            .NotEmpty()
            .Length(1, 40);

        RuleFor(p => p.IsBestBefore).Must(DropDownSelected).WithMessage("Does this product have a Best Before Date?");

        RuleFor(p => p.QuantityInvolved)
            .Cascade(CascadeMode.Stop)
            .NotEmpty()
            .Length(1, 50);

        RuleFor(p => p.Details)
            .Cascade(CascadeMode.Stop)
            .NotEmpty()
            .Length(10, 700);

        RuleFor(p => p.Comments)
            .Cascade(CascadeMode.Stop)
            .Length(0, 700);
    }

    protected bool DropDownSelected(int item)
    {
        if (item == -1)
        {
            return false;
        }
        else
        {
            return true;
        }
    }
}

Resource

This is the Youtube video I have used to make the queries etc..

https://www.youtube.com/watch?v=Et2khGnrIqc&t=2901s

\$\endgroup\$
7
  • \$\begingroup\$ Can you try putting this in your ORD number text box and doing the search? ' UNION ALL SELECT 'Google' as Product, 'SQL' as PackSize, 'Injection' AS CustomerName, 'Attacks,' as Email, 'Please' as Telephone -- and see what happens \$\endgroup\$
    – RobH
    Nov 18, 2020 at 8:42
  • \$\begingroup\$ including the ' before UNION? \$\endgroup\$
    – EdGzi
    Nov 18, 2020 at 9:13
  • \$\begingroup\$ Yeah, all of it. I'm not that up to date on Dapper but I think you've got a problem. \$\endgroup\$
    – RobH
    Nov 18, 2020 at 9:20
  • \$\begingroup\$ @RobH This is the output: An unhandled exception of type 'System.Data.SqlClient.SqlException' occurred in System.Data.dll Additional information: All queries combined using a UNION, INTERSECT or EXCEPT operator must have an equal number of expressions in their target lists. \$\endgroup\$
    – EdGzi
    Nov 18, 2020 at 9:21
  • 1
    \$\begingroup\$ @RobH Thank you - I'll have to add a parameter in my dapper function. \$\endgroup\$
    – EdGzi
    Nov 18, 2020 at 9:33

1 Answer 1

1
\$\begingroup\$

Here is my take on your solution. Please note that some of the things I mention are my personal prefference.

ComplaintModel

  • I preffer to name my Id field like TableNameId, so in your case I would change Id to CustomerComplaintId (please note that this is my personal prefference).
  • IsCustomerSelected (and IsProductSelected) sounds like a bool flag to me, I see you put SelectedIndex in there. I would rename it to CustomerId and if you need the flag "is selected" then make a new property, make it a bool and change it's value accordingly.
  • Regarding DB design are BachNumber, QuantityInvolved numerics? If so consider making them a proper type (int, decimal etc.), both in DB and model.

ComplaintForm

  • Use PascalCase for class, method name and public properties. I would change Complaints_Form to ComplaintsForm, every method where you use camelCase, like loadCustomers -> LoadCustomers.
  • I strictly specify modifier for private fields and prefix them with an underscore, in your case the top private fields would become private List<ComplaintModel> _mjmOrderNumber = new List<ComplaintModel>(); (please note that this is my personal prefference).
  • I'm not sure I understand how you reuse the ComplaintModel for Customers and Products, it seems they should be their own model (entity) which comes from Database.
  • clearBindings method should be private, no need to expose it outside.
  • You construct a new ComplaintModel inside FormValidation and indirectly in insertComplaintform. I would make a helper function which would populate and return your ComplaintModel which you can reuse in two locations mentioned above. You would need to change the DataAccess InsertComplaint method to accept a ComplaintModel instead of multitude of parameters.
  • In the end of FormValidation method you execute the insertComplaintForm method, I would change the FormValidation method to return a bool isFormValid and execute the insertComplaintForm in saveBtn_Click event after checking if form is valid. Try and keep methods responsible for a single task.
  • This one is food-for-thought and I'd actually like some feedback from others about this - after making numerous "ClearForm", "ClearFields" "EmptyInput" methods I later started reusing a function which would traverse all controls on the form and clear them - if TextBox then Clear() if ComboBox then SelectedIndex = -1. This way I never forgot to clear a field I added long time after I implemented the original functionality.

DataAccess

  • I see now what you did with the ComplaintModel, you packed it with all database properties you need later on and simply fill the properties you need for lets say a ComboBox datasource. While this works it makes the ComplaintModel unneccesary complicated. I would return Customer(s) object from LoadCustomers, Product(s) object from LoadProducts and use those for control binding.
  • As mentioned above I would change InsertComplaint to accept ComplaintModel object which would be constructed from a helper method in the Form itself.

ComplaintsValidator

  • You can set CascadeMode globally or per Validator, no need to specify it for every Rule. Like this CascadeMode = CascadeMode.Stop;
\$\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.