Adding a mix of strings and string arrays to an array

Question

Original question:

Function to split either a string array or string into a string array

I was asked why I didn't write multiple functions or something like that. The point of the function is to be a single one and to be able to receive an indefinite number of strings or string arrays 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);

• temp is the original string array I want to add things to.
• thing1 and thing2 are both things I want to add to that array, one after another, in the order of addition to the object[], obviously.

Disclaimer: I'm not trying to cut bytes; I want aesthetic code! This is my passion.

Answer 1

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

• downrep_nation: 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
• radarbob: 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.

Answer 2

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

• Reacher Gilt: 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))