Retrieving a csv header

Question

I have two seperate calls that returns a .csv file. I need to extract a couple of headers from the files depending on which one is called. The headers does not always sit in the same index, and the CourseId name changes to Asset_ID in the one call.

How can I refactor the code to make it more maintainable?

Also, I think the naming is not really good.

public static class CsvHeader
        {
            public struct Index
            {
                public string Email { get; set; }
                public string CompletionDate { get; set; }
                public string CourseId { get; set; }
            }
            public static Index GetCsvHeaderIndex(string[] headers)
            {
                string emailIndex = null;
                string completionDateIndex = null;

                var email = headers.FirstOrDefault(_ => _.Contains("Email"));
                var completionDate = headers.FirstOrDefault(_ => _.Contains("Completion"));
                var courseId = headers.FirstOrDefault(_ => _.Contains("CourseID"));


                if (!string.IsNullOrEmpty(email)) emailIndex = headers.First(_ => _.Contains("Email")).Split('(', ')')[1]; /*3*/
                if (!string.IsNullOrEmpty(completionDate)) completionDateIndex = headers.First(_ => _.Contains("Completion")).Split('(', ')')[1]; /*30*/
                var courseIdIndex = string.IsNullOrEmpty(courseId) ? 
                    headers.First(_ => _.Contains("Asset_ID")).Split('(', ')')[1] : 
                    headers.First(_ => _.Contains("CourseID")).Split('(', ')')[1];

                return new Index
                {
                    Email = emailIndex,
                    CompletionDate = completionDateIndex,
                    CourseId = courseIdIndex
                };
            }
        }

Answer 1

You have some duplicated code here like for instance if email is not null you can get the emailIndex by just splitting the email field. The same is true for completitionDate and courseId.

This

Email = emailIndex,
CompletionDate = completionDateIndex,
CourseId = courseIdIndex  

looks not that good. You should rename the method variables xxxIndex to something better (will show in code) .

I would like to encourage you to use braces {} although they are optional and although you are placing the instruction on the same line as the if. Its intent just gets more clear.

public static Index GetCsvHeaderIndex(string[] headers)
{
    string email = null;
    string completionDate = null;

    var emailHeader = headers.FirstOrDefault(_ => _.Contains("Email"));
    var completionDateHeader = headers.FirstOrDefault(_ => _.Contains("Completion"));
    var courseIdHeader = headers.FirstOrDefault(_ => _.Contains("CourseID"));

    if (!string.IsNullOrEmpty(emailHeader)) { email = emailHeader.Split('(', ')')[1]; /*3*/}
    if (!string.IsNullOrEmpty(completionDateHeader)) { completionDate = completionDateHeader.Split('(', ')')[1]; /*30*/}

    var courseId = string.IsNullOrEmpty(courseIdHeader) ?
        headers.First(_ => _.Contains("Asset_ID")).Split('(', ')')[1] :
        courseIdHeader.Split('(', ')')[1];

    return new Index
    {
        Email = email,
        CompletionDate = completionDate,
        CourseId = courseId
    };
}

Answer 2

• Kudos for creating a struct to strong type the data, but what does Index mean? It's an odd term for course information. I'd look for a better name.

• Also, don't string type all the things. You have a date here, you should store it as a date.

  public string CompletionDate { get; set; }
  

  MSDN article on parsing dates from strings.

• You're duplicating string literals like "Email". Extract constants for these.

• You're splitting on the same character array in several places, but recreating the array for each call. Extract a local variable for ['(', ')']

Answer 3

Mutable structs are a really bad idea. There are loads of reasons on this SO question.

If you still want to do it have a look at what MSDN says

X DO NOT define mutable value types.

So this is not good:

public struct Index
{
    public string Email { get; set; }
    public string CompletionDate { get; set; }
    public string CourseId { get; set; }
}

You could make it a class, or make it immutable. I'd favour making it a class because of this (also in the MSDN guidelines)

In general, structs can be very useful but should only be used for small, single, immutable values that will not be boxed frequently.

Your struct isn't a single value, it's a collection of 3 somewhat related values. It should definitely be a class.

You are also using a convention which I find odd:

headers.FirstOrDefault(_ => _.Contains("Email"));

In several languages, e.g. I think Scala, using underscore implies the parameter can be ignored! That's not the case here and I had an additional couple of seconds of thinking "what?!" because I automatically assumed _ meant you weren't using the parameter. Even without that convention, it's a header so you should call it so:

headers.FirstOrDefault(header => header.Contains("Email"));