current community

your communities

more stack exchange communities

Stack Exchange Inbox Reputation and Badges
tour help
Sign up ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 8 down vote favorite
1

Below is the current code which is used to update/insert records from Oracle view to SQL Server table using Dapper. There is not a field to check last record updated date in the oracle view so I have added a method to get hashcode by using property values. Since Oracle table has more than 15k records and each record has more more than 60 columns, this approach take more than 5 minutes. Any ideas/suggestions to improve below code?

using System;
using System.Configuration;
using System.Data.OracleClient;
using System.Data.SqlClient;
using System.Linq;
using System.Reflection;
using Dapper;

namespace SyncSQLSvrWithHRDB
{
    internal class Program
    {
        public static PropertyInfo[] PropertyNames = typeof(Employee).GetProperties();

        private static void Main(string[] args)
        {
            var OracleConStr = ConfigurationManager.ConnectionStrings["OracleCon"].ConnectionString;
            var SqlSvrConStr = ConfigurationManager.ConnectionStrings["SqlSvrCon"].ConnectionString;
            using (OracleConnection OraCon = new OracleConnection(OracleConStr))
            {
                var res = OraCon.Query<Employee>(Constants.SelectSql).ToList();

                res = res.GroupBy(x => x.EmpNumber.ToUpper()).Select(x => x.LastOrDefault()).ToList();
                using (SqlConnection Sqlcon = new SqlConnection(SqlSvrConStr))
                {
                    Sqlcon.Open();
                    for (int i = 0; i < res.Count; i++)
                    {
                        var item = Sqlcon.Query<Employee>(Constants.SelectEmpSql, new { EmpNumber= res[i].EmpNumber}).FirstOrDefault();
                        if (item == null) // new record found
                        {
                            Sqlcon.Execute(Constants.InsertSql, res[i]);
                        }
                        else if (GetHashcode(res[i]) != GetHashcode(item)) // record updated
                        {
                            Sqlcon.Execute(Constants.UpdateSql, res[i]);
                        }
                    }
                }
            }
        }

        public static int GetHashcode(Employee o)
        {
            int ret = 0;
            foreach (var prop in PropertyNames)
            {
                object propValue = o.GetType().GetProperty(prop.Name).GetValue(o, null);

                if (propValue != null)
                {
                    ret += propValue.GetHashCode();
                }
            }

            return ret;
        }
    }

    public class Employee
    {
        public String EmpNumber{ get; set; }

        // ... other properties (70)
    }
}
share|improve this question
4  
Consider creating a linked server to Oracle from your SQL Server. You can then perform your necessary work in T-SQL, in a set-based manner. Consider using the new MERGE syntax in SQL Server too. –  gvee Sep 23 '13 at 7:29
1  
As an aside, you should really implement GetHashCode on Employee, both for better encapsulation and for speed. Hash code generation is supposed to be fast, and reflecting the fields like that are the opposite of fast. –  Dan Lyons Nov 15 '13 at 18:57
    
Have you done any profiling? You're running a zillion queries against SQL instead of merging the two data sets, I'd guess that's your problem. –  Jon of All Trades Nov 18 '13 at 23:17

1 Answer 1

up vote 6 down vote

One thing I would say is that you don't need to create those connection string variables, you are only using them once in this program and you have them nicely hidden where they should be, so you can just call them when you use them, which in this case is once.

so instead of this

var OracleConStr = ConfigurationManager.ConnectionStrings["OracleCon"].ConnectionString;
var SqlSvrConStr = ConfigurationManager.ConnectionStrings["SqlSvrCon"].ConnectionString;
using (OracleConnection OraCon = new OracleConnection(OracleConStr))
{
    var res = OraCon.Query<Employee>(Constants.SelectSql).ToList();

    res = res.GroupBy(x => x.EmpNumber.ToUpper()).Select(x => x.LastOrDefault()).ToList();
    using (SqlConnection Sqlcon = new SqlConnection(SqlSvrConStr))
    {

Do this instead

using (OracleConnection OraCon = new OracleConnection(ConfigurationManager.ConnectionStrings["OracleCon"].ConnectionString))
{
    var res = OraCon.Query<Employee>(Constants.SelectSql).ToList();

    res = res.GroupBy(x => x.EmpNumber.ToUpper()).Select(x => x.LastOrDefault()).ToList();
    using (SqlConnection Sqlcon = new SqlConnection(ConfigurationManager.ConnectionStrings["SqlSvrCon"].ConnectionString))
    {

You have already named them nicely so you know exactly what they are, meaning there is no reason to create variables for these, they are only used once.

and instead of a for loop here:

using (SqlConnection Sqlcon = new SqlConnection(SqlSvrConStr))
{
    Sqlcon.Open();
    for (int i = 0; i < res.Count; i++)
    {
        var item = Sqlcon.Query<Employee>(Constants.SelectEmpSql, new { EmpNumber= res[i].EmpNumber}).FirstOrDefault();
        if (item == null) // new record found
        {
            Sqlcon.Execute(Constants.InsertSql, res[i]);
        }
        else if (GetHashcode(res[i]) != GetHashcode(item)) // record updated
        {
            Sqlcon.Execute(Constants.UpdateSql, res[i]);
        }
    }
}

you could use a foreach loop

using (SqlConnection Sqlcon = new SqlConnection(SqlSvrConStr))
{
    Sqlcon.Open();

    foreach (var record in res)
    {
        var item = Sqlcon.Query<Employee>(Constants.SelectEmpSql, new { EmpNumber= record.EmpNumber}).FirstOrDefault();
        if (item == null) // new record found
        {
            Sqlcon.Execute(Constants.InsertSql, record);
        }
        else if (GetHashcode(record) != GetHashcode(item)) // record updated
        {
            Sqlcon.Execute(Constants.UpdateSql, record);
        }
    }
}

This is a little bit cleaner and is more straight to the point about what you are doing. I would probably change some variable names around, but I will let you have that fun.

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.