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.

Should the following GetPhysicalFileLocation() method in the Car class have its own Service Layer?

public class Car
{
    public int id;
    public string model;
    public int year;
    public IList<Pic> lstCarPics;
}

Then we have the respective class:

public class Pic
{
    public string filename;
    public string fileLocation;
    public string GetPhysicalFileLocation()
    {
        string LocalDirectory = "~\\CarFiles\\";
        DirectoryInfo diPath = new 
            DirectoryInfo(HttpContext.Current.Server
                         .MapPath(LocalDirectory + car.id);
    }
}

Seeing that this method and others will access physical file locations throughout the application, would something like this justify having it's own FileService?

I'm having trouble understanding when methods like this should fall under it's own service layer vs leaving it in its class.

share|improve this question
add comment

2 Answers 2

up vote 4 down vote accepted

As @Vogel612 hinted, a class shouldn't expose its fields publicly - this:

public class Car 
{
    public int Id { get; set; }

    // ...
}

Is equivalent to this:

public class Car
{
    private int _id;
    public int Id 
    { 
        get { return _id; }
        set { _id = value; }
    }

    // ...
}

As you can see, encapsulation is easily achieved with auto-properties - don't expose your fields!


Method GetPhysicalFileLocation has dependencies on the file system and on an HttpContext, so you definitely want that to be abstracted away, so you can write a test that uses that code and doesn't actually hit the file system.

I'd move it to some IFileSystemService implementation:

public interface IFileSystemService
{
    string GetPhysicalFileLocation(int carId);

    // ...
}

And then make a class that implements it:

public class FileSystemService : IFileSystemService
{
    // load this value from application settings?
    private readonly string _localDirectory = "~\\CarFiles\\";

    public string GetPhysicalFileLocation(int carId)
    {
        var diPath = new DirectoryInfo(HttpContext.Current
                                                  .Server.MapPath(_localDirectory + carId);
    }
}

And now you can have client code like this:

public class MyClass
{
    private readonly IFileSystemService _service;

    public MyClass(IFileSystemService service)
    {
        _service = service;
    }

    public void DoSomething(int carId)
    {
        var fileName = _service.GetPhysicalFileLocation(carId);
        // ...
    }
}

This way you can inject a "fake"/"mock" implementation for IFileSystemService and test DoSomething without hitting the file system or depending on an HttpContext; the key being that depending on the file system and on an HttpContext, is an implementation detail that the client code doesn't need to know about.

share|improve this answer
    
Ok. How would this get "glued" back up? So then would I have a GetPhysicalFileLocation method within the Pic class that calls the IFileService? –  TheTruthIsInCode Apr 25 at 15:56
1  
I'd make a separate class to implement the interface, and have whoever needs to GetPhysicalFileLocation call the interface's method (which would take a fileName parameter). –  Mat's Mug Apr 25 at 15:59
    
Ok I'm lost in the code-woods now :( Can you create a quick sample using my code above how this would work? That's the part I'm not getting. Thanks! –  TheTruthIsInCode Apr 25 at 16:11
    
@TheTruthIsInCode sorry that was confusing - edited with [hopefully] less confusing details ;) –  Mat's Mug Apr 25 at 17:46
    
Hyper Clear!!! Thanks Mat!! –  TheTruthIsInCode Apr 25 at 18:28
add comment

The convention for public fields and properties in c# is PascalCasing. This would make the following:

public class Car
{
    public int Id; //{ get; set; } this makes it a property.
    public string Model;
    public int Year; 
    public IList<Pic> LstCarPics;
}

public class Pic
{
    public string FileName;
    public string FileLocation;
    //...
}

Furthermore there is (from the information you posted) no real necessity to shorten the classname to Pic. Why not just use Picture?

share|improve this answer
add comment

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.