0
\$\begingroup\$

I am trying to implement a search routine for an application written in C#. The requirements are as follows:

The user can select one of the following options for search:

  1. Property Search(Exact): the text entered in the search box has to match exactly with the results, e.g. searching for PL should only show the results whose properties contain PL.
  2. Property Search(Contains): the text entered in the search box can be part of the results, e.g. searching for PL should show all the results whose properties contain PL, e.g. PLApp, Planning, playing, etc.

I have implemented it in my code by passing a boolean flag to my method, and using if..else to perform the searches accordingly.

    public override bool PropertySearch(string value, bool searchPartial = false)
    {
        if(searchPartial)
        {
            if (dataValue.IndexOf(value, StringComparison.OrdinalIgnoreCase) >= 0)
                return true;
        } else
        {
            if (dataValue.Equals(value, StringComparison.OrdinalIgnoreCase))
                return true;
        }
        return false;
    }

This was my first implementation, and I have a feeling there might be a better and concise way to achieve this. Any thoughts?

\$\endgroup\$
  • \$\begingroup\$ (Welcome to finally posting on CR!) Your option descriptions both read the results whose properties contain PL - maybe we need a specificationreview.stackexchange.com. \$\endgroup\$ – greybeard Feb 23 '18 at 8:01
  • \$\begingroup\$ Sorry I don't understand, what do you mean? Exact should return all results containing exact search text, Partial should return all results containing search text. So Partial is a super set of Exact. Hope that helps. \$\endgroup\$ – Akshay Khot Feb 23 '18 at 23:57
  • \$\begingroup\$ The problem is that contains is interpreted loosely enough to serve as a description of both cases that need to be told from each other. *Exact (searchPartial = false) should return true where, ignoring case, dataValue equals value. \$\endgroup\$ – greybeard Feb 24 '18 at 0:09
  • \$\begingroup\$ Exactly! Contains resulted in a lot of false positives so I was tasked with giving the user option to choose from a partial search and an exact search. Hence the searchPartial flag. \$\endgroup\$ – Akshay Khot Feb 24 '18 at 0:20
3
\$\begingroup\$

The flow control seems excessively verbose. This would do the same:

public override bool PropertySearch(string value, bool searchPartial = false)
{
    return searchPartial ? (dataValue.IndexOf(value, StringComparison.OrdinalIgnoreCase) >= 0)
                         :  dataValue.Equals(value, StringComparison.OrdinalIgnoreCase);
}

I would also question your decision to lump these two operations together into the same method. The code would likely be clearer if you wrote two separate methods. But you haven't shown the superclass or rest of this class, so I can't say for sure.

\$\endgroup\$
  • \$\begingroup\$ Thanks for the answer, this is much clearer than mine. I am a novice, and just inherited a large codebase, so still trying to figure out the structure. Is this the best way to solve such type of search problems? \$\endgroup\$ – Akshay Khot Feb 23 '18 at 0:33
  • \$\begingroup\$ FYI, the existing code already has methods which perform exact search using Equals, and I have been tasked with enhancing the search so it can perform partial search, without modifying the exact search, if that makes sense. \$\endgroup\$ – Akshay Khot Feb 23 '18 at 0:41
  • \$\begingroup\$ Furthermore, there are more than 20 variations of PropertySearch method, each performing a search on different dataValue. \$\endgroup\$ – Akshay Khot Feb 23 '18 at 0:45
  • 1
    \$\begingroup\$ Why don't you amend the question to provide the complete context: what the class does, and how you modified the code. \$\endgroup\$ – 200_success Feb 23 '18 at 0:47
  • 2
    \$\begingroup\$ What I'm saying is, 20 variations of PropertySearch(…) is a code smell, and perhaps you should have the whole class reviewed. \$\endgroup\$ – 200_success Feb 23 '18 at 1:06
1
\$\begingroup\$

I don't like this method using two different searche techniques. Sticking to only IndexOf is in my opinion cleaner so I'd refactor this method as:

public static bool PropertySearch(string value, bool partialMatch = false)
{
    var indexOf = dataValue.IndexOf(value, StringComparison.OrdinalIgnoreCase);
    var contains = indexOf >= 0;
    var sameLength = indexOf == 0 && dataValue.Length == value.Length;
    var exactMatch = contains && sameLength;
    return 
        partialMatch
            ? contains
            : exactMatch;
}

Although a Regex one-liner is even nicer:

public bool PropertySearch(string value, bool partialMatch = false)
{
    return Regex.IsMatch(dataValue, partialMatch ? value : $"^{value}$", RegexOptions.IgnoreCase);
}
\$\endgroup\$
0
\$\begingroup\$

I think, pass a parameter and change code flow is not a good idea. is it possible do it like this?

PartialPropertySearch(...)
PropertyEqualsSearch(...)

of-course if it's already defined by the framework and you can't change it. then you can do like this style:

public override bool PropertySearch(string value, bool searchPartial = false)
{
  if(searchPartial)
      return (dataValue.IndexOf(value, StringComparison.OrdinalIgnoreCase) >= 0)
  else
      return (dataValue.Equals(value, StringComparison.OrdinalIgnoreCase))
}

By the way, I don't like the style return searchPartial ? true clause... : false clause ...

\$\endgroup\$
  • \$\begingroup\$ (Welcome to CR!) By the way, I don't like the style … pretty much apparent. Can you point out how your second suggestion differs essentially from the code presented in the question and how it is an improvement? \$\endgroup\$ – greybeard Feb 23 '18 at 7:55
  • \$\begingroup\$ I just don't like The conditional operator... but it's ok. the benifit to use if/else as it will more clear for code scan by eye. Because , for me, it read a code in "conditional operater" is slower than if/else statement. \$\endgroup\$ – Ariso Y2009 Feb 23 '18 at 23:38
  • \$\begingroup\$ There is no conditional operator in the code from the question, it is not in your 2nd suggestion. Do you see said suggestion notably different from the code to review? Is it better, in case it is: how so? \$\endgroup\$ – greybeard Feb 23 '18 at 23:45

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.