3
\$\begingroup\$

I have this C# method which get the character and its count which has highest occurrence

    public KeyValuePair<char, int> CountCharMax_Dictionary_LINQ(string s)
    {
        char[] chars = s.ToCharArray();

        var result = chars.GroupBy(x => x)
            .OrderByDescending(x => x.Count())
            .ToDictionary(x => x.Key, x => x.Count())
            .FirstOrDefault();

        return result;
    }

It works but is there any better or efficient way to get the same result? Is there any difference if I return it as Tuple vs KeyValuePair?

Test data:

  • Input: 122. Expected Output: 2,1
  • Input: 122111. Expected Output: 1,4
\$\endgroup\$
  • 2
    \$\begingroup\$ The expected output for input "122" should be "2, 2", not "2, 1". \$\endgroup\$ – Jesse C. Slicer Jun 27 at 23:37
  • \$\begingroup\$ Ops yeah. Indeed. \$\endgroup\$ – Steve Ngai Jun 28 at 0:09
7
\$\begingroup\$

As per this answer, it would be wiser to use a tuple in this case. I use lowercase "t"uple as I highly recommend the built-in language support for tuples (char, int) over the explicit Tuple<T1, T2> declaration, etc. Few more points:

  • What should happen in a tie? Which gets returned? If any is acceptable, it's fine as-is. To be more of a determinate function, secondarily order by the character itself.
  • Don't need to convert it to a character array as a string already is a character array.
  • Re-order the OrderByDescending and the Select so that Count only has to be called once.
  • It can be made static since it doesn't access class instance data.
  • It can be made an extension method if in a static class.
  • It can be made into an expression-bodied method.
  • Maybe consider a better name; one that confers what it does rather than how it does it.

All that said, here's my take:

    public static (char, int) MaxCharacterFrequency(this string s) =>
        s.GroupBy(x => x)
         .Select(x => (x.Key, x.Count()))
         .OrderByDescending(x => x.Item2)
         .ThenBy(x => x.Item1)
         .FirstOrDefault();
| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ It can be made static since it doesn't access class instance data. If the method doesn't access to class instance data, with static, it is safer and use less memory? \$\endgroup\$ – Steve Ngai Jun 28 at 0:38
  • \$\begingroup\$ I would say that objectively it does use less memory for two reasons: 1. The class instance is not required to be created and 2. The implicit this parameter is not passed to the method. It's not anything substantial, but it's more of an idiomatic correctness to do so. \$\endgroup\$ – Jesse C. Slicer Jun 28 at 15:52
2
\$\begingroup\$

There is not much to add to Slicers answer other than underscore in names is rather un-C#-ish, and you should avoid single character names (s) as well.

You ask for more efficient ways for the algorithm. One could be the following:

public (char value, int count) MaxByFrequency(string input)
{
  var chars = input.ToCharArray();
  Array.Sort(chars);
  char value = ' ';
  int max = 0;

  for (int i = 0; i < chars.Length; )
  {
    int j = i;
    char ch = chars[j];
    while (j < chars.Length && chars[j] == ch) j++;

    if (j - i > max)
    {
      max = j - i;
      value = ch;
    }

    i = j;
  }

  return (value, max);
}

It is efficient time wise (in my measures more than twice as fast) but of cause is rather "expensive" in memory usage because of the temporary array of chars.


Another linq approach using Aggregate could be:

public (char value, int count) MaxByFrequency(string input)
{
  (char value, int count) seed = (' ', 0);
  return input
    .GroupBy(c => c)
    .Aggregate(seed, (acc, gr) =>
    {
      int cnt = gr.Count();
      if (cnt > acc.count)
        return (gr.Key, cnt);
      return acc;
    });
}
| improve this answer | |
\$\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.