Two times same code in methode except one little line

Question

In Service.Insert(), I call DeviceRepo.Insert() and in Service.Update(), I call DeviceRepo.Update(). All my other code is exactly the same.

I'm thinking I can make one method that do the same thing update. I want to update and insert when I will insert a device.

The methods respectively insert a new or update an existing (IoT) device into my database with entity framework 6.

How could I refactor the code? You can see I have the same methods twice where only another method is called.

public class DeviceService : IDeviceService
{
 internal IGenericRepo<Device> _deviceRepo;
 public DeviceService()
 {
 _deviceRepo = new DeviceRepo(); // P.S.: DeviceRepo overrides GenericRepo<Device> who implements IGenericRepo<Device>
 }
 public void Insert(Device device, IEnumerable<int> selectedFrameworks, IEnumerable<int> selectedOSs)
 {
 device.Framework = GiveObjects<Framework>(selectedFrameworks, _frameworkRepo);
 device.OS = GiveObjects<OS>(selectedOSs, _osRepo);
 SetStateUnchanged<Framework>(device.Framework);
 SetStateUnchanged<OS>(device.OS);
 _deviceRepo.Insert(device);
 _deviceRepo.SaveChanges();
 }
 public void Update(Device device, IEnumerable<int> selectedFrameworks, IEnumerable<int> selectedOSs)
 {
 device.Framework = GiveObjects<Framework>(selectedFrameworks, _frameworkRepo); 
 device.OS = GiveObjects<OS>(selectedOSs, _osRepo);
 SetStateUnchanged<Framework>(device.Framework);
 SetStateUnchanged<OS>(device.OS);
 _deviceRepo.Update(device);
 _deviceRepo.SaveChanges();
 }
}

GenericRepo<Device>:

public class GenericRepo<TEntity> : IGenericRepo<TEntity> where TEntity : class
{
 internal ApplicationDbContext context;
 internal DbSet<TEntity> dbSet;
 public GenericRepo()
 {
 context = new ApplicationDbContext();
 dbSet = context.Set<TEntity>();
 }
 public virtual TEntity Insert(TEntity entity)
 {
 return dbSet.Add(entity);
 }
 public virtual void Update(TEntity entityToUpdate)
 {
 context.Entry(entityToUpdate).State = EntityState.Modified;
 dbSet.Attach(entityToUpdate);
 }
 public virtual void SaveChanges()
 {
 context.SaveChanges();
 }
}
public class DeviceRepo : GenericRepo<Device>
{ /* not important methods I override for this question */ }

• Phrancis: Please take a moment to edit your question's title and body to say something about what the code does. We all want to improve our code on this site so there is no need to ask for improvements.

Answer 1

I'm hesitant to post this as an answer as I'm not sure how I feel about it from a design perspective. But it's certainly one way to reduce the repetition.

public class DeviceService : IDeviceService
{
 internal IGenericRepo<Device> _deviceRepo;
 public DeviceService()
 {
 _deviceRepo = new DeviceRepo(); // P.S.: DeviceRepo overrides GenericRepo<Device> who implements IGenericRepo<Device>
 }
 public void Insert(Device device, IEnumerable<int> selectedFrameworks, IEnumerable<int> selectedOSs)
 {
 DoXXX(device, selectedFrameworks, selectedOSs, _deviceRepo.Insert);
 }
 public void Update(Device device, IEnumerable<int> selectedFrameworks, IEnumerable<int> selectedOSs)
 {
 DoXXX(device, selectedFrameworks, selectedOSs, _deviceRepo.Update);
 }
 //Obviously call this something that makes sense
 private void DoXXX(Device device, IEnumerable<int> selectedFrameworks, IEnumerable<int> selectedOSs, Action<Device> deviceAction)
 {
 device.Framework = GiveObjects<Framework>(selectedFrameworks, _frameworkRepo); 
 device.OS = GiveObjects<OS>(selectedOSs, _osRepo);
 SetStateUnchanged<Framework>(device.Framework);
 SetStateUnchanged<OS>(device.OS);
 deviceAction(device);
 _deviceRepo.SaveChanges();
 }
}

• H. Pauwelyn: Thanks for your answer, it is good. But I think I've forgot one thing to say, Insert() returns TEntity (in this case is this Device) and Update is a void (See also code in my question)...
• 404: Hmm, hasn't noticed that. You could pass d => _deviceRepo.Insert(d) as the action to make it fit the delegate signature (since you don't appear to care about the return value), though it's starting to feel a little hacky.

Answer 2

I think in this case I would simply make a private method to capture the duplicated code. I probably wouldn't mind about the extra call to the SaveChanges() as it's only one line and removes any need for further complexity on the code.

public void Insert(Device device, IEnumerable<int> selectedFrameworks, IEnumerable<int> selectedOSs)
{
 UpdateDeviceProperties(device, selectedFrameworks, selectedOSs);
 _deviceRepo.Insert(device);
 _deviceRepo.SaveChanges();
}
public void Update(Device device, IEnumerable<int> selectedFrameworks, IEnumerable<int> selectedOSs)
{
 UpdateDeviceProperties(device, selectedFrameworks, selectedOSs);
 _deviceRepo.Update(device);
 _deviceRepo.SaveChanges();
}
private void UpdateDeviceProperties(Device device, IEnumerable<int> selectedFrameworks, IEnumerable<int> selectedOSs) 
{
 device.Framework = GiveObjects<Framework>(selectedFrameworks, _frameworkRepo); 
 device.OS = GiveObjects<OS>(selectedOSs, _osRepo);
 SetStateUnchanged<Framework>(device.Framework);
 SetStateUnchanged<OS>(device.OS);
}

I might also consider returning either a boolean (or some other error result) from the methods to raise awareness of whether the db action actually succeeded to help with raising failures back to the caller of the method.

In the case of Insert it could be as simple as (assuming device has an Id of course) return device.Id > 0;