Inserting employee records

Question

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.)

Answer 1

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)
                }
            );
        }
    }