0
\$\begingroup\$

After some advice, which method do you think is better to use when searching a dictionary for the matching value(s)?

Method One

Console.WriteLine("ENTER A PERSON/NUMBER TO SEARCH FOR");
string input = Console.ReadLine().ToUpper();
bool found = false;

if (myPhoneBook.ContainsKey(input)) // used with a persons name and returns number
{
    Console.WriteLine(myPhoneBook[input]);
    found = true;
}
if (!(myPhoneBook.ContainsKey(input))) //searches the number and returns the name
{
    var searchingfornumbers = from num in myPhoneBook
                              where num.Value == input
                              select num.Key;

    foreach(var person in searchingfornumbers)
    {
        Console.WriteLine(person);
        found = true;
    }
}
if(found == false)
{
    Console.WriteLine("NO RECORD FOUND");
}

Method Two

Console.WriteLine("ENTER A PERSON/NUMBER TO SEARCH FOR");
string input = Console.ReadLine();
bool found = false;
var query0 = from person in myPhonebook //searches for the number, returns name
             where person.Value == input
             select person.Key;

foreach(var person in query0)
{
    Console.WriteLine(person);
    found = true;
}

var query1 = from person in myPhonebook //searches for the name, returns numbers
             where person.Key == input
             select person.Value;

foreach (var person in query1)
{
    Console.WriteLine(person);
    found = true;
}
if(found == false)
{
    Console.WriteLine("NO RECORD FOUND");
}
Console.ReadKey();
\$\endgroup\$
  • \$\begingroup\$ Welcome to Code Review, I removed a bit of indentation from copy&pasting, otherwise looks good; hope you get some good reviews! \$\endgroup\$ – ferada Sep 15 '16 at 21:21
2
\$\begingroup\$

I don't like either of them because they look like multiple numbers could be assigned to multiple persons which is obviously wrong.

You can get only one result for a search because you are not searching with wildcards so consider this. A person can have multiple numbers so for a person you can have a single hit. If you search for an exact number you'll also get a single hit. This can be made clear by naming the method appropriately: FindExact. Later you can add another one that can search with wildcards (patterns).

class Phonebook
{
    private readonly Dictionary<string, List<string>> _entries = 
        new Dictionary<string, List<string>>();

    public void Add(string person, string number)
    {
        List<string> numbers;
        if (_entries.TryGetValue(person, out numbers))
        {
            numbers.Add(number);
        }
        else
        {
            _entries[person] = new List<string> { number };
        }
    }

    public KeyValuePair<string, List<string>> FindExact(string personOrNumber)
    {
        List<string> numbers;
        return
            _entries.TryGetValue(personOrNumber, out numbers)
            ? new KeyValuePair<string, List<string>>(personOrNumber, numbers)
            : _entries.FirstOrDefault(x => x.Value.Contains(personOrNumber));
    }
}

Currently searching for a persons would be faster because it is a key in the dictionary. You could add another dictionary that maps numbers to persons and make the number search as fast as person search by looking at the key instead of searching all values:

private readonly Dictionary<NUMBER, PERSON> _numberPerson...

In your second solution you don't need a dictionary at all because use don't use it like it should be used. You loop twice over it to find either value. In this case you can also use a list.

If you want to use a dictionary then use it appropriately otherwise it doesn't make any sense.

Use two dictionaries to make the exact search even faster but don't loop twice over it.

You can also replace both queries with a single one:

var query = 
    from x in myPhonebook
    where x.Key == input || x.Value == input
    select x;

Even better like so:

var result = myPhonebook.FirstOrDefault(x => x.Key == input || x.Value == input);
\$\endgroup\$
  • 1
    \$\begingroup\$ This, because TryGetValue is the right way to search dictionaries. I came across a new project recently that iterated through all the keys multiple times (and converted each to uppercase to do a case sensitive string comparison), I cannot express how angry that made me :) \$\endgroup\$ – 404 Sep 16 '16 at 13:35
  • \$\begingroup\$ Hey guys, thanks for your insight, I haven't had much to do with dictionaries before, so that's for your comments, I'll will take them on board \$\endgroup\$ – user1234433222 Sep 16 '16 at 14:34
1
\$\begingroup\$

I would prefer the second method. It prints out more possibilities, making it more likely that what the user is looking for is being printed out. Further down I make a point about adding the results to a list instead of printing them out immediately. Using such logic would allow you to sort the list so that relevant results could be displayed first with less likely, but possible matches further down.

There are also some style points I would like to make:

  1. When using a boolean variable in an if statement you never need to do if(booleanVariable == false) you can simply do if(!booleanVariable)
  2. Its not entirely clear that checking if the dictionary has the available key means that you are searching by name (as opposed to number). I would extract that into a method called searchByName() and then use if(searchByName()). This is a lot more human readable and can allow you to get rid of the comment explaining what the code does. (In general if you feel like you need a comment to say what the code is doing, the code could use a bit of change to make it more self explanatory.)
  3. Instead of using multiple but separate if's, use if-else.

    if (searchByName()) { //search by name functionality } else { //search by number functionality }

Alternatively:

if (searchByName()) {
    //search by name functionality
} else if(searchByNumber()) {
    //search by number functionality
}

This makes it more clear what it being checked for in each if statement and what is expected to happen inside that block of code.

  1. Don't use flags such as found. A much better option would be to add all the results into a List instead of printing them out right then and there. You can then print what is in the list if it has results or print that no results were found if the list is empty. As mentioned earlier this also allows you to present the results in a more meaningful way to the user either by sorting by relevancy or allowing them to filter out so they only see names or only see numbers etc.
\$\endgroup\$
  • \$\begingroup\$ Thanks for your comment mate, I prefer the 2nd too. I definitely like the idea of adding found results to a list and then searching the list which will give me easier list options rather than writing a query for it. \$\endgroup\$ – user1234433222 Sep 15 '16 at 21:27
  • \$\begingroup\$ i could also use a Yield like method too I believe \$\endgroup\$ – user1234433222 Sep 15 '16 at 22:38

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.