Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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

I have asked a very similar question on codereview before with great feedback - basically im writing a language and im adding a function to add any strings or string arrays to a string array using a single function.

i have learned my lesson form the last question Function to split either a string array or string into a string array and used a few methods i didnt know you can use beforehand,and now im seeing if there is more room for improvements.

now before people ask me why i didnt write multiple functions or something like that. the POINT of the function is to be a single one and to be able to recieve an indefinite amount of strings or stringarrays and get the job done. restrictions are not allowed.

The function i have written (creatively named)

    public string[] Add(string[] ogarr,object[] toadd)
    {

        var results = new System.Collections.Generic.List<string>();
        results.AddRange(ogarr);

        for (int i = 0; i < toadd.Length; i++)
        {
            if (toadd[i].GetType() == typeof(string[]))
                results.AddRange((string[])toadd[i]);
            else
                results.Add((string)toadd[i]);
        }

            return results.ToArray();
    }

i use this to test its functionality

        string[] temp = new string[]{"123","456","789"};

        string[] thing1 = new string[]{"abc","def"};
        string thing2 = "ghi";

        object[] toadd = new object[] {thing1,thing2 };

        temp = Add(temp,toadd);

where temp is the original string array i want to add things to.

and thing 1/2 are both things i want to add to that array one after another in the order of addition to the object[] obviously.

exclaimer: im not trying to cut bytes, i want aesthetic code! this is my passion

share|improve this question
up vote 4 down vote accepted

Naming Conventions

As the Naming Guidelines states:

The goal of this chapter is to provide a consistent set of naming conventions that results in names that make immediate sense to developers.

In that sense, the creatively defined method name Add and the parameter names ogarr and toadd will not do.

The General Naming Conventions states:

Avoid using identifiers that conflict with keywords of widely used programming languages.

Do not use abbreviations or contractions as part of identifier names.

Do favor readability over brevity.

Method name Add is a common method name used by collection types to add a new element to the collection. An Add method with source as the first parameter and the items to be added as the second parameter does not make sense.

ogarr and toadd are bad names for parameters and should be changed at least to sourceArray and itemsToAdd or similar.

And in Capitalization Conventions

Do use camelCasing for parameter names.

I'm not suggesting to change ogarr to ogArr and toadd to toAdd of course. The parameters should have descriptive names in the first place. You can consider sourceArray and itemsToAdd or similar.

Possible Improvements

Being consistent with code formatting is important. Consider using curly brackets, always:

if (toadd[i].GetType() == typeof(string[]))
{
    results.AddRange((string[])toadd[i]);
}
else
{
    results.Add((string)toadd[i]);
}

You say

restrictions are not allowed.

Then, why restricting the second method parameter to object[]? Let's get that as an IEnumerable. This way, your method can also operate on string[] or List<T> instances

public string[] Add(string[] sourceArray, IEnumerable itemsToAdd)

You can also change the method to be an extension method of string[]. This will clearly state the purpose of the method if declared with a good name:

public static string[] AddItemsOrRanges(this string[] sourceArray, IEnumerable itemsOrRangesToAdd)

You should always check if the parameters are passed in as null. Your code would fail with a null refererence exception at toadd.Length if toadd is passed as null.

if (toadd == null)
{
    // throw ArgumentException() ?
    return ogarr;
}

You should also check the items of the passed in collection for null references. If any of the items of the object[] toadd would be null, your code would fail with a null refererence exception at x.GetType()

if (toadd[i] == null)
{
    // throw ArgumentException() ?
    continue;
}

Finally, the implementation, with all the naming convention violations and functional confusions fixed, might look like the following:

public static string[] AddItemsOrRanges(this string[] sourceArray, IEnumerable itemsOrRangesToAdd)
{
    if (itemsOrRangesToAdd == null)
    {
        // throw ArgumentException() ?
        return sourceArray;
    }

    // It is possible with extension methods to have the self instance this as null
    // because extension methods are actually static methods and can be invoked
    // on a null reference (interesting?)
    // string[] myStringArray = null;
    // string[] finalStringArray = myStringArray.AddItemsOrRanges(new object[] { "Test" });
    // will work!
    if (sourceArray == null)
    {
        // throw ArgumentException() ?
        return sourceArray;
    }

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

    foreach (var itemOrRangeToAdd in itemsOrRangesToAdd)
    {
        if (itemOrRangeToAdd != null)
        {
            if (itemOrRangeToAdd.GetType() == typeof(string[]))
            {
                results.AddRange(itemOrRangeToAdd as string[]);
            }
            else if (itemOrRangeToAdd.GetType() == typeof(string))
            {
                results.Add(itemOrRangeToAdd as string);
            }
            else
            {
                // What to do if the element is neither a string nor a string[]?
                // The (string) cast in the else block would fail in your source code
                // throw ArgumentException() ?
            }
        }
        else
        {
            // How would you determine if the element is to be added or not?
            // is it a null string instance, or is it a null string[] instance?
            // throw ArgumentException() ?
            continue;
        }
    }

    return results.ToArray();
}

And calling it would be like:

string[] temp = new string[] { "123", "456", "789" };

string[] thing1 = new string[] { "abc", "def" };
string thing2 = "ghi";

object[] toadd = new object[] { thing1, thing2 };

temp = temp.AddItemsOrRanges(toadd);
share|improve this answer
    
I love the added function to the string[] idea, i will definitely implement that in my future things,my programming language is type safe,therefor unexpected variable types are not a concern. But you are definitely right,my naming of things is not very good. Thank you – downrep_nation Dec 28 '15 at 8:03
    
You're welcome. – Oguz Ozgul Dec 28 '15 at 8:23
    
restricting the second method parameter to object[]? Let's get that as an IEnumerable. ... BTW, A string IS an array of char. This has proved to be a very useful insight. – radarbob Dec 30 '15 at 14:50

Just a little thing you can add to Oguz Ozgul answer.. if you always do negative checks first then why not in foreach loop? You can easily terminate else block.

foreach (var itemOrRangeToAdd in itemsOrRangesToAdd)
{
    if (itemOrRangeToAdd == null)
    {
        continue;
    }

    if (itemOrRangeToAdd.GetType() == typeof(string[]))
    {
        results.AddRange(itemOrRangeToAdd as string[]);
    }
    else if (itemOrRangeToAdd.GetType() == typeof(string))
    {
        results.Add(itemOrRangeToAdd as string);
    }
    else
    {
        // What to do if the element is neither a string nor a string[]?
        // The (string) cast in the else block would fail in your source code
        // throw ArgumentException() ?
    }
}
share|improve this answer
    
I think having that kind of check inside a loop is cumbersome. I would either gather the non-null values before the loop, or change the foreach to foreach (var itemOrRangeToAdd in itemsOrRangesToAdd.Where(x=>x!=null)) – Reacher Gilt Dec 31 '15 at 20:13

You can use Linq. (sample code w/o checking nulls etc.)

public static string[] AddItemsOrRanges (string[] source, IEnumerable<object> itemsOrRanges)
{
    return source.Concat  (itemsOrRanges.OfType<string>())
                 .Concat  (itemsOrRanges.OfType<string[]>().SelectMany(x => x))
                 .ToArray ();
}
share|improve this answer
1  
Linq... One day ill master you – downrep_nation Dec 28 '15 at 13:28

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.