7
\$\begingroup\$

Is there any possibility of refactoring this code?

In class A:

public List<Item> GetItems() {
  var result = new List<Item>();
  foreach(var item in repo.GetItems1()) {
    var x = repo.GetOtherItems1(item.Id, "param1", param2); // this part is different
    if (x.Value > 5)
      result.Add(x);
  }  

  return result;
}

In class B:

public List<Item> GetItems() {
  var result = new List<Item>();
  foreach(var item in repo.GetItems2()) {
    var x = repo.GetOtherItems2(param1, param2, item.Id); // this part is different
    if (x.Value > 5)
      result.Add(x);
  }  

  return result;
}

I tried to use template method, but due to other params in GetOtherItemsX(...), is it now possible?

\$\endgroup\$

migrated from stackoverflow.com Dec 12 '11 at 10:20

This question came from our site for professional and enthusiast programmers.

  • \$\begingroup\$ Is it the same repository type you are work with in both classes? Otherwise it doesn't make much sense to try and re-work. \$\endgroup\$ – WestDiscGolf Dec 12 '11 at 11:04
11
\$\begingroup\$

Well, you could receive a delegate for getting the elements - that is, to make the action that is different:

public List<Item> GetItems(Func<Item, Repository> getOtherItems) {
  if( getOtherItems == null ) {
    throw new ArgumentException();
  }
  var result = new List<Item>();
  foreach(var item in repo.GetItems2()) {
    var x = getOtherItems();
    if (x.Value > 5)
      result.Add(x);
  }  

  return result;
}

And then you could call it like so:

// In class A
this.GetItems( repo => repo.GetOtherItems1(item.Id, "param1", param2) );

// In class B
this.GetItems( repo => repo.GetOtherItems2(param1, param2, item.Id) );

But I notice that you also use repo.GetItems1() in A and repo.GetItems2() in B.
So the difference you pointed out is not the only one.

With this in mind, I would advise rethinking your class structure. These two could inherit from the same base class, and merely extend it. (Or it could be polymorphism, or...)

Maybe:

public interface IMyList<Item>
{
    List<int> GetItems();
    // If it returns a single item, the name should NOT be pluralized!!
    Item GetOtherItems();
}

Or maybe an abstract class?


edit: read @JoeGeeky 's comment on the performance impact of using delegates; this may become relevant if the delegate is used on a very intensive cycle or under high loads.

\$\endgroup\$
  • \$\begingroup\$ I like the idea of a base abstract class which has the GetItems method and then the classes which implement it can pass in the desired Func<>. \$\endgroup\$ – WestDiscGolf Dec 12 '11 at 12:58
  • \$\begingroup\$ These are not bad ideas but will come with a performance penalty... You didn't mention this as an issue, but it's worth considering. \$\endgroup\$ – JoeGeeky Dec 17 '11 at 23:11
  • \$\begingroup\$ @JoeGeeky What comes with performance penalty, using the delegate? Would this penalty be addressed by instead deriving from an interface or abstract class? \$\endgroup\$ – ANeves Dec 19 '11 at 10:04
  • 1
    \$\begingroup\$ Invoking a method (especially an inlined method) is substantially cheaper then invoking a delegate chain with one subscribed member. By way of example... invoking an inlined instance method costs ~0.2ns while invoking a delegate costs ~41ns. The numbers will vary depending on your hardware but the relative difference will essentially remain the same. Func<> and Action<> invokations have very similar costs. If this is occuring in tight loops or high loads, this can start to add up. \$\endgroup\$ – JoeGeeky Dec 19 '11 at 10:17
5
\$\begingroup\$

I would make a couple suggestions:

  • Consider making the return type of your GetItems methods IEnumerable<Item>, ICollection<Item>, or IList<Item>, depending on how you intend to use the result of your function. This will make refactoring later easier.
  • Consider renaming your repo.GetOtherItemsN methods. Their names suggest that multiple items are being returned rather than a single item, but they appear to be returning single items. (ANeves mentions this in the comments for his interface)
  • Consider updating the signature of both your repo.GetOtherItemsN methods to take in parameters in a similar order. It seems strange that one takes in Id, then param1 (as a string), then param2, while the other takes in param1 (unknown type), then param2, then Id. Id should probably come first.
  • Consider using a constant/enum value in place of 5 within your if (x.Value > 5) statements. If both classes A and B have a common base class (suggested, given the other answers), I would put it there.
  • Building on ANeves answer, I would also pass in an IEnumerable<Item> parameter to the GetItems function, since you use repo.GetItems1 versus repo.GetItems2 in the two classes and you do not do anything more involved than enumerating the collection.

If you are a LINQ-fan, you could also transform this to a LINQ statement:

public IEnumerable<Item> GetItems(IEnumerable<Items> source, Func<Item, Repository> getOtherItems) 
{
    if (getOtherItems == null) 
    {
        throw new ArgumentException();
    }
    var result = from item in source
                 let x = getOtherItems()
                 where x.Value > MIN_VAL
                 select x;
}

If you need it evaluated early rather than lazy-loaded, you just need to toss on a .ToList () or .ToArray() before returning.

\$\endgroup\$
0
\$\begingroup\$

After taking Dan and the others suggestions for cleaning up the repository interface, you could also have some benefit from using a common expression for a Linq where statement.

[TestMethod]
public void CanUseCommonWhereExpression()
{
    var repo = SetupRepo();

    Assert.AreEqual(1, GetItems1(repo).Where(ValueMoreThan(5)).Count());
    Assert.AreEqual(2, GetItems2(repo).Where(ValueMoreThan(5)).Count());
    Assert.AreEqual(
        2,
        GetItems2(repo).AsQueryable().Where(QueryableValueMoreThan(5)).Count());
}

// Fine for in-memory linq
private Func<Item, bool> ValueMoreThan(int value)
{
    return i => i.Value > value;
}

// Also fine for database query translation
private Expression<Func<Item, bool>> QueryableValueMoreThan(int value)
{
    return i => i.Value > value;
}

private static IEnumerable<Item> GetItems2(IRepository repo)
{
    return repo.GetItems2().SelectMany(i => repo.GetOtherItems2(i.Id, 0));
}

private static IEnumerable<Item> GetItems1(IRepository repo)
{
    return repo.GetItems1().SelectMany(i => repo.GetOtherItems1(0, i.Id));
}

private static IRepository SetupRepo()
{
    var repo = MockRepository.GenerateMock<IRepository>();
    repo.Stub(r => r.GetItems1()).Return(new[] { new Item { Id = 1, Value = 3 } });
    repo.Stub(
        r => r.GetOtherItems1(0, 1)).Return(new[] { new Item { Id = 2, Value = 3 },
        new  Item { Id = 3, Value = 6 } });
    repo.Stub(r => r.GetItems2()).Return(new[] { new Item { Id = 4, Value = 3 } });
    repo.Stub(
        r => r.GetOtherItems2(4, 0)).Return(new[] { new Item { Id = 5, Value = 6 },
        new Item { Id = 6, Value = 7 } });
    return repo;
}

public interface IRepository
{
    IEnumerable<Item> GetItems1();
    IEnumerable<Item> GetOtherItems1(int foo, int id);
    IEnumerable<Item> GetItems2();
    IEnumerable<Item> GetOtherItems2(int id, int bar);
}

public class Item
{
    public int Id { get; set; }
    public int Value { get; set; }
}
\$\endgroup\$
  • \$\begingroup\$ Welcome to the Code Review, which is part of the Stack Exchange network. A network wide convention is not to add greetings or footers in your posts. Please refrain from doing so in the future. Thanks. \$\endgroup\$ – Steven Jeuris Dec 13 '11 at 13:39
0
\$\begingroup\$

Create a class that holds your parameters and instead of passing individual parameters, pass in the class object. something like:

class MyClass
{

    public MyClass(){}

    public int ItemId { get; set;} 
    public string ItemParam1 {get; set;}
    ....
}
\$\endgroup\$
  • 7
    \$\begingroup\$ This could be made cleaner by using auto properties. \$\endgroup\$ – WestDiscGolf Dec 12 '11 at 11:22

Your Answer

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