Is Switch by definition a Code Smell?

I am fed up of coming across this kind of shite code all the time, whenever I need to fix a bug I need to change 400 things not 1 thing.

   22     public class CrapiolaManager : ICrapiolaManager

   23     {

   24         public void DoSomething(SomeEnumOfTypesWtf someType)

   25         {

   26             if (someType == SomeEnumOfTypesWtf.SomeType1)

   27             {

   28                 //1. Do some work and calculations etc and more crap code...

   29             }

   30             if (someType == SomeEnumOfTypesWtf.SomeType2)

   31             {

   32                 //2. Do some work and calculations etc and more crap code...

   33             }

   34             if (someType == SomeEnumOfTypesWtf.SomeType3)

   35             {

   36                 //3. Do some work and calculations etc and more crap code...

   37             }

   38             if (someType == SomeEnumOfTypesWtf.SomeType4)

   39             {

   40                 //4. Do some work and calculations etc and more crap code...

   41             }

   42             if (someType == SomeEnumOfTypesWtf.SomeType5)

   43             {

   44                 //5. Do some work and calculations etc and more crap code...

   45             }

   46 

   47             throw new InvalidOperationException("Invalid type passed");

   48         }

   49     }

It’s absolutely nonsense!! We are using an object orientated language ffs! Can we not see the impact of the problems this kind of code leads too?

• Lots of bugs/issues!!!
• New stories take longer to do!!!
• Its duplication of code!
• It’s not easy to Test!!
• Not easy to read when you have to follow code through so many layers, you want a piece of logic in ONE place… Domain Model Entity?
• It’s a problem when a new type appears! Somehow the person working on the new type needs to magically pull out of his ass some knowledge to understand where all the switch’s and if’s are in the code to alter them to add a new case! Hmmm… I wonder how many of these if’s and switch’s  get missed when adding a new type? Oooo let me guess… ALL OF THEM! It’s completely unmaintainable rubbish!

Having an interface on that manager is also completely pointless!
What’s it there for, are you going to create another manager from the same interface(highly unlikely)? Is it there for the Dependency Injection instead? Then I am sure you can inject it without an interface. Even if you’re going down this road asking yourself these questions, you may realise 'ooo' the code is in the wrong place and we shouldn’t have Managers and Helpers littered everywhere! And this is another post all in itself (Say NO to managers/Helpers)

Throwing an exception if the type doesn’t exists is also crap!
I mean seriously if you were testing this and for some reason you never hit this if/switch statement after adding your new type, that exception will never be thrown!! Oh yes and I guarantee it will be thrown when a user uses it!

• BIG Problem!
• Support costs!
• Customer trust levels!
• The list carries on…

 This code is a time bomb waiting to happen!

A suggestion to resolve this problem! (Not too much detail here)

(This is the basics of the start of the fix but should pretty much be an answer for this issue). Hmmm… how about use an interface or a base class (depending on what you require) and derive these types from that interface/base class to force implementation of the method that does the work in them dumb ass if’s and switch’s! It takes 2 minutes to write, and NOT 2 days to fix bugs when adding new types!

   22     public interface IDoSomething

   23     {

   24         void DoSomething();

   25     }

   26 

   27     public class DoSomehting1 : IDoSomething

   28     {

   29         public void DoSomething()

   30         {

   31             //1. Do some work and calculations etc and more crap code...

   32         }

   33     }

   34     public class DoSomehting2 : IDoSomething

   35     {

   36         public void DoSomething()

   37         {

   38             //2. Do some work and calculations etc and more crap code...

   39         }

   40     }

   41     public class DoSomehting3 : IDoSomething

   42     {

   43         public void DoSomething()

   44         {

   45             //3. Do some work and calculations etc and more crap code...

   46         }

   47     }

   48 ...

   49     public class DoTheWork

   50     {

   51         public void Work(IDoSomething doSomething)

   52         {

   53             //The object does the work not some external object that would be a nightmare to maintain.

   54             doSomething.DoSomething();

   55 

   56             //The above is called instead of doing...

   57             //ICrapiolaManager someManager;

   58             //someManager.DoSomething();

   59         }

   60     }

It’s not all about finishing as fast as possible and just letting your fingers typing your brain farts for 100% of the day, surprisingly it’s a very high amount of THINKING! The chances are when you come to the solution in your head after thinking around all the issues, scenarios and solutions to the problem you will find you will hardly type any code!

Here is a link I found very quickly (I am not sure who it is by) on the issue there is a lot of info here and was hoping to find something by the main man himself Martin Fowler who we should all know and love! It pretty much goes over a lot of issues with using a switch (or ifs in our case from this code, it’s the same thing) in an OO language. IMO we should be able to turn the ‘switch’ and ‘if’ statements OFF in the compiler and to be only allowed to turn them on where we need them, more than likely in the Domain Model Entities only!

I can’t stress how important something as simple as this is, It’s a simple piece of code and if thought about for at least 30 seconds, the problem could be solved much more elegantly and safely. If you’re unsure on something don’t hesitate to ask the question to a colleague. I don’t have all the answers but I can see some of them. Just by asking the question sometimes you actually answer it yourself without hearing an answer (I do that a LOT).

If this doesn’t seem to be an important issue to you and want to continue to switch and if on types I am sure there are plenty of job opportunities out there as a farmer! Let us developers write robust code and not fix someone else's problems all the time.

Nhibernate, Unit Of Work, Repository and StructureMap

This could be a long blog...

Well I have spent some time recently trying to understand how to tie all this lot together and I think I have finally cracked it in its simplest form, I am hoping at least. This is my first stab at it and I am already thinking of changing it slightly, but I want to show what I did first.
If there is anything you don't like on here or want to question please come and comment as I would love to get some feedback on what people thought of this. Problematic, see problems with it or even helped to understand it better.

NHibernate
So first of all I modelled a simple domain, just an account user and mapped this to NHibernate to do its thing with the db. Understood then that the NHibernate's SessionFactory should be once per application as its very expensive to create and that ISession is what to use every time you want to make a single or group of changes to the db, or to just retrieve anything.

Code that I Wanted/Needed
So the following are the general things I used and set up simply as possible for now.

Repository
First off I read up loads of articles on creating a repository to wrap up simple NHibernate commands. This could be extended in the future to provide a more advanced implementation. Some would argue not to have the repository at all and use the NHibernate calls directly. I like the idea of keeping the repository implementation as I am hoping it decouples NHibernate from the application (don't know why I will ever need to not use NHibernate yet but its there). I would like to be able to create as many of these as I would like although at the moment I am unsure why I would need more than 1 per thread. This will contain a UnitOfWork.

    5     class Repository : IRepository

    6     {

    7         readonly IUnitOfWork _unitOfWork;

    8 

    9         public Repository(IUnitOfWork unitOfWork)

   10         {

   11             _unitOfWork = unitOfWork;

   12         }

   13 

   14         #region IRepository<AccountUser> Members

   15 

   16         public T GetById<T>(Guid id)

   17         {

   18             return _unitOfWork.CurrentSession.Get<T>(id);

   19         }

   20 

   21         public void Add<T>(T entity)

   22         {

   23             _unitOfWork.CurrentSession.SaveOrUpdate(entity);

   24         }

   25 

   26         public void Remove<T>(T entity)

   27         {

   28             _unitOfWork.CurrentSession.Delete(entity);

   29         }

   30 

   31         #endregion

   32     }

UnitOfWork
This implements IDisposable and is what I used to manage the transaction of the NHibernate session. I would like it to dispose of the session once its been commited or rolledback, it just has a simple commit and rollback and manages to close the session. The session and transaction is created in the constructor here, so the UnitOfWork is ready to be commited or rolledback. The more I am typing here the more I am feeling this class is clunky around the edges of cleaning up. I am not sure if I need to rollback the transaction by default in the dispose if its active already, or whether the Session manages maintains that on a dispose of itself? If anyone knows let me know :-)

    6     public class UnitOfWork : IUnitOfWork

    7     {

    8         private readonly ISessionFactory _sessionFactory;

    9         private ISession _currentSession;

   10         private readonly ITransaction _currentTransaction;

   11 

   12         public UnitOfWork(ISessionFactory sessionFactory)

   13         {

   14             _sessionFactory = sessionFactory;

   15             _currentTransaction = CurrentSession.BeginTransaction();

   16         }

   17 

   18         #region IUnitOfWork Members

   19 

   20         public ISession CurrentSession

   21         {

   22             get { return _currentSession ?? (_currentSession = _sessionFactory.OpenSession()); }

   23         }

   24 

   25         protected ITransaction BeginTransaction()

   26         {

   27             return _currentSession.BeginTransaction();

   28         }

   29 

   30         public void Commit()

   31         {

   32             if (_currentTransaction.IsActive)

   33                 _currentTransaction.Commit();

   34         }

   35         public void Rollback()

   36         {

   37             if (_currentTransaction.IsActive)

   38                 _currentTransaction.Rollback();

   39         }

   40 

   41         #endregion

   42 

   43         #region IDisposable Members

   44 

   45         public void Dispose()

   46         {

   47             if (_currentSession == null) return;

   48 

   49             _currentSession.Dispose();

   50             _currentSession = null;

   51         }

   52 

   53         #endregion

   54     }

As you can probably see there is no creation of the interfaces SessionFactory or UnitOfWork in repository and this is where I have used StructureMap to help out.

StructureMap
I went the Ninject route initially as it had a cool looking site and seemed quite nice to use also. I managed to achieve what I wanted with it and it wasn't difficult at all but I wont show that here. I researched further into which DI/IOC to use and I kind of had a good feel for what people were saying about StructureMap so I changed to use StructureMap instead.
OK so I figured I needed

• 1 SessionFactory per application running.
• 1 UnitOfWork per thread running.
• No limit to the number of Repositories. (not sure why this can't be 1 instance per thread also but I don't think there is harm in having more than 1 as it will use the same UnitOfWork if its in the same thread.)

    9    public static class ContainerBootstrapper

   10     {

   11         public static void BootstrapStructureMap()

   12         {

   13             ObjectFactory.Initialize(x =>

   14             {

   15                 x.ForSingletonOf<ISessionFactory>().UseSpecial(y =>

   16                     y.ConstructedBy(context =>

   17                     {

   18                         var nhConfig = Fluently

   19                          .Configure()

   20                          .Database(MsSqlConfiguration

   21                                     .MsSql2008

   22                                     .ConnectionString(connstr =>

   23                                         connstr.FromConnectionStringWithKey("db")))

   24                          .Mappings(mappings => mappings

   25                                                 .FluentMappings

   26                                                 .AddFromAssemblyOf<AccountUser>())

   27                          .BuildConfiguration();

   28 

   29                         return nhConfig.BuildSessionFactory();

   30                     }));

   31                 x.For<IUnitOfWork>().HybridHttpOrThreadLocalScoped().Use<UnitOfWork>();

   32                 x.For<IRepository>().Use<Repository>();

   33             });

   34         }

   35     }

So in here, I am setting up the

• 1 single SessionFactory ever for the application. The x.ForSingletonOf<ISessionFactory> will sort this out. It uses the Fluent style configuration of the sessionFactory using the database connection and mappings from the domain model set up.
• Per thread UnitOfWork, we want this as we may have more than 1 save, update and delete on the same transaction, so when its injected into the repository its the same UnitOfWork that is used before the repository has begun to be used, I will show a test later on with this.
• Repository.

A Couple of Tests
I will try and show a couple of tests that I set up for this, and show how it could be used from the outside. I hope my tests aren't too bad, I know I find it difficult to write a good test, come slate them let me know what you would do, naming them something different? anything? I need to learn more about tests.

Test to write data to the database was successful.

   67         [TestMethod]

   68         public void Can_SaveSomeUserDataViaStructureMap()

   69         {

   70             ContainerBootstrapper.BootstrapStructureMap();

   71 

   72             using (var uoW = ObjectFactory.GetInstance<IUnitOfWork>())

   73             {

   74                 var repo = ObjectFactory.GetInstance<IRepository>();

   75                 var newUser = new AccountUser

   76                                   {

   77                                       Name = "Test",

   78                                       Email = "Email Test",

   79                                       Password = "Password Test"

   80                                   };

   81                 var newUser2 = new AccountUser

   82                                    {

   83                                        Name = "Test2",

   84                                        Email = "Email Test",

   85                                        Password = "Password Test"

   86                                    };

   87                 repo.Add(newUser);

   88                 repo.Add(newUser2);

   89 

   90                 uoW.Commit();

   91 

   92                 //Clear the session so its forced to go to the database and not pull from the cache

   93                 uoW.CurrentSession.Clear();

   94 

   95                 var returnedAccountUser = repo.GetById<AccountUser>(newUser.Id);

   96                 Assert.AreEqual("Test", returnedAccountUser.Name, "Failed to get user from database.");

   97             }

   98         }

so this test was to tell me that I could write more than 1 thing to the database at the same time on the same UnitOfWork. I did use the nhibernate profiler to spy on a little of what was going on here, which may I add is superb, easy to use, understand and set up. Going back to the test though, the UnitOfWork and StructureMap did their job nicely and only created one of them. One UnitOfWork was created as part of the 'using' and the one that was injected into the Repository creation (on line 74 of the test) was the same one. Superb! The data was added, the UnitOfWork commited and job done. I cleared the session at this point to force nhibernate to fetch the data from the database again rather than retrieve it from its cache.

Test to prove that there was a different UnitOfWork created when created in another thread.

  110        [TestMethod]

  111         public void Must_HaveNewSessionPerUoWPerThread()

  112         {

  113             ContainerBootstrapper.BootstrapStructureMap();

  114             //Create this instance on this thread.

  115             var uoW = ObjectFactory.GetInstance<IUnitOfWork>();

  116             IUnitOfWork uoW2;

  117 

  118             var thread = new Thread(x =>

  119             {

  120                 uoW2 = ObjectFactory.GetInstance<IUnitOfWork>();

  121                 Assert.AreNotSame(uoW, uoW2);

  122                 Assert.AreNotSame(uoW.CurrentSession, uoW2.CurrentSession);

  123             });

  124             thread.Start();

  125         }

with this test I am thinking along the lines I could use the UnitOfWork per HttpRequest using MVC some where along the lines, creating the UnitofWork in ActionExecting maybe and the commit/rollback on what ever the opposite one is. When I get to working that out I will post that next I think.

Changes I would like to make for an MVC web application.
I will get round to bloging what I did for this, but for short I am considering removing the UnitOfWork and Repository all together and let StructureMap create the Session and inject it into the controller constructors when they are created, but my only hesitation in this is that it will couple NHibernate to the controllers as they will need an ISession injected into them and also need to manage the Transaction.

Just a quick fix, just a quick fix, boom! Game Over Man!... Game Over...

What are these quick fixes? Are they the start of the end? Or is it already too late, as when you've found there are a few quick fixes, does that mean there are thousands of them in already, more than likely yes!

What causes them?

• Friday afternoon? 
• No time as other things are important to get done? But what is more important than the current fix you are doing, if you have to rush it to get onto something else, then don't do it!
• Because it doesn't break any unit tests? Are then the tests written correctly, if they are stopping quality being writing?
• Is it the lack of pride and care in what one is doing, or any thought for one's colleagues as one is not going to be around long enough when these fixes come back to bite you in the ar!*e!

All these questions and no answers, not sure I can give any answers yet, but if I find some I will surely post back here.

Is this what agile is? Do a fix in the first place you can see the easiest place? Or is it to refactor code into the correct places to fix the issue, maybe its neither?

What do the sum of these quick fixes add up too? I tell you what! A bloody mess of looking at random, undocumented code here, there and everywhere, slow down of progress, and skewed estimations. Code spread, where these fixes are not structured in the correct layer! More than likely more fixes will be required to fix the fixes! Superb! That's some special mess!

Are developers being continuously squeezed to write code to be “finished” rather than “completed with quality”? Or do developers wear blinkers and only see progress as a happy product owner? Is there a way to prove this?

Time is always squeezed and quality drops! This will inevitably get worse over time ‘Mr Anderson’…

Well commented code, we hopefully don’t need too most of the time (well you shouldn't so much) as the code is readable (isn't it, you good coders you), but the more quick fixes (I suspect highly uncommented)… ...who knows what randomness is being produced.

I guess there is a time line where the quick fix is fine, for a small, completed, un-evolving project. Are there such projects, really?

I recently came across an interesting article and I haven't looked into it yet, but could be very interesting. Convincing team members that something is a good idea.

Calling all Devs, Calling all Managers (if that's what you can call them)

Seriously! The guys in my links need to be followed by all Devs and Managers especially. I've only just started doing this reading myself, but quickly realising that these guys are well worth reading.

Just read them, they are not big articles. They make a lot of sense and are very experienced people, so why wouldn't we want to listen to them absolutely confuses me.

Its probably wise to be spending some time, reading at least one article a day, and/or learning one new feature of a piece of software that we use every day/week. Visual Studio or Resharper or maybe some software framework you are using, automapper, nhibernate, etc. I am trying to do this but not religiously yet. I will get there.

I have also just been recently told that  looking at somebody elses source code once a week, also helps. I have not done this yet but I think I am going to start. I'm going to start with Jimmy Bogard's Automapper. Take a look around see how he does things, learn any ideas from it hopefully and also how he writes his code and his unit tests, especially his unit tests as I'm finding this difficult to understand how these are meant to be written at the moment.

If I learn something I will report back on my blog some where hopefully.
I think I hear you ask where the hell do you get the time to do any of this, well don't worry I am also asking myself this question, and now I am writing a blog too! I still want to do all the above and more...

I'll be adding more links to articles I find in here and to 'My Links' also.

Hope you find something useful from this, or if you have any more useful links, let me know and I'll have a read and add them :-)

Some links...

Stand Up Meetings
http://codebetter.com/jameskovacs/2011/03/28/stand-up-meetings-are-about-more-than-standing/

Martin Fowler
http://martinfowler.com/bliki/TradableQualityHypothesis.html
http://martinfowler.com/bliki/TechnicalDebt.html

42, 1, 0, temp1, fred, squid, WTF!

STOP IT! STOP IT! STOP IT!

Seriously what do these numbers mean to anyone else looking at your code, or in fact to yourself when you come back in a week or two (in my case a day)?

Damn magic numbers and random variable names, awesome tactics if you want to spend time looking at the same code day in and day out trying to figure out what they mean, especially when the same number means two different things in the database or even three. Genius!

Use an enum damn it! Give those numbers meaning! Tell me what they are! Tell yourself what they are! Rather than me/you hunting through the damn code to try and work out what they mean!

Name your variables something useful, and your methods too!

not so nice way...

   15         public void CheckEngineManagement()

   16         {

   17             //Call something to get the data of the engine.

   18             var engineData = GetEngineData();

   19 

   20             if (engineData.State > 0)

   21             {

   22                 if (engineData.State == 1)

   23                 {

   24                     //Do some engine checks for oil,

   25                     //seatbelt checks etc, blah blah,

   26                     //or something else

   27                 }

   28             }

   29         }

I seem to be stuck in the car world at the moment, I will have to think of some better examples.
So there are the different types of EngineData coming back from the database, all in the same table with a column to state the different type. Maybe bad maybe not, but I am trying to find an example.

I get my engine data back for a particular car, ferrari engineData, superb, then I do a check on the State if it's less than zero don't do anything, if it's 1 then do something. IF IT'S 1, IF IT'S 0 WTF does that mean? The door is open, ignition is on, car started, what!? Especially for different engine data 1 could mean something else and it probably does when written magically!

nicer way hopefully...

    8     public enum CarState

    9     {

   10         CarLocked,

   11         PassengerOpen,

   12         DriverDoorOpen,

   13         KeyInIgnition,

   14     }

   23         public void CheckEngineManagement()

   24         {

   25             //Call something to get the data of the engine.

   26             var engineData = GetEngineData();

   27 

   28             if (engineData.State != CarState.CarLocked)

   29             {

   30                 if (engineData.State == CarState.DriverDoorOpen)

   31                 {

   32                     //Do some engine checks for oil,

   33                     //seatbelt checks etc, blah blah,

   34                     //or something else

   35                 }

   36             }

   37         }

Ignore the nasty if nesting, I will have to think of a better example, but I can't at the moment I am fuming!
I have taken away all the nasty numbers, now you can read that the car is locked or the driver door is open and see what its trying to do and why its doing its checks for oil, seatbelt checks etc. not having to spend a larger amount of time understanding the whole code to work out what the numbers are and why they are there!

Sooner or later those ifs would have ended up all over the place nested like crazy with a mix of the engine data type amongst it all no doubt, can you imagine trying to read all that lot with just numbers?

Give me a slap for nesting if's like that, probably could have used a switch or probably even something else. Or suggest a better idea for my demonstrations. Or just a slap anyway. I am just trying to get some of the simpler things across at the moment.