locked
Good practise? RRS feed

  • Question

  • User2096782806 posted

    Good day!

    I have recently created a web application to access an excel file and load the information into a dataset.

    The piece of code that I have posted below is the code that puts that excel files data into a sql Database.

    Please bare with me, I would only like to get some feedback on the architecture of the code. Is it good practise to use so many inf statments and control structures nested to obtain the desired result. And if not, what can I do to speed up the code?

    Any critism will be very much appreciated! Good or bad!

    DataTable myReferenceTable;
    
    protected string checkRecordStatus(string theCropName, string theMarket, string theYear, string theMonth)
            {
                string theStatus = "";
                try
                {
                    SqlCommand myCommand = new SqlCommand("SELECT theStatus FROM " + tablToBeAccessed + " WHERE CropName='" + theCropName + "' AND Market='" + theMarket + "' AND YearSelect='" + theYear + "' AND MonthSelect='" + theMonth + "'", myConnection);
                    myConnection.Open();
                    SqlDataReader myReader = myCommand.ExecuteReader();
    
                    if (myReader.HasRows)
                    {
                        myReader.Read();
                        theStatus = myReader.GetString(0);
                    }
    
                    myReader.Close();
                    myConnection.Close();
                }
                catch (Exception ex)
                {
                    Response.Write(ex.ToString());
                }
                return theStatus;
            }
    
    
    protected DataTable getReferencesTable()
            {
                DataTable myDataTable = new DataTable();
                try
                {
                    SqlCommand myCommand = new SqlCommand("SELECT nameAtDatabase, idAtDatabase, nameAtSource, tableName FROM tblCommodities WHERE cropType='" + theCommodityType + "'", myConnection);
                    myConnection.Open();
                    SqlDataAdapter myAdapter = new SqlDataAdapter();
    
                    myAdapter.SelectCommand = myCommand;
                    myAdapter.Fill(myDataTable);
                    myConnection.Close();
                }
                catch (Exception ex)
                {
                    Response.Write(ex.ToString());
                }
                return myDataTable;
            }
    
    protected void importTheData()
            {
                try
                {
                    DataSet myDataSet = new DataSet();
                    myDataSet = readDataFromFile();
    
                    myReferenceTable = getReferencesTable();
    
                    string theMarketName = "";
                    int theMonth = Convert.ToInt16(myDataSet.Tables[0].Rows[0][1].ToString()) + 1;
                    for (int k = 0; k < myDataSet.Tables[0].Rows.Count; k++)
                    {
                        if (myDataSet.Tables[0].Rows[1][0].ToString().Substring((myDataSet.Tables[0].Rows[1][0].ToString().ToLower().IndexOf("market,") + 8), (myDataSet.Tables[0].Rows[1][0].ToString().Length - (myDataSet.Tables[0].Rows[1][0].ToString().ToLower().IndexOf("market,") + 8)) - 1) == myDataSet.Tables[0].Rows[0][0].ToString())
                        {
                            if (myDataSet.Tables[0].Rows[k][2].ToString().ToLower() != "j")
                            {
                                if (myDataSet.Tables[0].Rows[k][0].ToString() != "" && myDataSet.Tables[0].Rows[k][1].ToString() == "" && myDataSet.Tables[0].Rows[k][2].ToString() == "")
                                {
                                    theMarketName = myDataSet.Tables[0].Rows[k][0].ToString().Substring((myDataSet.Tables[0].Rows[k][0].ToString().ToLower().IndexOf("sold on") + 7), (myDataSet.Tables[0].Rows[k][0].ToString().ToLower().IndexOf("fresh") - (myDataSet.Tables[0].Rows[k][0].ToString().ToLower().IndexOf("sold on") + 7)) - 1);
                                }
    
                                if (myDataSet.Tables[0].Rows[k][1].ToString().ToLower().Trim() == "r")
                                {
                                    if (myDataSet.Tables[0].Rows[k][0].ToString().ToLower().Trim().Contains("total"))
                                    {
    
                                    }
                                    else
                                    {
                                        for (int y = 0; y < myReferenceTable.Rows.Count - 1; y++)
                                        {
                                            if (myDataSet.Tables[0].Rows[k][0].ToString().Trim().ToLower() == myReferenceTable.Rows[y][2].ToString().Trim().ToLower())
                                            {
                                                if (checkRecordStatus(myReferenceTable.Rows[y][0].ToString().Trim().ToLower(), theMarketName.ToLower().Trim(), myDataSet.Tables[0].Rows[0][0].ToString(), (theMonth - 1).ToString()) != "approved")
                                                {
                                                    SqlCommand myCommand = new SqlCommand("UPDATE " + tablToBeAccessed + " SET RValue = '" + myDataSet.Tables[0].Rows[k][theMonth].ToString() + "', TValue = '" + myDataSet.Tables[0].Rows[k - 1][theMonth].ToString() + "', theStatus='unchecked' WHERE CropName='" + myReferenceTable.Rows[y][0].ToString().Trim().ToLower() + "' AND Market='" + theMarketName.ToLower().Trim() + "' AND YearSelect='" + myDataSet.Tables[0].Rows[0][0].ToString() + "' AND MonthSelect = '" + (theMonth - 1).ToString() + "'", myConnection);
                                                    //SqlCommand myCommand = new SqlCommand("UPDATE " + tablToBeAccessed + " SET RValue = '5000', TValue = '186365', theStatus='unchecked' WHERE CropName='" + myReferenceTable.Rows[y][0].ToString().Trim().ToLower() + "' AND Market='" + theMarketName.ToLower().Trim() + "' AND YearSelect='" + myDataSet.Tables[0].Rows[0][0].ToString() + "' AND MonthSelect = '" + (theMonth - 1).ToString() + "'", myConnection);
                                                    myConnection.Open();
                                                    myCommand.ExecuteNonQuery();
                                                    myConnection.Close();
                                                }
                                            }
                                        }
                                    }
                                }
                            }
                        }
                    }
                }
                catch (Exception ex)
                {
                    Response.Write(ex.ToString());
                }
            }
    Monday, October 10, 2011 2:08 AM

Answers

  • User-757609608 posted

    Hi,

     Multilple IF can be Replaced by single if with && opereter in a sequence.

      if(ds !=null)

    {

      if(ds.tables.count>0)

    {

     if(ds.tables[0].rows.count>0)

    {

    //// your code.

    }

    }

    }

    ELSE

    better to use

    if(ds!=null && ds.Tables.Count>0 && ds.Tables[0].Rows.count>0)

    {

      

    }

    • Marked as answer by Anonymous Thursday, October 7, 2021 12:00 AM
    Monday, October 10, 2011 5:52 AM

All replies

  • User-757609608 posted

    Hi,

     Multilple IF can be Replaced by single if with && opereter in a sequence.

      if(ds !=null)

    {

      if(ds.tables.count>0)

    {

     if(ds.tables[0].rows.count>0)

    {

    //// your code.

    }

    }

    }

    ELSE

    better to use

    if(ds!=null && ds.Tables.Count>0 && ds.Tables[0].Rows.count>0)

    {

      

    }

    • Marked as answer by Anonymous Thursday, October 7, 2021 12:00 AM
    Monday, October 10, 2011 5:52 AM
  • User3866881 posted

    Hello UberMeister :)

    Looking at your codes, I think wholly speaking it's good, because you've splitted them into different kinds of functions.

    However in my mind, I think your "importTheData" has a very complicated logic (with so many if in the "for"?)

    Maybe you can simplify it.

    Tuesday, October 11, 2011 3:28 AM