» Duplicate Code and Ceremony in Java » Managed Chaos by Naresh Jain

Duplicate Code and Ceremony in Java

How would you kill this duplication in a strongly typed, static language like Java?

private int calculateAveragePreviousPercentageComplete() {
    int result = 0;
    for (StudentActivityByAlbum activity : activities)
        result += activity.getPreviousPercentageCompleted();
    return result / activities.size();
}
 
private int calculateAverageCurrentPercentageComplete() {
    int result = 0;
    for (StudentActivityByAlbum activity : activities)
        result += activity.getPercentageCompleted();
    return result / activities.size();
}
 
private int calculateAverageProgressPercentage() {
    int result = 0;
    for (StudentActivityByAlbum activity : activities)
        result += activity.getProgressPercentage();
    return result / activities.size();
}

Here is my horrible solution:

private int calculateAveragePreviousPercentageComplete() {
    return new Average(activities) {
        public int value(StudentActivityByAlbum activity) {
            return activity.getPreviousPercentageCompleted();
        }
    }.result;
}
 
private int calculateAverageCurrentPercentageComplete() {
    return new Average(activities) {
        public int value(StudentActivityByAlbum activity) {
            return activity.getPercentageCompleted();
        }
    }.result;
}
 
private int calculateAverageProgressPercentage() {
    return new Average(activities) {
        public int value(StudentActivityByAlbum activity) {
            return activity.getProgressPercentage();
        }
    }.result;
}
 
private static abstract class Average {
    public int result;
 
    public Average(List<StudentActivityByAlbum> activities) {
        int total = 0;
        for (StudentActivityByAlbum activity : activities)
            total += value(activity);
        result = total / activities.size();
    }
 
    protected abstract int value(StudentActivityByAlbum activity);
}

if this were Ruby

@activities.inject(0.0){ |total, activity| total + activity.previous_percentage_completed? } / @activities.size
@activities.inject(0.0){ |total, activity| total + activity.percentage_completed? } / @activities.size
@activities.inject(0.0){ |total, activity| total + activity.progress_percentage? } / @activities.size

or even something more kewler

average_of :previous_percentage_completed?
average_of :percentage_completed?
average_of :progress_percentage?
 
def average_of(message)
	@activities.inject(0.0){ |total, activity| total + activity.send message } / @activities.size
end

This entry was posted on Thursday, July 21st, 2011 at 5:07 PM and is filed under Agile, Code Smells, Java, Programming, Programming Languages. You can follow any responses to this entry through the RSS 2.0 feed. You can skip to the end and leave a response. Pinging is currently not allowed.

Mark Stijnman

I’d use generics or templates. I’m more familiar with C++, so here’s a working possible solution:

struct A
{
  int result() const { return 0; }
};
struct B
{
  B(int i): m_i(i) {}
  int someOtherResult() const { return m_i; }
private:
  int m_i;
};
struct C
{
  C(double c): m_val(c) {}
  double yetAnotherResult() const { return m_val; }
private:
  double m_val;
};

template 
ResultType avg(const std::list& l, ResultType (T::*ValueFunction)() const)
{
  ResultType sum = 0;
  for (std::list::const_iterator it = l.begin(); it != l.end(); ++it)
  {
    sum += ((*it).*(ValueFunction))();
  }
  return sum / l.size();
}

BOOST_AUTO_TEST_CASE(genericsTest)
{
  std::list a; a.push_back(A()); a.push_back(A());
  std::list b; b.push_back(B(2)); b.push_back(B(4));
  std::list c; c.push_back(C(4.5)); c.push_back(C(5.5));
  BOOST_CHECK_EQUAL(avg(a, &A::result), 0);
  BOOST_CHECK_EQUAL(avg(b, &B::someOtherResult), 3);
  BOOST_CHECK_EQUAL(avg(c, &C::yetAnotherResult), 5.0);
}

If your language of choice doesn’t support member function pointers or something equivalent, you can always use small functors to extract the value to be averaged. Other solutions involving lambda functions etc. might be possible too.

Mark Stijnman

Sorry about the messed up markup – I forgot that “pre” doesn’t prevent tags from being picked up. Allow me to try again (feel free to edit or delete comments to save screen space):

struct A
{
  int result() const { return 0; }


};
struct B
{
  B(int i): m_i(i) {}


  int someOtherResult() const { return m_i; }


private:
  int m_i;
};
struct C
{


  C(double c): m_val(c) {}


  double yetAnotherResult() const { return m_val; }


private:
  double m_val;
};

template <typename ResultType, typename T>


ResultType avg(const std::list<T>& l, ResultType (T::*ValueFunction)() const)


{
  ResultType sum = 0;
  for (std::list<T>::const_iterator it = l.begin(); it != l.end(); ++it)


  {
    sum += ((*it).*(ValueFunction))();


  }
  return sum / l.size();
}


BOOST_AUTO_TEST_CASE(genericsTest)
{
  std::list<A> a; a.push_back(A()); a.push_back(A());


  std::list<B> b; b.push_back(B(2)); b.push_back(B(4));


  std::list<C> c; c.push_back(C(4.5)); c.push_back(C(5.5));


  BOOST_CHECK_EQUAL(avg<int>(a, &A::result), 0);


  BOOST_CHECK_EQUAL(avg<int>(b, &B::someOtherResult), 3);


  BOOST_CHECK_EQUAL(avg<double>(c, &C::yetAnotherResult), 5.0);


}

Mark Stijnman

Ugh, that’s even worse. Please remove these last few comments, sorry…

Steve Freeman

- use a collection library, such as google’s, to do the collecting (more cycles, but a more direct mapping to the problem)
- consider doing all the averages together so there’s only one loop
http://twitter.com/gclaramunt Gabriel Claramunt

Ok, I’ll bite,
It has nothing to do with strong static typing. In Scala you can have

def average_of( f:activities =>Float)= activities.foldLeft(0.0)( _+f(_)) / activities.size

average_of(_.previousPercentageComplete)
average_of(_.percentageComplete)
average_of(_.progressPercentage)

Or generalized in the collection:
def average_of[T](xs:List[T], f:T =>Float)= xs.foldLeft(0.0)( _+f(_)) / xs.size

And then (here you need a little more type annotations):
average_of (activities, {a:StudentActivityByAlbum=>s.previousPercentageComplete})
…etc…

It can be even more concise (and safe) in Haskell or ML
- http://blogs.agilefaqs.com Naresh Jain
  
  I guess I should have not mentioned (“Static Language” like Java.) I agree its not a static language limitation, its a Java limitation as my little suggests.
http://twitter.com/easyangel Oleg Ilyenko

I agree with Gabriel. Here is other solution in strongly typed, static language Scala:

avg(activities map (_.previousPercentageCompleted)) avg(activities map (_.percentageCompleted)) avg(activities map (_.progressPercentage))
def avg(list: List[Int]) = list reduceOption (_ + _) map (_ / list.size) getOrElse 0

I find it a little bit simpler than Gabriel’s. I also noticed, that all other solution don’t handle empty list – they all will divide by zero if list is empty. My solution uses reduce with Option so it properly handles empty lists and returns 0.
http://profiles.google.com/tkmagesh77 Magesh K

Not sure if it is a “static” language problem… Though C# is also a strongly typed, static language, a little functional programming capabilities sprinkled in the current version of the language has helped in these scenarios…

For example, in C# the ceremony can be avoided using LINQ (and lambda expressions, of course), as given below:

double averagePreviousPercentageCompleted = activities.Average(activity => activity.GetPreviousPercentageCompleted());
double averageCurrentPercentageCompleted = activities.Average(activity => activity.GetPercentageCompleted());
double averageProgressPercentage = activities.Average(activity => activity.GetProgressPercentage());
http://profiles.google.com/tkmagesh77 Magesh K

Not sure if it is a “static” language problem… Though C# is also a strongly typed, static language, a little functional programming capabilities sprinkled in the current version of the language has helped in these scenarios…

For example, in C# the ceremony can be avoided using LINQ (and lambda expressions, of course), as given below:
double averagePreviousPercentageCompleted = activities.Average(activity => activity.GetPreviousPercentageCompleted()); double averageCurrentPercentageCompleted = activities.Average(activity => activity.GetPercentageCompleted()); double averageProgressPercentage = activities.Average(activity => activity.GetProgressPercentage());
Anonymous

Hi Naresh,

here is a functional solution using http://jedi.codehaus.org/ : https://gist.github.com/1100862
Anonymous

Removed any remaining duplication, simplified things a bit, and added some sugar so that the code to sum and average a given field of all activities becomes: addAll(collect( activities, fieldGetter)) / activities.size;

Here is a gist of the code: https://gist.github.com/1101512