1
\$\begingroup\$

There is nothing wrong, but is there a nicer way to re-write this program?

public IEnumerable<Employee> InsertEmployee(dynamic employees)
        {
            var newEmployeeRecord = new List<Employee>();

            foreach (dynamic employee in employees)
            {
                newEmployeeRecord.Add(

                    new Employee
                    {
                        EmployeeId = Convert.ToInt32(employee.EmployeeId)
                        ,
                        EmployeeName = Convert.ToString(employee.EmployeeName)
                        ,
                        Age = Convert.ToInt32(employee.Age)
                        ,
                        Salary = Convert.ToInt32(employee.Salary)
                        ,
                        DepartmentId = Convert.ToInt32(employee.DepartmentId)
                    }
                );
            }
            return EmployeeRecords.Concat(newEmployeeRecord);  
        }

Also, can we avoid typecasting? (I think it's not possible, so it could require an extension method.)

\$\endgroup\$
4
  • 3
    \$\begingroup\$ It helps to provide some plain English explanation of what the code is supposed to do. Part of a code review is to verify it does what you think it does. Without the explanation from you, all we know about the code is what it actually does, instead of what it should do. \$\endgroup\$ – RubberDuck Apr 12 '15 at 11:04
  • 3
    \$\begingroup\$ Why are you using dynamic? \$\endgroup\$ – Jeroen Vannevel Apr 12 '15 at 12:33
  • 5
    \$\begingroup\$ And, you should probably include the Employee class. Converting what seems to be Int32 to Int32 and String to String makes no sense. \$\endgroup\$ – Bjørn-Roger Kringsjå Apr 12 '15 at 12:34
  • \$\begingroup\$ @Bjørn-RogerKringsjå Since employee is dynamic rather than having any type, its members are dynamic also, and so must be converted to the appropriate type in order to compile. The run-type type identification will choose the correct converter overload. \$\endgroup\$ – Snowbody Apr 13 '15 at 2:29
3
\$\begingroup\$

Looking this over a second time, I see a few more problems, one of which was my mistake.

I believe that InsertEmployee is a bad name, since the number of employees inserted depends on the input parameter. A better name is InsertEmployees. Similarly, newEmployeeRecord is a bad name since it's also about an arbitrary set of employees, but it's extraneous anyway.

I notice that this code throws an exception if any of the Convert.To*() fails, maybe due to the CEO making $3 billion a year, or employeee IDs changing to have 10 digits. you mention wanting to avoid the type conversion, was this the main idea or was it speed?

I had missed the line about EmployeeRecords. What is it? It's not defined anywhere in the code provided; we can determine from context (though we shouldn't have to) that it is IEnumerable<Employee> or a derived class and that it's either a global, or a member of the same class that InsertEmployee() is, and that no matter what its protection status is, it's available to anyone who can call InsertEmployee(), which is everybody. Which brings up two questions: (a) why does it need to be returned from InsertEmployee() if it's accessible everyhwere that InsertEmployee() is? Just make it public. (b) Why wouldn't InsertEmployees just .Add the Employees directly to it?

public void InsertEmployees(dynamic employees)
{
    foreach (dynamic employee in employees)
    {
        EmployeeRecords.Add(new Employee
        {
            EmployeeId = Convert.ToInt32(employee.EmployeeId)
            ,
            EmployeeName = Convert.ToString(employee.EmployeeName)
            ,
            Age = Convert.ToInt32(employee.Age)
            ,
            Salary = Convert.ToInt32(employee.Salary)
            ,
            DepartmentId = Convert.ToInt32(employee.DepartmentId)
        });
    }
}

But let's get to the real underlying issue, why this whole code stinks. Several commenters hinted at the absurdity of using dynamic here. It's clear that you know the database schema; thus there's no reason to be using dynamic. .NET has good language features for working with database natively and in a strongly typed manner; it's called LINQ-to-SQL. Use it; you'll do less grunt work and have better code. Please see https://msdn.microsoft.com/en-us/library/bb386976%28v=vs.110%29.aspx

Old answer (incorrect, given that I missed what the function was returning):

Since you're returning an IEnumerable<>, you can simplify things greatly by using yield return to give elements back one at a time, rather than creating a temporary List and then throwing it away:

    public IEnumerable<Employee> InsertEmployee(dynamic employees)
    {
        foreach (dynamic employee in employees)
        {
            yield return new Employee
                {
                    EmployeeId = Convert.ToInt32(employee.EmployeeId)
                    ,
                    EmployeeName = Convert.ToString(employee.EmployeeName)
                    ,
                    Age = Convert.ToInt32(employee.Age)
                    ,
                    Salary = Convert.ToInt32(employee.Salary)
                    ,
                    DepartmentId = Convert.ToInt32(employee.DepartmentId)
                }
            );
        }
    }

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