Code optimization by constant if-structure iteration removal

Question

Below you will find the specific code that I am attempting to optimize. I have a method previous to called getCount() which, based on similar parameters as below, calculates how many record sets of 75 will be pulled based on one of the below query.

The Query that is executed is based on the department, projLead and status string while projSet is used to skip records.

So basically, count returns the number of iterations that will need to be performed to obtain the full result set. An ajax call inside a for loop iterates until i> the last projSet() and calls the below method.

The problem with this is that every time it gets the next sub result set of 75 records it needs to go through the if structure below. Is there a way to basically have the below method determine which query to use, return the query as a String or something and then in another method pass the queryString and then run it as a query until the number of sets is greater than the total number of sets requiring pulling? This way, the if structure is just used to determine the query which can be passed into another method with just the query being pass and run inside it and it returns the result sets.

The iterative part would be handled via ajax, but I do not know how to return the query and have it passed into another method through another ajax call where it just executes the query.

 public ActionResult ProjList(String department, String projLead, String status, int projSet)
 {
 //Query where all filters are applied
 var query = db.LatestStatus.Where(x => (x.Department.CompareTo(department) == 0) 
 && (x.ProjectLeader.CompareTo(projLead) == 0) 
 && (x.Status.CompareTo(status) == 0))
 .OrderBy(x => x.ProjectID).Skip(projSet * 75).Take(75).ToList();
 //Check to see which filters are not applied
 // - > all filters are not applied
 // - > find which filters are applied and return it's query
 if (department == null || (department.Equals("Department")
 && projLead.Equals("Project Leader") 
 && status.Equals("Status")))
 {
 query = db.LatestStatus
 .OrderBy(x => x.ProjectID)
 .Skip(projSet * 75).Take(75).ToList();
 }
 else if (projLead.Equals("Project Leader")){
 if (status.Equals("Status"))
 {
 query = db.LatestStatus
 .Where(x => x.Department.CompareTo(department) == 0)
 .OrderBy(x => x.ProjectID).Skip(projSet * 75).Take(75).ToList();
 }
 else if (!department.Equals("Department"))
 {
 query = db.LatestStatus
 .Where(x => (x.Department.CompareTo(department) == 0) && (x.Status.CompareTo(status) == 0))
 .OrderBy(x => x.ProjectID).Skip(projSet * 75).Take(75).ToList();
 } 
 else
 {
 query = db.LatestStatus
 .Where(x => x.Status.CompareTo(status) == 0)
 .OrderBy(x => x.ProjectID).Skip(projSet * 75).Take(75).ToList();
 }
 }
 else if (department.Equals("Department")){
 if (status.Equals("Status"))
 {
 query = db.LatestStatus
 .Where(x => x.ProjectLeader.CompareTo(projLead) == 0)
 .OrderBy(x => x.ProjectID).Skip(projSet * 75).Take(75).ToList();
 }
 else if (!department.Equals("Project Leader"))
 {
 query = db.LatestStatus
 .Where(x => (x.Status.CompareTo(status) == 0) && (x.ProjectLeader.CompareTo(projLead) == 0))
 .OrderBy(x => x.ProjectID).Skip(projSet * 75).Take(75).ToList();
 }
 else
 {
 query = db.LatestStatus
 .Where(x => x.Status.CompareTo(status) == 0)
 .OrderBy(x => x.ProjectID).Skip(projSet * 75).Take(75).ToList();
 }
 } 
 else if (department.Equals("Status")){
 if (status.Equals("Department"))
 {
 query = db.LatestStatus
 .Where(x => x.ProjectLeader.CompareTo(projLead) == 0)
 .OrderBy(x => x.ProjectID).Skip(projSet * 75).Take(75).ToList();
 }
 else if (!department.Equals("Project Leader"))
 {
 query = db.LatestStatus
 .Where(x => (x.Department.CompareTo(department) == 0) && (x.ProjectLeader.CompareTo(projLead) == 0))
 .OrderBy(x => x.ProjectID).Skip(projSet * 75).Take(75).ToList();
 }
 else
 {
 query = db.LatestStatus
 .Where(x => x.ProjectLeader.CompareTo(projLead) == 0)
 .OrderBy(x => x.ProjectID).Skip(projSet * 75).Take(75).ToList();
 }
 }
 return View(query);
 }

• Heslacher: What do you mean with getCount() and count? I don't seee this in your code.
• Malachi: you should edit the title to something a little less confusing.
• RubberDuck: Please try to create a title for your question that explains what your code does instead of what you're concerned about. We've found that better titles get more eyes on your question, which in turn leads to a better experience here.

Answer 1

Besides the two answers already given, I'd also suggest the following.

Rename projLead to projectLeader. Same for projSet.

But most importantly, I'd rethink your entire if. I suspect there are several errors -- department.Equals("Status")? department.Equals("Project Leader")? -- but more importantly I cannot figure out the logic and I suspect you're missing plenty of cases. Try to document the behavior you expect to see, and implement that as simple as possible.

Your method has four parameters, which IMHO is starting to become unwieldy. Can't you pass a class like this?

public class SearchParameters
{
 public string Department {get;set;}
 public string ProjectLeader {get;set;}
 public string Status {get;set;}
 public int ProjSet {get;set;}
}

Don't mix String and int; be consistent.

• Bialy R Bert: Thank you for pointing out the logic... its a result of copy and pasting similar code and not adjusting it...

Answer 2

You have two different Bracing styles in this code, which can be very confusing,

 else if (projLead.Equals("Project Leader")){
 if (status.Equals("Status"))
 {

I would not do that and many would agree that you should at the least pick one bracing style and stick with it.

The most popular bracing style is to have braces on their own line ( this is the more accepted bracing style for C#) and that is what I recommend for C#

else if (projLead.Equals("Project Leader"))
{
 if (status.Equals("Status"))
 {

• Bialy R Bert: Thank you, I always wondered if there was a preference. I will adhere to having my braces on a separate line

Answer 3

Flow

You should for sure change the flow of this method. The first query is called each time, wether it is needed or not.

You should as the simpliest change add a pure else to your big if statement where you execute the query.

For each query you are calling .OrderBy(x => x.ProjectID).Skip(projSet * 75).Take(75).ToList(). which you can do at returning the view to remove code duplication.

The else with the return will just be

else
{
 query = db.LatestStatus.Where(x => (x.Department.CompareTo(department) == 0)
 && (x.ProjectLeader.CompareTo(projLead) == 0)
 && (x.Status.CompareTo(status) == 0));
}
return View(query.OrderBy(x => x.ProjectID).Skip(projSet * 75).Take(75).ToList());

General

This method or this if..else if ..else beast is just to big. Split it into smaller methods.

Extract the outer if conditions e.g if projLead.Equals("Project Leader") into separate methods which returns an IEnumerable<whatever> like

private IEnumerable<whatever> GetProjectLeaderQuery(String department, String status)
{
 if (status.Equals("Status"))
 {
 return db.LatestStatus
 .Where(x => x.Department.CompareTo(department) == 0);
 }
 else if (!department.Equals("Department"))
 {
 return db.LatestStatus
 .Where(x => (x.Department.CompareTo(department) == 0) && (x.Status.CompareTo(status) == 0));
 } 
 return db.LatestStatus
 .Where(x => x.Status.CompareTo(status) == 0);
}

This would reduce your ProjList() methods if.. like

....
else if (projLead.Equals("Project Leader"))
{
 query = GetProjectLeaderQuery(department, status);
}
else if (department.Equals("Department"))
{
 query = GetDepartmentQuery(projLead, status);
} 
else if (department.Equals("Status"))
{
 query = GetStatusQuery(projLead, department);
}
....

Naming

Method names should be made out of verbs or verb phrases. See naming guidline.

So ProjList should be something like RetrieveProjectView.

Also, for readability purpose you should not shorten the parameter names like projLead or projSet.

Comments

Comments should be used to comment on why something is done. The code itself should tell what is done.