-5
\$\begingroup\$

I was looking for optimizing a search method for an assignment where I had to search a file for employees, I should be able to search by having any of the four filters or even none; I've imagined it like a sample space Venn with 4 events where I'd be looking for intersections (no filters should return all,...).

The problem is that I couldn't think of any other way of implementing this but using 4 nested ifs to check for matches, this of course meant I'd use those nested ifs 16 times (I can only imagine how inefficient this sounds like).

Here's the code that I wrote:

private void SearchBtn_Click(object sender, EventArgs e)
    {
        string selectedGen;
        if (radio_Female.Checked)
            selectedGen = "Female";
        else if (radio_Male.Checked)
            selectedGen = "Male";
        else
            selectedGen = "";
        FileStream stream = new FileStream("Employee.txt", FileMode.Open);
        BinaryFormatter formatter = new BinaryFormatter();
        List<Employee> listOfEmp = new List<Employee>();
        while (stream.Position < stream.Length)
        {
            Employee temp = new Employee();
            temp = (Employee)formatter.Deserialize(stream);
            {
                if (txt_name.Text == "")
                    if (txt_Id.Text == "")
                        if (combo_Department.Text == "")
                            if (selectedGen == "")
                                listOfEmp.Add(temp);

                if (txt_name.Text == "")
                    if (txt_Id.Text == "")
                        if (combo_Department.Text == "")
                            if (selectedGen == temp.gender)
                                listOfEmp.Add(temp);

                if (txt_name.Text == "")
                    if (txt_Id.Text == "")
                        if (combo_Department.Text == temp.department )
                            if (selectedGen == "")
                                listOfEmp.Add(temp);

                if (txt_name.Text == "")
                    if (txt_Id.Text == temp.id)
                        if (combo_Department.Text == "")
                            if (selectedGen == "")
                                listOfEmp.Add(temp);

                if (txt_name.Text == temp.name)
                    if (txt_Id.Text == "")
                        if (combo_Department.Text == "")
                            if (selectedGen == "")
                                listOfEmp.Add(temp);

                if (txt_name.Text == "")
                    if (txt_Id.Text == "")
                        if (combo_Department.Text == temp.department)
                            if (selectedGen == temp.gender)
                                listOfEmp.Add(temp);

                if (txt_name.Text == "")
                    if (txt_Id.Text == temp.id)
                        if (combo_Department.Text == "")
                            if (selectedGen == temp.gender)
                                listOfEmp.Add(temp);

                if (txt_name.Text == temp.name)
                    if (txt_Id.Text == "")
                        if (combo_Department.Text == "")
                            if (selectedGen == temp.gender)
                                listOfEmp.Add(temp);

                if (txt_name.Text == "")
                    if (txt_Id.Text == temp.id)
                        if (combo_Department.Text == temp.department)
                            if (selectedGen == "")
                                listOfEmp.Add(temp);

                if (txt_name.Text == "")
                    if (txt_Id.Text == temp.id)
                        if (combo_Department.Text == temp.department)
                            if (selectedGen == temp.gender)
                                listOfEmp.Add(temp);

                if (txt_name.Text == temp.name)
                    if (txt_Id.Text == temp.id)
                        if (combo_Department.Text == temp.department)
                            if (selectedGen == "")
                                listOfEmp.Add(temp);

                if (txt_name.Text == temp.name)
                    if (txt_Id.Text == "")
                        if (combo_Department.Text == temp.department)
                            if (selectedGen == "")
                                listOfEmp.Add(temp);

                if (txt_name.Text == temp.name)
                    if (txt_Id.Text == "")
                        if (combo_Department.Text == temp.department)
                            if (selectedGen == temp.gender)
                                listOfEmp.Add(temp);

                if (txt_name.Text == temp.name)
                    if (txt_Id.Text == temp.id)
                        if (combo_Department.Text == "")
                            if (selectedGen == "")
                                listOfEmp.Add(temp);

                if (txt_name.Text == temp.name)
                    if (txt_Id.Text == temp.id)
                        if (combo_Department.Text == "")
                            if (selectedGen == temp.gender)
                                listOfEmp.Add(temp);

                if (txt_name.Text == temp.name)
                    if (txt_Id.Text == temp.id)
                        if (combo_Department.Text == temp.department)
                            if (selectedGen == temp.gender)
                                listOfEmp.Add(temp);

            }
        }
        data.DataSource = listOfEmp;
        stream.Close();
    }
\$\endgroup\$

closed as unclear what you're asking by t3chb0t, hjpotter92, paparazzo, Sᴀᴍ Onᴇᴌᴀ, tinstaafl Apr 14 '18 at 2:51

Please clarify your specific problem or add additional details to highlight exactly what you need. As it's currently written, it’s hard to tell exactly what you're asking. See the How to Ask page for help clarifying this question. If this question can be reworded to fit the rules in the help center, please edit the question.

  • 1
    \$\begingroup\$ Is this working code? A few things seem off with the binary formatter, the while loop and the deserialization. What is stored in Employee.txt? I find myself having to make too many assumptions when reviewing this code \$\endgroup\$ – Nkosi Apr 13 '18 at 1:33
  • \$\begingroup\$ If you're going to use a text file anyway. .xml would be better choice. I think you'll find much better serialization options as well. Also, with that many different conditions for a particular instance of the class to be added, wouldn't if be easier to filter out the ones you don't want? \$\endgroup\$ – tinstaafl Apr 13 '18 at 3:50
  • \$\begingroup\$ May a Employee have an empty name, empty gender, empty id or empty department ? \$\endgroup\$ – Heslacher Apr 13 '18 at 4:17
  • \$\begingroup\$ 4 for nested loops to check for matches, this of course meant I'd use those nested loops 16 times - but I see only one loop there and it's the while one... where are the other for loops? Do you happend to incorrectly call the nested ifs loops? \$\endgroup\$ – t3chb0t Apr 13 '18 at 6:42
  • \$\begingroup\$ Lot of ways to listOfEmp.Add(temp); and even after you add you keep on testing. \$\endgroup\$ – paparazzo Apr 13 '18 at 9:45
4
\$\begingroup\$

There is a much simpler way of looking at this. I'm going to rely on an analogy to help paint the picture:

In order to make it to the final list, every employee must pass several trials. If they fail any of the trials, they will not make it to the list.

Think of it like a rite of passage, or an elimination race. Sometimes, it really helps to draw real-life analogies, especially when you lose your way (which, based on your code, I think you have).

You might already see that your validation rules are these trials.

The pseudocode for this is relatively simple:

temp = (Employee)formatter.Deserialize(stream);

if(FailsNameTrial(temp))
{
     //stop processing! They're not eligible!
}

if(FailsIDTrial(temp))
{
     //stop processing! They're not eligible!
}

if(FailsDepartmentTrial(temp))
{
     //stop processing! They're not eligible!
}

if(FailsGenderTrial(temp))
{
     //stop processing! They're not eligible!
}

//If we reach this point, he has passed all the trials.
listOfEmp.Add(temp);

Now let me flesh out the pseudocode.


How to stop processing.

This depends on what you're doing here.

If you were trying to validate a single employee, you'd generally put a return statement there to exit the method.

However, you are processing a list of employees. In this context, "stop processing" means "stop processing this employee and move to the next".

Instead of the return keyword, you should use the continue keyword, which instructs the iteration to skip to the next loop. It's works like a return, but only for the current iteration.

A simple example:

foreach(var employee in employees)
{
     if(!PassesTrial(employee))
         continue;

     finalList.Add(employee);
}

Only employees who did not pass their trial will make it to the final list.

Ignore the foreach, it's just a bit easier to read than your while loop but continue works the same way in both cases.

How to define a trial

This is where you made the biggest mistake in your code. I think this is (in part) due to the fact that you have two ways to pass the trial:

  • If the filter is empty, the employee passes the trial
  • If the filter is not empty:
    • The employee passes the trial if his property matches the filter.
    • The employee fails the trial if his property does not match the filter.

Note that we're trying to find failure here, not victory. A failure occurs only if two conditions are met:

  • The filter is not empty (txt_Name.Text != "")
  • The filter is different from the employee's name (txt_Name.Text != temp.Name)

Knowing this, we can fix our pseudocode:

if(FailsNameTrial(temp))

becomes

if(txt_name.Text != "" && txt_Name.Text != temp.Name)

Let me put it into words:

If the filter is not empty and the filter is different from the employee name => Failure

This is the only situation in which the trial fails. Any case that deviates from this (the filter is empty, or the emplyee name matches the filter, or both!) will cause the employee to pass the trial.


Putting it all together

We've fleshed out the unknown parts of the pseudocode. Look at the finished product:

while (stream.Position < stream.Length)
{
    Employee temp = (Employee)formatter.Deserialize(stream);

    //Name trial
    if(txt_name.Text != "" && txt_Name.Text != temp.Name)
        continue;

    //ID trial
    if(txt_ID.Text != "" && txt_ID.Text != temp.ID)
        continue;

    //Department trial
    if(combo_Department.Text != "" && combo_Department.Text != temp.Department)
        continue;

    //Gender trial
    if(selectedGen  != "" && selectedGen  != temp.Gender)
        continue;

    //The employee has passed all of the trials
    listOfEmp.Add(temp);
}

Further improvements

  • For the sake of convention, call it a "validation" instead of a "trial". Functionally, it's the same thing. I called it a "trial" for the sake of the real world analogy, but it's more idiomatic to call it a "validation" in the context of programming.
  • Instead of doing txt_Name.Text != "", you're better off with !String.IsNullOrWhiteSpace(txt_Name.Text). This checks for null, "" but also strings which contain nothing but whitespace characters (spaces, newlines, tabs, ...).
  • LINQ will make this even easier to do. However, given your struggles with the current code, I suggest you first familiarize yourself with this example before immediately trying to use LINQ (if you do it wrong, LINQ is harder to debug).
  • You could also do the opposite, where you evaluate trial success instead of failure. That is what you were doing in the original version. However, this comes with a few drawbacks:
    • You need to nest 4 if blocks in each other. Nesting isn't wrong, but excessive nesting is ugly.
    • Another fix would be to put all 4 success evaluations in a single if, but having an if with 8 different boolean evaluations is ugly, unreadable, and hard to maintain if you need to change something.
  • Notice that I removed your Employee temp = new Employee(); line. There's no point to creating a new Employee object if you're then going to immediately overwrite it with a different object.
  • You could make these "trials" into methods of their own. This is worth it if you have several trials that work exactly the same. In your case, you have 4 trials that work exactly the same way, only with different values. As a simple example:
public bool IsTrialFailed(string filter, string value)
{
    return filter != "" && filter != value;
}

Which makes your code even cleaner:

while (stream.Position < stream.Length)
{
    Employee temp = (Employee)formatter.Deserialize(stream);

    //Name trial
    if(IsTrialFailed(txt_Name.Text, temp.Name))
        continue;

    //ID trial
    if(IsTrialFailed(txt_ID.Text, temp.ID))
        continue;

    //Department trial
    if(IsTrialFailed(combo_Department.Text, temp.Department))
        continue;

    //Gender trial
    if(IsTrialFailed(selectedGen, temp.Gender))
        continue;

    //The employee has passed all of the trials
    listOfEmp.Add(temp);
}
\$\endgroup\$
  • \$\begingroup\$ I knew all along, even while writing the code, that it was as bad as it looked like. I tried to think of something similar to what you've wrote above but it wouldn't just occur to me then (I guess because of deadlines...). I'm struggling a bit studying and understanding C# stuff (such as LINQ), but anyways, thanks a lot! :) \$\endgroup\$ – Youssef Ashraf Apr 14 '18 at 1:47
  • \$\begingroup\$ @YoussefAshraf We all need a guide through the forest the first few times. Someone explained this to me too :) \$\endgroup\$ – Flater Apr 14 '18 at 8:58
2
\$\begingroup\$

BLUF: Long ramble to get to two lines of code - but the journey and understanding how to tease the information out is the important lesson here.

I am going to focus on your nested if constructs. I am not a C# coder, so I will use pseudo code for my examples.

The first point is the large amount of repetition. Your nested ifs all have the same pattern. Consecutive ifs (e.g. if A { if B ...) without corresponding else statements are functionally equivalent to if A and B ....

I see the opportunity for a helper function here - but because of some complexity in your code it is not as helpful as it seems. But, we can use this exercise to expose some of your logic.

private bool CanAdd(string Name, string ID, string Dept, string Gender, string srcName, string srcId, string srcDept, string srcGender) {
    return (Name == srcName) && (ID == srcID) && (Dept == srcDept) && (Gender == srcGender)
}

In my opinion, a function with eight parameters is not really tidying things up, but bear with me.

The nested ifs within your while loop now become a series of

If CanAdd(<a>,<b>,<c>,<d>, txt_Name.Text, txt_ID.Text, comboDepartment.Text,selectedGender)
    listOfEmp.Add(temp);

This exposes two issues that can be fixed. Firstly - your source data which you use repeatedly is coming from fractured sources (three of which are not even defined within your procedure). Secondly, you have some hidden logic in the first four parameters that you are not exploiting.

Fixing the first issue is easy. Create a custom data type (e.g. CustomDataType) that holds those four pieces of text. Assign it once before you enter the while loop - this will gain some efficiency because instead of trying to access three objects you are only accessing one. In addition, your helper function now becomes:

private bool CanAdd(string Name, string ID, string Dept, string Gender, CustomDataType Source, string srcId, string srcDept, string srcGender) {
    return (Name == Source.Name) && (ID == Source.ID) && (Dept == Source.Dept) && (Gender == Source.Gender)
}

The custom call is now down to 5 parameters.

If CanAdd(<a>,<b>,<c>,<d>, <src>)
    listOfEmp.Add(temp);

Now to address the second issue. Your calls to CanAdd will look like this:

("","","","")
("","","",temp.gender)
("","",temp.department,"")
("",temp.id,"","")
(temp.name,"","","")
("","",temp.department, temp.gender)
("", temp.id, "", temp.gender)
(temp.name, "","",temp.gender)
("", temp.id, temp.department, "")
("", temp.id, temp.department, temp.gender)
(temp.name, temp.id, temp.department, "")
(temp.name, "", temp.department, "")
(temp.name, "", temp.department, temp.gender)
(temp.name, temp.id, "", "")
(temp.name, temp.id, "", temp.gender)
(temp.name, temp.id, temp.department, temp.gender)

And as I typed those out, I thought "am I being pranked"? But, to progress.

The logic can be simplified. Your filter is basically a series of if <a> or "".

I am going to use another helper function (and this one is useful)

bool IsFilterMatch(string filterValue, string propertyValue) {
    return (filterValue == propertyValue) || (filterValue == "")
}

Now we can clean up your logic. All those nested ifs become this next two lines.

bool found = IsFilterMatch(txt_name.Text,temp.name) && IsFilterMatch(txt_id.Text, temp.id) && IsFilterMatch(combo_Department.Text, temp.department) && IsFilterMatch(selectedGender, temp.gender)
if found listOfEmp.Add(temp);

I know I have rambled to get to the final point - but the journey here is important - when you write complex code you must be prepared to tease it out!

In the end, all that is required is the IsFilterMatch helper function and the two lines of code to address what you have done in the nested ifs. I have discarded the CanAdd function but it was useful in getting to the answer. My two key points:

  • If you are working with a consistent set of data, consider setting up a data structure or object that makes your life easier.

  • When faced with a complex logic situation, work through it and try to understand all the permutations - this can expose the shortcuts.

\$\endgroup\$
  • \$\begingroup\$ Given the asymmetric nature of theCheck() method (source1 being empty makes the check pass, but source2 being empty doesn't necessarily make the check pass), I would advocate using more distinct names than source1 and source2 (since they work slightly differently). E.g. filterValue and propertyValue. Check() itself could also do with a more clear name, e.g. IsFilterMatch() or IsCheckPassed() \$\endgroup\$ – Flater Apr 13 '18 at 11:11
  • \$\begingroup\$ return (source1 = source2) || (source1 = "") also, that should be a == in both cases, not a = \$\endgroup\$ – Flater Apr 13 '18 at 11:12
  • \$\begingroup\$ @Flater: Yes, I was running out of time, so the deeper thought on the naming relating to Check was a bit thin. \$\endgroup\$ – AJD Apr 13 '18 at 21:02

Not the answer you're looking for? Browse other questions tagged or ask your own question.