Simplifying logic to avoid duplicate error messages

Question

So far I have the following:

// Gets all the drives 
DriveInfo[] allDrives = DriveInfo.GetDrives();

// checks if any CD-Rom exists in the drives
var cdRomExists = allDrives.Any(x => x.DriveType == DriveType.CDRom);

// Get all the cd roms
var cdRoms = allDrives.Where(x=>x.DriveType==DriveType.CDRom);

if (cdRomExists.Equals(true))
{
    // Loop through the cd roms collection
    foreach(var cdRom in cdRoms)
    {
        Console.WriteLine("Drive {0}", cdRom.Name);
        Console.WriteLine("  File type: {0}", cdRom.DriveType);

        if (cdRom.IsReady == true)
        {
            if (cdRom.DriveType == DriveType.CDRom)
            {
                DirectoryInfo di = new DirectoryInfo(cdRom.RootDirectory.Name);

                var file = di.GetFiles("*.csv", SearchOption.AllDirectories).FirstOrDefault();

                if (file == null)
                {
                    errorwindow.Message = LanguageResources.Resource.File_Not_Found;
                    dialogService.ShowDialog(LanguageResources.Resource.Error, errorWindow);
                }
                else
                {
                    foreach (FileInfo info in di.GetFiles("*.csv", SearchOption.AllDirectories))
                    {
                        Debug.Print(info.FullName);
                        ImportCSV(info.FullName);
                        break;      // only looking for the first one
                    }
                }
            }
        }
        else if (cdRom.IsReady == false)
        {
            errorwindow.Message = LanguageResources.Resource.CDRom_Not_Ready;
            dialogService.ShowDialog(LanguageResources.Resource.Error, errorWindow);              
        }
    }
}
else
{
    errorwindow.Message = LanguageResources.Resource.CDRom_Error;
    dialogService.ShowDialog(LanguageResources.Resource.Error, errorWindow);
}

The problem with the following is, an error message pops up twice in a row to indicate if there is no CD-ROM in the drive because my computer contains both a DVD and blu ray Drive. If there is a CD Rom that contains CSV file, it gets successfully imported but another message pops up because of the foreach loop that runs to the blu ray drive and pops up.

I only want to display one error message for each of these situations: -If there is no CD Rom that is ready and contains csv in the drive -If the CD Rom drive does not contain csv

I think my logic is too convoluted and I need help adjusting my logic statements.

You might find it easier if you adapt the original query to only pull back drives that are Ready. You also don't need both the `Any()` and `Where()` calls; if there are no such drives, then `Where()` will return a sequence of length 0, which you can either test for beforehand, or just iterate over the sequence, as nothing would be done to an empty sequence. — dyson, Jul 23 '14 at 12:04

score 0 · Answer 1 · answered Jul 23 '14 at 12:25

You just need to keep track of whether at least one of the drives worked for you. If none of them did, then you want to output the error message. There's also some other stuff you could do (no need to do the Any/Where, no need to do .Equals(true), etc. And more specifically, no need to continually check if its the right kind of drive. The cdRoms collection will only contain drives with the right type because that's what you specify in your Where clause.

// Gets all the drives 
DriveInfo[] allDrives = DriveInfo.GetDrives();

// Get all the cd roms
var cdRoms = allDrives.Where(x=>x.DriveType==DriveType.CDRom);

if (cdRoms.Count() > 0)
{
    bool found = false;
    // Loop through the cd roms collection
    foreach(var cdRom in cdRoms)
    {
        Console.WriteLine("Drive {0}", cdRom.Name);
        Console.WriteLine("  File type: {0}", cdRom.DriveType);

        if (cdRom.IsReady == true)
        {
            DirectoryInfo di = new DirectoryInfo(cdRom.RootDirectory.Name);

            var file = di.GetFiles("*.csv", SearchOption.AllDirectories).FirstOrDefault();

            if (file == null)
            {
                errorwindow.Message = LanguageResources.Resource.File_Not_Found;
                dialogService.ShowDialog(LanguageResources.Resource.Error, errorWindow);
            }
            else
            {
                foreach (FileInfo info in di.GetFiles("*.csv", SearchOption.AllDirectories))
                {
                    Debug.Print(info.FullName);
                    ImportCSV(info.FullName);
                    found = true;
                    break;      // only looking for the first one
                }
            }
        }
        else
        {
            Debug.Print(string.Format("Drive {0} is not ready", cdRom.Name));

        }
    }
    if (!found)
    {
        errorwindow.Message = LanguageResources.Resource.CDRom_Not_Ready;
        dialogService.ShowDialog(LanguageResources.Resource.Error, errorWindow);              
    }
}
else
{
    errorwindow.Message = LanguageResources.Resource.CDRom_Error;
    dialogService.ShowDialog(LanguageResources.Resource.Error, errorWindow);
}

score 0 · Answer 2 · answered Jul 23 '14 at 12:28

Your code can be rewritten to following:

var cdRoms = allDrives.Where(x => x.DriveType == DriveType.CDRom && x.IsReady);

if (cdRoms.Any())
{
    foreach(var cdRom in cdRoms)
    {
        Console.WriteLine("Drive {0}", cdRom.Name);
        Console.WriteLine("  File type: {0}", cdRom.DriveType);

        var di = new DirectoryInfo(cdRom.RootDirectory.Name);
        var file = di.GetFiles("*.csv", SearchOption.AllDirectories).FirstOrDefault();

        if (file == null)
        {
            errorwindow.Message = LanguageResources.Resource.File_Not_Found;
            dialogService.ShowDialog(LanguageResources.Resource.Error, errorWindow);
        }
        else
        {
            foreach (var info in di.GetFiles("*.csv", SearchOption.AllDirectories))
            {
                Debug.Print(info.FullName);
                ImportCSV(info.FullName);
                break;
            }
        }
    }
}
else
{
    errorwindow.Message = LanguageResources.Resource.CDRom_Error;
    dialogService.ShowDialog(LanguageResources.Resource.Error, errorWindow);
}

Changes:

No need to use Where and Any like you did, filter the CD-Rom drives from all the drives and see if any exist
Only select drives that are CD-Rom drives and are ready
Use var where possible, let the compiler do the job

score 0 · Answer 3 · answered Jul 23 '14 at 12:32

This is my approach to your problem:

bool anyCdrom = false;
bool anyReady = false;
bool fileFound = false;

// Loop through the cd roms collection
foreach(var cdRom in  DriveInfo.GetDrives().Where(drive => drive.DriveType == DriveType.CDRom))
{
    anyCdrom = true;
    Console.WriteLine("Drive {0}", cdRom.Name);
    Console.WriteLine("  File type: {0}", cdRom.DriveType);   
    if (cdRom.IsReady) // You may want to put in into the intial where
    {
        anyReady = true;        
        foreach (string file in Directory.EnumerateFiles(cdRom.RootDirectory.Name, "*.csv", SearchOption.AllDirectories))
        {
            fileFound = true;
            Debug.Print(file);
            ImportCSV(file);
            break;      // only looking for the first one
        }
        if(fileFound)
            break;                                      
    }       
}

if(!anyCdrom)
{
    errorwindow.Message = LanguageResources.Resource.CDRom_Error;
    dialogService.ShowDialog(LanguageResources.Resource.Error, errorWindow);
}   
else if(!anyReady)
{
    errorwindow.Message = LanguageResources.Resource.CDRom_Not_Ready;
    dialogService.ShowDialog(LanguageResources.Resource.Error, errorWindow);                     
}
else if(!fileFound)
{
    errorwindow.Message = LanguageResources.Resource.File_Not_Found;
    dialogService.ShowDialog(LanguageResources.Resource.Error, errorWindow);
}

It only prints an error when:

there is no cdrom
there is no cdrom ready
there is no csv file in any ready cdrom

Simplifying logic to avoid duplicate error messages

3 Answers3