current community

your communities

more stack exchange communities

company blog
Stack Exchange Inbox Reputation and Badges
tour help
Sign up ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 4 down vote favorite

I am programming a language interpreter in c# recently and i have created a set of functions that receive either a string or a string[] and splits it by a received string.

For example: with a string Input-

"Hey:123:hello:456"
":"

Will return this array

{"hey","123","hello","456"}

And with a string[] Input-

{"a:b","c","d:e:f"}
":"

Will return this array

{"a","b","c","d","e","f"}

So this is my code

public string[] SplitRows(object thevar, string delimiter) {

            if (delimiter == "<newline>") delimiter = "\n";

            if (thevar.GetType() == typeof(string)) {
                string temp = (string) thevar;
                return temp.Split(new string[] {
                    delimiter
                }, System.StringSplitOptions.None);
            }
            if (thevar.GetType() == typeof(string[])) {
                return stringarraysplitter((string[]) thevar, delimiter);
            }
            return null;
        }


        public string[] stringarraysplitter(string[] arr, string delimiter) {
            string[][] tempr = new string[arr.Length][];
            for (int i = 0; i < arr.Length; i++) {
                tempr[i] = arr[i].Split(new string[] {
                    delimiter
                }, System.StringSplitOptions.None);
            }
            System.Collections.Generic.List < string > templist = new System.Collections.Generic.List < string > ();
            for (int i = 0; i < tempr.Length; i++) {
                for (int j = 0; j < tempr[i].Length; j++) {
                    templist.Add(tempr[i][j]);
                }
            }

            return templist.ToArray();
        }

How can i improve efficiency and maybe make it tidier.

Side note:SplitRows() is the only main function allowed to receive either a string array or string,even tho i use 2 functions,i can not create 2 for the 2 variable types.

Edit:

I'm sorry for <newline> its a replacement string for \n in my language. Can be ignored

share|improve this question
up vote 4 down vote accepted 
if (delimiter == "<newline>") delimiter = "\n";

Why do you need this? Why not use normal strings?

if (thevar.GetType() == typeof(string)) {

What's wrong with normal method overloading?

if (thevar.GetType() == typeof(string)) {...}
if (thevar.GetType() == typeof(string[])) {...}
return null;

If arguments is not valid throw an ArgumentException. Don't say "I'll just return null, and see where it will go wrong."

temp.Split(new string[] {delimiter}, System.StringSplitOptions.None);

This bit repeats, and should be extracted into a method.

for (int i = 0; i < tempr.Length; i++) {
    for (int j = 0; j < tempr[i].Length; j++) {

Use Linq to simplify loop operations.

Here is the cleaned up version:

    public IEnumerable<string> Split(string str, string delimiter)
    {
        return str.Split(new[]{delimiter}, StringSplitOptions.None);
    }

    public IEnumerable<string> Split(IEnumerable<string> arr, string delimiter) {
        return arr.SelectMany(s => Split(s, delimiter));
    }
share|improve this answer
    
Im sorry for <newline> its a replacement string for \n – downrep_nation 2 days ago
    
I added return null because the ide asked me too return anything,in my language no other option but string or string[] could ever be passed to it – downrep_nation 2 days ago
    
If you replace return null with throw new ArgumentException("string or string[] only") IDE (that is compiler) will not complain. – abuzittin gillifirca 2 days ago
    
I will do so,thanks – downrep_nation 2 days ago
    
Could you explain to me,or give me some resourceful links or information on how the 2 last functions you wrote are written? I have never use ienumerable.. – downrep_nation yesterday
up vote 1 down vote

The method stringarraysplitter()'s name doesn't follow the NET naming guideline which states method names should be made out of a verb or a verb-phrase and the method should be name using PascalCase casing.
A better name would be SplitStringArray().

Because you already use a List<string> you could take advantage of the AddRange() method which makes the use of the nested loops redundant and omits the need to have a jagged array at all.

Because the method is public you should check the arguments against null.

By using the var type it will become more readable.

The placing of the opening brace { is different to what a C# developer is used to. In C# we usually place the opening brace on a new line.

Like so 

public string[] stringarraysplitter(string[] arr, string delimiter) 
{
    if (arr == null || arr.Length == 0) { return arr; }
    if (delimiter == null) { delimiter = "\n"; }

    var results = new System.Collections.Generic.List<string>(arr.Length);

    for (int i = 0; i < arr.Length; i++) 
    {
        results.AddRange(arr[i].Split(new string[] {delimiter}, System.StringSplitOptions.None));
    }

    return results.ToArray();
}
share|improve this answer
    
I love that addrange() beautiful! – downrep_nation 2 days ago

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.