1
\$\begingroup\$

I create this class method to Enter Selected Items Form CheckboxList to associated resource I use ADO.net & store Proc to insert data can someone help me to improve it and make to so clean instead of my Hacks BTW I use Microsoft Enterprise Lib

public  static void Insert(List<int> listSkills, int rID)
        {
            var connection = ConfigurationManager.ConnectionStrings["SiteSqlServer"]; 
            Database objDB = new SqlDatabase(connection.ConnectionString);
            //int val = 0;
            using (DbCommand cmd = objDB.GetStoredProcCommand("Insert_Skills_Resources"))
            {
                foreach (var s in listSkills)
                {
                    try
                    {
                        objDB.AddInParameter(cmd, "@SkillID", DbType.Int32, s);
                        objDB.AddInParameter(cmd, "@ResourceID", DbType.Int32, rID);

                        objDB.ExecuteNonQuery(cmd);
                        cmd.Parameters.Clear();


                    }
                    catch (Exception ex)
                    {

                        throw ex;

                    }
                }

            } 
\$\endgroup\$
  • \$\begingroup\$ Any reason why you're not using some ORM? \$\endgroup\$ – svick Apr 25 '13 at 16:48
  • \$\begingroup\$ because I don't know how to use EF \$\endgroup\$ – NinjaDeveloper Apr 25 '13 at 17:42
  • \$\begingroup\$ @a_Elnajjar I updated my answer with some important info about the parameter name i.e. using Database.BuildParameterName to create it. \$\endgroup\$ – RobH Apr 30 '13 at 8:06
2
\$\begingroup\$

If you are using the Enterprise Library it is often better to set up a default connection in the configuration file:

<configuration>
  <configSections>
    <section name="dataConfiguration" 
       type="Microsoft.Practices.EnterpriseLibrary.Data.Configuration.DatabaseSettings,
            Microsoft.Practices.EnterpriseLibrary.Data, Version=5.0.414.0, 
            Culture=neutral, PublicKeyToken=null" 
        requirePermission="false"/>
  </configSections>

  <dataConfiguration defaultDatabase="SiteSqlServer" />

  <!-- Rest of your settings here, including your SiteSqlServer connection string -->
</configuration>

Now you don't need to look up the connection string - you can rely on the Enterprise library to get it for you:

public static void Insert(IEnumerble<int> skills, int resourceId)
{
    // No dependency on SQL anymore - the connection string is found
    // from the dataConfiguration section in your config file. If you change
    // to another (supported) provider, you don't need to change this code.
    Database db = DatabaseFactory.CreateDatabase();

    // Add in your schema to the stored proc name (I've assumed dbo).
    using (var cmd = db.GetStoredProcCommand("dbo.Insert_Skills_Resources"))
    {
        // This method populates the parameters collection of the DbCommand
        // by interrogating the database about the command.
        db.DiscoverParameters(cmd);

        // As the parameters are now known, we can just set the value of 
        // them directly, no need specify direction or type etc.
        cmd.Parameters["ResourceID"].Value = resourceId;

        foreach (var skill in skills)
        {
            cmd.Parameters["SkillID"].Value = skill;
            db.ExecuteNonQuery(cmd);
                    // Don't need to clear the parameters now.
        }
        // <Pointless try catch removed.>
    }
}   

EDIT:

Sorry, it's been a year or more since I used the Enterprise Library I can't remember whether you need the @ at the start of the parameter name or not - it might be cmd.Parameters["@SkillID"].

EDIT 2:

I remembered why I couldn't remember if you need the @ or not (if that makes sense). You do need the @ BUT, you should use the Database.BuildParameterName method to create the parameter in the format the current provider expects - for sql server this is an @ symbol before the parameter name.

This is how you should do it:

cmd.Parameters[db.BuildParameterName("ResourceID")].Value = resourceId;

With this final correction the code is actually ignorant of the provider!

\$\endgroup\$
1
\$\begingroup\$
catch (Exception ex)
{
    throw ex;
}

This code doesn't make any sense. If you want to rethrow an exception, use just throw;, so that the stack trace is not reset. But if that's all you do in your catch block, then there is no reason to use try-catch, you should just remove it.

\$\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.