1
\$\begingroup\$

I have the Repository below, but I have noticed that there is a recurrent pattern try / catch which does not seem to be that clean, is there a better way to achieve or to extract that sort logic elsewhere?

public class FlightRepository : IFlightRepository
{
    private readonly FlightManagerDbContext _context;

    public FlightRepository(FlightManagerDbContext context)
    {
        _context = context;
    }

    public Result CreateFlight(Flight flight)
    {
        try
        {
            var tmp = _context.Flight.FirstOrDefault(f => f.Code == flight.Code);
            if (tmp != null)
                return Result.Conflict($"Flight {flight.Code} already exists");
            _context.Flight.Add(flight);
            _context.SaveChanges();
            return Result.Ok("Flight successfully saved");
        }
        catch (Exception e)
        {
            return Result.Fail(new List<string>() {$"An error occured, {e.Message}"});
        }
    }

    public Result UpdateFlight(Flight flight)
    {
        try
        {
            var tmp = _context.Flight.FirstOrDefault(f => f.Code == flight.Code);
            if (tmp == null)
                return Result.NotFound($"Flight { flight.Code} not found");

            var toUpdate = _context.Flight.First(f => f.Code == flight.Code);
            toUpdate.Distance = flight.Distance;
            toUpdate.DepartureLatitude = flight.DepartureLatitude;
            toUpdate.DepartureLongitude = flight.DepartureLongitude;
            toUpdate.DepartureName = flight.DepartureName;
            toUpdate.DepartureTime = flight.DepartureTime;
            toUpdate.ArrivalLatitude = flight.ArrivalLatitude;
            toUpdate.ArrivalLongitude = flight.ArrivalLongitude;
            toUpdate.ArrivalName = flight.ArrivalName;
            toUpdate.ArrivalTime = flight.ArrivalTime;
            toUpdate.ConsumptionPerKm = flight.ConsumptionPerKm;
            toUpdate.TakeOffEffort = flight.TakeOffEffort;
            toUpdate.FlightTime = flight.FlightTime;
            toUpdate.FuelNeeded = flight.FuelNeeded;

            _context.SaveChanges();

            return  Result.Ok($"Flight {flight.Code} successfully updated");
        }
        catch (Exception e)
        {
            return Result.Fail(new List<string>() {$"An error occured, {e.Message}"});
        }
    }

    public Result<Flight> SelectFlight(string code)
    {
        try
        {
            var flight = _context.Flight.FirstOrDefault(f => f.Code == code);
            if (flight == null)
                return Result<Flight>.NotFound($"Flight {code} not found");
            return  Result<Flight>.Ok(flight);
        }
        catch (Exception e)
        {
            return Result<Flight>.Fail(new List<string>(){$"An error occured, {e.Message}"});
        }
    }

    public Result<List<Flight>> SelectFlights()
    {
        try
        {
            return Result<List<Flight>>.Ok(_context.Flight.Any()
                ? _context.Flight.ToList()
                : new List<Flight>());
        }
        catch (Exception e)
        {
            return Result<List<Flight>>.Fail(new List<string>() {$"An error occured,{e.Message}"});
        }
    }
}
\$\endgroup\$
4
\$\begingroup\$

The repository does not need to take responsibility for the errors - remove them from here. The flight repository should add CRUD operations to Flight Dto and not return a result - remove the result. Validation should be done before calling the methods. You are not an automapper, use a tool. You do not need to invent a bicycle every time you need a new repository, use generics. Additionally, you do not need to repeat Flight in every method name, they exist in the Flight repository you can infer that from context. All error messages should be unique.

For example, implement an interface:

public interface IRepository<T> where T : EntityBase
{
    T GetById(int id);
    IEnumerable<T> List();
    void Add(T entity);
    void Delete(T entity);
    void Edit(T entity);
}

public abstract class EntityBase
{
   public int Id { get; protected set; }
}

For more info: https://deviq.com/repository-pattern/

The model that contains the list of flights should contain the validation rules and validate flights or the model of the Flight should validate itself.

What you get from the database is not a Flight model, but a dto. You should use a translator to create a model from dto. Dto's should not contain any business logic just data. Clear separation of concerns helps is needed for clear code.

\$\endgroup\$
  • 2
    \$\begingroup\$ As for the try-catch, it is fine if you write code like that, this is not the kind of duplication that you need to worry about. Before you start using Rx observables or enterprise level patterns try-catch without failover policy and logging is fine as is. \$\endgroup\$ – Margus Jan 19 at 16:09
  • \$\begingroup\$ not sure then, what kind of layer is taking care of the error then? For now the chain is something like: DB => Repository => Service => Controller => UI (React / Angular). \$\endgroup\$ – Ehouarn Perret Jan 19 at 19:04
  • 1
    \$\begingroup\$ First of all, any DB admin would tell you that unique indexes/constraints/data validation/triggers should be in the database. On the other hand, React / Angular are full MVVM frameworks themselves and can do everything themselves and use an ex: Firebase. Typically service and the controller should do validation. In short - everything except the repository itself. :P \$\endgroup\$ – Margus Jan 20 at 13:06

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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