3
\$\begingroup\$

This is working, but I want to know if is this a place where IEnumerable can be used?

public static int LongerThanStingCompair(string s, int n)
    {

        int counter = 0;
        // splits based on nextLine break
        var sCount = s.Split('\n');
        // Cast to list
        sCount.ToList();
        foreach (var item in sCount)
        {
            //checks to see if words are a certain charLength
            if (item.Length > n)
            {
                counter++;
            }
        }

        return counter;
    }

Test class gives it the input like

Lion, Ape, Tiger, Zebra and 4

So it would return 2 since there are two words with more than 4 letters

\$\endgroup\$
  • \$\begingroup\$ Is there a performance problem? Where is the bottleneck? \$\endgroup\$ – spyr03 Sep 9 '15 at 14:50
  • \$\begingroup\$ Welcome on Code Review! Your question lacks a little clarity! Could you please provide a little context so we can review your code better? Also, do you not want to know if IEnumerable can be used here, or is this a typo? \$\endgroup\$ – IEatBagels Sep 9 '15 at 14:54
  • \$\begingroup\$ just a question of usage, could something else be done ? This seems solid in my thought process but C# is new to me and I think I could code it better somehow \$\endgroup\$ – RiePwe Sep 9 '15 at 14:58
  • \$\begingroup\$ You should edit your question in order to make it clearer :) If you want help on how to write a good question, 'cause good questions draw more reviewers, you can head to the help center : codereview.stackexchange.com/help/how-to-ask \$\endgroup\$ – IEatBagels Sep 9 '15 at 15:02
  • \$\begingroup\$ Good job, I think your question is much better now :) \$\endgroup\$ – IEatBagels Sep 9 '15 at 15:11
6
\$\begingroup\$

Signature:

public static int LongerThanStingCompair(string s, int n)

Compair is misspelled, as is Sting - they should be Compare and String, respectively. Although, a better name might be CountLinesLongerThan.

s and n aren't great names for parameters.

On to your code:

// splits based on nextLine break
var sCount = s.Split('\n');

The comment is unnecessary and slightly wrong. The code splits on all new line characters.

// Cast to list
sCount.ToList();

This comment is also incorrect - that doesn't simply cast it creates a new list from an array. It's unnecessary because you only need to enumerate the lines - you don't need the functionality of the list.

Here's a Linq based alternate for you:

public static int CountLinesLongerThan(string source, int characterLimit)
{
    if (source == null) 
    {
        throw new ArgumentNullException("source");
    }
    return source.Split('\n')
        .Count(line => line.Length > characterLimit);
}
\$\endgroup\$
  • \$\begingroup\$ @RiePwe - you're very welcome. Apologies if it sounded a bit harsh, not my intention at all! \$\endgroup\$ – RobH Sep 9 '15 at 15:24
2
\$\begingroup\$

Since this is tagged with performance:

var sCount = s.Split('\n');

If you want performance in .net you want to avoid allocations. If the string is millions of lines this call will allocate an array with millions of strings. Allocating is relatively cheap but cleaning up is not. The garbage collector will have to work hard to clean up after each call to your method.

sCount is a poor name should be rows.

sCount.ToList();

This is another disaster for many reasons, it copies the array of strings to a list that is then not used. You definitely don't want to allocate a list here for performance reasons and you don't want list for semantic reasons.

You want a List<T> when you are adding and or removing items from it. In release build this will be optimized away so it will have no cost but it is very smelly.

For performance you want something like this:

private static int CountRowsLongerThan(string text, int min)
{
    var counter = 0;
    var index = 0;
    while (index < text.Length)
    {
        if (text[index] == '\n') // using '\n' although Environment.NewLine is probably what you want.
        {
            index++;
        }
        var rowLength = GetRowLength(text, index);
        if (rowLength > min)
        {
            counter++;
        }

        index += rowLength;
    }

    return counter;
}

private static int GetRowLength(string text, int index)
{
    var result = 0;
    while (index < text.Length && text[index] != '\n')  // again using '\n' although Environment.NewLine is probably what you want.
    {
        index++;
        result++;
    }
    return result;
}

As you can see the optimized code reads very poorly so only do this kind of optimization if it solves a real problem. The profiler will tell you if it is a problem although profiling allocations is nontrivial since the big cost comes when cleaning up.

This kind of code is nontrivial and requires unit tests. Using LINQ makes it trivial enough to not need to be tested.

General remarks You probably want to split on Environment.NewLine Like so:

var count = yourText.Split(new[] { Environment.NewLine }, StringSplitOptions.RemoveEmptyEntries)
                    .Count(x => x.Length > 4);

Using LINQ I would do it inline and not wrap it in a method since it is a trivial call.

\$\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.