In my introductory post, I talked about my “solution mechanic” work philosophy, and like any good and trusted auto mechanic would do when you bring your car in for service, I like to take a thorough look around an application’s codebase and operating environment, whether the check engine light is on or not.

I have a list of keywords, naming conventions, anti-patterns, etc. that trigger an investigation and discussion with fellow developers. Sometimes referred to as code smells, addressing these can be an excellent starting point in prioritizing and remediating problems. SOLID programming principals can guide and help identify some; others require experience and deeper understanding.

If you have a codebase that fails to address these issues (there are lots more that aren’t listed), you are probably experiencing brittleness and a high degree of regressions between release cycles. Whether you have inherited this codebase or are the original author, sooner or later you will feel their negative effects.

C# “new” keyword.

I can think of three uses for this keyword: instantiating an object, hiding an inherited member’s implementation, and adding a constraint to a generic type. I am only going to focus on the first two; the constraint use case poses no overt problems.

There is nothing inherently wrong with explicitly creating objects if you know exactly what you want, but in modern application development (and possibly life in general), we often don’t know or don’t care exactly how something gets done, just that we can find someone or something that can do it.

Those familiar with Dependency Injection and interface-based programming know this well. We design an interface/abstraction, a contract if you will, that ensures whatever agrees to (implements) it, we can call upon it “do” without regard to “how”. If you take a dependency on the contract (ability) and not the implementation, you can easily swap out different ones. This flexibility is often referred to as modularization and is core to decoupling a codebase.

The Dependency Inversion Principle states we should depend on abstractions, not concretions. If you are explicitly creating objects (using “new”), those areas of code are locked into that way of getting something done. Think “new is glue”.

The second use of “new” is more subtle and can produce unexpected results. As a class developer, you should be making your intentions clear about what is allowable. If you intend to provide an opportunity for others to extend the functionality of your object, then you add the “virtual” modifier to a member, or “abstract” if they must provide an implementation. Either way, a subclass will “override” the member and that implementation is the one that will be used. Not so if one chooses to “new” the member. The “new” modifier does not actually override the method in the base class. It just hides the method. When casting the derived class to the base, unexpected behaviors arise. When using “override”, you are going to get the derived member’s implementation. If the member was “newed”, you will get the base’s implementation.

There was probably a good reason why the base class developer did not allow extending a certain member. Appling “new” circumvents that intention. I assume that backwards compatibility is what Microsoft had in mind when allowing this modifier. If a base class introduces a new member that happens to be the same as one in a derived class somewhere, the derived class’s member will continue to operate independent of the base’s. This is a bit of a edge case, but something to question when you see it.

Static objects (usually with the name “Utility”, “Helper”, “Manager”, etc.).

What we are hoping for in our software is low coupling so we can test or modify behaviors as things evolve. Static classes are just as bad as directly instantiating objects as we are locking into a specific implementation at that point. Sure, there are acceptable times when we just need a simple utility that will only ever do one thing one way like create a UNIX timestamp.

Naming your utility classes really boils down to a matter or preference – just stick with the same suffix (e.g. TimeUtility, DateUtility). I suggest treating these types of classes in your application as a service just like any other, complete with an interface so you can get that low coupling we are constantly working toward.

Lack of using { } or .Dispose().

This one is huge, especially when IoC is not a controlling factor throughout the codebase. There is a good reason a developer chooses to implement IDisposable – something they are doing holds onto something significant, often unmanaged resources. Unmanaged resources are not cleaned up by the CLR garbage collector, and can contribute to memory leaks or worse. The using statement/block ensures disposal is trivial and always happens, even during an exception.

For applications with properly managed dependency injection, a resolved service’s lifetime is managed implicitly on your behalf. The IoC container and/or runtime host orchestrate resource cleanup. IDisposable implementers are properly disposed of at the end of their configured lifetime.

“Internal” and “sealed” keywords.

So many things are improved, and bad practices prevented by applying these modifiers. Public members are the surface of your API. Don’t provide visibility and access to things on the off chance you think someone might need them. Once public, you are responsible for their maintenance. Using “internal” shields the outside world from changes you may want to make.

For those that design REST services, there are a multitude of functions that are called upon. One does not indecently expose all that functionality to external systems, so why do we feel the need to publicize everything in our service layers and beyond? Probably because we got ourselves into a bind and needed to satisfy an edge case, we weren’t following good encapsulation principles, or we made some excuse such as our unit tests required it (Did you know about the InternalsVisibleToAttribute?). Maybe we weren’t aware that IoC service registration could be done in an encapsulating way, or we never thought our library would be shared with other projects.

In a well-layered system, each component cannot see beyond the immediate layer they are interacting with. Marking classes “internal” can ensure this and prevent you from littering your higher layer codebase with lower layer objects, such as database entities or service layer DTOs. In the simplest case, a project should at least separate database entity objects from the presentation layer. One of the best ways of forcing this separation of concern is by not giving visibility to these objects in higher layer assemblies, which will necessitate creating translatable/adaptable objects to move data back and forth (up and down). The same approach should be taken with service layer implementations. The service interface is your public contract; a same layer DI module/extension method can setup your choice of internalized objects to do the work.

“Sealing” or preventing inheritance might sound unnecessarily cruel, because of course, why wouldn’t you want to pass on your richly functional object? There is a simple answer: you don’t need it to succeed. There is little you can do with an abstract class that you can’t do some other way. Composition should be your preferred design choice and you can implement any number of interfaces or take any number of dependencies toward that goal. You might also see a slight increase in performance with sealed classes as the JIT compiler can produce more efficient code by calling methods non-virtually.

What about extensibility? The designer of a sealed class is only responsible up to that point, probably because that’s the limit of any guarantee they are willing or able to offer. You are, of course, free to write extension methods, but then that makes you responsible for the additional functionality, which should give you pause about your approach.

Multiple classes per file.

This is just a matter of preference and dev team culture. It can be useful in source control check-in review and locating things outside of your IDE, but a lot of times it comes down to ease of visual reference and basic organization practices. Could you lump everything into the root and perform a search? Sure. Should you?... If you do go down the route of multiple classes per file, they should probably be related or very small. Treat the common file as a way to micro-organize.

Methods or classes with a high number of lines of code.

Probably a sign that your class is not adhering to the Single Responsibility Principle. There isn’t an exact number that indicates “high”, but lots of #region blocks are sometimes, but not always, a good indication that the code should be broken up.

Custom Exception classes/flow governed by exceptions.

I put these together because one usually is a sign of another. It is not necessarily bad to be descriptive about something exceptional within your codebase, but that extra information should be used to log/diagnose an unrecoverable issue, not make conditional decisions about what to do next. The if statement expresses conditions, whose intended usage IS control flow. If you get in the habit of using exceptions in “normal” situations, how do you locate the unusual, truly exceptional ones? Expect to find a good answer from the author when you come across a try-catch pattern.

I typically run across this in service layers, for example, a custom NotFoundException is thrown, so that higher layer code can choose to deal with the exception differently (web layer would return a 404 status or a background service just create a log entry). Throwing exceptions is one of the most expensive operations in .NET because it must create a stack trace. If your application can’t suffer this pattern, you may want to investigate returning a ServiceResult<> object, with some factory methods to help generate actionable success and failure states.

What about throwing exceptions as guards or for validation? These constructs are typically meant to provide integrity and throwing an exception seems appropriate since a corrective action needs to be taken to properly evaluate inputs or the state of an object.

Service-locator anti-pattern.

The problem with the service-locator anti-pattern is obvious the moment you go looking for it – it is subtle and crops up in the most peculiar places. Using explicit dependencies (e.g., constructor injection), it is clear what is required to create an object. There may even be opportunities for compile-time errors instead of runtime errors. Every place you discover code that explicitly resolves objects from a service collection could be an area that introduces a breaking change. And don’t even think about injecting the service collection as an explicit dependency (as opposed to a static IoC container) - you just identified that the class requires potentially everything, and you may end up interfering with resolved objects’ lifetime. For those embracing good OOP practices, service-locator violates encapsulation because preconditions are hidden, thus you aren’t sure what the proper usage of the object is.

Are there times when this anti-pattern is acceptable? Yes. Even necessary. Some .NET middleware is registered as a singleton but executes its functionality within a certain context. It is from this context that we need an appropriate service (transient or scoped services cannot be resolved to create a singleton) and the ability to resolve what we need is typically provided through the called function’s parameters.

Significant logic beyond simple variable initialization in constructors.

A constructor is really meant for variable initialization and making an object ready to use. So long as you don’t need to make asynchronous calls, you may indeed have a non-trivial amount of work to properly initialize your object. Asynchronous constructors, while not even possible, make no sense because constructors are meant to immediately return a fully initialized object, not one at some undefined point in the future (e.g., awaiting a void or Task<>).

Adapt or die trying.

AutoMapper and Mapster are great libraries for translating the needs of one thing into another. They are the epitome of the Adapter pattern and can create one type of object from another, typically when we want to cross a boundary in our application with some data. However, these libraries have expanded to support all sorts of pre- and -post processing delegates. Resist the urge to consume these sugary extensions because, not only are we breaking the Single Responsibility Principle, but it can become hard to track down object state changes.

As a dangerous example, I worked on a codebase where AutoMapper’s translation function was used to update EntityFramework entities. I guess the assumption was that all entities were tracked, which they were, as they were passed around functions and layers. Odd constructs were put in place to resolve a DbContext from what I could only term as an “ambient singleton” since they rolled their own notion of a scoped lifetime, tied to a single thread. This was done to ensure the tracked entities were saved back to the instance they were created from, though DbContexts were still created explicitly from time-to-time. Blame was put on EF for being a memory hog and difficult to work with. Partial updates/data inconsistencies resulted and were hard to track down. They were always hunting down integrity issues. Some developers resorted to writing periodically executed database validation and clean-up scripts. There was more than one architectural and design deficiency in that codebase.

Ambient transaction management.

There must have been an article written by someone who just learned about MVC action filters and saw them as a global solution to transaction management, and your project confidently adopted. After all, calling an endpoint should be an idempotent or atomic operation (depending on the REST verb), basically a unit-of-work so we are taught. What better way to ensure consistency than to apply an action broadly - global filters embody the DRY principle, right? The problem is that long transactions can be expensive and cause locking issues. Depending on the amount of interaction all the way down the stack, a lot can be contended for. A TimeoutException could cause everything to unwind. The user tries again, and again, and now your data layer becomes thrashed; the log file is screaming. You resort to blaming Microsoft and buy a beefier SQL Server.

Transactions are meant to provide integrity around a use case, and this does not always translate into a course-grained service call. There are likely smaller checkpoints (not necessarily in the database sense of the word - an external API call cannot be rolled back) that should compose a higher-order function. For those with complex needs, out-of-band, long running processes may be the better way to handle the work, allowing the web service to respond within a reasonable time.

Internally developed authentication mechanisms.

This should go without saying, but rolling your own security system is extremely risky, and with the wealth of libraries out there, one would be challenged to justify an in-house solution. Leave this type of work to people who think about it every day.

Poor solution/project file and naming organization.

Software developers are generally well-organized people, with all the complexity we manage, we must be. So, when I see this, it really makes me wonder who is in charge. Maybe it has something to do with this.

Hard-coded values.

I briefly mentioned the problem in a previous blog post, but it bears repeating. The hard-coded value exists without any context. Replacing it with an aptly named constant provides that context and is self-documenting. Constants (in a common location) reduce brittleness and are the simplest example of the DRY principle I can think of.

Calling .Wait() or .Result.

If you have found yourself in this position, you took a wrong turn somewhere. Doing either of these things instead of properly awaiting the task is going to (eventually) cause a deadlock. The general advice is to “await” all the way down. That is to say, your top-level method (API endpoint, event handler, etc.) should be marked as async (returning void or Task/ValueTask), then you can await calls to service layers, which in turn can await calls into the data layer or other services.

I’m not sure there is an argument to be made where the top-level method cannot be asynchronous. Even the program entry point static Main() method can be async now. More and more of .NET and 3rd-party libraries are only being delivered with asynchronous methods. As Pink Floyd sang, “Welcome, to the [state] machine.”

There is one case where you don’t have to await and that is typically if the very last line is a call to a function returning a Task. You can just return that line. Awaiting it is only going to cause the compiler to generate state machine constructs for what essentially is a passthrough at that point.

Lack of tests.

I’m not a big proponent of TDD. typically, the motivation behind it is to increase confidence; however, no matter how good we think we wrote our tests, unit or otherwise, there is always a chance that coverage is incomplete or assumptions we made about runtime scenarios were wrong. Maybe you can gain confidence in your code, but can you ever have complete confidence in your tests?

I tend to favor more realistic approaches to testing such as integration/functional tests and especially user acceptance testing. Am I saying don’t write unit tests? Not at all. There is certainly a time and a place for encapsulating limited scope proof. Just be careful about what you should be testing – a (typically public) method that produces expected outputs for specific inputs, not the behavior of a dependency within the method. We are looking to validate the smallest atomic working units within the application.

One of the reasons I believe user acceptance testing is far more important is that it can form the basis of a contract with a customer. Interacting with software that operates as it is understood to, is truly all that matters. Customers don’t care that you can show 1000+ units tests passing if their expectations for the application are not met.

There are plenty of testing frameworks available, even ones that simulate multi-step user interaction. You should be employing at least something in your development/release process, but if you really want to gain confidence in your work, may I suggest the dogfooding approach.

Reliance on convention over configuration.

I’m not a big fan of magic. Given what I do for a living, I really need to know how some things work. The wonderous “magic” that happens when we give a folder a special name, apply a certain prefix or suffix to a class name, or just so happen to name a method “Configure” in a Startup.cs file is not helping me one bit to track down configuration issues (if I’m even sure that is what the problem is). I get that we want to be efficient and may not like all the ceremony that may go into configuring an application, but for me, visibility enlightens and provides opportunities for in-code documentation, and that’s truly wonderful.

Watch out for (k)nots.

The exclamation point: a logic altering character so skinny that it can easily go unnoticed. Sure, does it make the code more concise, yes, but at the cost of readability. Is it easier to read if(boolean == false) rather than if(!boolean)? Your eyes can decide in the middle of the night as you are hunting down a production problem. This simple negative (re)mark has frustrated and wasted many hours of my life.

Can so many programming pitfalls be so densely packed, and can we untangle an example that isn’t just academic? Let’s start with some code.

public static IEnumerable<T> Part<T>(T[] a, int n, int s, int c)
{
	var A = new T[c > s ? c : s];
	var t = 0;  //track how many times we add

	for (var i = (n - 1) * s; i < a.Length; i++)
		if (++t <= s)
			A[t - 1] = a[i];

	return A;
}

If you could only see the method signature, would you have any idea what this does? Does having visibility into the body logic help you understand it any better? This function is used for paging arrays, and while it does work and may even perform extremely well, the developer who wrote this is relying upon others to be as clever as them. To quote Martin Fowler, “Any fool can write code that a computer can understand. Good programmers write code that humans can understand.” This is typical of code that I’ve encountered and it’s full of poor practices, antipatterns, and code smells:

1. Naming Things – First, the function name should reflect what it is doing, performing a paging operation. Poor or short variable names make it difficult to know what they are used for. If you must leave a comment describing a variable, why not just name the thing appropriately?
2. Code Readability/Simplicity – Good code should be easily readable, whether you are a novice or seasoned developer. Also, is this the simplest way of achieving the desired result?
3. Primitive Obsession/Long Parameter List – With all those integers in a row, it’s hard to keep straight the correct value I should be passing.
4. You Aren’t Going to Need It (YAGNI) – Resist the temptation to add features until you actually need them. Do we really need an option to produce an array larger than a page of items?
5. Magic Numbers – Why are we subtracting “1”?
6. Not Invented Here – Some developers distrust things they did not create themselves. There are LINQ extension methods that work great, so why not use them?

Let’s fix this mess.

public static IQueryable<T> ApplyPaging<T>(this IOrderedQueryable<T> query, (int PageNumber, int PageSize) options)
{
	const int PAGE_START_NUMBER = 1;
	return query.Skip((options.PageNumber - PAGE_START_NUMBER) * options.PageSize).Take(options.PageSize);
}

This is one of the first pieces of code that I often contribute to projects. Paging operations are so common, it is a wonder why this extension method isn’t included in the framework. Implementing paging is fairly straightforward by calling .Skip()/.Take() everywhere you need it, but that would be repetitive.

Even if you couldn’t see the logic, from the name, it is obvious that it produces a page of items, and it will only do this when it makes sense. Have you ever pondered the meaning of a page of random data? The IOrderedQueryable type constraint ensures that this is used appropriately and provides an assurance that you are going to get a meaningful result.

Notice the named tuple parameter. It is a combination of related information, with communicative names. Why not just keep two integer parameters? Primitive obsession and long parameter lists can be problematic especially when like types are defined next to each other. Was that “PageSize” first, then “PageNumber”, or worse with integers named “n”, “s”, and “c”? A variable to control collection capacity adds unnecessary complexity and is a violation of YAGNI, so it is gone. The remaining options are so closely related that they naturally lend themselves as a data object. A named tuple is the quickest and easiest way of achieving this. Could we have created a class just for these options? Sure, but that is not doing the simplest thing that could possibly work.

What’s with the constant? We could have easily subtracted 1. Hard-coding a value is problematic because it is a magic number and the "1" doesn’t tell me anything about why it needs to be there. You can tell by the constant’s name what it is used for, leading to better code documentation.

There you have it. A useful, repeatable function that produces predictable results with unambiguous input. I like the simplicity of this, and it is infinitely more readable!

Improve Performance

In production I have always been bothered by the extra performance hit required to obtain the total number of pages. Unless you are implementing some sort of infinite scroll of items, it is usually helpful to know how much data one must page through. If you are using EF, then I recommend adding the Z.EntityFramework.Plus library. It has a nifty way of deferring execution of queries. All deferred queries will be executed in a batch, resulting in just one connection to your database server but with multiple result sets returned. You execute the batch when you start using any of the results.

What does an extension method look like if we are batching?

public static ICollection<T> GetPage<T>(this IOrderedQueryable<T> query, (int PageNumber, int PageSize) options, out int totalRecords)
{
	//use our new extension method, defer executing the query
	var futureQuery = query.ApplyPaging(options).Future();

	//defer executing a count for all the records in the same query
	totalRecords = query.DeferredCount().FutureValue();

	//execute batch
	return futureQuery.ToList();
}

Note that we created two deferred queries and then executed them immediately. We could execute asynchronously (await futureQuery.ToListAsync()), but we would have to move the “out” variable into the return object. Also, the return type should not be an IQueryable (the library doesn't allow for it anyway). Paging should be the last thing you do; otherwise, you end up manipulating pages. The first extension method does return an IQueryable because the .Future() extension requires that type.

Can this method be improved? Of course, but it would require more lines of code and this post was about brevity. The improvements that could be made are adding guard clauses, or better yet, encapsulating valid integers as value objects for “PageNumber” and “PageSize”.

I like getting work done in simple and practical ways. Abstraction allows me to better understand problems, and this is fundamental to the scalability of myself and the systems I build. Discovering and employing better methodologies fills my need to produce elegant and robust work, and often times, algorithms and craftmanship steal much needed sleep. This journey of practicality, scalability, and restlessness has guided the evolution of my work and I have been on a quest to discover and distill what “good” looks like. Ownership, frustration, and experience have shaped my approach and I have found that ego-free, empathetic practicality is at the core of many successes.

So, what does “good” look like? Well, that depends on who is doing the looking. To a customer this probably means receiving quality work that embodies usability and solves the actual problem they described. To development peers, this is code that you can all feel proud of and confident working with. To product managers, you have helped them with delivery and enabled them to speak about adaptability (and it be true). Of course, there should be a financial benefit to stakeholders resulting from a savings in time, process, or resources. Ultimately to me, good is something I can rely upon, and I know I have achieved it when there is stability in production.

I have a “solution mechanic” work philosophy and it goes hand-in-hand with the Boy Scout rule. As someone who enjoys paying off tech debt related to entrenched thinking and implementing “good” by refactoring and modernizing codebases, I start with a “multi-point” inspection of the application. This thoroughness will often uncover latent problems, and in their remediation, leaves things better than how they were found. A solution mechanic is skilled at assessment, possesses a preference for simplicity, and has a broad knowledge base of experience to draw from. They will use their can-do attitude and Swiss Army knife of tools to find the easiest and most cost-efficient solutions. Getting things accomplished and moving forward is fundamental to my philosophy, and a solution mechanic, as a trusted third-party, is how I symbolize it.

Myopia is natural for project members. Your development team has probably been staring at the same codebase for a while and can become quite rooted in the proverbial “it’s always been done this way”. Inviting a third-party can help bring unbiased focus to systemic issues and offer a way of clearing project distortions or ego-driven practices. When you know you have a problem, but you can’t quite identify it. When significant things need to be done, mechanically, to support an architecturally sound future. When you recognize that your team either simply can’t or won’t address the issues. This is when you need a trusted and practical third-party. This is when you need a solution mechanic.

Practical Software Development

Practicality is rooted in getting things done rather than theorizing about it. Few projects are greenfield. Most are compositions or orchestrations of existing work. There is little left to invent or an impetus to re-build, rather, it is the awareness and appropriate application of existing building blocks that embodies practical software development. Another way of thinking about this is the old “buy vs. build” axiom. Will developing a feature in-house add to the application’s competitive advantage or will it add tech debt because we are responsible for maintaining it? If your business is in the business of building a templating engine, security token service, or data access layer, then great; otherwise, use an existing library maintained by people who think about those things every day. Reduce risk for the project and its stakeholders. Focus on what makes you special. Get work done quicker and move forward. Be practical.

To embrace this level of practicality, one must relinquish ego and take nothing personal. The idea of collective code ownership should be promoted. Writing obtuse or poorly documented code may seem like a path toward cleverness and job security, but the long-term difficulty in understanding such work just adds to tech debt. Demand for effective and quality results will always elevate developers and their teams. Nothing good is achieved alone, so setup your team for success and don’t be so singularly focused on your own.

Exhibit project emotional intelligence (PEQ) through empathy. Make people’s problems you own, walk a mile in their shoes, relate to their pain – embracing these PEQ aphorisms can make the single greatest difference in the success of a project. When I speak of the importance of PEQ, I am talking about the relationships you build with the project team and its stakeholders. Understanding and empathizing with the issues they face will enable you to better communicate, and things clearly understood will prevent disagreements and frustrate what might already be an apprehensive group. To start, there are three questions I typically ask stakeholders and their teams so that we can start communicating effectively.

1. What are you trying to achieve? – This lets me know that they know what they want and what should be prioritized.
2. What are your constraints? – Are the goals realistic, are there sufficient resources, and what level of creativity or flexibility is possible?
3. What are your fears? – What is driving the project? Fear is one of the two biggest motivating factors for human beings. By embracing the problems of others, it forces one to take a high degree of responsibility, ultimately pushing the project towards success.

The following posts will be an exploration of practicality in practice. There are enumerable software development blogs that do a great job of explaining patterns and best practices. I do not intend to repeat their tenets, rather I hope to steer conversations and projects toward practicality as a critical success factor. Topics can become paralyzed by excessively arguing over theory, philosophy, and style. I am going to try and offer practical advice through architectural or design changes or even simple tweaks that can help you accomplish your goals. I’m not trying to win any arguments, rather I want to offer some guidance that has helped me build maintainable solutions (80% of software’s lifecycle is spent in maintenance). For the purists, I will be framing suggestions and ideas upon a few simple principles, namely: encapsulation, dependency inversion, the SOLID software design principle, domain-driven development, and my personal maxim, separation of concerns.

The opinions expressed are of course to be taken with a grain of salt. As developers, product managers, and consultants, we all have our own experiences. The lessons learned and shared in these pages are what has enabled one person to bring multiple enterprise applications from idea to market with no outside capital or hiring additional personnel. The ROI on most of these projects exceed 90%, meet 99.99% SLAs, with refactoring and feature enhancements taking mere hours. Take from that what you will.

I have been asked why there is no comment or discussion section. The reason is simple. Laws like Texas HB 20 and judges who rule without merit or issuing an opinion can have an alarming effect on anyone who runs a website. By allowing social content, this website would become a platform that “enables users to communicate with other users”. Therefore, I would not have the ability to moderate something off-topic, enforce a standard (such as no hate speech), or even provide fact-checking notes as it may constitute discrimination. Consequently, this infringes on my company’s first amendment right because I would be forced to publish something I don’t want to.