My ASP.NET MVC controllers seem like in mess

Question

I am developing program with asp.net MVC and I feel like I'm doing it wrong in these parts:

1. I'm populating all process in controller, because I don't know how to put it in model (how to call it). I have read some reference and people say that we must put process in model (?)

2. Many of my action have same process (repetitive). For example, many view has to load data from same SQL table, so I repeat the process to call the data in many view.

Here is part of my controller, can you please tell me if I'm doing it right or wrong etc.

public class EmployeeController : Controller
{
    private UserContext db = new UserContext();

    public async Task<ActionResult> Index()
    {

        string strquery = "Select employee.EmpId,EmpName,OrgName,EmpJobLvl,JobLvlName,EmpJobTtl,JobTtlName,employee.EmpType,"
                            + "EmpDateStart,EmpEmail,EmpHandphone,EmpJoinDate,EmpSignDate,EmpNPWP,EmpResAddr,city1.CityName AS CityName1,EmpResZipCode, "
                            + "EmpResPhone,EmpResStatus,EmpOriAddr,city2.CityName AS CityName2,EmpOriZipCode,EmpOriPhone,EmpOriStatus,EmpMaritalSt,EmpSex"
                            + " FROM employee "
                            + " join Organization (nolock) on employee.emporg = organization.orgcode "
                            + " join jobtitle (nolock) on employee.empjobttl = jobtitle.jobttlcode "
                            + " join JobLevel (nolock) on employee.EmpJobLvl = JobLevel.JobLvlCode"
                            + " join EmpType (nolock) on employee.EmpType = EmpType.EmpType"
                            + " INNER JOIN City city1 ON employee.EmpResCity = city1.CityCode"
                            + " INNER JOIN City city2 ON employee.EmpOriCity = city2.CityCode"
                            + " WHERE employee.EmpId = {0}";
        string rtsquery = "Select employee.EmpID,EmpFamRelation,EmpFamName,EmpFamDateBrith,CityName"
                            + ",EmpFamSex,EmpFamAlive,MaritalSt,EmpFamMaritalDate,EmploymentSt,EmpFamOccupation "
                            + ",EmpFamComp,EmpCompAddr,EmpFamEdu,EmpFamEduIns FROM employee  "
                            + "join EmpFamilys on employee.EmpId = EmpFamilys.EmpId "
                            + "join City on City.CityCode = EmpFamilys.EmpFamCityBirth "
                            + "WHERE employee.EmpId = {0}";
        string srtquery = "Select employee.EmpID,EmpEduStatus,EmpEduLevel,EmpEduName,EmpEduIns,CityName,EmpEduGraduate"
                            + ",EmpEduGrade,EmpEduResult,EmpEduFrontTitle,EmpEduEndTitle FROM employee  "
                            + "join EmpEducations on employee.EmpId = EmpEducations.EmpId "
                            + "join City on City.CityCode = EmpEducations.EmpEduCity "
                            + "WHERE employee.EmpId = {0}";
        var model = new PersonalDataViewModels();

        model.A = db.EmpFamilys.SqlQuery(rtsquery, Convert.ToInt32(@User.Identity.Name)).ToList();
        model.X = await db.Employee.SqlQuery(strquery, Convert.ToInt32(@User.Identity.Name)).FirstOrDefaultAsync();
        model.C = db.EmpEducations.SqlQuery(srtquery, Convert.ToInt32(@User.Identity.Name)).ToList();



        return View(model);

    }


    public ActionResult Details()
    {
        string strquery = "Select employee.EmpId,EmpName,OrgName,EmpJobLvl,JobLvlName,EmpJobTtl,JobTtlName,employee.EmpType,"
                            + "EmpDateStart,EmpEmail,EmpHandphone,EmpJoinDate,EmpSignDate,EmpNPWP,EmpResAddr,city1.CityName AS CityName1,EmpResZipCode, "
                            + "EmpResPhone,EmpResStatus,EmpOriAddr,city2.CityName AS CityName2,EmpOriZipCode,EmpOriPhone,EmpOriStatus,EmpMaritalSt,EmpSex"
                            + " FROM employee "
                            + " join Organization (nolock) on employee.emporg = organization.orgcode "
                            + " join jobtitle (nolock) on employee.empjobttl = jobtitle.jobttlcode "
                            + " join JobLevel (nolock) on employee.EmpJobLvl = JobLevel.JobLvlCode"
                            + " join EmpType (nolock) on employee.EmpType = EmpType.EmpType"
                            + " INNER JOIN City city1 ON employee.EmpResCity = city1.CityCode"
                            + " INNER JOIN City city2 ON employee.EmpOriCity = city2.CityCode"
                            + " WHERE employee.EmpId = {0}";
        string srtquery = "Select employee.EmpID,LvAppId,SubmitDate,LvPeriod,LvDesc,Status,Type,Comment"
                           + " FROM employee  "
                           + "join LeaveApps on employee.EmpId = LeaveApps.EmpId "
                           + "WHERE employee.EmpId = {0}";

        var model = new LeaveApplicationViewModels();
        model.X = db.Employee.SqlQuery(strquery, Convert.ToInt32(@User.Identity.Name)).FirstOrDefault();
        model.C = db.LeaveApps.SqlQuery(srtquery, Convert.ToInt32(@User.Identity.Name)).ToList();

        foreach (var row in model.C)
        {
            string wwwquery = "Select LvAppId,LvDate FROM LeaveDate WHERE LvAppId= {0}";
            row.D = db.LeaveDate.SqlQuery(wwwquery, row.LvAppId).ToList();
        }


        /*info*/
        string sssquery = "Select LeaveDate.LvAppId,LvDate FROM LeaveDate "
                          + " join LeaveApps (nolock) on LeaveDate.LvAppId = LeaveApps.LvAppId "
                          + " WHERE LeaveDate.EmpId= {0} AND Status <> {1} AND Type = {2} ";
        var used =db.LeaveDate.SqlQuery(sssquery, Convert.ToInt32(@User.Identity.Name), "rejected","Annual").ToList().Count;

        string maxquery = "Select Max,LeaveType,ID,TypeDetail FROM LeaveRule WHERE LeaveType='Annual'";
        var max = db.LeaveRule.SqlQuery(maxquery).FirstOrDefault().Max;

        var rem = max - used;
        ViewBag.a = used;
        ViewBag.b= rem;
        ViewBag.c = max;

        /*rule*/
        int comp = 0;
        var pendingtoogle = model.C.Where(x => x.Status == "pending" && x.Type == "Annual").ToList().Count;
        if (used < 12 && pendingtoogle == 0)
        { comp = 1; }
        else {comp = 0; }
        ViewBag.comp = comp;

        return View(model);
    }

Answer 1

...can you please tell me if I'm doing it right or wrong?

Unfortunately, most here would probably disagree that this the right way to structure things.

There is a number of issues that I can see:

You have no layering to the application, your controller populates the view with result that it, itself, pulls out of the database. Split the logic into layers, Data Access, Business Rules, Presentation... maybe others. You should then be able to reuse those layers between controllers and even between applications if you separate them into their own modules.

You are writing SQL strings in c#. Personally, any time I have a block of red on my screen where another approach could replace it with a structured and syntactically validated block of legible code, I opt for the ladder. Use your database framework to create a procedure that can be used for a number of these data retrieval operations against the same table-set. Or you could use Entity Framework to allow you write the SQL using LINQ. Anything is better then using string SQL commands.

Variable Naming:

string strquery...string rtsquery...string srtquery

Those are a problem, anyone other than you will have a hard time following along trying to read the code. If these variables are used or updated in different places it magnifies your code complexity.

This could be a big problem too:

foreach (var row in model.C)
    {
        string wwwquery = "Select LvAppId,LvDate FROM LeaveDate WHERE LvAppId= {0}";
        row.D = db.LeaveDate.SqlQuery(wwwquery, row.LvAppId).ToList();
    }

That looks like we are asking db for something per row in whatever model.C is. That will cause a problem if model.C decides to have 1,000 rows.

Refactor that to make a single db call with a list of LvAppId's

That's all I've had the change to really look at, there may well be more issues.

Answer 2

first of all you can move your db context to a separate class and preferably project as well. this class would be your data layer, it should have all the queries in it. You would call them as functions like this (which sould be better named to say what they are getting)
model.A = getStrQueryData(EmpId) model.X = getRtsQueryData(EmpId) model.C = getSrtQueryData(EmpId)
You could then go further and in your model class either add a constructor that populates that data when given an employee id or use a separate function. your controller would then be:
var model = new PersonalDataViewModels(EmpId); return View(model);

or

var model = new PersonalDataViewModels(); model.getPersonalData(EmpId); return View(model);

Answer 3

Basically what you did wrong was to copy/paste code.

DO NOT copy code.

Instead, if you want to reuse code

• Move the code in a function (this is what functions were invented for).
• Call the function.

There are different opinions on where to put the function (model, service layer, controller, base controller) but that is the icing on the cake - you can sort that out later.

Practise creating functions and reusing code first. Once you have that skill, the rest will come easily.