1
\$\begingroup\$

I made an algorithm to calculate when the user has to purchase material adjusting the date in which the order has to be made, I'm using datagridviews to make it work, but I was wondering if there is a better way using a DataTable.

private void Suggestion()
{ 
  for (int i = 1; i < dataGridView9.RowCount; i++)    
    {
        string PartNumber = dataGridView9.Rows[i - 1].Cells["PartNumber"].Value.ToString();
        string Requested_Date = dataGridView9.Rows[i - 1].Cells["Requested_Date"].Value.ToString();               
        string material = dataGridView9.Rows[i - 1].Cells["Material"].Value.ToString();               
        string requierd_Qty = dataGridView9.Rows[i - 1].Cells["Requierd_Qty"].Value.ToString();

  for (int l = 1; l < dataGridView10.RowCount; l++)
    {
        string partNumber = dataGridView10.Rows[l - 1].Cells["PartNumber"].Value.ToString();
        double leadTime = double.Parse(dataGridView10.Rows[l - 1].Cells["PUR LT"].Value.ToString());

        if (partNumber == material)
        {
            DateTime _requested_Date = Convert.ToDateTime(Requested_Date);
            DateTime suggestedDate = GetWorkDays.AddWorkdays(_requested_Date, Convert.ToDouble(leadTime));

            try
            {
                oleDbCmd = new OleDbCommand("InsertSuggested", connection);
                oleDbCmd.CommandType = CommandType.StoredProcedure;

                oleDbCmd.Parameters.Add("@PartNumber", OleDbType.VarChar, 30).Value = material.ToString();
                oleDbCmd.Parameters.Add("@AdjustedDate", OleDbType.Date, 50).Value = _requested_Date;
                oleDbCmd.Parameters.Add("@SuggestedDate", OleDbType.Date, 50).Value = suggestedDate;                         
                oleDbCmd.Parameters.Add("@Qty", OleDbType.VarChar, 30).Value = requierd_Qty;

                connection.Open();
                oleDbCmd.ExecuteNonQuery();
            }
            catch (OleDbException error)
            {
                MessageBox.Show(error.ToString());
            }
            finally
            {
                connection.Close();
            }
            break;
        }
      }
    }
  }

I want to know if there a better way of achieving this. Right now there inst that much data, but in the long run their might be. If there are other comments of the code please let me know.

Edit:changed variable names to correct ones.

\$\endgroup\$
  • \$\begingroup\$ You have 2 times string partNumber within the same scope. This won't work. You can safely remove the first because you don't use it. \$\endgroup\$ – Heslacher Feb 5 '15 at 10:30
  • \$\begingroup\$ Where does Requested_Date of DateTime _requested_Date = Convert.ToDateTime(Requested_Date); come from ? \$\endgroup\$ – Heslacher Feb 5 '15 at 12:09
  • \$\begingroup\$ @Heslacher, you are correct, when I edited before pasting the post I dint notice, changed them to the correct variables. \$\endgroup\$ – MoralesJosue Feb 5 '15 at 15:29
2
\$\begingroup\$

Mixing GUI code and database code violates the single responsibility principle. One class (here your form) should have at the best one responsibility. One method (here Suggestion()) should have at best one responsibility. Your method is retrieving items from the GUI and is storing them into the database.


Based on the naming guidelines

  • method names should be made out of verbs or verb phrases.
  • variables should be named (also it is not mentioned explicitly for variables which are local to a method) using camelCase casing. You shouldn't use any type of snake_Case casing.

At least if you have choosen a style, also this style isn't following the guidelines you should stick to the choosen style. Right now you are mixing the casing styles.


Calling Convert.ToDouble() on a double or calling ToString() on a String is redundant.


Your loops are starting at 1 and you are accessing the items by [iterator -1]. You should start at 0 as every array in NET.

The starting at 1 and the ending condition of < thegrid.RowCount will let you miss one row.


You should retrieve Cells["PUR LT"] only if partNumber == material.


Both OleDbConnection and OleDbCommand are implementing IDisposable so enclosing the usage in a using block will automatically call Dispose() on the objects and therefor also close the connection.


Instead of iterating over the rows of the datagridviews you should consider to use linq to do the job.


You should always assume that a cell of the datagridview can be DBNull.Value. So a checking for DBNull should be done.


Refactoring the former method with implementing the mentioned points will lead to a new class for which I have choosen the name RowContent

public class RowContent
{
    public bool IsValid { get; private set; }
    public DateTime RequestedDate { get; private set; }
    public String Material { get; private set; }
    public String Quantity { get; private set; }
    public DateTime SuggestedDate { get; private set; }

    public RowContent(string requestedDate, string material, string quantity)
    {
        RequestedDate = Convert.ToDateTime(requestedDate);
        Material = material;
        Quantity = quantity;
    }

    public void SetPureLeadTime(String leadTime)
    {
        SuggestedDate = GetWorkDays.AddWorkdays(RequestedDate, Convert.ToDouble(leadTime));
        IsValid = true;
    }
}

next we add a method for checking if the givven DataGridViewCell's value is DBNull.

private bool IsNotDBNull(DataGridViewCell cell)
{
    return !cell.Value.Equals(DBNull.Value);
}  

No we extract the retrieving of the datagridviews values to a separate method.

private IEnumerable<RowContent> GetSuggestedContents(DataGridView firstGrid, DataGridView secondGrid)
{
    IEnumerable<RowContent> contents =
        firstGrid.Rows
                        .Cast<DataGridViewRow>()
                        .Where(r=> IsNotDBNull(r.Cells["Requested_Date"]) &&
                                   IsNotDBNull(r.Cells["Material"]) &&
                                   IsNotDBNull(r.Cells["Requierd_Qty"]))
                        .Select(r => new RowContent(r.Cells["Requested_Date"].Value.ToString(),
                                                    r.Cells["Material"].Value.ToString(),
                                                    r.Cells["Requierd_Qty"].Value.ToString()));

    foreach (var rowContent in contents)
    {
        String leadTime =
            secondGrid.Rows
                            .Cast<DataGridViewRow>()
                            .Where(r => IsNotDBNull(r.Cells["PartNumber"]) &&
                                r.Cells["PartNumber"].Value.ToString() == rowContent.Material)
                            .Select(r => r.Cells["PUR LT"].Value.ToString()).FirstOrDefault();

        if (String.IsNullOrEmpty(leadTime)) { continue; }

        rowContent.SetPureLeadTime(leadTime);

    }
    return contents.Where(c => c.IsValid);
}  

which can be called like

IEnumerable<RowContent> contents = GetSuggestedContents(dataGridView9, dataGridView10);  

And last but not least the database manipulating code which returns an IList<string> containing the Message property of any occured OleDbException

private IList<string> UpdateSuggestedContents(IEnumerable<RowContent> contents)
{
    IList<string> errorMessages = new List<string>();

    using (OleDbConnection connection = new OleDbConnection("your connection string here"))
    using (OleDbCommand oleDbCmd = new OleDbCommand("InsertSuggested", connection))
    {
        oleDbCmd.CommandType = CommandType.StoredProcedure;

        OleDbParameter partNumberParameter = oleDbCmd.Parameters.Add("@PartNumber", OleDbType.VarChar, 30);
        OleDbParameter adjustedDateParameter = oleDbCmd.Parameters.Add("@AdjustedDate", OleDbType.Date, 50);
        OleDbParameter suggestedDateParameter = oleDbCmd.Parameters.Add("@SuggestedDate", OleDbType.Date, 50);
        OleDbParameter quantityParameter = oleDbCmd.Parameters.Add("@Qty", OleDbType.VarChar, 30);

        connection.Open();

        foreach (RowContent content in contents)
        {
            partNumberParameter.Value = content.Material;
            adjustedDateParameter.Value = content.RequestedDate;
            suggestedDateParameter.Value = content.SuggestedDate;
            quantityParameter.Value = content.Quantity;
            try
            {
                oleDbCmd.ExecuteNonQuery();
            }
            catch (OleDbException ex)
            {
                errorMessages.Add(ex.Message);
            }
        }
    }
    return errorMessages;
}
\$\endgroup\$
  • \$\begingroup\$ Amazing, still have much to learn in OOP, will get to work on the details and star reading about some good points you made, will stick to one specific style and such, I will review the code and try it out. thank you for the comments and suggestions. @Heslacher \$\endgroup\$ – MoralesJosue Feb 5 '15 at 16:39
2
\$\begingroup\$

I'm building on Heslacher's review.

First I'd move the names of cells into a static class with constants:

public static class CellName
{
    public const string RequestedDate = "Requested_Date";
    public const string Material = "Material";
    public const string RequiredQuantity = "Requierd_Qty";
}

I'd also improve his Linq query by extracting some logic into methods:

private bool IsValidRow(DataGridViewRow row)
{
    return IsNotDBNull(row.Cells[CellName.RequestedDate]) 
        && IsNotDBNull(row.Cells[CellName.Material]) 
        && IsNotDBNull(row.Cells[CellName.RequiredQuantity]);
}

private RowContent GetRowContent(DataGridViewRow row)
{
    return new RowContent(GetCellValue(row, CellName.RequestedDate),
        GetCellValue(row, CellName.Material),
        GetCellValue(row, CellName.RequiredQuantity));
}

private string GetCellValue(DataGridViewRow row, string cellName)
{
    return row.Cells[cellName].Value.ToString();
}

So his query then becomes:

IEnumerable<RowContent> contents = firstGrid.Rows
            .Cast<DataGridViewRow>()
            .Where(r => IsValidRow(r))
            .Select(r => GetRowContent(r));
\$\endgroup\$
  • \$\begingroup\$ Looks much neater ;-) \$\endgroup\$ – Heslacher Feb 5 '15 at 14:22
  • \$\begingroup\$ Great, will review it when I finish with @Heslacher code. Thank you for the comments and suggestions, looks great. \$\endgroup\$ – MoralesJosue Feb 5 '15 at 16:38

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.