Selecting multiple filters based on the true/false conditions

Question

I wrote a method to select the filters (two or more) based on the true/false conditions. Here is the method I used for selecting the multiple filters:

    public T SetPropertyTypes<T>(bool residential, bool commercial) where T : IPage, new()
{

    // Residential Property Type Check logic
    if (residential && (ElementIsNotActive(() => FindElement(By.CssSelector(propertyTypeResidentialCss))).Invoke(Driver)))
        ClickButton(() => FindElement(By.CssSelector(propertyTypeResidentialCss)), "Residential");
    else if (ElementIsActive(() => FindElement(By.CssSelector(propertyTypeResidentialCss))).Invoke(Driver))
        ClickButton(() => FindElement(By.CssSelector(propertyTypeResidentialCss)), "Residential");

    // Commercial Property Type Check logic
    if (commercial && (ElementIsNotActive(() => FindElement(By.CssSelector(propertyTypeCommercialCss))).Invoke(Driver)))
        ClickButton(() => FindElement(By.CssSelector(propertyTypeCommercialCss)), "Commercial");
    else if (ElementIsActive(() => FindElement(By.CssSelector(propertyTypeCommercialCss))).Invoke(Driver))
        ClickButton(() => FindElement(By.CssSelector(propertyTypeCommercialCss)), "Commercial");
}

Later I found that the code is redundant and it could be better if I make it simpler and non-ambiguous. I'm new to C# and am not aware of many features. Is there any way to refactor this using any features of C#? I tried checking the variables using if-else-if but that's not working as intended.

score 2 · Accepted Answer · 2017-09-20 02:07:23Z

2

Yes your code has a lot of redundancy. It can be simplified down to this:

if (residential)
{
    ClickButton(() => FindElement(By.CssSelector(propertyTypeResidentialCss)), "Residential");
}
if (commercial)
{
    ClickButton(() => FindElement(By.CssSelector(propertyTypeCommercialCss)), "Commercial");
}

edited Sep 20, 2017 at 2:07

answered Sep 19, 2017 at 20:35

user33306

\$\begingroup\$ Using If-Else won't work if I want to click both the buttons, (Residential and Commercial).Say the calling will be like, ii) SetPropertyTypes<>(true, true) -> Here it have to select/click both Resident and commercial i) SetPropertyTypes<>(false, false) -> Here it should not select/click Resident and not commercial \$\endgroup\$

irs102info
– irs102info

2017-09-20 01:10:11 +00:00
Commented Sep 20, 2017 at 1:10
\$\begingroup\$ @irs102info - Taking out the else should work. I didn't think you'd expect both buttons to be pressed together. That seems kind of odd. \$\endgroup\$

user33306
– user33306

2017-09-20 02:12:54 +00:00
Commented Sep 20, 2017 at 2:12

Add a comment |

BenKoshy · Accepted Answer · 2017-09-21 14:31:23Z

Perhaps reconsider the if statements

I cannot comment too much on the logic because I don’t really understand what’s going on: But it definitely does like there is something funny with the if—else statements. If the IsActiveCommercial() method is the exact opposite of the IsInactiveCommercial() method then you could definitely simply the logic of the if statements dramatically.

Improve code readability

It’s very hard to understand, so I tried refactoring and simplifying it. Hopefully this will make a little more sense – or at the very least give you some ideas:

    public T SetPropertyTypes<T>(bool residential, bool commercial) where T : IPage, new()
    {
        // Residential Property Type Check logic
        if (residential && IsInactiveResidential())
        {
            ResidentialHandler();
        }
        else if (ActiveResidential())
        {
            ResidentialHandler();
        }

        // Commercial Property Type Check logic
        if (commercial && IsInactiveCommercial())
        {
            CommercialHandler();
        }
        else if (IsActiveCommercial())
        {
            CommercialHandler();
        }
    }

    private static object IsActiveCommercial()
    {
        return ElementIsActive(() => FindElementCommercial()).Invoke(Driver);
    }

    private static object IsInactiveCommercial()
    {
        return (ElementIsNotActive(() => FindElementCommercial()).Invoke(Driver));
    }

    private static object ActiveResidential()
    {
        return ElementIsActive(() => FindElementResidential()).Invoke(Driver);
    }

    private static object IsInactiveResidential()
    {
        return (ElementIsNotActive(() => FindElementResidential()).Invoke(Driver));
    }

    private static void ResidentialHandler()
    {
        ClickButton(() => FindElementResidential(), "Residential");
    }

    private static void CommercialHandler()
    {
        ClickButton(() => FindElementCommercial(), "Commercial");
    }

    private static object FindElementCommercial()
    {
        return FindElement(By.CssSelector(propertyTypeCommercialCss));
    }

    private static object FindElementResidential()
    {
        return FindElement(By.CssSelector(propertyTypeResidentialCss));
    }

Redundancy

Looks like there is a little bit of redundancy but unless performance is an issue – I would leave it as it is - you're not going to gain anything by introducing a temporary variable (unless it improve readability) but in that case you might as well call the method again.

Stack Exchange Network

Selecting multiple filters based on the true/false conditions

2 Answers 2

Perhaps reconsider the if statements

Improve code readability

Redundancy

You must log in to answer this question.

Hot Network Questions

Selecting multiple filters based on the true/false conditions

2 Answers 2

Perhaps reconsider the if statements

Improve code readability

Redundancy

You must log in to answer this question.

Related

Hot Network Questions