CODEWITHSUNDEEP
c#variablesmethods
Greg
Greg

Reputation: 11480

Good or Bad Practice for Return Members

I'm not sure if this would be invalid practice; or good practice. The reason I'm at a loss is should I be utilizing a property instead of a local variable?

My reasoning and goal; was a very basic detection for a local disk drive.

Some things I'd like to point out:

My example:

    public class Drive
    {
        // Variable:
        public string nameOfDrive;

        public Drive()
        {
            // Call Method.
            DriveName();
        }

        public string DriveName()
        {
            DriveInfo [] drives = DriveInfo.GetDrives();
            foreach (DriveInfo d in drives)
            {
                // Verify Valid 'C:' is Present.
                if (d.Name == @"C:")
                {
                    // Set Name:
                    nameOfDrive = d.Name;
                    // Return Result.
                    return d.Name;
                }
            }
            // Exception:
            throw new Exception("Unable to locate the C: Drive... Please map the correct drive.");
        }

    }
    /*
     * The above method and class contains a verification
     * for the 'C:' Drive.  Once the items are validated;
     * it will create a return variable for the 'C:'.  
     * Otherwise it will throw an Exception.
    */

Now this is where I'm unsure of what is better practice. Should I be using a property instead of public string nameOfDrive. Or am I really far off all together- is this not the best way to return a value that can be utilized in other classes? Or is it bad practice to reference directly to the member variable?

Second Example:

    public class Drive
    {
        private string nameOfDrive;
        public string NameOfDrive
        {
            get { return nameOfDrive; }
        }
        public Drive()
        {
            // Call Method.
            DriveName();
        }
        public string DriveName()
        {
            // Obtain Drive Information:
            DriveInfo [] drives = DriveInfo.GetDrives();
            foreach (DriveInfo d in drives)
            {
                // Verify Valid 'C:' is Present.
                if (d.Name == @"C:")
                {
                    // Set Name:
                    nameOfDrive = d.Name;
                    // Return Result.
                    return d.Name;
                }
            }
            // Exception:
            throw new Exception("Unable to locate the C: Drive... Please map the correct drive.");
        }
    }
    /*
     * The above method and class contains a verification
     * for the 'C:' Drive.  Once the items are validated;
     * it will create a return variable for the 'C:'.  
     * Otherwise it will throw an Exception.
    */

So that way it is marked as a Read Only and will ensure that it reads the proper value from the method?

Update:

I appreciate the answers; but why is it better practice?

What makes it a better solution; that is what I'm attempting to understand.

Upvotes: 0

Views: 266

Answers (5)

Earlz
Earlz

Reputation: 63835

Although, it isn't necessarily bad practice, it isn't good either.

For the most part, fields should be used when you have a simple data class(usually with no real code involved, just a way to store some values). If you go beyond that level of complexity, you should usually have your class use properties. A few reasons:

  1. Converting from fields to properties later will break dependencies and require all code using your class to be recompiled
  2. Properties have more fine-grained control. From a quick glance at your use-case, it looks like you should have a getter that will automatically populate and cache the drive letter and make the default setter private so it's read-only
  3. Properties can be virtual. This means it's much easier for people to extend your class beyond what you first imagined possible.

Upvotes: 2

Servy
Servy

Reputation: 203830

You can use the Lazy class to do this. It's specifically designed to solve this exact problem of lazily initializing a value that can take some time to compute. You can give the Lazy object a method which is used to compute the value, the first time the value is requested it uses the function to generate the value, and all subsequent calls just return that first value. It also has the advantage of being thread safe (the function will only ever be called once, no matter how many people request the value before it's been generated, and they all wait until it's computed to return).

public class Drive
{
    private Lazy<string> nameOfDrive = new Lazy<string>(DriveName);

    public string NameOfDrive
    {
        get { return nameOfDrive.Value; }
    }

    private static string DriveName()
    {
        DriveInfo[] drives = DriveInfo.GetDrives();

        foreach (DriveInfo d in drives)
        {
            if (d.Name == @"C:")
                return d.Name;
        }

        throw new Exception("Unable to locate the C: Drive... Please map the correct drive.");
    }
}

Upvotes: 4

ΩmegaMan
ΩmegaMan

Reputation: 31616

I would create a class (or even an extension) to extract the cdrive. Have the consumer throw the error depending on their needs.

By creating a generic method, that allows for the process to be reused depending on the situation and adheres to the use of object oriented principle of isolating concepts into unique objects.

void Main()
{

  if (Drive.AcquireCDrive() == null)
      throw new Exception("Unable to locate the C: Drive... Please map the correct drive.");

}

public class Drive
{
    public static DriveInfo AcquireCDrive()
    {
       return DriveInfo.GetDrives()
                       .OfType<DriveInfo>()
                       .Where (drive => drive.IsReady)
                       .FirstOrDefault( drive => drive.Name.Contains(@"C:"));
    }
}

Upvotes: 1

EkoostikMartin
EkoostikMartin

Reputation: 6911

Probably a better practice would be to use a read only property, but lazy load it.

Note below, I made the DriveName() method private and did not call the DriveName() method in the constructor.

public class Drive
{
    private string nameOfDrive = null;

    public string NameOfDrive
    {
        get 
        {
            if (nameOfDrive == null)
                nameOfDrive = DriveName();
            return nameOfDrive; 
        }
    }

    public Drive()
    {    }

    private string DriveName()
    {
        DriveInfo[] drives = DriveInfo.GetDrives();

        foreach (DriveInfo d in drives)
        {
            if (d.Name == @"C:")            
                return d.Name;
        }

        throw new Exception("Unable to locate the C: Drive... Please map the correct drive.");
    }
}

Upvotes: 0

Dave Zych
Dave Zych

Reputation: 21887

I would use a read only property with a private member variable. This way it's easier to update the class if you ever change the way you look up the drive letter without breaking the calling code.

Why does your DriveName method return anything? Is it used publicly or is it just used to populate your NameOfDrive property? I would make it private and void if it's only used inside the class.

EDIT: Thinking about it, this also seems like an odd way of not only checking for the existence of a drive, but also checking the letter to begin with. Why do you require that the drive letter of the user be C:? The way the user has their machine set up shouldn't matter. They should be able to have their OS drive be Q: if they wanted to, and it shouldn't break your code.

Upvotes: 3

Related Questions