Lessons Learned Pt 2.0: To Comment XOR not to Comment that is the Question π€
mfalconi
Posted on April 18, 2024
Good day fellow nurdz π
In my last post I touched on the importance of documentation and how it can be a useful tool for both developers and organizations alike. Today I want to focus on clarifying proper use cases for another important facet of any dev toolbox π§°. Now be warned, this is a controversial stance BUT:
In this segment I want to explore the importance of an often underappreciated and frequently misunderstood and misused concept in programming.
Lesson 2.0 // Comments
This is a topic that I see often hotly debated in various developer forums. There's typically dichotomous a division in such discussions wherein one groups' premise is that code should be self documenting and obvious and therefore comments are redundant and/or unnecessary. Party two often rebuttals no, comments should be used to explain code. Now these sentiments are not reflective of the real world. Life often isn't as polarizing IRL but these perspectives do exist, and having spent some time in a variety of code bases, I'd like to offer my insight for those just starting out.
I think both arguments above make valid points, but like many binary options; the gray areas of which the real world is made from are totally ignored. Both perspectives offer important insight but this does not make them mutually exclusive.
- Code should definitely be self documenting and in most cases, understandable to any competent developer familiar with the language/framework. This is a simple concept in theory but in practice it becomes more complicated. What is readable to you might not be immediately clear to me or vice versa. Example: Working with new or less commonly used language features in teams comprised of developers of varying experience might lead to instances where some people have no issue with code readability. Whereas others may look at it and be uncertain what to make of it.
Comments can help elucidate logic to unfamiliar eyes so that at least the reader can know where to start looking to begin developing their understanding.
- In Most cases you really shouldn't need to explain what your code does, BUT and this is a big πcomments SHOULD be used to provide context around the code. Why is this class needed, how/where is it used, and often this can include business requirements for certain things. Oftentimes in long lifecycle projects, the maintainer of some code will not be the original author of it. They may have no idea why certain decisions were made, and comments can provide the context behind the code. A good example of this is GEO IP Restrictions. Take a look at this VB.NET Server code.
VB.Net Code
select case s_country
'--------------------------------------------------------------------------------
'Download is not available in some countries:
'Afghanistan, Belarus, Burma, Central African Republic, Cuba, Democratic Republic of the Congo, Ethiopia, Iran, Iraq, North Korea, Kosovo:, Lebanon, Libya, Mali, Nicaragua, Russia, Samoa, Somalia, South Sudan, Sudan, Syria, Venezuela, Yemen, Zimbabwe - strings are ISO Country Codes
' XK is temp code for Kosovo
case "AF","BY","MM","CF","CU","CG","ET","IR","IQ","KP","LB","LY","ML","NI","RU","WS","SO","SS","SD","SY","VE","YE","ZW", "XK" 'ISO 3166 Country Codes
s_msg="<html><body onload=""alert('Download not available in your region');""></body></html>"
Response.ContentType = "text/html"
Response.Write s_msg
Response.end
'--------------------------------------------------------------------------------
'Download is available in any other countries
case else
if s_site="www.supercoolwebsite.com" then
FileCheck(s_file_relative_link1)
end if
SendFileToResponse s_file_relative_link1, s_file_name1
Response.end
'--------------------------------------------------------------------------------
end select
Now Imagine trying to make sense of that string at the top without any indication what the string represents. Sure the case is s_Country so most likely we can quickly deduce they are country codes, but perhaps you wonder why are we returning an error response for these country codes? In this case the comments make it clear. This is a business logic rule. It provides insight into why this function exists, as well as some basic explanation for how it works. It would have been better had there been some explanation for what and where the variables represent and come from respectively, but still something is always better than nothing.
Here's another example where I think having some brief explanation can do more than any self documentation coding style can. The below snippet is some code I wrote for a new product filter system my team was working on. Take a look at the first sample. No comments or anything, just code with semantic names and clear and concise implementation. How long would it take you to understand the below code and get an idea of how its used within a massive web application?
Comment free snippet
public class ProductSelectorUtil
{
private readonly ProductContext _productContext;
Func<ProductContext, IEnumerable<Converter_options_view>> LoadConverterOptions =
EF.CompileQuery(LoadConverterOptionsExp);
Func<ProductContext, IEnumerable<Converter_options_view>> LoadSwitchOptions =
EF.CompileQuery(LoadSwitchOptionsExp);
Func<ProductContext, IEnumerable<Supply_options_view>> LoadSupplyOptions =
EF.CompileQuery(LoadSupplyOptionsExp);
public ProductSelectorUtil(ProductContext productContext)
{
_productContext = productContext;
_productContext.ChangeTracker.QueryTrackingBehavior = QueryTrackingBehavior.NoTracking;
}
public static Func<ProductContext, List<MediaConverterView>> LoadAllConverters =
EF.CompileQuery((ProductContext context) => context.MediaConverterView.ToList());
public static Func<ProductContext, List<SwitchView>> LoadAllSwitches =
EF.CompileQuery((ProductContext context) => context.SwitchView.ToList());
public static Func<ProductContext, List<PowerSupplyView>> LoadAllSupplies =
EF.CompileQuery((ProductContext context) => context.PowerSupplyView.ToList());
static Expression<Func<ProductContext, IQueryable<Converter_options_view>>> LoadConverterOptionsExp = context =>
context.Converter_options_view
.OrderBy(option => option.Id)
.ThenBy(option => option.Columnvalue);
static Expression<Func<ProductContext, IQueryable<Converter_options_view>>> LoadSwitchOptionsExp = context =>
context.Converter_options_view
.OrderBy(option => option.Id)
.ThenBy(option => option.Columnvalue);
static Expression<Func<ProductContext, IQueryable<Supply_options_view>>> LoadSupplyOptionsExp = context =>
context.Supply_options_view
.OrderBy(option => option.Id)
.ThenBy(option => option.Columnvalue);
public ProductSelectorViewModel InitLoadMediaConverters()
{
ProductSelectorViewModel viewModel = new();
List<Converter_options_view> options = LoadConverterOptions(_productContext).ToList();
viewModel.MediaConverters = LoadAllConverters(_productContext);
var propertyNames = typeof(Product_Selector_Media_Converter_Tables)
.GetProperties()
.Select(prop => prop.Name)
.ToList();
foreach (var propName in propertyNames)
{
var distinctValues = viewModel.MediaConverters
.Select(
item => item.GetType().GetProperty(propName)?.GetValue(item)?.ToString()
)
.Where(value => !string.IsNullOrEmpty(value))
.Distinct()
.ToList();
if (
propName != "data_order" & propName != "model"
&& propName != "image_alt_tag"
&& propName != "part_number"
&& propName != "category_id"
)
{
ProductFilter filter = new();
filter.FilterLabel = $"{propName}";
distinctValues.ForEach(v =>
{
int? order = options
.Where(o => o.Columntitle == propName && o.Columntext == v)
.Select(o => o.Columnvalue)
.FirstOrDefault(-1);
CheckBox c = new(0, propName, v, false, order);
filter.FilterOptions.Add(c);
});
filter.FilterOptions = filter.FilterOptions.OrderBy(o => o.Order).ToList();
viewModel.ProductFilters.Add(filter);
}
}
return viewModel;
}
}
Here is the same code with some basic comments to help elucidate some context and use cases. How much quicker does the codes' usage become clear?
Commented snippet
/// <summary>
/// Injectable dependency
/// Product Selector Utility class
/// Contains all logic used to manage the data and view state not handled by controller or view model.
/// Includes as DB access logic.
/// Implements Static Compiled Queries for common db operations
/// </summary>
public class ProductSelectorUtil
{
private readonly ProductContext _productContext;
Func<ProductContext, IEnumerable<Converter_options_view>> LoadConverterOptions =
EF.CompileQuery(LoadConverterOptionsExp);
Func<ProductContext, IEnumerable<Converter_options_view>> LoadSwitchOptions =
EF.CompileQuery(LoadSwitchOptionsExp);
Func<ProductContext, IEnumerable<Supply_options_view>> LoadSupplyOptions =
EF.CompileQuery(LoadSupplyOptionsExp);
public ProductSelectorUtil(ProductContext productContext)
{
_productContext = productContext;
_productContext.ChangeTracker.QueryTrackingBehavior = QueryTrackingBehavior.NoTracking;
}
/*
* Compliled Queries
* These are optimized queries used for EF core
*/
public static Func<ProductContext, List<MediaConverterView>> LoadAllConverters =
EF.CompileQuery((ProductContext context) => context.MediaConverterView.ToList());
public static Func<ProductContext, List<SwitchView>> LoadAllSwitches =
EF.CompileQuery((ProductContext context) => context.SwitchView.ToList());
public static Func<ProductContext, List<PowerSupplyView>> LoadAllSupplies =
EF.CompileQuery((ProductContext context) => context.PowerSupplyView.ToList());
static Expression<Func<ProductContext, IQueryable<Converter_options_view>>> LoadConverterOptionsExp = context =>
context.Converter_options_view
.OrderBy(option => option.Id)
.ThenBy(option => option.Columnvalue);
static Expression<Func<ProductContext, IQueryable<Converter_options_view>>> LoadSwitchOptionsExp = context =>
context.Converter_options_view
.OrderBy(option => option.Id)
.ThenBy(option => option.Columnvalue);
static Expression<Func<ProductContext, IQueryable<Supply_options_view>>> LoadSupplyOptionsExp = context =>
context.Supply_options_view
.OrderBy(option => option.Id)
.ThenBy(option => option.Columnvalue);
/// <summary>
/// Loads all initial data for mc product selector
/// including all filter options and product list
/// </summary>
/// <returns></returns>
public ProductSelectorViewModel InitLoadMediaConverters()
{
ProductSelectorViewModel viewModel = new();
List<Converter_options_view> options = LoadConverterOptions(_productContext).ToList();
viewModel.MediaConverters = LoadAllConverters(_productContext);
//grab all possible property filters from Model
var propertyNames = typeof(Product_Selector_Media_Converter_Tables)
.GetProperties()
.Select(prop => prop.Name)
.ToList();
foreach (var propName in propertyNames)
{
//filter all distinct options -- required due to redundant / unnormalized database data
var distinctValues = viewModel.MediaConverters
.Select(
item => item.GetType().GetProperty(propName)?.GetValue(item)?.ToString()
)
.Where(value => !string.IsNullOrEmpty(value))
.Distinct()
.ToList();
//exclude all options we wont filter by but still need to show
if (
propName != "data_order" & propName != "model"
&& propName != "image_alt_tag"
&& propName != "part_number"
&& propName != "category_id"
)
{
ProductFilter filter = new();
filter.FilterLabel = $"{propName}";
distinctValues.ForEach(v =>
{
int? order = options
.Where(o => o.Columntitle == propName && o.Columntext == v)
.Select(o => o.Columnvalue)
.FirstOrDefault(-1);
CheckBox c = new(0, propName, v, false, order);
filter.FilterOptions.Add(c);
});
filter.FilterOptions = filter.FilterOptions.OrderBy(o => o.Order).ToList();
viewModel.ProductFilters.Add(filter);
}
}
return viewModel;
}
}
The above examples illustrate some important points. Compiled Entity Framework Core Queries are typically only used to optimize large and repetitive database queries. This syntax and usage is not exactly common, and for some "Full Stack" developers, they may have very little exposure to dealing with database queries, but they may one day have to fix or update them. And in the first example, a new hire with no business or domain knowledge would likely have a very hard time understanding the purpose of a switch case consisting of 2 character strings. Comments are minimalistic ways to save time to your future self, and other developers. And they aren't just for future dev who replace you. They are also for you. Think on how many times you have had to review a bug in code you wrote. Can you always recall your decision making process? I know I often forget code I've written < a month ago.
A common trap for many of us (myself included) is this "developers fallacy" that when we write code we'll definitely, π―%, without a doubt remember the reasons why we wrote this code the way we did and the context surrounding its existence.
π
But as I am sure all of you know in your heart of hearts nothing is further from the truth.
Comments are possibly one of the most basic efficiency tools you can utilize as a developer. Learning how to balance proper coding style with useful but minimalist comments can go a long way in enhancing you developer workflow and efficacy. And not just writing them. Learning to read and understand other people's code is such a crucial skill. Especially for new and novice developers, you will almost never be the person creating and writing new and shiny features. And You will rarely be the only person working on a piece of software. In many cases, especially at the start of your career you may often find yourself maintaining applications written by developers long gone. The developer who wrote said code may have been promoted, may have quit or just maybe vanished off the face of the earth. In any case, they can't help you now. But if they had any respect for their work, their past self (through their use of comments) can likely help guide your success.
What do you think? Would love to hear other people's opinions, leave your thoughts below. To Comment or not to comment that is the question.
Posted on April 18, 2024
Join Our Newsletter. No Spam, Only the good stuff.
Sign up to receive the latest update from our blog.
Related
April 18, 2024