search

HTBox / allReady

This repo contains the code for allReady, an open-source solution focused on increasing awareness, efficiency and impact of preparedness campaigns as they are delivered by humanitarian and disaster response organizations in local communities.
http://www.htbox.org/projects/allready
MIT License
891 stars 627 forks source link

Tech Debt (Placeholder) #730

Open MisterJames opened 8 years ago

MisterJames commented 8 years ago

Things we need to be thinking about/expanding on:

  1. Unifying the signup/confirmation process (we can always elevate to admin with claims through some other process)
  2. Getting a templating process in place for emails/SMS such that templates and related language are defined external to the source code (but preferably under source control)
  3. Ensuring users can manage notification preferences (SMS, email, digest, DND)
  4. Pushing the code base to use a unified messaging service/command in such a way that the caller doesn't need to know how the user prefers to be contacted.
mgmccarthy commented 8 years ago

big fan of numbers 3 and 4.

stevejgordon commented 8 years ago

Adding one item to the list for discussion and planning:

  1. Domain driven design and single responsibility. We're starting to get some quite complex logic on our EF model classes and sometimes repeated in the ViewModels as well. Ideally it feels as if we're at the point where some of the more complex business logic may be better suited to domain models groups into bounded contexts. These classes would model the behaviours the campaign types and not concerned with persistence.
mgmccarthy commented 8 years ago

@stevejgordon, 100% agree. I work with bounded context at my current job. Simple logic on the model classes is not necessarily a problem, but I think the ViewModels are WAY too complex, use persistence, etc...

starting with the ViewModels would be a great place to start. Not every mapping between model and ViewModel and vice-versa need the same logic, and I'd argue that some of the logic in the existing extension methods do not belong in those extension methods...

MisterJames commented 8 years ago

@stevejgordon @mgmccarthy Yeup, a bounded approach is a good thing.

Pragmatically speaking, some of what is going on that we don't like (smart model classes, persisty-ish VMs) are artifacts of a) first attempts by early contributors b) symptoms of missing bits from early ASP.NET builds or c) when people were trying to get things in for a demo etc. So, I'd just like to suggest that even before we make wholesale changes we get our current architecture leveled off. And I do believe we're getting close.

I want to avoid the whole lava-layer architecture bits that tends to happen on projects where the hands on the keyboards change frequently.

We inherited this code base and not all the bits are handsome, but this is our date and the dance is quickly approaching. :dancers: :laughing:

The move to command/query is a significant jump forward in architecture from the everything-in-one-repository approach, and it may not be perfect...but it is approachable. I can explain command/query in 10 minutes to a junior programmer, walk them through a sample at a code-a-thon and set them loose on the code. It's just a hammer and not always the right tool, but I feel compelled to balance architectural bliss with approach-ability. Does that make sense?

This would be a great conversation over :beers: or other non-alcoholic beverages if that's your tickle.

mgmccarthy commented 8 years ago

@MisterJames, CQRS is definitely the way to go here, and like you said, it's not that tough to explain to another programmer. Mediator makes that possible as a framework. Approaching the system this way makes it easier to spot duped code or code in the wrong place b/c we should name commands in terms of the domain. To me, it's a way to clarify what the business meaning is as an operation after how the command/events are named.

One more thing i'd like to add to this is list is to get rid of the entire IAllReadyDataAccess API. CRUD-based API's obfuscate the business intent. The artifacts of this are present in the implementation of these interfaces, where some of these methods are doing a LOT of DB work, checks, rules, update, deletes etc.... does all this need to be in one handler? Should two different areas of the site be sending commands to do the same thing? etc... this ties directly into identification and enforcement of bounded context.

You could argue that the Web API controllers we have are also CRUD-based API's, and as such, COULD be renamed to something clearer (or it they're not being used, done away with altogether) ,but it's not that important in the long run.