General Code Audit

[roridev]

Meta-issue with all points on the code base that may need to be looked again. If said issue is big enough another issue may be created because of it. For better organization, new issues must be posted as a comment first then edited on the original message.

Reason : Discord webhook only notifies new comments not edits.

Issues here can range from code quality to major bugs.

Tags

NRE - Can cause / causes a NullReferenceException.
EXC - Can cause / causes an unhandled exception.
CQ - Code Quality.
BUG - Any bug. Includes unexpected behavior.
API - Issues on the current API design. Should have an proposal that addresses said issue annexed.

Format

Use the copy permalink feature on github and describe the issue on that piece of code.

List

CQ | API - Behaviours class works like a namespace, unnecessary nesting.

CQ | API - EmbedBase.ListEmbed uses IEnumerable<T> which is inconsistent with the api name, and likely could cause issues since IEnumerable can be anything that provides an enumerator.

CQ - Complex nested code could be made into a function

[roridev]

CQ | API - Behaviours class works like a namespace, unnecessary nesting.

sisbase/Utils/Behaviours.cs

Lines 6 to 11 in 64c33b8

	public static class Behaviours
	{
	/// <summary>
	/// The counting behaviour for ListEmbeds
	/// </summary>
	public enum CountingBehaviour

This makes the calling side to have the following code:

using static sisbase.Utils.Behaviours;

Proposed solution

Remove the nesting and move said enums to their own namespace so the calling side would be this instead:

using sisbase.Utils.Enums;

[roridev]

CQ | API - EmbedBase.ListEmbed uses IEnumerable<T> which is inconsistent with the api name, and likely could cause issues since IEnumerable can be anything that provides an enumerator.

Eg : Dictionary<T> ConcurrentDictionary<T> ConcurrentBag<T>...

sisbase/Utils/EmbedBase.cs

Line 163 in 64c33b8

public static DiscordEmbed ListEmbed<T>(IEnumerable<T> list, string name)

Proposed Solution

Change the function declaration to use an List<T> or IList<T>.

[roridev]

CQ - Complex nested code could be made into a function

sisbase/Utils/EmbedBase.cs

Lines 182 to 204 in 64c33b8

	if (command.Overloads?.Any() == true)
	{
	string use = "";
	var o = command.Overloads.ToList();
	var arguments = new List<CommandArgument>();
	o.RemoveAll(x => x.Arguments.Count == 0);
	foreach (var overload in o)
	{
	string inner = "";
	var args = overload.Arguments.ToList();
	foreach (var argument in args)
	{
	if (!arguments.Contains(argument))
	{
	arguments.Add(argument);
	}
	inner += $"`{argument.Name}` ";
	}
	use += $"[{command.Name} {inner}] ";
	}
	string argumentExplanation = "";
	arguments.ForEach(x => argumentExplanation += $"{x.Name} - {x.Description}\n");

[roridev]

CQ - Manual foreach be simplified by a linq expression

sisbase/Utils/EmbedBase.cs

Lines 56 to 62 in 64c33b8

	var RegisteredCommands = cne.RegisteredCommands.Values.ToList();
	var groups = new List<CommandGroup>();
	foreach (var command in RegisteredCommands)
	{
	if (command is CommandGroup group)
	{
	if (groups.Contains(group)) continue;

Proposed expression

var groups = RegisteredCommands.Where(x => x is CommandGroup).Distinct();

[roridev]

CQ - Duplicated code that should be made into a function

sisbase/Utils/EmbedBase.cs

Lines 63 to 64 in 64c33b8

	if ((await group.RunChecksAsync(ctx, true)).Count() > 0) continue;
	if (group.IsHidden && !showHidden) continue;

sisbase/Utils/EmbedBase.cs

Lines 84 to 85 in 64c33b8

	if ((await command.RunChecksAsync(ctx, true)).Count() > 0) continue;
	if (command.IsHidden && !showHidden) continue;

Proposed function

/* public? */ async Task<bool> IsValidAsync(/*this?*/ Command c, bool hidden);

[roridev]

CQ | API - Unused inferface. Should be deprecated/removed.

sisbase/Utils/IStaticSystem.cs

Lines 3 to 8 in 64c33b8

	/// <summary>
	/// Interface for systems that don't require connection to <see cref="DSharpPlus"/>
	/// </summary>
	public interface IStaticSystem : ISystem
	{
	}

[roridev]

CQ - Poor variable names

sisbase/Utils/SMC.cs

Lines 34 to 35 in 64c33b8

	var Ts = assembly.ExportedTypes.Where(T => T.GetTypeInfo().IsSystemCandidate());
	foreach (var T in Ts)

[roridev]

BUG | EXC - Non-exaustive call to string.Join which generates null once a list is empty. Embed fields can't have null values leading to an invalid embed generation.

sisbase/Commands/Developer.cs

Lines 64 to 67 in 64c33b8

	.AddField("Permanently disabled systems [Systems.json]",
	string.Join("\n",
	SisbaseBot.Instance.SystemCfg.Systems.Where(kvp => !kvp.Value.Enabled)
	.Select(kvp => kvp.Key))));