5

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?

  • 1
    Can you provide an exact or an abstract code sample reflecting the current code so we can have a more detailed look at the code in question? – Andy Jun 30 '16 at 10:34
  • Go pick up a copy of Michael Feather's Working Effectively with Legacy Code. That book answers your question superbly. – RubberDuck Jun 30 '16 at 10:35
  • 4
    just wanna point out that data-only classes don't violate OOP and neither do classes with a single method. in fact, MOST classes should have very few methods, although all might not have 1. – sara Jun 30 '16 at 10:36
  • 5
    Sounds you want to change your code by converting SolutionBuilder and NuGetUpdater to methods of your class Solution, because you believe that will make the program more "OOP". But actually, that will typically lead to the opposite - your Solution would get too many responsibilities. What you describe sounds each of the classes already has a single responsibility - better leave it that way. OOP does not mean "any thinkable operation for an object must be a member of that object". – Doc Brown Jun 30 '16 at 10:48
  • 6
    From what you've said, it sounds worryingly like you want to degrade the quality of this app; not improve it. None of your "design flaws" sound like flaws at all and if the classes SolutionBuilder, NuGetUpdater etc all only have one method then, coupled with their names, they sound like they are complying with SRP already. I think you may need to provide more info as it's unclear at the moment as to why you want to make these changes. – David Arno Jun 30 '16 at 10:55
7

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.

  • 2
    Technically, Uncle Bob says that there should only be one person who will request that a class be changed. +1 for pragmatism. – RubberDuck Jun 30 '16 at 22:24
  • 1
    The overengineered pattern is probably impaired with the curse of knowledge mentioned in this answer about IoC containers. I think it could apply here too. – Andy Jul 1 '16 at 14:10

Your Answer

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

Not the answer you're looking for? Browse other questions tagged or ask your own question.