Nsryjdtyk

I am a beginner web developer and I have to do an exercise to show an Employee table using a WCF service, 10 entries at a time, with the paging of the table controlled by a stored procedure. What I would like to know is if the code complies with best practices, and how I can improve it?

The service1.svc.cs class:

public class Service1 : IService1

{

    private string connection_string = ConfigurationManager.ConnectionStrings["EmployeeDB"].ConnectionString.ToString();



    public List<Employee> getEmployees(int startRowIndex, int maximumRows)

    {

        List<Employee> employees = new List<Employee>();

        SqlConnection connection = new SqlConnection(connection_string);



        SqlCommand command = new SqlCommand();

        command.CommandText = "getEmployees";

        command.CommandType = CommandType.StoredProcedure;

        command.Parameters.Add(new SqlParameter("@startRowIndex", startRowIndex));

        command.Parameters.Add(new SqlParameter("@maximumRows", maximumRows));



        try

        {

            connection.Open();

            SqlDataReader dr = command.ExecuteReader();



            while (dr.Read())

            {

                Employee employee = new Employee();

                employee.ID = Convert.ToInt32(dr["EmployeeID"]) == 0 ? 0 : Convert.ToInt32(dr["EmployeeID"]);

                employee.Name = dr["Name"].ToString() == string.Empty ? "" : dr["Name"].ToString();

                employee.Surname = dr["Surname"].ToString() == string.Empty ? "" : dr["Surname"].ToString();

                employee.Address = dr["Address"].ToString() == string.Empty ? "" : dr["Address"].ToString();

                employee.Telephone = dr["Telephone"].ToString() == string.Empty ? "" : dr["Telephone"].ToString();

                employees.Add(employee);

            }

            connection.Close();

            return employees;

        }

        catch (Exception e) { throw e; }

    }

    public int getTotalEmployees()

    {

        int total = 0;



        using (SqlConnection connection = new SqlConnection(connection_string))

        {

            SqlCommand command = new SqlCommand();

            command.CommandText = "getTotalEmployees";

            command.CommandType = CommandType.StoredProcedure;

            try

            {

                connection.Open();

                total = Convert.ToInt32(command.ExecuteScalar());

            }

            catch (Exception ex) { throw ex; }                

        }

        return total;

    }

}

The service interface:

[ServiceContract]

public interface IService1

{

    [OperationContract]

    List<Employee> getEmployees(int startRowIndex, int maximumRows);

    [OperationContract]

    int getTotalEmployees();

}



[DataContract]

public class Employee

{

    [DataMember]

    public int ID { get; set; }

    [DataMember]

    public string Name { get; set; }

    [DataMember]

    public string Surname { get; set; }

    [DataMember]

    public string Address { get; set; }

    [DataMember]

    public string Telephone { get; set; }

}

Data Access Layer:

public class DALEmployee

{

    EmployeeService.Service1Client client = new EmployeeService.Service1Client();

    public List<Employee> getEmployees(int startRowIndex, int maximumRows)

    {

        List<Employee> employees = new List<Employee>();

        var employeeData = client.getEmployees(startRowIndex, maximumRows);

        if (employeeData.Count() > 0)

        {

            foreach (var item in employeeData)

            {

                Employee employee = new Employee() { ID = item.ID, Address = item.Address, Name = item.Name, Surname = item.Surname, Telephone = item.Telephone };

                employees.Add(employee);

            }

        }

        return employees;

    }

    public int getTotalEmployees()

    {

        return client.getTotalEmployees();

    }

}

Controller:

public class HomeController : Controller

{

    private EmployeeService.Service1Client client = new EmployeeService.Service1Client();

    int maximumRows = 10;



    public ActionResult Index()

    {            

        double startRowIndex = Convert.ToDouble(Session["startRowIndex"]) == 0 ? 1 : Convert.ToDouble(Session["startRowIndex"]);

        Session["startRowIndex"] = startRowIndex;

        int totalEmployees = new DALEmployee().getTotalEmployees();

        ViewBag.PageNumber = Math.Floor(startRowIndex / maximumRows) == 0 ? 1 : Math.Floor(startRowIndex / maximumRows);

        List<Employee> employees = new DALEmployee().getEmployees(Convert.ToInt32(startRowIndex), maximumRows);                   

        return View(employees);

    }



    public ActionResult Next()

    {

        int startRowIndex = Convert.ToInt32(Session["startRowIndex"]) <= maximumRows ? 1 + maximumRows : Convert.ToInt32(Session["startRowIndex"]) + maximumRows;

        Session["startRowIndex"] = startRowIndex;

        return RedirectToAction("Index");

    }



    public ActionResult Previous()

    {

        int startRowIndex = Convert.ToInt32(Session["startRowIndex"]) <= maximumRows ? 1 : Convert.ToInt32(Session["startRowIndex"]) - maximumRows;

        Session["startRowIndex"] = startRowIndex;

        return RedirectToAction("Index");

    }

}

View:

@model IEnumerable<ExerciseER.Models.Employee>

@{

    ViewBag.Title = "Employee Information";

}

<div class="jumbotron">

    <table class="table table-bordered">

        <thead>

            <tr>

                <th>@Html.DisplayNameFor(model => model.Name)</th>

                <th>@Html.DisplayNameFor(model => model.Surname)</th>

                <th>@Html.DisplayNameFor(model => model.Address)</th>

                <th>@Html.DisplayNameFor(model => model.Telephone)</th>

            </tr>

        </thead>

        <tbody>

            @foreach (var item in Model)

            {

                <tr>

                    <td>@Html.DisplayFor(model => item.Name)</td>

                    <td>@Html.DisplayFor(model => item.Surname)</td>

                    <td>@Html.DisplayFor(model => item.Address)</td>

                    <td>@Html.DisplayFor(model => item.Telephone)</td>

                </tr>

            }

            <tr>

                <td>@Html.ActionLink("Previous", "Previous", null, new { @class = "btn btn-info"})</td>

                <td>@ViewBag.PageNumber</td>

                <td>@Html.ActionLink("Next", "Next", null, new { @class = "btn btn-info" })</td>

            </tr>

        </tbody>

    </table>

</div>

getEmployees SP with paging:

create procedure getEmployees

(

    @startRowIndex int, 

    @maximumRows int

)

AS 



declare @firstInt int, @startRow int

if (@startRowIndex <= (select COUNT(EmployeeID) from dbo.[Employee]))

begin

    set ROWCOUNT @startRowIndex



    select @firstInt = EmployeeID from dbo.[Employee] order by EmployeeID



    set ROWCOUNT @maximumRows



    select EmployeeID, Name, Surname, Address, Telephone

    from dbo.[Employee] where EmployeeID >= @firstInt order by EmployeeID



    set ROWCOUNT 0

end

GO

getTotalEmployees SP:

create procedure getTotalEmployees

as 



select COUNT(EmployeeId) from dbo.[Employee] where [Status] = 1



go

The main things that I consider wrong or I don't fully get.

service1

• Probably is just a test case but if not, never name classes in that way.


• The try/catch/throw makes no sense to me. You can simply remove them.


• 
  SqlConnection, Command and reader should always be used within a using clause.

DALEmployee

What is the concept behind this class? Do you really need it?
If yes, my recommendation would be to inject IService1. You can google dependency injection to get more details.

Controller

If you don't need DALEmployee, you can simply inject IService1 directly in the controller.

getEmployees

Generally is weird to me how you achieve the pagination. Generally i would use as parameters CurrentPage and PageSize and for me it seems you that you are using the employee id. How would you extend this if you have to sort by other criteria rather than employee id? Additionaly, you are using a different where on the get count and on the procedure itself, what will lead to wrong pages.
Finally, depending on your sql server version, you may use this operators to paginate.

SELECT * FROM TableName ORDER BY whatever OFFSET 10 ROWS FETCH NEXT 10 ROWS ONLY;

I can't speak to the application code but I can comment on the stored procedures and hopefully help a little in that area...

@oromrub beat me to the punch with regard to the use of the offset fetch...

Here is how I would typically lay out a stored procedure & why...

-- SET ANSI_NULLS & QUOTED_IDENTIFIER ON...

-- The server "should have these set as the default

-- but it's still a good idea to set them yourself

-- at the procedure level.

SET ANSI_NULLS ON;

GO

SET QUOTED_IDENTIFIER ON;

GO

CREATE PROCEDURE dbo.GetEmployees

/* =========================================================================================================

10/5/2017 Your Name or Initials,  The reason for the procedures existance. Commenting you code is important.

========================================================================================================= */

/* -- sample execution --

EXEC dbo.GetEmployees 

    @PageNumber = 1,

    @RowsPerPage = 30

*/

    @PageNumber INT = 1,

    @RowsPerPage INT = 30

AS

BEGIN

    SET NOCOUNT ON; -- (SET NOCOUNT ON prevents the sending of DONE_IN_PROC messages to the client for each statement in a stored procedure.)



    SELECT

        e.EmployeeId,

        e.FirstName,

        e.LastName  

    FROM

        dbo.Employee e

    ORDER BY

        e.EmployeeId

    OFFSET (@PageNumber - 1) * (@RowsPerPage + 1) ROWS FETCH NEXT @RowsPerPage ROWS ONLY;

END;

GO

Here are a few things that should become automatic... Staring from the top...

1) Comment your code! Don't expect to remember why you coded something the way you did a year from now. I like having a dedicated section at the top so it's always easy to figure out what the proc is for, when it was initially created and what revisions have been made over time. Plus it's just a common courtesy for the people you'll be working with.

2) Set NOCOUNT ON; The count messages that SQL Server sends back to let you know how many rows you've affected is fine while you're working SSMS but you don't want them going back to you application.

3) Use aliases!!! Forums are littered with the panicked cries of people who can't figure out why their query is inexplicable returning data that it shouldn't. Here's an example I pulled from an article on simple talk...

SELECT sale_date, sale_amount

FROM Sales AS S

WHERE sale_date IN (SELECT sale_date 

                    FROM Calendar AS C

                    WHERE holiday_name IS NOT NULL);

Even if that wasn't as issue, it would still be important. Once you get more than a few tables joined in a query, it can be a real pain trying to decipher which columns come from which tables.

5) Always include the schema name when referencing a table. For one, it helps SQL Server by saving if from having to check different schemas.

The real biggie... You're querying the same table 3 times to do something really simple. As a rule, you want you code to be as efficient as possible. That means don't make 3 calls to the same table when you can get what you need in one trip. It means a lot more than that, but that's way to broad a topic for a wee forum post.

Anyway, best of luck with the exercise. Hopefully some of this was helpful.

Cheers! :)