patterncsharpMinor
Counting the most frequently appearing character in a string (i.e. the statistical mode)
Viewed 0 times
thecountingmodecharacterappearingfrequentlystatisticalstringmost
Problem
This is my sample code to find maximum occurrence of characters in string in C#.
How can I make it better?
How can I make it better?
public static int GetMaximumOccurence(string str)
{
int _maxcount = 0;
foreach (char c in str)
{
int ix = str.IndexOf(c);
int count = 0;
while (ix!=-1)
{
StringBuilder _s = new StringBuilder();
_s.Append(str);
_s[ix] = ' ';
str = _s.ToString();
count++;
ix = str.IndexOf(c);
}
if (count > _maxcount)
{
_maxcount = count;
}
}
return _maxcount;
}Solution
I'm worried this is turning into Code Golf, so I'll preface my alternative with some actual review.
There is a pretty big bug in your code, as the following will not terminate:
Can you see why?
The basic structure of your code is good. That is, the part that effectively goes
Now how to we complete that TODO? Using
Now,
If you prefer, this can be written as a
However, this solution is not terribly efficient. For each character in the string, we're iterating over the string again with our calls to
In fact, we've basically just written this:
@BlueTrin's answer shows how you can trade space to reduce the time complexity to \$O(n)\$, so I won't go over that here.
All that said, there is an elegant way to express this using LINQ:
There is a pretty big bug in your code, as the following will not terminate:
Console.WriteLine(GetMaximumOccurence(" "));Can you see why?
The basic structure of your code is good. That is, the part that effectively goes
var maxCount = 0;
foreach (var c in input)
{
// TODO: Count occurrences of c in input.
maxCount = Math.Max(count, maxCount);
}
return maxCount;Now how to we complete that TODO? Using
IndexOf like you do seems like a good idea.var maxCount = 0;
foreach (var c in input)
{
var count = 0;
var i = input.IndexOf(c);
while (i != -1)
{
// TODO: Keep counting occurrences of c.
}
maxCount = Math.Max(count, maxCount);
}
return maxCount;Now,
string has an overload of IndexOf that takes a starting position. But where do we start searching? We start searching one position after our last found occurrence. var maxCount = 0;
foreach (var c in input)
{
var count = 0;
var i = input.IndexOf(c);
while (i != -1)
{
count++;
i = input.IndexOf(c, i + 1);
}
maxCount = Math.Max(count, maxCount);
}
return maxCount;If you prefer, this can be written as a
for-loop.var maxCount = 0;
foreach (var c in input)
{
var count = 0;
for (var i = input.IndexOf(c); i != -1; i = input.IndexOf(c, i + 1))
{
count++;
}
maxCount = Math.Max(count, maxCount);
}
return maxCount;However, this solution is not terribly efficient. For each character in the string, we're iterating over the string again with our calls to
IndexOf, so our time complexity is \$O(n^2)\$.In fact, we've basically just written this:
var maxCount = 0;
foreach (var c in input)
{
var count = 0;
foreach (var d in input)
{
if (c == d)
{
count++;
}
}
maxCount = Math.Max(count, maxCount);
}
return maxCount;@BlueTrin's answer shows how you can trade space to reduce the time complexity to \$O(n)\$, so I won't go over that here.
All that said, there is an elegant way to express this using LINQ:
return input.Length == 0
? 0
: input.GroupBy(c => c).Max(group => group.Count());Code Snippets
Console.WriteLine(GetMaximumOccurence(" "));var maxCount = 0;
foreach (var c in input)
{
// TODO: Count occurrences of c in input.
maxCount = Math.Max(count, maxCount);
}
return maxCount;var maxCount = 0;
foreach (var c in input)
{
var count = 0;
var i = input.IndexOf(c);
while (i != -1)
{
// TODO: Keep counting occurrences of c.
}
maxCount = Math.Max(count, maxCount);
}
return maxCount;var maxCount = 0;
foreach (var c in input)
{
var count = 0;
var i = input.IndexOf(c);
while (i != -1)
{
count++;
i = input.IndexOf(c, i + 1);
}
maxCount = Math.Max(count, maxCount);
}
return maxCount;var maxCount = 0;
foreach (var c in input)
{
var count = 0;
for (var i = input.IndexOf(c); i != -1; i = input.IndexOf(c, i + 1))
{
count++;
}
maxCount = Math.Max(count, maxCount);
}
return maxCount;Context
StackExchange Code Review Q#58424, answer score: 5
Revisions (0)
No revisions yet.