1
\$\begingroup\$

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.

\$\endgroup\$
2
\$\begingroup\$

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");
}
\$\endgroup\$
  • \$\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 Sep 20 '17 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\$ – tinstaafl Sep 20 '17 at 2:12
1
\$\begingroup\$

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.
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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