General Code Audit

[siscodeorg]

General Code Audit

[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))));