tags:

views:

67

answers:

7

I've got the following method I created which works fine if there actually is the delimiter in question. I want to keep this out of LINQ for now...

e.g.

If I pass in the string "123;322;323" it works great.

But if I only pass in one string value without the delimiter such as "123" it obviously is not going to split it since there is no delimiter. I am just trying to figure out the best way to check and account for this and be able to spit out that one value back in the list

public static List<int> StringToList(string stringToSplit, char splitDelimiter)
{
    List<int> list = new List<int>();

    if (string.IsNullOrEmpty(stringToSplit))
        return list;

    string[] values = stringToSplit.Split(splitDelimiter);

    if (values.Length < 1)
        return list;

    foreach (string s in values)
    {
        int i;
        if (Int32.TryParse(s, out i))
            list.Add(i);
    }

    return list;
}

UPDATED: This is what I came up with that seems to work but sure is long

    public static List<int> StringToList(string stringToSplit, char splitDelimiter)
    {
        List<int> list = new IntList();

        if (string.IsNullOrEmpty(stringToSplit))
            return list;

        if (stringToSplit.Contains(splitDelimiter.ToString()))
        {
            string[] values = stringToSplit.Split(splitDelimiter);

            if (values.Length <= 1)
                return list;

            foreach (string s in values)
            {
                int i;
                if (Int32.TryParse(s, out i))
                    list.Add(i);
            }
        }
        else if (stringToSplit.Length > 0)
        {
            int i;
            if(Int32.TryParse(stringToSplit, out i))
                list.Add(i);
        }

        return list;
    }
+4  A: 

Change this condition:

if (values.Length <= 1)
    return list;

To:

if (values.Length <= 0)
    return list;

This works because String.Split will return the original string if it can't find the delimiter:

// stringToSplit does not contain the splitDelimiter
string[] values = stringToSplit.Split(splitDelimiter);
// values is a string array containing one value - stringToSplit
Oded
Or alternatively, `if(!values.Any())`, but +1 for finding the bug and explanation of what `.Split` is doing for him.
Marc
if Split is returning the original string if the delimiter cannot be found then values.Length should be 1 right? (and the item is at values[0]) Checking for <= 1 is then correct.
PoweRoy
@PowerRoy - He is trying to check if the array is empty and then return an empty list (read the whole code). The array is empty when there are no items in it (`values.Length <= 0`).
Oded
so really you don't need the < in the <=. It should just be if(values.length == 0)
CoffeeAddict
The better question is, why are we looking for an early exit right before an iteration of said potentially empty collection?
Marc
A: 

A shorter implementation you might consider.

public static List<int> StringToList(string stringToSplit, char splitDelimiter)
{
    int i;
    return stringToSplit.Split(splitDelimiter)
        .Where(str => int.TryParse(str, out i))
        .Select(str => int.Parse(str))
        .ToList();
}
tster
Doesn't `i` end up being the last parse-able string? Imo, side-effects in linq predicates = smelly.
Marc
who cares what i ends up being? It's just there to allow me to call TryParse.
tster
wouldn't it allow you to replace the .Select(str => int.Parse(str)) with .Select(str => i)
JDunkerley
I suppose you could do that, but I was be scared of it.
tster
I'm not using LINQ for this right now. Yes, yes I know it's easier and nice, but I'm just not going to for this one. I will later on prob change it but for now I want to know I can do this and understand it.
CoffeeAddict
@tster: You're basically using `TryParse` as a synonym for `IsParsable` here; might as well *use* the output and not do more work than necessary, if you ask me (see my answer).
Dan Tao
A: 

Either it must have a delimiter or, it part must have a fixed length or, the string must follow a pattern for repeat behavior.

this. __curious_geek
I don't know what the querystring will bring in meaning I'm passing a querystring value as the stringToSplit. I am calling this method on a querystring value to split it into a list and it just depends, at times it may have one, sometimes more than one incoming (split by a ;) so I have to account for any situation...1 or 1 to many
CoffeeAddict
+1  A: 

Personally, I haven't tried out your code, but it looks functional. The Split method should return an array with one element.

Since you say it's not, I believe you, and I would add this to the top of your method:

if (!stringToSplit.Contains(splitDelimiter))
{
    int i;
    if (Int32.TryParse(stringToSplit, out i))
        list.Add(i);
    return list;
}
JustLoren
A: 

You don't need to account for that situation, as string.Split() returns an array with a single element when no delimiter is found:

If this instance does not contain any of the strings in separator, the returned array consists of a single element that contains this instance.

See msdn at "Remarks"

Femaref
sorry, which lines of code are you referring to?
CoffeeAddict
Ok, but I want to add the single value to the List<int>
CoffeeAddict
So you're saying list.Add(string.Split(';')) ?? I don't follow
CoffeeAddict
+1  A: 

There are a LOT of unnecessary checks for conditions that don't matter to the core logic of the method.

public static List<int> StringToList(string stringToSplit, char splitDelimiter) 
{ 
    List<int> list = new IntList(); 

    if (string.IsNullOrEmpty(stringToSplit)) 
        return list; 

    //this if is not necessary. As others have said, Split will return a string[1] with the original string if no delimiter is found
    if (stringToSplit.Contains(splitDelimiter.ToString())) 
    { 
        string[] values = stringToSplit.Split(splitDelimiter); 

        //why check this? if there are no values, the foreach will do nothing and fall through to a return anyway.
        if (values.Length <= 1) 
            return list; 

        foreach (string s in values) 
        { 
            int i; 
            if (Int32.TryParse(s, out i)) 
                list.Add(i); 
        } 
    } 
    //again, this is rendered redundant due to previous comments
    else if (stringToSplit.Length > 0) 
    { 
        int i; 
        if(Int32.TryParse(stringToSplit, out i)) 
            list.Add(i); 
    } 

    return list; 
}

Try this. You hopefully have some unit tests calling this method to make sure it works...right?

public static List<int> StringToList(string stringToSplit, char splitDelimiter) 
{ 
    List<int> list = new IntList(); 

    if (string.IsNullOrEmpty(stringToSplit)) 
        return list;

    foreach(var s in stringToSplit.Split(splitDelimiter))
    {
        int i;
        if(int.TryParse(s, out i))
            list.Add(i);
    }
    return list;
}
Andy_Vulhop
Thanks a lot. This is good feedback and yea I'm trying to cut down my unnecessary code in general...this was a great thread to realize just how much clutter I had in there. I did not see it myself initially.
CoffeeAddict
CoffeeAddict
@coffeeaddict, unit testing will make you develop faster so not being able to "find the time" to write tests is no excuse since you will deliver functionality slower without unit tests.
tster
@coffeeaddict, if you don't like your job (and clearly it's not a good situation), either change where you work, or change where you work. (meaning, change the way things are done at your office via a grassroots movement, or find a new place to work that cares more about the quality of your craft). I'd try the former, but, if your office is like mine, you'll end up giving up and doing the latter.
Andy_Vulhop
+1  A: 

Might I suggest an extension method to help you in cases like this in general?

The idea would be to yield return all results from a collection of strings that can be parsed to a specified type.

First you'd need a delegate to match the signature of your standard TryParse method:

public delegate bool Parser<T>(string input, out T value);

Then you can write an extension method that takes an instance of this delegate type and enumerates over a collection of strings, parsing everywhere it can:

// Notice: extension methods must belong to a class marked static.
public static class EnumerableParser
{
    // modified code to prevent horizontal overflow
    public static IEnumerable<T> ParseAll<T>
    (this IEnumerable<string> strings, Parser<T> parser)
    {
        foreach (string str in strings)
        {
            T value;
            if (parser(str, out value))
                yield return value;
        }
    }
}

Now your StringToList method becomes trivial to implement:

public List<int> StringToList(string stringToSplit, char delimiter)
{
    // Notice: since string.Split returns a string[], and string[] implements
    // IEnumerable<string>, so the ParseAll extension method can be called on it.
    return stringToSplit.Split(delimiter).ParseAll<int>(int.TryParse).ToList();
}
Dan Tao
Exactly, that's the natural choice that makes the MOST sense in terms of maintainability and reuse. However BOSS, a dictator right down to even wanting to use List<int> and forces me to use his custom class IntList : List<int> just so he doesn't have to type <int> also doesn't want to use extension methods (yes, I hate this place). If I argue with him he'll have me fired. I'm not kidding.
CoffeeAddict
Where are you putting that method in order for it to be an "extension method" and therefore picked up by intellisense as a method off the split?
CoffeeAddict
when you called ParseAll, I see you are only passing in the delegate. But how are you passing in the strings param?
CoffeeAddict
@coffeeaddict: To make a method an extension method, you put it in a `static` class and mark the first parameter with the `this` keyword, as I've done. Calling the method then works very much the same way as calling an *instance* method on an object -- you call the method "on" the `this` parameter (so the parameters you pass "to" the method start with the *second* argument). I will update my example to illustrate this.
Dan Tao