StringSplitter Exceptions

[rgomez90]

The method StringSplitter.SplitTextIntoChunks raises exception by a null input string or for maxChunkSize < 1.

This exceptions will not be raised "directly".

Example:

//Passing a null string will raise an ArgumentException
var chunks = StringSplitter.SplitTextIntoChunks(null, 250, false);
var firstChunk = chunks.First();    //Exception will be raised here!!!!

The exceptions will be raised first when accessing the enumerable, due to the enumerator nature of the method.

I am not sure if this is the intended behaviour, since I would expect the exception to be raised when calling StringSplitter.SplitTextIntoChunks(null, 250, false)

[rgomez90]

Possible solution: not use an enumerator.

[PromoFaux]

Does the exception still not get raised at the right place after merging #18 ?

[rgomez90]

They just will be raised when accessing the enumerator.

So if you just call var chunks = StringSplitter.SplitTextIntoChunks(null,250,false) no exception will be raised.
It will be raised first when caling for ex. foreach (var chunk in chunks) {...} or chunks.First() the exception will be raised.

[PromoFaux]

That said, for the message text we at least check if the value is null before attempting to split it

if (inMessage.Text != null)
{
    //Split messages text into chunks of maxMessageLength in size.
    var textChunks = StringSplitter.SplitTextIntoChunks(inMessage.Text, maxMessageLength).ToList();

    //iterate through chunks and create a MattermostMessage object for each one and add it to outMessages list.
    foreach (var chunk in textChunks)
    {
        outMessages[msgIdx].Text = chunk;
        if (msgIdx < textChunks.Count - 1)
        {
            outMessages.Add(inMessage.Clone());
            msgIdx++;
        }
    }
}

But the same thing is not in place for the attachment text part.

[rgomez90]

We should check also for the max chunk size, since a value smaller than 1 would make no sense.

I still think we should check in the method and not outside, otherwise we need to check both values every time we call the method.

So I would eliminate the enumerator and just return an IEnumerable.

[rgomez90]

We could do sth like this (just remove iterators and return List<string>/Collection<string> as IEnumerable

public class StringSplitter
    {
        public static IEnumerable<string> SplitTextIntoChunks(string str, int maxChunkSize, bool preserveWords = true)
        {
            if (string.IsNullOrEmpty(str)) throw new ArgumentException("Text can't be null or empty","str");
	    if (maxChunkSize < 1) throw new ArgumentException("Text can't be null or empty","maxChunkSize");
            if (str.Length < maxChunkSize) return new List<string>{str};
            if (preserveWords) return SplitTextBySizePreservingWords(str, maxChunkSize);
            else return SplitTextBySize(str, maxChunkSize);            
        }

        private static IEnumerable<string> SplitTextBySize(string str, int maxChunkSize)
        {
            if (str.Length < maxChunkSize) return new List<string>{str};
	    var list = new List<string>();
            for (var i = 0; i < str.Length; i += maxChunkSize)
            {
                list.Add(str.Substring(i, Math.Min(maxChunkSize, str.Length - i)));
            }
	    return list;
        }

        private static IEnumerable<string> SplitTextBySizePreservingWords(string str, int maxChunkSize)
        {
            if (str.Length < maxChunkSize) return new List<string>{str};
            var words = str.Split(' ');
            var tempString = new StringBuilder("");
	    var list = new List<string>();
            foreach (var word in words)
            {
                if (word.Length + tempString.Length + 1 > maxChunkSize)
                {
                    list.Add(tempString.ToString());
		    tempString.Clear();
                }
                tempString.Append(tempString.Length > 0 ? " " + word : word);
	     }
	     if(tempString.Length >= 1)
	     list.Add(tempString.ToString());
             return list;
        }
   }

This way when calling var chunks = StringSplitter.SplitTextIntoChunks(null, 250, false) or var chunks = StringSplitter.SplitTextIntoChunks(str, 0, false) an exception will be directly raised instead of raising when the iterator gets called.

[PromoFaux]

Hold up, does calling ToList() not cause it to enumerate, and therefore throw the exception?

var attTextChunks = StringSplitter.SplitTextIntoChunks(att.Text, 6600).ToList();

Not saying your idea is bad :) Just thinking if it's strictly necessary or not