current community

your communities

more stack exchange communities

company blog
Stack Exchange Inbox Reputation and Badges
tour help
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. Join them; it only takes a minute:

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top
up vote 4 down vote favorite

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

For example, with a string:

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

It will return this array:

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

And with a string[]:

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

It will return this array:

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

How can I improve efficiency and maybe make it tidier?

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();
        }

Side notes:

  • SplitRows() is the only main function allowed to receive either a string array or string. Even though I use 2 functions, I cannot create 2 for the 2 variable types.
  • <newline> is a replacement string for \n in my language and 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 Dec 21 '15 at 8:38
    
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 Dec 21 '15 at 8:55
    
If you replace return null with throw new ArgumentException("string or string[] only") IDE (that is compiler) will not complain. – abuzittin gillifirca Dec 21 '15 at 8:58
    
I will do so,thanks – downrep_nation Dec 21 '15 at 9:06
    
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 Dec 22 '15 at 6:27
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 Dec 21 '15 at 8:52

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.