1
\$\begingroup\$

I'm making a simple scheduler for a bot on Discord.NET and I'd appreciate some help with proper async structure of this since most of DNet is async-based. There are 2 components to this: a service and a module that uses service.

MODULE

[Group("scheduler")]
public class Scheduler : ModuleBase<SocketCommandContext>
{
    private readonly ScheduleService _service;
    private readonly string _urlPrefix = @"https://www.twitch.tv/";
    private readonly string _usage = @"!scheduler add https://www.twitch.tv/channelname time\nwhere *time* is either hours (XXh) or minutes (XXM)\n**Examples:**\n!scheduler add https://www.twitch.tv/coolguy 1h --- will anmounce stream in 1h from now on";

    public Scheduler(ScheduleService service)
    {
        _service = service;
    }

    [Command("stop")]
    [RequireOwner]
    public async Task StopCmd()
    {
        _service.Stop($"Manual request by {Context.User.Username}");
    }

    [Command("start")]
    [RequireOwner]
    public async Task RestartCmd()
    {
        _service.Start($"Manual request by {Context.User.Username}");
    }

    [Command("add")]
    public async Task Schedule(string url, string date)
    {
        var finalURL = ParseURL(url);
        if (string.IsNullOrEmpty(finalURL))
        {
            await ReplyAsync($"{Context.User.Mention} Incorrect syntax. Usage:\n{_usage}");
            return;
        }
        TimeSpan time;
        if (string.IsNullOrEmpty(date) || !TimeSpan.TryParse(date, out time) || time.TotalMinutes > 720 || time.TotalMinutes < 30)
        {
            await ReplyAsync($"{Context.User.Mention} Incorrect timespan specified. Usage:\n{_usage}");
            return;
        }
        bool canAdd = Context.Guild.Owner.Username == Context.User.Username || await _service.CanAdd(Context.User);
        if (!canAdd)
        {
            await ReplyAsync($"{Context.User.Mention} You can add 1 entry per 10 minutes, max 5 entries per 24hrs.");
            return;
        }
        var contains = await _service.CheckEntry(finalURL);
        if (contains.user != null)
        {
            await ReplyAsync($"{Context.User.Mention} Entry for {finalURL} already exist.\nAdded by {contains.user.Username}, target time {contains.date.ToUniversalTime().ToString()} UTC");
            return;
        }
        await _service.AddEntry(finalURL, DateTime.Now.AddMinutes(time.TotalMinutes), Context.User);
        await ReplyAsync($"{Context.User.Mention} New entry added: {finalURL}, announce in: {time.TotalMinutes} minutes");
    }



    [Command("remove")]
    public async Task Remove(string url)
    {
        var finalURL = ParseURL(url);
        if (string.IsNullOrEmpty(finalURL))
        {
            await ReplyAsync($"{Context.User.Mention} Incorrect syntax. Usage:\n!scheduler remove {_urlPrefix}channel");
            return;
        }
        var result = await _service.RemoveEntry(finalURL, Context.User, Context.Guild.Owner.Username == Context.User.Username);
        await ReplyAsync($"{Context.User.Mention} Result: {result}");
    }

    [Command("list")]
    [RequireOwner]
    public async Task GetList()
    {
        await ReplyAsync($"{await _service.GetList()}");
    }

    [Command("status")]
    public async Task Status()
    {
        await ReplyAsync($"{await _service.Status()}");
    }


    // Helpers
    private string ParseURL(string url)
    {
        if (string.IsNullOrEmpty(url)) { return string.Empty; }
        int i = url.IndexOf(_urlPrefix);
        string name = (i < 0) ? url : url.Remove(i, _urlPrefix.Length);
        name = string.IsNullOrEmpty(name) ? string.Empty : name.RemoveSpecialCharacters();
        if (string.IsNullOrEmpty(name) || name.Length < 4 || name.Length > 25)
        {
            return string.Empty;
        }
        else
        {
            return _urlPrefix + name;
        }
    }
}

SERVICE

public class ScheduleService
{
    private readonly DiscordSocketClient _client;
    private readonly Timer _timer;

    private List<ScheduleData> _queue;
    private ConcurrentDictionary<SocketUser, UserStamp> _bouncer;

    private DateTime startTime;
    private object busy = new object();
    private StringBuilder SBuilder;
    private string streamChannelmask = "bot";
    private SocketTextChannel streamChannel;

    public int Count => _queue.Count;

    public ScheduleService(DiscordSocketClient client)
    {
        //Client stuff
        _client = client;
        _client.Ready += OnClientReady;
        // Initializing collections
        _queue = new List<ScheduleData>();
        _bouncer = new ConcurrentDictionary<SocketUser, UserStamp>();
        startTime = DateTime.Now;
        SBuilder = new StringBuilder(); // lol optimization
        // setting up Timer, currently hardcoded
        _timer = new Timer();
        _timer.Interval = 30000;
        _timer.AutoReset = true;
        _timer.Elapsed += OnTimedEvent;


    }

    private async Task FindChannel()
    {
        foreach (var guild in _client.Guilds)
        {
            if (guild.Name.ContainsIC("iconoclasts"))
            {
                foreach (var channel in guild.TextChannels)
                {
                    if (channel.Name.ContainsIC(streamChannelmask)) { streamChannel = channel; break; }
                }
            }
        }
        Logger.LogConsoleInfo($"Default Scheduler channel: {streamChannelmask}");
    }

    // Initializing some post-constructor stuff when client is ready and connected
    private async Task OnClientReady()
    {
        Start("OnReady event");
        FindChannel();
    }

    // Timer Event fired every XXms that does most of thw work - going through list
    // finding stuff that needs to be announced, etc
        // using soft lock because timer resolution is big enough to allow any new requests fall through
    private async void OnTimedEvent(object sender, ElapsedEventArgs e)
    {
        if (System.Threading.Monitor.TryEnter(busy, 1000))
        {
            try
            {
                if (_queue.Count > 0)
                {
                    if (_client.ConnectionState == ConnectionState.Connected)
                    {
                        if (streamChannel != null)
                        {
                            StringBuilder announce = new StringBuilder();
                            int count = -1;
                            for (int i = 0; i < _queue.Count; i++)
                            {
                                if ((_queue[i].date - DateTime.Now).TotalSeconds < 5)
                                {
                                    announce.Append($"Scheduled stream starting: {_queue[0].URL} (added by {_queue[0].user.Username})\n");
                                    count++;
                                }
                                else { break; }
                            }
                            if (count > -1)
                            {
                                await streamChannel.SendMessageAsync(announce.ToString());
                                _queue.RemoveRange(0, count + 1);
                                Logger.LogConsoleInfo($"Scheduler pass finished, removed entries {count + 1}");
                            }
                        }
                    }
                    else
                    {
                        int count = -1;
                        for (int i = 0; i < _queue.Count; i++)
                        {
                            if ((_queue[i].date - DateTime.Now).TotalSeconds < 5) { count++; }
                            else { break; }
                        }
                        if (count > -1)
                        {
                            _queue.RemoveRange(0, count + 1);
                            Logger.LogConsoleInfo($"Timer check passed but not connected. Removed expired entries {count + 1}");
                        }

                    }
                }
                else { Stop("Nothing in queue"); }
            }
            finally { System.Threading.Monitor.Exit(busy); }
        }
        // 24hrs check, gonna need to figure out something else for this
        if ((DateTime.Now - startTime).TotalHours > 23)
        {
            startTime = DateTime.Now;
            ResetUsers();
        }
    }

    //Synchronous start of the timer
    public void Stop(string reason)
    {
        if (_timer.Enabled)
        {
            _timer.Stop();
            Logger.LogConsoleInfo($"Scheduler has been stopped: {reason}");
        }
    }

    //Synchronous stop of the timer
    public void Start(string reason)
    {
        if (!_timer.Enabled)
        {
            _timer.Start();
            Logger.LogConsoleInfo($"Scheduler has been restarted: {reason}");
        }
    }

    //Adding new entry to the queue List
        // using explicit lock because every request needs to be processed
    public async Task AddEntry(string url, DateTime time, SocketUser user)
    {
        UpdateUser(user);
        lock (busy)
        {
            int count = -1;
            for (int i = 0; i < _queue.Count; i++)
            {
                if (_queue[i].date < time) { count++; }
                else { break; }
            }
            if (count > -1)
            {
                _queue.Insert(count + 1, new ScheduleData(url, time, user));
            }
            else
            {
                _queue.Add(new ScheduleData(url, time, user));
            }
            Start("New entry added to empty queue");
        }
    }

    //Removing entry from the queue List
        // using explicit lock because every request needs to be processed
    public async Task<string> RemoveEntry(string url, SocketUser user, bool owner)
    {
        lock (busy)
        { 
            var data = new ScheduleData(url, DateTime.Now, null);
            int indx = -1;
            indx = _queue.IndexOf(data);
            if (indx > -1)
            {
                var usr = _queue[indx].user.Username;
                if (usr == user.Username || owner)
                {
                    _queue.RemoveAt(indx);
                    return $"Removed entry {url} (by {usr})";
                }
                else
                {
                    return $"Entry {url} exist, but it must be removed by the original user: {usr}";
                }
            }
            else
            {
                busy = false;
                return $"Entry {url} not found in queue.";
            }
        }
    }

    // Checking if specific entry exist in the List, using first result
    // Comparer only cares about URL so dummy SheduleData made
    public async Task<ScheduleData> CheckEntry(string url)
    {
        // Need to fix the Socketuser null part since it is bad way to check if no results returned
        // SU can be null if user left server?
        lock (busy)
        {
            var data = new ScheduleData(url, DateTime.Now, null);
            int indx = -1;
            indx = _queue.IndexOf(data);
            if (indx > -1) { return _queue[indx]; }
            else { return data; }
        }

    }


        // using soft lock because this request can be ignored if collection is busy
    public async Task<string> GetList()
    {
        if (System.Threading.Monitor.TryEnter(busy, 1000))
        { 
            try
            {
                SBuilder.Clear();
                foreach (var item in _queue)
                {
                    SBuilder.Append($"[{item.URL}] [{item.date}] [{item.user}]\n");
                }
                return SBuilder.ToString();
            }
            finally { System.Threading.Monitor.Exit(busy); }
        }
        return "Queue is busy";
    }

    public async Task<string> Status()
    {
        return $"Entries: {Count} | Timer enabled: {_timer.Enabled} | Resolution {_timer.Interval}ms";
    }

    //
    // User-bouncer checks
    // Using ConcurrentDic for this so dont care for locks and stuff

    public async Task<bool> CanAdd(SocketUser user)
    {
        return !_bouncer.ContainsKey(user) || _bouncer[user].CanAdd();
    }

    public async Task UpdateUser(SocketUser user)
    {
        if (!_bouncer.ContainsKey(user))
        {
            var result = _bouncer.TryAdd(user, new UserStamp(DateTime.Now, 1));
            if (!result) { Logger.LogConsoleInfo("Unable to add user to timestamp DB"); }
        }
        else
        {
            _bouncer[user].lastTimestamp = DateTime.Now;
            _bouncer[user].count = _bouncer[user].count + 1;
        }
    }

    public async Task ResetUsers() { _bouncer.Clear(); }


}

// Struct for List of queue
public struct ScheduleData : IEquatable<ScheduleData>
{
    public string URL;
    public DateTime date;
    public SocketUser user;

    public ScheduleData(string url, DateTime DT, SocketUser usr)
    {
        URL = url;
        date = DT;
        user = usr;
    }

    public override bool Equals(object obj)
    {
        if (!(obj is ScheduleData))
            return false;

        ScheduleData SD = (ScheduleData)obj;
        return this.Equals(SD);
    }

    public bool Equals(ScheduleData other)
    {
        return URL.Equals(other.URL);
    }

    public override int GetHashCode()
    {
        unchecked
        {
            return 17 * 23 + URL.GetHashCode();
        }
    }
}

public class UserStamp
{
    public DateTime lastTimestamp;
    public int count;

    public UserStamp(DateTime timestamp, int Count)
    {
        lastTimestamp = timestamp;
        count = Count;
    }

    public bool CanAdd() { return count < 5 && (DateTime.Now - lastTimestamp).TotalSeconds > 600; }

}

Now, most of the module implementations I saw use async Task so I assume that is somewhat normal for modules on DNet. What concerns me is my implementation of the service. I feel like I use a lot of async where it is not required. For example, the cases where I work with ConcurrentDictionary - seems like I could just use "fire and forget" Task without any async and dont bother?

I did read up bunch of articles on async and tasking in C# but still having hard time to grasp it. Any pointers on my example code would be nice.

\$\endgroup\$
1
  • 1
    \$\begingroup\$ You can't just put a random text in your question after receiving answers. I rolled-back your changes. \$\endgroup\$
    – t3chb0t
    Jan 22, 2018 at 8:40

1 Answer 1

1
\$\begingroup\$

Your code is not that bad. It can (and should) be optimized in few places but I find it's quite easy to understand thanks to your good habit of using helper variables like

var finalURL = ParseURL(url);

or

bool canAdd = Context.Guild.Owner.Username == Context.User.Username || await _service.CanAdd(Context.User);

or

StringBuilder announce = new StringBuilder();

Some names could be better and and more precise but most people don't even start using them and put everything in a giant if or use meaningless names. So you're on a good track.


As far as the async/await pattern is concerend most methods don't need the async. Because if you don't await anything, you don't need the async keyword. The IDE should already have warned you about it.

There's also a convention that names of asynchrouns methods - one that have Task or Task<T> as a return value - should end with the Async suffix like e.g. AddEntryAsync. We do this to avoid conflicts in case there is another method doing the same thing but in a synchrouns way. So in your case you can remove most of the asyncs.

On the other hand if you have methods returning a Task then you usually want to await for their results at some point.

In your AddEntry method you call

UpdateUser(user);

but you don't wait until it's finished. The proper way to handle it would be either

await UpdateUser(user);

or

var updateUserTask = UpdateUser(user);

// do some other work while user is being updated

// now wait for the other result
await updateUserTask;

// do some other work...

Unless you really can fire-and-forget it.


There are a few other small sins like:

  • Deep if nesting that could be avoided by using other conditions and early returns.
  • Inconsistent field names. Some of them start with and _ underscore whereas other don't.
  • Week variables with too general names e.g SBuilder or _queue.
  • Too many statements in a single line e.g. if (..) { streamChannel = channel; break; }
  • The Equals method of ScheduleData is not implemented properly. It'll blow if other is null.
  • GetHashCode of the same class does not have to calculate it with 17 * 23 + URL.GetHashCode() because URL is already a class that returns a valid hash-code - unless there is a reason you want to return a different one but then you should place a comment that explains it.
\$\endgroup\$
7
  • \$\begingroup\$ Thanks for the reply. All these "small sins" you've mentioned I'm aware of and they are postponed to be prettied up when I figure out how to fix more serious stuff. Speaking of it, regarding your note about UpdateUser: my line of thought was like that - since most of the pre-processing was done already and the Service accepts proper data, I know that I'm going to complete the AddEntry either way and in order to prevent any more extra calls from same user I instantly want to update user info via UpdateUser. Therefore I was thinking I could use simple direct Task and dont bother? \$\endgroup\$
    – Digika
    Jan 20, 2018 at 22:05
  • \$\begingroup\$ > As far as the async/await pattern is concerend most methods don't need the async. Because if you don't await anything, you don't need the async keyword. The IDE should already have warned you about it. . So you are saying I could just switch my service to normal Tasks and it wont be much different? I'm mostly concerned about overhead since bots needs to be placed on a pretty barebones box later. \$\endgroup\$
    – Digika
    Jan 20, 2018 at 22:07
  • \$\begingroup\$ @Digika I don't mean you shouldn't call UpdateUser first but you don't wait until it's finished. I find it should be explained why you are not awaitng it if this is what you wanted. \$\endgroup\$
    – t3chb0t
    Jan 20, 2018 at 22:08
  • \$\begingroup\$ @Digika I guess you are not writing your code in notepad, aren't you? ;-) Then you should listen to your IDE (Visual Studio or LINQPad) and read carefully what they are saying about your async methods without await inside. You can safely remove them for any method without an await. \$\endgroup\$
    – t3chb0t
    Jan 20, 2018 at 22:10
  • \$\begingroup\$ That's precisely why I've wanted some review - I'm trying to understand if I should use async and await it or not. From my pov I see no reason but you said this is incorrect and I should await it. I'm just trying to understand why should I do that? May be my logic does not make sense here? People do it differently, waiting for the task to return bool or something? Basically that's the gist of it - I'm trying to understand the convention here to gauge how "bad" from "ethalon" my approach it and understand why. \$\endgroup\$
    – Digika
    Jan 20, 2018 at 22:11

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.