Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

How can I simplify this code?

  if (directory.Exists)
  {
    smallFileNames = directory.GetFiles("*.csv").Select(i => i.FullName).ToList();
    if(smallFileNames.Count == 0)
      smallFileNames = DivideIntoFiles(fileName);  
  }
  else
    smallFileNames = DivideIntoFiles(fileName);

I confused by identical lines...

share|improve this question

3 Answers 3

up vote 12 down vote accepted
List<string> smallFileNames = null;

if (directory.Exists)
{
    smallFileNames = directory.GetFiles("*.csv").Select(i => i.FullName).ToList();
}

if (smallFileNames == null || smallFileNames.Count == 0)
    smallFileNames = DivideIntoFiles(fileName);
share|improve this answer
    
Yes, thank you. Sometimes simple decisions don't strike me. –  Chepene Nov 20 '12 at 13:02

Jumping in after the horse has bolted with a very minor different approach (using Any rather than Count and null).

IEnumerable<string> GetSmallFileNames(Directory directory, string fileName, string filter) 
{
   var smallFileNames = new List<string>();

   if (directory.Exists)
   {
       smallFileNames = directory.GetFiles(filter).Select(i => i.FullName);           
   }

   return smallFileNames.Any() ? smallFileNames : DivideIntoFiles(fileName);
}

Then used such as

IEnumerable<string> smallFileNames = GetSmallFileNames(directory, fileName, "*.csv");
share|improve this answer
    
Isn't Count faster than Any()? Maybe that wasn't your point though. –  Anders Holmström Nov 23 '12 at 14:52
    
Yes it probably is. But I would think the speed is actually so small that in this case it's irrelevant. Also people often mix .Count with .Count() which are subtly different. So .Any() just removes that confusion.... –  dreza Nov 23 '12 at 18:39

...and you could simplify the second conditional by initializing smallFileNames :

List<string> smallFileNames = new List<string>

if (directory.Exists)
{
    smallFileNames = directory.GetFiles("*.csv").Select(i => i.FullName).ToList();
}

if (smallFileNames.Count == 0)
    smallFileNames = DivideIntoFiles(fileName);

However, the whole logic seems unclear to me: there seems to be a hidden relation between directory and fileName. I would consider reviewing the surrounding code also.

share|improve this answer
    
You could also do something like this: var smallFileNames = directory.Exists ? directory.GetFiles("*.csv").Select(i => i.FullName).ToList() : new List<string>(); –  Jeff Vanzella Nov 20 '12 at 19:15
    
But it would always create a list, even when not needed. –  Amiram Korach Nov 20 '12 at 20:23
2  
I'm a big fan of always creating a list. Takes out the if (list == null) checks every time you want to use it. An empty list is just that, empty, so an iteration or check if there are values will fail if there are any. –  Jeff Vanzella Nov 20 '12 at 22:08
    
hidden relation between directory and fileName Thank you. I think it's good idea. –  Chepene Nov 21 '12 at 6:40

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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