Liz Douglass

Posts Tagged ‘Refactoring

Refactoring using an Effect Sketch

with one comment

Today I worked on my own rather than pairing. What made the day even more unusual was that there were no other developers in the room – oddly enough lots of people have taken leave at the same time.

I spent a good part of the day refactoring the one class. This refactoring was the last chunk of work for a story that Silvio and I paired on for a couple of days last week. This morning the code looked a bit like this:

@Component
public class FooStrategyModelFactory {

    private final MemberRepository memberRepository;
    private final BarRepository barRepository;
    private final FooStrategyRepository fooRepository;
    private final BarViewFactory barViewFactory;
    private final JsonConverter jsonConverter;

    @Autowired
    public FooStrategyModelFactory(MemberRepository memberRepository,
                                   BarRepository barRepository,
                                   FooStrategyRepository fooRepository,
                                   BarViewFactory barViewFactory,
                                   JsonConverter jsonConverter) {

        this.memberRepository = memberRepository;
        this.barRepository = barRepository;
        this.fooRepository = fooRepository;
        this.barViewFactory = barViewFactory;
        this.jsonConverter = jsonConverter;
    }

    public ModelMap createViewModel(UserAccount userAccount, Context context, HttpServletRequest request,
                                    List changeData) {

        ModelMap model = new ModelMap();

        Member member = memberRepository.getMember(userAccount, context);

        List availableBars = getAvailableBarsForMember(context, changeData);

        List availableWidgetBars = Lists.select(availableBars, new WidgetBarMatcher());

        List BarViews = barViewFactory.createBarViews(availableWidgetBars, member.getAccountType());

        model.addAttribute("Bars", jsonConverter.toJson(BarViews));

        addBarStrategiesToModel(userAccount, context, request, model, member, availableBars);

        return model;
    }

    private void addBarStrategiesToModel(UserAccount userAccount, Context context,
                                         HttpServletRequest request,
                                         ModelMap model,
                                         Member member,
                                         List availableBars) {

        List strategies = getCurrentStrategiesForMember(userAccount, context, availableBars, request);

        List strategyViews = barViewFactory.createBarStrategyViews(
                strategies, availableBars, member.getAccountType());

        model.addAttribute("cashFlowStrategyItems", jsonConverter.toJson(strategyViews));
    }

    protected List getAvailableBarsForMember(Context context, List changeData) {
        List thing = barRepository.getThingsForMember(context);

        List memberBars = Lists.map(things, new Function() {
            public Bar apply(Thing thing) {
                return thing.getBar();
            }
        });

        List newlyAddedChangeData = Lists.select(changeData, new NewlyAddedChangeDataMatcher());
        for (ChangeData changeDataItem : newlyAddedChangeData) {
            memberBars.add(barRepository.getBarByBarNumber(changeDataItem.getBarNumber()));
        }

        List newChangeData = Lists.select(changeData, new NewChangeDataMatcher());

        return Lists.reject(memberBars, new OtherChangeDataMatcher(newChangeData));
    }

    protected List getCurrentStrategiesForMember(UserAccount userAccount, Context context,
                                                              List Bars, HttpServletRequest request) {

        String json = (String) WebUtils.getSessionAttribute(request, UpdatedBarStrategy.SESSION_KEY);
        if (StringUtils.isNotEmpty(json)) {
            List strategies = jsonConverter.fromJsonArray(json, UpdatedBarStrategy.class);
            return Lists.map(strategies, new BarStrategyFactory(Bars));
        }
        return fooRepository.load(userAccount, context);
    }
}

Hmmm, where to begin?….. I decided to try to tackle this class by drawing an effect sketch in my notebook. This idea is detailed by Michael Feathers in his book Working Effectively With Legacy Code. We’ve been reading this book at book club and have had several group discussions about the approach. On my diagram I drew nodes for each of the class fields as well as the methods, plus the arguments that are passed into createViewModel. I ended up with loads of lines criss-crossing the page connecting them. I won’t even attempt to reproduce what I wound up with here. I did clarify a few things by doing this though:

  • The factory was doing lots of data retrieval and manipulation as well as creating the ModelMap.
  • The getAvailableBarsForMember method was assembling query information from several sources. It had protected scope and there were three unit tests in place that were testing this method.
  • The getCurrentStrategiesForMember method was also querying information from multiple sources. This was the only method that used the fooRepository. It also had a protected scope and a number of dedicated unit tests.
  • The addBarStrategiesToModel method was extracted from the createViewModel method during an earlier refactoring. At one point there was also an addInvestmentsToView method that had since been in-lined. We were left with an inconsistent abstraction level in the createViewModel method.

I refactored this class in a few steps. First up I moved the getAvailableBarsForMember method into its own class called the FooBarRepository. I also kept the method on FooStrategyModelFactory as a wrapper so that all my unit tests would still compile and run. Then I moved the three unit tests for this logic into a test class for my new FooBarRepository.

It took me a little while to come up with a name for this class because I debated over whether this was a service or a repository. I ended up going with a repository name because it does retrieve domain objects.

@Component
public class FooBarRepository {
    private BarRepository barRepository;

    @Autowired
    public FooBarRepository(BarRepository BarRepository) {
        this.barRepository = BarRepository;
    }

    protected List getAvailableBarsForMember(Context context, List changeData) {
        List thing = barRepository.getThingsForMember(context);

        List memberBars = Lists.map(things, new Function() {
            public Bar apply(Thing thing) {
                return thing.getBar();
            }
        });

        List newlyAddedChangeData = Lists.select(changeData, new NewlyAddedChangeDataMatcher());
        for (ChangeData changeDataItem : newlyAddedChangeData) {
            memberBars.add(barRepository.getBarByBarNumber(reweightDataItem.getBarNumber()));
        }

        List newChangeData = Lists.select(changeData, new ChangeDataMatcher());

        return Lists.reject(memberBars, new OtherChangeDataMatcher(newChangeData));
    }

    public List getAvailableWidgetBarsForMember(List availableBarsForMember) {
        return Lists.select(availableBarsForMember, new WidgetBarMatcher());
    }
}

I did something similar with the getCurrentStrategiesForMember method. I moved it into a class called FooBarStrategyRepository and moved the relevant unit tests from the FooStrategyModelFactory test class.

@Component
public class FooBarStrategyRepository {
    private JsonConverter jsonConverter;
    private BarRepository barRepository;
    private FooStrategyRepository fooRepository;

    @Autowired
    public FooBarStrategyRepository(JsonConverter jsonConverter,
                                    BarRepository BarRepository,
                                    FooStrategyRepository fooRepository) {
        this.jsonConverter = jsonConverter;
        this.barRepository = BarRepository;
        this.fooRepository = fooRepository;
    }

    protected List getBarStrategiesForMember(UserAccount userAccount, Context context,
                                                          List candidateBars,
                                                          HttpServletRequest request) {

        String json = (String) WebUtils.getSessionAttribute(request, UpdatedBarStrategy.SESSION_KEY);
        if (StringUtils.isNotEmpty(json)) {
            List strategies = jsonConverter.fromJsonArray(json, UpdatedBarStrategy.class);
            return Lists.map(strategies, new BarStrategyFactory(candidateBars, barRepository));
        }
        return fooRepository.load(userAccount, context);
    }
}

My final step was to assess what was left in the FooStrategyModelFactory. I inlined the addBarStrategiesToModel method and then noticed that I really had two themes:
– Gathering and manipulation of data
– Creating and populating a model using that data

I decided to extract out the model creation and population into a separate method. This is the how FooStrategyModelFactory ended up looking:

@Component
public class FooStrategyModelFactory {

    private final MemberRepository memberRepository;
    private final BarViewFactory barViewFactory;
    private final JsonConverter jsonConverter;
    private FooBarRepository fooBarRepository;
    private FooBarStrategyRepository fooStrategyRepository;

    @Autowired
    public FooStrategyModelFactory(MemberRepository memberRepository,
                                   BarViewFactory barViewFactory,
                                   JsonConverter jsonConverter,
                                   FooBarRepository fooBarRepository,
                                   FooBarStrategyRepository fooStrategyRepository) {

        this.memberRepository = memberRepository;
        this.barViewFactory = barViewFactory;
        this.jsonConverter = jsonConverter;
        this.fooBarRepository = fooBarRepository;
        this.fooStrategyRepository = fooStrategyRepository;
    }

    public ModelMap createViewModel(OnlineUserAccount userAccount, Context context, HttpServletRequest request,
                                    List changeData) {

        Member member = memberRepository.getMember(userAccount, context);

        List availableBars = fooBarRepository.getAvailableBarsForMember(context, changeData);

        List availableWidgetBars = fooBarRepository.getAvailableWidgetBarsForMember(availableBars);

        List strategies = fooStrategyRepository.getBarStrategiesForMember(userAccount, context, availableBars, request);

        return createModel(member.getAccountType(), availableBars, availableWidgetBars, strategies);
    }

    private ModelMap createModel(AccountType membersAccountType, List availableBars,
                                 List availableWidgetBars, List membersFooBarStrategies) {

        ModelMap model = new ModelMap();

        List BarViews = barViewFactory.createBarViews(availableWidgetBars, membersAccountType);

        model.addAttribute("Bars", jsonConverter.toJson(BarViews));

        List strategyViews = barViewFactory.createBarStrategyViews(membersFooBarStrategies,
                availableBars, membersAccountType);

        model.addAttribute("cashFlowStrategyItems", jsonConverter.toJson(strategyViews));

        return model;
    }

}

I think the code is now cleaner than it was at the beginning of the day, although I’m sure there could be more done to it. The effect sketch did help and I think I would use it again, perhaps even when pairing.

(Please note that WordPress removed all the Java generics ‘tags’ from the code in this post)

Advertisements

Written by lizdouglass

December 7, 2009 at 7:23 am

Posted in Uncategorized

Tagged with ,