Clean OOP-Design: How to implement single responsibility and no procedural programming

Question

I am currently trying to refactor a piece of C# code that is somewhat procedurally written. I want to make the design clean, object oriented and using classes with single responsibilities.

The code contains a class "Solution", which represents a Visual Studio solution and only contains data. This class is used by other classes, which have the responsibility to build the solution (class SolutionBuilder, interface ISolutionBuilder), update the solution's NuGet packages (class NuGetUpdater, interface INuGetUpdater), and commit the changes to Subversion (class SubversionClient, interface ISubversionClient).

I believe, the current implementation has the following design flaws:

• The class "Solution" only holds data, is no real OOP-class with behaviour
• The other classes are not really OOP-classes either, because they only have one method and are rather procedural
• Code smell: Class SolutionBuilder and NuGetUpdater end with "er"

Concrete code:

public class Solution
{
    public string Name { get; set; }
    public string SolutionFileName { get; set; }
}

public interface ISolutionBuilder
{
    void Build(Solution solution);
}

public interface INuGetUpdater
{
    void Update(Solution solution, string packageSourceUrl, bool safe = true, bool prerelease = false);
}

...

I agree that it is totally okay if a class has only one method, but recently I have read many blogs saying this is bad OOP and rather procedural coding. So my first idea really was to move all those methods into the solution class, but then I thought this would certainly violate the single responsibility principle and also make the class Solution become quite large.

Links to the blog posts I read:
Your Coding Conventions are Hurting You
Don't make objects that end with 'er'

Did I misunderstand the authors of these blog posts? Does anyone have a better idea how to structure this code in a object oriented way? Or is this a case where this is the best way to do it?

Answer 1

I agree that it is totally okay if a class has only one method, but recently I have read many blogs saying this is bad OOP and rather procedural coding.

If a class has only one method, it's usually Execute(), or something equivalent. The question you have to ask yourself is, what are you encapsulating by using a class, if you only have one method? That's why those blogs say you might want to rethink it.

So my first idea really was to move all those methods into the solution class, but then I thought this would certainly violate the single responsibility principle and also make the class Solution become quite large

A "responsibility" is simply an "area of expertise." Classes specialize in something. They might do more than one thing with that something. It's up to you to figure out where that balance is. Bob Martin says that a responsibility is a "reason to change," and that classes should have only one reason to change, ergo one responsibility.

Did I misunderstand the authors of these blog posts?

Yes, you did.

The point of Your Coding Conventions are Hurting You is that we are drowning in architectural principles, and a lot of it is unnecessary.

Sure, you can throw around GOF and SOLID and FactoryFactoryFactory objects all day long. Your house of cards will be beautiful, but how much of it actually does useful work?

Or you can go write some code and get something done.

I've seen this firsthand, on more than one occasion. Armies of developers doing everything right, following all of the latest architectural principles, but writing software that is utterly inflexible, difficult to reason about, and requiring reams of code to evolve.

Meanwhile, a small agile team is writing a new UI for a system using a cutting-edge framework that, while it probably breaks all sorts of architectural rules, allows for flexibility in design, leverages a ton of work done by others, drastically simplifies the coding and reduces time to market. It can do all of these things because it is pragmatic, not dogmatic.

Steve Yegge illustrates it better than I can in his "Kingdom of Nouns" article:

For the lack of a nail,
    throw new HorseshoeNailNotFoundException("no nails!");

For the lack of a horseshoe,
    EquestrianDoctor.getLocalInstance().getHorseDispatcher().shoot();

For the lack of a horse,
    RidersGuild.getRiderNotificationSubscriberList().getBroadcaster().run(
      new BroadcastMessage(StableFactory.getNullHorseInstance()));

For the lack of a rider,
    MessageDeliverySubsystem.getLogger().logDeliveryFailure(
      MessageFactory.getAbstractMessageInstance(
        new MessageMedium(MessageType.VERBAL),
        new MessageTransport(MessageTransportType.MOUNTED_RIDER),
        new MessageSessionDestination(BattleManager.getRoutingInfo(
                                        BattleLocation.NEAREST))),
      MessageFailureReasonCode.UNKNOWN_RIDER_FAILURE);

For the lack of a message,
    ((BattleNotificationSender)
      BattleResourceMediator.getMediatorInstance().getResource(
        BattleParticipant.PROXY_PARTICIPANT,
        BattleResource.BATTLE_NOTIFICATION_SENDER)).sendNotification(
          ((BattleNotificationBuilder)
            (BattleResourceMediator.getMediatorInstance().getResource(
            BattleOrganizer.getBattleParticipant(Battle.Participant.GOOD_GUYS),
            BattleResource.BATTLE_NOTIFICATION_BUILDER))).buildNotification(
              BattleOrganizer.getBattleState(BattleResult.BATTLE_LOST),
              BattleManager.getChainOfCommand().getCommandChainNotifier()));

For the lack of a battle,
    try {
        synchronized(BattleInformationRouterLock.getLockInstance()) {
          BattleInformationRouterLock.getLockInstance().wait();
        }
    } catch (InterruptedException ix) {
      if (BattleSessionManager.getBattleStatus(
           BattleResource.getLocalizedBattleResource(Locale.getDefault()),
           BattleContext.createContext(
             Kingdom.getMasterBattleCoordinatorInstance(
               new TweedleBeetlePuddlePaddleBattle()).populate(
                 RegionManager.getArmpitProvince(Armpit.LEFTMOST)))) ==
          BattleStatus.LOST) {
        if (LOGGER.isLoggable(Level.TOTALLY_SCREWED)) {
          LOGGER.logScrewage(BattleLogger.createBattleLogMessage(
            BattleStatusFormatter.format(BattleStatus.LOST_WAR,
                                         Locale.getDefault())));
        }
      }
    }

For the lack of a war,
    new ServiceExecutionJoinPoint(
      DistributedQueryAnalyzer.forwardQueryResult(
        NotificationSchemaManager.getAbstractSchemaMapper(
          new PublishSubscribeNotificationSchema()).getSchemaProxy().
            executePublishSubscribeQueryPlan(
              NotificationSchema.ALERT,
              new NotificationSchemaPriority(SchemaPriority.MAX_PRIORITY),
              new PublisherMessage(MessageFactory.getAbstractMessage(
                MessageType.WRITTEN,
                new MessageTransport(MessageTransportType.WOUNDED_SURVIVOR),
                new MessageSessionDestination(
                  DestinationManager.getNullDestinationForQueryPlan()))),
              DistributedWarMachine.getPartyRoleManager().getRegisteredParties(
                PartyRoleManager.PARTY_KING ||
                PartyRoleManager.PARTY_GENERAL ||
                PartyRoleManager.PARTY_AMBASSADOR)).getQueryResult(),
        PriorityMessageDispatcher.getPriorityDispatchInstance())).
      waitForService();

All for the lack of a horseshoe nail.

If your head hurts looking at that, well... it hurts mine too.

The point of Don't make objects that end with 'er' is simply that, if your class ends in er, you might be better off putting that functionality into the actual object that your helper class is helping. That's all.

A final note: There is no right or wrong way. There is only the way that best meets your software requirements and business needs. If all that ceremony works because you're a Java shop, the legacy code is all Kingdom of Nouns and everyone understands it, then by all means, continue along that path.

It's not the only path, though.