12
\$\begingroup\$

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
                };
            }
        }
\$\endgroup\$
10
\$\begingroup\$

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
    };
}
| improve this answer | |
\$\endgroup\$
12
\$\begingroup\$
  • 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 ['(', ')']

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ I was under the impression that the struct contains "column" heading name of the CSV file, so that CompletionDate is the column name, not an actual date. \$\endgroup\$ – hangy Dec 17 '15 at 10:32
  • \$\begingroup\$ You know what, you could be right about that @hangy. I guess since the method took in an array of strings, I assumed it was a misnomer. \$\endgroup\$ – RubberDuck Dec 17 '15 at 10:33
8
\$\begingroup\$

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"));
| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ +1 for the underscore, this is the first thing that stood out to me as a Haskell programmer. \$\endgroup\$ – Erik Ambrož Dec 17 '15 at 21:08
  • \$\begingroup\$ I only use the underscore, when I don't care about the parameter. \$\endgroup\$ – R4nc1d Dec 17 '15 at 21:44
  • 1
    \$\begingroup\$ @R4nc1d but if you use it, isn't that caring for the parameter? I would only use _ to denote it isn't used at all. \$\endgroup\$ – Kroltan Dec 18 '15 at 2:52

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.