6
\$\begingroup\$

I am new to C# and I have big functions that receives the following arguments:

(Package inPackage, string materialIDs, string functionIDs, string companyIDs)

Asides from the first argument, which is an object of type Package, all the other arguments are strings of IDs.

So, for example, a valid input for this function would be: myPackage, "1,2,3", ",2,3,4", ",45,2".

A package is an object that contains a list of materials, a list of functions and a list of companies.

The objective of this function, is to look up in the DB all the objects that contain those IDs, and add them to myPackage.

To achieve this objective, this is what I have created:

private Package StringToCollections(Package inPackage, string materialIDs, string functionIDs, string companyIDs)
        {
            //Check Materials Data - Case WITHOUT FORMAT PROTECTION
            var materialList = new List<Material>();
            var materialStringArray = materialIDs.Split(',');
            var materialIdsArray = materialStringArray.Where(x => x != "").Select(int.Parse);
            materialList.AddRange(DB.Materials.Where(x => materialIdsArray.Contains(x.MaterialId)));
            inPackage.GATE_Material = materialList;

            //Check Employee_Functions Data - HAS FORMAT PROTECTION ... KINDOFF?
            List<Function> functionsList = new List<Function>();
            string[] functionStringArray = functionIDs.Split(',');
            List<int> functionIdsArray = new List<int>();
            for (int i = 0; i < functionStringArray.Length; i++)
            {
                try
                {
                    functionIdsArray.Add(int.Parse(functionStringArray[i]));
                }
                catch (FormatException)
                {
                    //not a number, ignore - IS THIS A GOOD IDEA!?
                }
            }
            functionsList.AddRange(DB.Function.Where(x => functionIdsArray.Contains(x.FunctionId)));
            inPackage.Function = functionsList;

            //Check Company Data - ANOTHER WAY OF FORMAT PROTECTION ... KINDOFF?
            if (!companyIDs.IsNullOrWhiteSpace())
            {
                var companiesList = new List<Company>();
                var companyStringArray = companyIDs.Split(',');
                var companyIdsArray = companyStringArray.Where(x => x != "").Select(int.Parse);
                try
                {
                    companiesList.AddRange(DB.Companies.Where(x => companyIdsArray.Contains(x.ID)));
                    inPackage.Companies = companiesList;
                }
                catch (FormatException)
                {
                    //do nothing. If the string has an incorrect format, we add nothing and continue.
                }
            }

            return inPackage;
        }

I honestly believe this code is a mess and was hoping that it could be fixed by using C# generics like <T>, but I am not sure if this is possible due to the differences in getting each fields ID and Name.

Is it possible to optimize this function and make it more generic with protection from format exception? if so, how?

\$\endgroup\$
  • 1
    \$\begingroup\$ Why are your parameters strings instead of, say, int[] or IEnumerable<int>? \$\endgroup\$ – mjolka Dec 3 '14 at 9:50
  • \$\begingroup\$ Do you use any own extension methods which you don't show ? \$\endgroup\$ – Heslacher Dec 3 '14 at 9:51
  • \$\begingroup\$ @mjolka: The parameters are strings because of a jquery plugin that requires them to be strings. It is a long explanation, but if there was another way of doing it, I woudl have done it. \$\endgroup\$ – Flame_Phoenix Dec 3 '14 at 9:54
  • \$\begingroup\$ @Heslacher: I dont even know what an extension method is :S All I know is that I am accessing the DB and reading information from it, like any MVC5 C# web application that uses Entity Framework does. \$\endgroup\$ – Flame_Phoenix Dec 3 '14 at 9:55
  • \$\begingroup\$ I ask because of companyIDs.IsNullOrWhiteSpace() vs String.IsNullOrWhiteSpace(companyIDs) \$\endgroup\$ – Heslacher Dec 3 '14 at 9:57
11
\$\begingroup\$

Consistency:

Don't mixup using var and declaring your variables explicitly. Choose one of the two, preferably var.

Combining methods:

Use method chaining to shorten your code. There's no need to create a variable for every step your code does. This:

var materialStringArray = materialIDs.Split(',');
var materialIdsArray = materialStringArray.Where(x => x != "").Select(int.Parse);

can be changed to:

var materialIdsArray = materialIDs.Split(',').Where(x => x != "").Select(int.Parse);

Generics:

You cannot make one magic generic method to do this. Why? Simply because you do not know the type ahead of time. What you can do however is to create separate method for getting the valid ids from the string:

public IEnumerable<int> GetValidIDs(string ids)
{
    foreach (var id in ids.Split(','))
    {
        int x;
        if (Int32.TryParse(id, out x))
        {
            yield return x;
        }
    }
}

private Package StringToCollections(Package inPackage, string materialIDs, string functionIDs, string companyIDs)
{
    var validMaterialIDs = GetValidIDs(materialIDs);
    var materials = DB.Materials.Where(x => validMaterialIDs.Contains(x.MaterialId));

    var validFunctionIDs = GetValidIDs(functionIDs);
    var functions = DB.Function.Where(x => validFunctionIDs.Contains(x.FunctionId)));

    var validCompanyIDs = GetValidIDs(functionIDs);
    var companies = DB.Companies.Where(x => validCompanyIDs.Contains(x.FunctionId)));

    inPackage.GATE_material = materials;
    inPackage.Function = functions;
    inPackage.Companies = companies;

    return inPackage;
}

Now your code looks much cleaner and is easier to read and maintain, not only by you but also other people.

You can also assign the database call directly to the according property of inPackage, for example:

var validMaterialIDs = GetValidIDs(materialIDs);
inPackage.GATE_material = DB.Materials.Where(x => validMaterialIDs.Contains(x.MaterialId));
\$\endgroup\$
  • \$\begingroup\$ Comments are not for extended discussion; this conversation has been moved to chat. \$\endgroup\$ – rolfl Dec 4 '14 at 17:06
  • \$\begingroup\$ This is being discussed on meta \$\endgroup\$ – rolfl Dec 4 '14 at 19:28
6
\$\begingroup\$

You've got some odd patterns in your code: first you define a List<T>, which you then don't use until much later, only to use the AddRange method once, and then you assign this list to a property of Package. For instance, the first five lines of your method can be reduced to two easily:

var materialIdsArray = materialIDs.Split(',').Where(x => x != "").Select(int.Parse);
inPackage.GATE_Material = DB.Materials.Where(x => materialIdsArray.Contains(x.MaterialId)).ToList();

Instead of the try...catch logic, use int.TryParse(). Also, use descriptive names and avoid calling things "somethingArray", especially when they're not arrays.

var validFunctionIds = new List<int>();
foreach(var functionId in functionIDs.Split(','))
{
    int validFunctionId;
    if(int.TryParse(functionId, out validFunctionId))
    {
        validFunctionIds.Add(validFunctionId);
    }
}

I don't think companyIDs.IsNullOrWhiteSpace() is valid code unless you've got an extension method somewhere, it should be string.IsNullOrWhiteSpace(companyIDs).

Again multiple lines can be reduced significantly:

inPackage.Function = DB.Function.Where(x => validFunctionIds.Contains(x.FunctionId)).ToList();

The try...catch logic in that second block makes even less sense to me and seems to me to be in the wrong place. Again the code can be reduced significantly:

var validCompanyIds = new List<int>();
foreach(var companyId in companyIDs.Split(','))
{
    int validCompanyId;
    if(int.TryParse(companyId, out validCompanyId))
    {
        validCompanyIds.Add(validCompanyId);
    }
}
inPackage.Companies = DB.Companies.Where(x => validCompanyIds.Contains(x.ID)).ToList();

But now I've obviously copy-pasted methods and simply renamed items, so we need to extract that into a method of its own:

private List<int> SplitAndParse(string ids)
{
    var validIds = new List<int>();
    foreach(var id in ids.Split(','))
    {
        int validId;
        if(int.TryParse(companyId, out validId))
        {
            validIds.Add(validId);
        }
    }
    return validIds;
}

Thus a lot of my code above can be reduced to:

var validFunctionIds = SplitAndParse(functionIDs);
var validCompanyIds = SplitAndParse(companyIDs);

I would even urge you to move that SplitAndParse() outside of this class and parse the arguments you're sending to this method before you call it, so it can become:

private Package StringToCollections(Package inPackage, List<int> materialIds, List<int> functionIds, List<int> companyIds)

I'd change that method name too, but I'm low on inspiration now. I also don't get why you'd treat each case slightly different (i.e. checking if parsing to int doesn't throw an error, checking if the string is null or empty,...): why not treat each parameter the same?

\$\endgroup\$
  • \$\begingroup\$ The problem is that I didn't create this code and I am just trying to fix it. I honestly did my best with the try catch statements but obviously I have a long way to go. In sum, I do want to treat each parameter the same way, but I am not sure how to achieve it. \$\endgroup\$ – Flame_Phoenix Dec 3 '14 at 11:46
  • \$\begingroup\$ @Flame_Phoenix Simply by putting all the logic (check if string isn't null, Split, TryParse, etc.) into a method that you then call, e.g. the SplitAndParse() I wrote but with the additional checks in there. \$\endgroup\$ – BCdotWEB Dec 3 '14 at 11:50
4
\$\begingroup\$

On Empty Strings

In your question, you have a Where checking against an empty string. Strings are, in general, tricky to deal with. They might be null, or they might be empty, or they might have a value. But more than that, the syntax for an empty string is a subset of the syntax for a string literal. It is all but unique in this regard.

""

The example below contains all the characters in the example above:

"Hello world!"

Now, this may seem minor, and to some extent it is. After all, no one when writing code can possibly mistake one for the other.

The problem arises, like so many in this industry, when you consider maintenance. When a maintenance programmer sees an empty string expressed as "", they must try to understand the original programmer's intent. Was this meant to be an empty string? Possibly. How about a zero-width character? You should be flogged if you ever cause that one to be correct, but it could be. What if you were simply writing the code quickly and needed the line to compile while working on something else, and never came back to add the correct value?

So "" can, in fact, mean at least three different things. Some even argue that it can be confused with " ", though that is fairly unlikely.

What can be done?

The simplest solution to this case of ambiguity is to directly signal your goal. If you want an empty string, use string.Empty. It is a few characters longer, but it has a single, unambiguous meaning.

Remember, the most important thing after getting your code working at all is to make it maintainable. If "" is never used when you want an empty string, any usage of it is clearer, because it now has only two possibilities.

In this particular case, it has been suggested that you use string.IsNullOrWhitespace instead. This I fully agree with, as it correctly signals your intent.

Is there anywhere else I need to watch out for this?

Ambiguity is always an issue. Strings represent a rather unique case, because of their syntax.

The closest common case is, surprisingly, with integer literals. Would you expect 010 == 8 to return false? It in fact returns true, because 0-prefixed integer literals are in fact octal.

This is not the kind of thing you want to leave for a maintenance programmer to figure out. Leaving a bunch of octal literals around is an easy way to cause new, confusing bugs during maintenance.

So tread carefully, and try to avoid leaving landmines in your source code. There are many ways to cause problems in the future, and you will thank yourself later on.

\$\endgroup\$
  • \$\begingroup\$ A most interesting reading, worthy of kudos ! \$\endgroup\$ – Flame_Phoenix Dec 5 '14 at 8:41

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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