DEV Community: Prince Tegaton

What makes a perfect Pull Request?

Prince Tegaton — Mon, 29 Mar 2021 17:56:04 +0000

What makes the perfect pull request if one should ask?

The answer to this will vary according to set standards in a team. Code review is a huge factor in ensuring quality codes are used for building a software, it also allow team members to have an insight on what the next guy is working on. Key-man risk is not a good thing to have in a software development team, frequent PR is a great way to keep everything in check.

Over the years working with various teams, I have come up with my top "must-do" for a pull request.

The Build-up

Pull Requests consist of code changes (addition, modification or removal) which have been built over a short period.

1. Code Commit to Branch

All pull requests starts from a commit to the local branch. These commits are more important than many engineers may think. The title of commits should be descriptive enough to tell what was done/committed into branch. This ease rollback or branch-out in case of issues that require you picking your last working commit.
PR titles like 'first change', 'second fix', 'commit before PR', 'working code' really do not cut it.

2. Size of Changes

A Pull Request is best reviewed when the changes focus on small chunk of codes and are related. Bundling 2k line of changes across 63 files is a sure way to say "hey dude, just approve, the code works". Different scenarios may prompt this such as working on an off-the-chart feature or engineers to review may be on work leave/vacation at the time, or a poor code review culture in a team.

3. Descriptive Naming and Summary

Naming is a problem for many developers (maybe only the one's I've worked with). From poor commit messages to terrible PR titles such as 'minor changes', 'fix', 'small update', 'add log', 'working code'. PR titles should be able to describe the purpose of code being checked in as it provide insights just like commit messages. The description of should also be filtered properly to give clear insight to the code reviewer. Below is a good sample PR naming.

The above is short and precise. It's really bad finding out the purpose of the PR after looking at the code with a title 'minor update'.

4. Link Related Work-items

In platforms like Azure DevOps that support work-items for tasks, user-stories and bugs, it's best to create these items that describe the scope of work to be done. It is also a link to keep reviewers updated with what the actual logic should do. Though reviewers barely open linked items to review according to how the code is written, it is still a good practice to "keep the code with the requirement".

5. Pay Attention

Yea! Pay attention to the destination branch, especially in cases where build pipelines are setup for similar branches like DEV and MASTER. This could be a skip for code reviewers as well, you may ask how?, if it's possible, then it's possible. Attention is required by the creator and reviewers of the PR to make sure codes are well scanned, the destination branch is the intended one as well.

6. Review Your Own Pull Request

I have made it a habit of always reviewing my own PR even before I finalize it. When done finessing the title and description, just switch to the 'File Changes' tab and review every line once again. This is the point where I make follow-up commits for 'cleanup' as sometimes I see some unwanted commented code, a debug log, typos and more. Be your no. 1 critic, this way you won't get cheap points scored on you for a minor under-sight.

EOL: End of Line

This is actually conclusion.
Maintaining a set standard in a team requires discipline, code reviews is such one that requires the team to enforce a standard and not shy away from making changes as new patterns are discovered. Do raise the red-flag when an anti-standard PR is seen. The whole essence is to ensure quality code and business continuity seamlessly. Sometimes, I do err in code review - just keep the percentage of good on the rise until it becomes a habit.

-:)

Cover image source: oroinc.com

Netssentials: Part 1 - Quick Tips for ASP.Net Core API Project

Prince Tegaton — Sat, 05 Dec 2020 13:52:39 +0000

This article is the first part of the Netssential series. View project on GitHub repo

Project organization and coding patterns will always differ for each individual's taste. Many developers do not have the chance to stamp their mark on a project as it is usually the work of a 'team lead' or 'architect' but these tips I've used over the years can be useful.

Here are my top 5 tips when organizing an ASP.Net Core Web API project from scratch.

1. Setup Logging

ASP.Net Core projects can frustrate you (just like any other) when logs are not setup. This can cost you precious time, mostly when moving the app within new environments. Luckily ASP.Net Core is built to handle startup errors easily when set to do so from the Program.cs file. My personal log library preference is NLog but similar configurations should exist for other libraries.

public class Program
{
    public static void Main(string[] args)
    {
        var logger = NLog.Web.NLogBuilder.ConfigureNLog("nlog.config").GetCurrentClassLogger();

        try
        {
            logger.Debug("Initializing application..");
            CreateHostBuilder(args).Build().Run();
        }
        catch (Exception ex)
        {
            logger.Error(ex, "An error occured while initializing application.. shutting down!");
            throw;
        }
        finally
        {
            NLog.LogManager.Shutdown();
        }
    }

    public static IHostBuilder CreateHostBuilder(string[] args) =>
        Host.CreateDefaultBuilder(args)
            .ConfigureWebHostDefaults(webBuilder =>
            {
                webBuilder
                .UseStartup<Startup>()
                .UseIISIntegration()
                .UseNLog(); // enable NLog
            });
}

2. Tier the Startup.cs file

This file can grow so big in a short period since almost every config goes here, this makes it harder to locate block of codes. Like many developers who do not like staring at a single 3k lines of code file, this should work. I usually separate the file according to content thereby leaving me basically with:

Startup.cs - basic API setup and configuration
Startup.DI.cs - dependency injection setups
Startup.Identity.cs - JWT and other identity setup

All files should have public partial class Startup as seen here for Startup.DI.cs

public partial class Startup 
{
    public void ConfigureDI(IServiceCollection services)
    {
        services.AddTransient<DbContext, SqlServerDbContext>();
    }
}

These are then called from the Startup.cs file:

public void ConfigureServices(IServiceCollection services)
{
    ConfigureLogging(services);
    ConfigureDI(services);
    ConfigureIdentity(services);
    // more section ...
}

This leave you with a well tiered and neat configuration setup.

3. Dynamic Configuration Options

This is a technique I hope to enforce on front-end applications real soon. The ability to turn a feature ON and OFF for the user without the need of killing a micro-service from the server. The below illustrate how you can use choose which config or services to enable on production environment, all from the appsettings.json file.

First, create a config option class to hold all toggleable features and other config options. I call this Settings.cs

public class Settings
{
    public bool EnableHttps { get; set; }
    public bool EnableSwagger { get; set; }
    public bool EnableHealthCheck { get; set; }
    public bool EnableMiniProfiler { get; set; }
    public string Datastore { get; set; }
}

Add a Settings node with all options to the appsettings.json as seen below.

{
  "ConnectionStrings": { ... },
  "Settings": {
    "Datastore": "SQLSERVER", //SQLSERVER, MYSQL, COSMOS
    "EnableHttps": false,
    "EnableSwagger": true,
    "EnableHealthCheck": true,
    "UseMiniProfiler": true
  }
}

All services are now being registered based on the config option

public partial class Startup 
{
    public Settings _settings { get; set; } = new Settings();

    public void ConfigureServices(IServiceCollection services)
    {
        // load config from Settings.json
        Configuration.Bind(nameof(Settings), _settings);
        services.AddSingleton(_settings);

        if (_settings.EnableMiniProfiler)
            ConfigureMiniProfiler(services);

        if (_settings.EnableHealthCheck)
        {
            services.AddHealthChecks()
        .AddDbContextCheck<SqlServerDbContext>("Microsoft SQL Server Database")
        .AddCheck<ThirdPartyServiceHealthCheck>("ThirdParty Services");
        }

        if (_settings.EnableHttps)
            app.UseHttpsRedirection();

        if (_settings.EnableSwagger)
        {
            app.UseSwagger();
            app.UseSwaggerUI(options => options.SwaggerEndpoint("v1/swagger.json", "Netssentials"));
        }

        if (_settings.EnableMiniProfiler)
        {
            app.UseMiniProfiler();
        }
    }
}

4. Error Handling

Errors must surely surface, and surely handled, not too sure about this for every dev. This can be done using a Middleware or a BaseController method. Users of an application should never see error details (type, source, stack trace, inner ex bla bla), but on development environment you surely want to see the error details (on front-end toasts or API calls) without having to check logs.

My BaseController.cs technique.

[Route("api/[controller]/[action]")]
public class BaseController : ControllerBase
{
    public readonly ILogger Logger;
    private readonly IWebHostEnvironment _env;

    public BaseController(ILogger logger, IWebHostEnvironment env)
    {
        Logger = logger;
        _env = env;
    }

    protected IActionResult HandleError(Exception ex, string customErrorMessage = null)
    {
        ApiResponse rsp = new ApiResponse
        {
            Code = "500"
        };

        Logger.LogError(ex, customErrorMessage);

        if (_env.IsDevelopment())
        {
            rsp.Message = $"{(ex?.InnerException?.Message ?? ex.Message)} --> {ex?.StackTrace}";
            return StatusCode(StatusCodes.Status500InternalServerError, rsp);
        }
        else
        {
            rsp.Message = customErrorMessage ?? "An error occurred while processing your request!";
            return StatusCode(StatusCodes.Status500InternalServerError, rsp);
        }
    }
}

Usage

public class UserController : BaseController 
{
    [HttpGet]
    public async Task<IActionResult> GetUserInfo(string uniquId)
    {
        try
        {
            return Ok(apiResult);
        }
        catch (Exception ex)
        {
            return HandleError(ex);
        }
    }
}

5. Easy Debugging

Whenever I step close to a developer debugging ASP.Net Core APIs from Visual Studio and they always have to click 'Start' or 'IIS Express' I really do pity them. This mean that for every single change you have to launch the app on a new console or browser tab.

But here is a short step to allow you just build the code with 'Ctrl + B' (project) or 'Ctrl + Shift + B (solution) and continue from the client end (Postman or Web). Just do this:
Project properties > Debug > Hosting Model: 'Out of Process'.

To prevent the app ending debug mode when you close the associated browser tab, do this:
VS Options > Web Projects > Stop debugger when browser window is closed: 'false'.

There are various easy hack around project structure and patterns that could be shared but these, for now, are my top 5.

console.log(x): The way out

Prince Tegaton — Wed, 25 Nov 2020 20:45:47 +0000

Logging is a critical technique developers need to trace code problems and bugs irrespective of the language. Logs can be written to flat files (text), database tables, IDE/web browser consoles and many more.

Web developers will always find a way to forget cleaning up the console.log(x) used in JavaScript codes. To me, this smells untidiness: leaving debug data to the eyes of the public is not clean for a professional (when left intentionally).

2 years ago I came up with a technique that allow me to ignore removing logs from every single portion of my JavaScript code. Same can be implemented for any language. This is done by creating a custom log class and methods. Instead of calling console.log(x), you can do logger.log(x). On the logger class, there is a boolean variable named 'is_active =true'. On production environment, this value is set to is_active =false and all log lines will be bypassed from not being printed to console.

View code on Github Repository

The snippet is shown below. The overloads can be ignored.

logger.js

let is_active = true;

const logtype = {
    "default": 1,
    "info": 2,
    "warning": 3,
    "error": 4,
    "exception": 5,
    "debug": 6,
    "table": 7
};

function log(data, type = logtype.default) {
    if (!is_active) return;

    try {
        switch (type) {
            case logtype.default:
                return console.log(data);
            case logtype.info:
                return console.info(data);
            case logtype.warning:
                return console.warn(data);
            case logtype.error:
                return console.error(data);
            case logtype.exception:
                return console.exception(data);
            case logtype.debug:
                return console.debug(data);
            case logtype.table:
                return console.table(data);
        }
    } catch (ex) {
        console.log(data);
    }
}

function info(data) {
    if (is_active) console.info(data);
}

function warning(data) {
    if (is_active) console.warn(data);
}

function error(data) {
    if (is_active) console.error(data);
}

function exception(data) {
    if (is_active) console.exception(data);
}

function debug(data) {
    if (is_active) console.debug(data);
}

function table(data) {
    if (is_active) console.table(data);
}

Implementation

<script>
    window.onload = function () {
    log('hello world..');
    log('I have an information', logtype.info);
    log('But i must warn you', logtype.warn);
    log('About the errors you have made', logtype.error);
    log('Well, exceptions are not the end of the world', logtype.exception);
    log('And that calls for more debugging', logtype.debug);

var array = [
    {
        name: 'James',
        age: 21,
        location: 'Nigeria',
        role: 'Backend'
    }, {
        name: 'Mike',
        age: 19,
        location: 'Los Angeles',
        role: 'Frontend'
    }, {
        name: 'Parker',
        age: 26,
        location: 'London',
        role: 'Tester'
    }];

log(array, logtype.table);

</script>

The result in web browser console is seen below.

Change the variable 'is_active=false' and all log activities will be stopped.

PS: this does not mean you should not clean up logs from code