General

  • License header missing; idea: use LicenseHeaderManager VS add-in (http://visualstudiogallery.msdn.microsoft.com/5647a099-77c9-4a49-91c3-94001828e99e)
  • Add a very short summary comment on every class and interface that explains the purpose of the type.
  • Since the specs were written after the code, the code does not match specs regarding nomenclature (e.g., should be "EnterSponsoredChild" instead of "RegisterChild") and functionality; should we rewrite it or modify it?

Sponsorship.Specs project

  • Why does this have a dedicated solution folder (with only one project)?
  • Is this still relevant?
  • If yes, should we add use case specific specs or do we keep only infrastructure specs (like: every command needs a handler, etc.)?
  • I really find the Machine.Specs syntax abominable :)

Sponsorship.Domain project

  • Contracts.tt
    • Commented fragments should be removed: SecurityDetails, etc.
    • Typos: TelephonNumber => telephoneNumber
    • Change the comment "this is the reference implementation..." to explain what this file is for
    • Do we need to have different commands for changing each informational detail of an entity or would a single command (e.g., "ChangeSponsor") be sufficient to match the CRUD style of the application?
    • Specs mismatch:
      • Nomenclature
      • Everything that's not absolutely required should be nullable (e.g., Child's Birthday, Person's Birthday)
      • Free text item for every entity (specs call it "FreeText")
      • AddAssociatedPerson doesn't really match the AddPerson/AssociatePerson use cases
  • Handles.cs
    • Why not call the interface "ICommandHandler<T>"? The implementation class is called ChildCommandHandler anyway.
    • What's the non-generic Handles interface for?
  • Child.cs
    • Camel casing for firstname, lastname
    • AddAssociatedPerson
  • Person should probably be a domain entity/aggregate root

Sponsorship.Infrastructure project

  • Registries
    • Remove commented Windsor code
    • Why is the EventHandlerRegistry obsolete? Probably because RegisterEventHandlersInBus (in BootStrapper.cs) uses a different mechanism to get the event handlers. But why was that different mechanism implemented? And if that different mechanism is better, can EventHandlerRegistry be removed?
    • EventStoreRegistry: Remove BuildSerializer, it's not used.
  • AggregateFactory.cs
    • The code says "todo" - what exactly do we need to do? We should probably set the ID and restore the snapshot. This means we should change the infrastructure default constructor on the domain objects to take an Id and an IMemento, right?
    • Should we change that constructor to be protected and change the Activator call to support protected members?
  • BootStrapper.cs
    • I don't like that the file holds multiple classes. If they are bound so tightly to BootStrapper, they should be made nested classes. Otherwise, we could move them to separate files.
    • Why is part of RegisterEventHandlersInBus static and part instance-based? Since all of the logic is "stateless", make all of it static (or all of it instance-based).
    • Initialize the static MethodInfo fields in the field initializers, or convert the fields into ordinary local variables (and pass them into the respective methods).
    • RegisterRoutes: The "events", "eventHandlers", etc. all hold Type values, not real event or event handler instances. Therefore, rename to "eventTypes", "eventHandlerTypes", etc. (Leave "injectedEventHandler" as is - it really is a handler instance.)
    • RegisterRoutes: Maybe log a warning for events without handlers.
    • The innermost foreach loop could also use ordinary Reflection instead of the indirect method invocations:
var injectedEventHandler = GetCorrectlyInjectedEventHandler(eventHandler);
var handleMethod = typeof (HandlesEvent<>).MakeGenericType (theEvent).GetMethod ("Handle");
var actionType = typeof (Action<>).MakeGenericType (theEvent);
var action = Delegate.CreateDelegate (actionType, injectedEventHandler, handleMethod, true);
RegisterTheCreatedActionWithTheMessageRouter(messageRouter, theEvent, action);
  • IBus.cs
    • I don't like that the file holds multiple types. Can we move them to separate classes?
    • Is the commented "transactionHandler" code relevant?
    • What's the DelegateAdjuster for? Can't we just write:
handlers.Add(x => handler ((T) x));

Sponsorship.ReadModel project

  • ChildListView.cs
    • I don't like that the file holds multiple types. Can we move them to separate classes?
    • Is ChildListDto a good name for something that isn't a list? Maybe better ChildListViewDto?
  • HandlesEvent.cs
    • Rename to IEventHandler?
  • IReadRepository.cs
    • I don't like that the file holds multiple types. Can we move them to separate classes?

Sponsorship.Ui project

  • Not really reviewed - this is too prototypical and will certainly be rewritten when the actual UI is created.

Last edited Nov 19, 2011 at 9:47 PM by FabianSchmied, version 1

Comments

No comments yet.