Skip to main content
Code Review

Return to Answer

added 165 characters in body
Source Link
t3chb0t
t3chb0t
  • 44.7k
  • 9
  • 85
  • 191

Ideally this method could look like this:

private static string concatenateContent(IEnumerable<KeyValuePair<string, int>> wordCount)
{
    const string openParenthese = "{";
    const string closeParenthese = "}";
    const string colon = ": ";
    const string comma = ", ";

    var result = new StringBuilder();
    result.Append(openParenthese);

    foreach (var x in wordCount.Select((item, index) => new { item, index }))
    {
        result
            .Append(x.index > 0 ? colon : string.Empty)
            .Append(x.item.Key)
            .Append(colon)
            .Append(x.item.Value);
    }

    result.Append(closeParenthese);
    
    return result.ToString();
}

Ideally this method could look like this:

private static string concatenateContent(IEnumerable<KeyValuePair<string, int>> wordCount)
{
    const string openParenthese = "{";
    const string closeParenthese = "}";
    const string colon = ": ";
    const string comma = ", ";

    var result = new StringBuilder();
    result.Append(openParenthese);

    foreach (var x in wordCount.Select((item, index) => new { item, index }))
    {
        result
            .Append(x.index > 0 ? colon : string.Empty)
            .Append(x.item.Key)
            .Append(colon)
            .Append(x.item.Value);
    }

    result.Append(closeParenthese);
    
    return result.ToString();
}
added 165 characters in body
Source Link
t3chb0t
t3chb0t
  • 44.7k
  • 9
  • 85
  • 191

Using the StringBuilder current = new StringBuilder(); inside a loop does not make much sense and you use the + concatenation anyway item.Key + colon + item.Value. You might as well concatenate the string in the normal way because the comptiler will optimize it. The StringBuilder will become useful when you define it outside the loop and then build the string.

You don't need the index++; because you can just check the words.Count property.

Using the StringBuilder current = new StringBuilder(); inside a loop does not make much sense. You might as well concatenate the string in the normal way because the comptiler will optimize it. The StringBuilder will become useful when you define it outside the loop and then build the string.

Using the StringBuilder current = new StringBuilder(); inside a loop does not make much sense and you use the + concatenation anyway item.Key + colon + item.Value. You might as well concatenate the string in the normal way because the comptiler will optimize it. The StringBuilder will become useful when you define it outside the loop and then build the string.

You don't need the index++; because you can just check the words.Count property.

Source Link
t3chb0t
t3chb0t
  • 44.7k
  • 9
  • 85
  • 191

There is not much LINQ in your code and this is a really long solution for such a simple task but let's try to change it.

You want to split a sentence like this one:

var sentence = " code review connects the world! share the code";

so why not use regex for this and split on every occurance of ,?!.. You then trim each word and filter the empty results out. Then you group each word igrnoring its case, sort the groups by count in a descending order and create strings.

var words = 
    Regex.Split(sentence, "[ ,?!.]")
    .Select(x => x.Trim())
    .Where(x => !string.IsNullOrEmpty(x))
    .GroupBy(x => x, StringComparer.OrdinalIgnoreCase)
    .OrderByDescending(g => g.Count());

var summary = $"{{{string.Join(", ", words.Select(g => $"{g.Key}: {g.Count()}"))}}}";

result

{code: 2, the: 2, review: 1, connects: 1, world: 1, share: 1}

Review

private static void concatenateContent(IList<string> words, IEnumerable<KeyValuePair<string, int>> wordCount)
{
    const string openParenthese = "{";
    const string closeParenthese = "}";
    const string colon = ": ";
    const string comma = ", ";

    words.Add(openParenthese);

    int length = wordCount.Count();
    int index = 0;

    foreach (var item in wordCount)
    {
        StringBuilder current = new StringBuilder();

        current.Append(item.Key + colon + item.Value);

        if (index < length - 1)
        {
            current.Append(comma);
        }

        words.Add(current.ToString());

        index++;
    }

    words.Add(closeParenthese);
}

You should not return a result via a parameter that is not a ref or out one because I'd be really shocked if you modified my list without giving me any hint that it will happen.

The IList<string> words should be a return value not a parameter.

Using the StringBuilder current = new StringBuilder(); inside a loop does not make much sense. You might as well concatenate the string in the normal way because the comptiler will optimize it. The StringBuilder will become useful when you define it outside the loop and then build the string.