6
\$\begingroup\$

I have the method below but I am going to have more methods similar to this one. They differ in is the last parameters being passed to the method. The first three parameters will always be there and then in the while loop with && conditions, I am going to use the same params that I am passing in.

I am going to need five or six of these methods. Do you think I can write this method in a more reusable way, or do I have to just copy/paste most of it and change the params and condition for each method?

private string FindFullRowForProvider(string id, string lastName, string firstName, string devSystem)
{
    string fullRow = string.Empty;
    StreamReader streamreader = new StreamReader(importSource);
    char[] delimiter = new char[] { '|' };
    int columnLocation = 0;

    string[] columnheaders = streamreader.ReadLine().Split(delimiter);
    foreach (string columnheader in columnheaders)
    {
        columnHeadertoLocation[columnheader.ToUpper()] = columnLocation;
        columnLocation++;
    }

    while (streamreader.Peek() > 0)
    {
        fullRow = streamreader.ReadLine();
        string[] currentRowValues = fullRow.Split(delimiter);

        string _NPI = currentRowValues[columnHeadertoLocation["ID"]];
        string _LName = currentRowValues[columnHeadertoLocation["LNAME"]];
        string _FName = currentRowValues[columnHeadertoLocation["FNAME"]];
        string _devPKCR = currentRowValues[columnHeadertoLocation["DEV_SYSTEM"]];

        if (npi.Trim().ToUpper() == _NPI.Trim().ToUpper()
            && _LName.Trim().ToUpper() == lastName.Trim().ToUpper()
            && _FName.Trim().ToUpper() == firstName.Trim().ToUpper()
            && _dev.Trim().ToUpper() == devSystem.Trim().ToUpper())
        {
            // woot! we found the row of this guy.
            return fullRow;
        }
    }

    return fullRow;
}
\$\endgroup\$
  • 1
    \$\begingroup\$ It would be helpful to know what the alternative method signatures would look like. \$\endgroup\$ – Brian S Oct 21 '14 at 14:25
8
\$\begingroup\$

I'm not a fan of npi.Trim().ToUpper() == _NPI.Trim().ToUpper(), because there is a better way. Use that solution in a method like this and your code will look a lot nicer:

private bool AreIdentical(string a, string b)
{
    return string.Equals(a.Trim(), b.Trim(), StringComparison.CurrentCultureIgnoreCase);
}

(Note that this doesn't catch NULL strings, but neither does your original code.)

I also note that your code as presented here doesn't actually work. The parameter string id is used nowhere and seems to have become npi. Ditto for _devPKCR.

_LName and _FName are IMHO bad names, and _NPI and _devPKCR are even worse. Moreover, they don't follow Microsoft guidelines.

If you're going to return fullRow; anyway, I wouldn't also do it inside the if-block. Just break; out of that one.

EDIT I just notice your code also fails here: fullRow will contain the value of the last line read from the file if there is no valid result.

You don't seem to close the StreamReader. It would be better if you used it in a using block:

using (StreamReader streamreader = new StreamReader(importSource))
{
    // do stuff
}

This review is in addition to previous ones, so other remarks also apply. RubberDuck's suggestion of a Person class to limit the amount of search parameters is a very good one for instance.

EDIT 2: To avoid the code returning the last row read from the file when there's no valid line, you'll need to restructure the logic like this:

private string FindFullRowForProvider([params])
{
    // remove this line: string fullRow = string.Empty;

    while (streamreader.Peek() > 0)
    {
        var fullRow = streamreader.ReadLine();

        if ([code to check if all requirements are met])
        {
            // woot! we found the row of this guy.
            return fullRow;
        }
    }

    return string.Empty;
}
\$\endgroup\$
  • \$\begingroup\$ Thanks for heads up about closing streamreader, will do. About npi, Id naming variables, yeah I just tried to change my actual code to make it less proprietary before posting to internet! \$\endgroup\$ – user1899082 Oct 21 '14 at 15:34
  • \$\begingroup\$ I didn't understand by "just break out of that one" , because I am using "return" in the //woot! section, so it will break out of the while loop as soon as it finds it, right? \$\endgroup\$ – user1899082 Oct 21 '14 at 15:37
  • \$\begingroup\$ Ignore the remark WRT the break, because there's a logical flaw in the code. Check the two EDITs I've added. (Though I am not that keen on doing a return when you're in a using block, but there's no problem if you decide to do it.) \$\endgroup\$ – BCdotWEB Oct 22 '14 at 7:49
11
\$\begingroup\$

What you're looking for is an Optional Argument. When using optional arguments, you have to supply a default value. So, you just have to check for that default value, and switch your control flow appropriately. It would look something like this. (Note I used empty quotes because String.Empty isn't a constant expression.)

private string FindFullRowForProvider(string id, string lastName, string firstName, string devSystem = "")
{
    //maybe some code here

    if (devSystem == String.Empty)
    {
        //handle that case
    }

    //maybe some more code here

}

But... this is Code Review after all, so let's take a look at what you've done.


private string FindFullRowForProvider(string id, string lastName, string firstName, string devSystem)

I'm making an assumption that id is tied to lastName and firstName, but it looks like you're missing a class in your design. Creating a Person class would simplify the signature an awful lot. Be wary of passing lots of strings to a method.

private string FindFullRowForProvider(Person somePerson, string devSystem = "")

I like your use of String.Empty. I really do prefer it over empty quotes. I think it makes for nice readable code. Well done.

string fullRow = string.Empty;

Maybe I'm missing something, but I think declaring a character array for delimiter is overkill here.

char[] delimiter = new char[] { '|' };
int columnLocation = 0;

string[] columnheaders = streamreader.ReadLine().Split(delimiter);

I'm not very good at C#, so there may be an even simpler way, but you could certainly just split it like this.

string[] columnheaders = streamreader.ReadLine().Split(new char[] { '|' });

Nice use of brackets and formatting here.

        if (npi.Trim().ToUpper() == _NPI.Trim().ToUpper()
            && _LName.Trim().ToUpper() == lastName.Trim().ToUpper()
            && _FName.Trim().ToUpper() == firstName.Trim().ToUpper()
            && _dev.Trim().ToUpper() == devSystem.Trim().ToUpper())
        {
            // woot! we found the row of this guy.
            return fullRow;
        }
\$\endgroup\$
  • \$\begingroup\$ It should however be noted that Microsoft recommends not using default parameters: msdn.microsoft.com/en-us/library/ms182135.aspx although there are also arguments for them: stackoverflow.com/questions/3291958/… \$\endgroup\$ – Nick Udell Oct 21 '14 at 13:47
  • \$\begingroup\$ Interesting @NickUdell, but if it's good enough for Jon Skeet... \$\endgroup\$ – RubberDuck Oct 21 '14 at 14:05
  • \$\begingroup\$ Just read through the Microsoft guidelines again, and I should add that they apply to public methods, in this case an optional argument is fine. \$\endgroup\$ – Nick Udell Oct 21 '14 at 14:16
7
\$\begingroup\$

Although 'RubberDuck' has already provided a nice answer, since CodeReview allows also reviewing the code as a whole and not specifically to only the question: here are a few tips. Note that my tips might not be perfect, just trying to help you write better code. :)

Shorter code:

Following code:

char[] delimiter = new char[] { '|' };
int columnLocation = 0;
string[] columnheaders = streamreader.ReadLine().Split(delimiter);

foreach (string columnheader in columnheaders)
{
    columnHeadertoLocation[columnheader.ToUpper()] = columnLocation;
    columnLocation++;
}

can be replaced with following:

var columnheaders = streamreader.ReadLine().Split('|');
var columnLocation = 0;

foreach (var columnheader in columnheaders)
    columnHeadertoLocation[columnheader.ToUpper()] = columnLocation++;

This way your code is shorter and cleaner which makes it easier to read and maintain.

Use the var keyword:

Definition from MSDN:

An implicitly typed local variable is strongly typed just as if you had declared the type yourself, but the compiler determines the type.

This also makes your code cleaner, though it's not mandatory of course.

Close the StreamReader:

You never closed your StreamReader-object, this is bad practice. You can do this in two ways:

1. Try/Catch/Finally

StreamReader sr = null;
try
{
    sr = new StreamReader(filename);
    var txt = sr.ReadToEnd();
    //More code here...
}
finally
{
    if (sr != null)
        sr.Dispose();     
}

You can also implement a catch statement here to handle Exceptions such as IO - exceptions.

2. using statement (better)

If you're using objects that implement the IDisposable() interface, you can always use this statement. It ensures that the Dispose() method is always called, even when an exception occurs.

using (var sr = new StreamReader(filename))
{
    var txt = sr.ReadToEnd();
    //More code here
}

More reading:

\$\endgroup\$

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.