Return the longest word (or words) in a string

Question

I'm coding for a function that checks the longest word (or words) in a string:

• If multiple words are of the same size, those words have to be displayed.
• If a single word is inputted, that word has to be displayed.
• Words should consist only of alphabets.

The following code has been tested and displays the correct input for the above conditions:

    //this method returns the longest word (or words) of a single string
    static void longestWord(string s)
    {
        int j = 0, k = 0, l = 0, tempLength = 0, maxLength = 0;
        List<int> indexList = new List<int>();
        char[] words = s.ToCharArray();
        StringBuilder longestWord = new StringBuilder();

        //a for loop to ignore spaces at the beginning of the string
        for (int i = 0; i <= s.Length - 1; i++)
        {
            if (((words[i] >= 'A' && words[i] <= 'Z') || (words[i] >= 'a' && words[i] <= 'z')))
            {
                j = i;
                l = i;
                break;
            }
        }     

        //a for loop to get the maximum length of the longest word
        for (; j <= s.Length - 1; j++)
        {
            if (((words[j] >= 'A' && words[j] <= 'Z') || (words[j] >= 'a' && words[j] <= 'z')))
            {
                tempLength++;

                if (j == (s.Length - 1))
                {
                    if (tempLength >= maxLength)
                    {
                        maxLength = tempLength;  //this ensures the maximum length of a single word is captured
                                                 //removing this will not display single words without non-alphabets
                    }
                }
            }
            else
            {
                if (tempLength >= maxLength)
                {
                    maxLength = tempLength;
                }
                tempLength = 0;
            }
        }

        tempLength = 0;

        //a for loop to add the longest words into a list
        for (; l <= s.Length - 1; l++)
        {
            if (((words[l] >= 'A' && words[l] <= 'Z') || (words[l] >= 'a' && words[l] <= 'z')))
            {
                tempLength++;

                if (l == (s.Length - 1))
                {
                    if (tempLength >= maxLength)
                    {
                        indexList.Add(l+1); //'l+1' ensures a word that has no non-alphabet after it is captured
                                            //removing l+1 displays as 'wanted remov' - 'e' is missing
                        k++;
                    }
                }
            }
            else
            {
                if (tempLength == maxLength)
                {
                    indexList.Add(l);
                    k++;
                }
                tempLength = 0;
            }
        }

        //a for loop to append the longest words into a single word
        for (int m = 0; m <= k - 1; m++)
        {
            for (int n = (indexList[m] - maxLength); n < indexList[m]; n++)
            {
                longestWord.Append(words[n]);
            }
            longestWord.Append(" ");
        }

        Console.WriteLine(longestWord);
    }

Honestly, although this code works, I'm unsatisfied with it and would like suggestions as to how I could improve it lengthwise and in readability.

Answer 1

Why do you have more parenthesis than needed in this statement?

if (((words[j] >= 'A' && words[j] <= 'Z') || (words[j] >= 'a' && words[j] <= 'z')))

if ((words[j] >= 'A' && words[j] <= 'Z') || (words[j] >= 'a' && words[j] <= 'z'))

Similarly:

if (l == (s.Length - 1))

Should be written as:

if (l == s.Length - 1)

As well:

for (int n = (indexList[m] - maxLength); n < indexList[m]; n++)

Should be:

for (int n = indexList[m] - maxLength; n < indexList[m]; n++)

---

Using j, k and l as a variable in this manner is frowned upon. These should only ever be used with local iterators. (I.e. where you have i.)

---

Some of your names are not idiomatic.

char[] words = s.ToCharArray();

Should be:

char[] characters = s.ToCharArray();

---

The string s parameter should be string input (et. al.).

---

You can take some of your operations to functions, delegates, Lambdas, etc.

if (((words[l] >= 'A' && words[l] <= 'Z') || (words[l] >= 'a' && words[l] <= 'z')))

Can more easily be written as a delegate, function, etc.

if (IsAlphaChar(words[l])

---

Use a foreach loop in place of:

for (int m = 0; m <= k - 1; m++)

Should be:

foreach (int index in indexList)

Then just use index in place of indexList[m].

---

Never do this:

int j = 0, k = 0, l = 0, tempLength = 0, maxLength = 0;

That is a readability nightmare. Break each variable to it's own line. (And don't declare all of them in one spot. Declare them when you need them.)

---

You shouldn't declare iterators outside a for loop. (I'm looking at you, j and l.)

---

Doing words[i] four times is slower than char currentCharacter = words[i];. (Of course, the location that is used should be a method instead.)

---

You don't use j or l after their respective loops, so don't define them as the first thing in the method. Instead, define them as:

for (int j = i; j <= s.Length; j++)
for (int l = i; l <= s.Length; l++)

Answer 2

Single responsibility principle

Your code is violating the SRP by doing way to much because it is evaluating and outputting the longest word(s). This should be separated into at least 2 different methods.

---

Basically you only need 2 loops in your code. The first to iterate over the method arguments character and another to output the longest words.

By using @Ebrowns suggestion to use a method to evaluate if a given char is in the desired range it will be easier to extend this to take other valid characters into account.

My suggestion would be to separate the algorithm of finding the longest word into 3 methods.

To validate a given char like so

private static bool IsValidChar(char c)
{
    return (c >= 'A' && c <= 'Z')
        || (c >= 'a' && c <= 'z');
}   

To separate the words like so

private static IEnumerable<string> GetWords(string input)
{
    if (string.IsNullOrEmpty(input)) { yield break; }

    int start = 0;
    int length = 0;
    foreach (char c in input)
    {
        if (IsValidChar(c))
        {
            length++;
            continue;
        }

        if (length > 0)
        {
            yield return input.Substring(start, length);
        }
        start += length + 1;
        length = 0;

    }

    if (length > 0)
    {
        yield return input.Substring(start, length);
    }
}  

and to get the longest of the separated words like so

private static IEnumerable<string> FindLongestWords(string input)
{

    int maxLength = int.MinValue;
    foreach (string word in GetWords(input).OrderByDescending(s => s.Length))
    {
        if (word.Length < maxLength)
        {
            break;
        }
        maxLength = word.Length;
        yield return word;
    }

}

Which can be called like this

string value = "This is a test to get the longest word";
IEnumerable<string> longestWords = FindLongestWords(value);

and can be outputed to the console like so

Console.WriteLine(string.Join(" ", longestWords ));

Answer 3

There's been already said actaully everything about the code itself so I won't comment on that however I'd like to show an alternative solution.

I happen to use the same method names as @Heslacher but there aren't really any other alternatives ;-) I also noticed that my implementation is very similar but it's not based on yours.

---

@kJo

• I think it's not necassary to use a custom method for checking if a character is a letter because the Char.IsLetter already can do that. The advantage of using it is that it can recognize letters in more then one language.

This method determines whether a Char is a member of any category of Unicode letter.

Knowing this we can simply scan the entire string and collect only letters into words.

• Then finding the longest word is as easy as group the list of words by their lengths, sort the groups in descending order and get the first one which would contain all the longest words.

This would then result as the following two simple methods:

GetWords

static IEnumerable<string> GetWords(string input)
{
    // initialize for the first word
    var word = new StringBuilder();
    foreach (var character in input)
    {
        if (char.IsLetter(character))
        {
            word.Append(character);
        }
        else
        {
            if (word.Length > 0)
            {
                yield return word.ToString();
            }
            // reset word
            word = new StringBuilder();
        }
    }
}

FindLongestWords

static IEnumerable<string> FindLongestWords(IEnumerable<string> words)
{
    return 
        words
        .GroupBy(w => w.Length)
        .OrderByDescending(g => g.Key)
        .First();
}

Answer 4

This kind of task is well suited to Linq and functional programming, imperative code will only reinvent basic algorithms, so following @Topin suggestion, I suggest this outline:

public static string[] words(string text) 
{
    // Use of Regex and Split
}

public static string longestWord(string text)
{
    // Make use of words and OrderBy
}

Answer 5

I modified my code to reflect @EBrown's suggestions. Also, I realized:

• The first for loop was unnecessary and as such, I edited it out.
• The subsequent two for loops could be incorporated into a single for loop. Previously, I used these two loops as a way to avoid the addition of lengths lower than that of the maximum length. To merge these two, I used List.Clear() to clear out the list if a new maximum length is identified.

    //this method returns the longest word (or words) of a single string
    static void longestWord(string str)
    {
        int tempLength = 0;
        int maxLength = 0;

        List<int> indexList = new List<int>();
        char[] characters = str.ToCharArray();
        StringBuilder longestWord = new StringBuilder();

        //a for loop to get the maximum length of the longest word
        for (int i = 0; i <= str.Length - 1; i++)
        {
            if ((characters[i] >= 'A' && characters[i] <= 'Z') || (characters[i] >= 'a' && characters[i] <= 'z'))
            {
                tempLength++;

                if (i == str.Length - 1)
                {
                    if (tempLength > maxLength)
                    {
                        indexList.Clear();       //this ensures that only indexes of words with the maximum length is stored
                                                 //thus, any index that was stored previously as the longest is replaced

                        maxLength = tempLength;  //this ensures the maximum length of a single word is captured
                                                 //removing this will not display single words without non-alaphabets

                        indexList.Add(i + 1);    //'l+1' ensures a word that has no non-alaphabet after it is captured
                                                 //removing l+1 displays as 'wanted remov' - 'e' is missing
                    }
                    else
                    {
                        if (tempLength == maxLength)
                        {
                            indexList.Add(i + 1);    //'l+1' ensures a word that has no non-alaphabet after it is captured
                            //removing l+1 displays as 'wanted remov' - 'e' is missing
                        }
                    }
                }
            }
            else
            {
                if (tempLength > maxLength)
                {
                    indexList.Clear();
                    maxLength = tempLength;
                    indexList.Add(i);
                }
                else
                {
                    if (tempLength == maxLength)
                    {
                        indexList.Add(i);
                    }
                }
                tempLength = 0;
            }
        }

        //a for loop to append the longest words into a single word
        foreach (int index in indexList)
        {
            for (int i = index - maxLength; i < index; i++)
            {
                longestWord.Append(characters[i]);
            }
            longestWord.Append(" ");
        }

        Console.WriteLine(longestWord);
    }