Is the following service layer correctly implemented with multiple repositories?

I'm new to design patterns and specifically the Service Layer implementation. I need some clarification on where to use Service Layer calls.

Below is an example of what I'm dealing with. I have data from 2 different repositories so I'm not quite certain when/where I'm supposed to call up any additional information that I need to build my Business Model.

I would like for it to do the following:

• Find Car in Repository

• Get Picture from another Repository

• Update Url within Model to be used for Presentation Layer.

What i'm stuck with is I'm not sure if that logic belongs in the Car Model or it should be handled by a Service layer and within the service layer all of this gets combined. Hopefully this isn't too confusing. Below I added what I thought was relevant.

Business Model/Logic:

public class Car
{
    private readonly IFileSystemService _service;
    public Car(IFileSystemService service)
    {
        _service = service;
    }

    public int id { get; set; }
    public string model { get; set; }
    public int year { get; set; }
    public Image GetImageFile(IGetImageService imgService);
}

Within the Service Layer:

public void GetImageFile(int carId)
{
    Image imgfile = _Repository.GetPhysicalFileLocation(carId);
    // ...
}

Repository 1:

public Car GetCar(int carId)
    {
        Car myCar = this.GetCar(carId);
        // ...
    }

Repository 2:

public Car GetPictureForCar(int carId)
    {
        Car myCar = this.GetFileForCar(carId);
        // ...
    }

Business Model/Logic

These are two distinct things: your model and your logic should be implemented in separate classes - the business model defines a Car class, and the business logic consumes it. In other words, I think your Car class is breaking SRP.

The way I see it (could be wrong) is pretty much like this (assuming MVC):

CarService, CarService->ImageService, CarService->CarRepository, ImageService->Image and CarRepository->Car, and a note saying the CarService gives the controller a model that includes an image and a car.">

In other words, CarController only needs to know about a CarService, which gives it a model that contains everything the view needs to know - the CarService "puts the pieces together" so that the business logic "layer" can do its part.

I think having a dependency on IFileSystemService in your Car class makes that class be more than just a model.

Also, in "repository 2":

public Car GetPictureForCar(int carId)

I don't think it makes much sense to return a whole Car from a method called GetPictureForCar - I'd expect that method to return an Image.

This is even more surprising:

Car myCar = this.GetFileForCar(carId);

I'd expect GetFileForCar to return either a File class, or a string with the file's name.

Same here:

Image imgfile = _Repository.GetPhysicalFileLocation(carId);

A method that's called GetPhysicalFileLocation should be returning a string with the file's name. And to follow C# naming conventions, _Repository should be _repository (assuming it's a private field).

StackExchange Code Review Q#48420, answer score: 4