Simplify and improve Rude. #1

moritzuehling · 2017-01-18T01:48:38Z

No description provided.

oberien · 2017-01-18T13:45:46Z

Rude/Rude.cs

            {
-                await target.Kick(REASONS[rng.Next(REASONS.Length)]);
+                RudeStatus.Remove(actor.UserId);


When a user is kicked, he gets removed all his rudes? He should still join with Rüdiger and his correct cooldown.

No, I don't think this is correct. Getting kicked should reset you completely.

Perhaps we should do away with kicking entirely. Now that auto-reconnect is gone it can be really annoying (especially if it happens while you're afk and then you spend the rest of the day wondering why no one is there).

Nah, I think that if you are ruded four times, you should still get kicked. It means that people agree on it, so ... yeah. You have been warned.

main--

You simplified things a lot, but I'm not sure this is the best approach here. Features you dropped (this delta is vs the old design by me, not what was actually implemented/tested):

Cooldown: As I already outline in a comment, cooldowns were quite an important feature and I feel like rude would be open to a lot of abuse if we actually were to drop it. The old implementation would even pick a random cooldown so you can't do it on a timer. If you try to rude someone else while your rude is still on cooldown, you rude yourself instead. Note that you can't normally rude yourself (your implementation allows that even if the bug I pointed out is fixed) because that would allow you to arbitrarily reset your rude level as you drop to zero whenever you get kicked. The punishment-rude doesn't do that. It's a lot of special cases, but I feel like in the end it all works out beautifully.
Independent duration: Your implementation refreshes the cooldown whenever one receives a new rude while I believe we planned on having the old one maintain durations independently for each rude. I realize that this makes the expiration timer slightly (but not excessively) more complicated, but I don't think our choice here should depend on what's easier to implement but rather what works better in practice. And I'm no longer sure which one that is.
Randomness: Everybody knows that random = fun. The old design would randomize not only cooldowns but rude durations as well.

main-- · 2017-01-18T13:47:00Z

Rude/Rude.cs

@@ -8,208 +8,77 @@

 namespace Rude
 {
-    public static class LinqExtension
+    public class RudePlugin : PluginBase


No need to rename it to RudePlugin, especially since the file is still called Rude.cs.

main-- · 2017-01-18T13:48:39Z

Rude/Rude.cs

-            if (target.UserId <= 0 || actor.UserId <= 0)
-                return;
+            EnsureRudeStatus(actor.UserId);
+            var rude = RudeStatus[actor.UserId];


This makes people always rude themselves instead of the user they selected.

... I'm an idiot. Well, that would be obvious if we had a debug-envrionment :D

main-- · 2017-01-18T13:49:46Z

Rude/Rude.cs

-            var inRudes = GetActiveInRudes(userId).Count();
-            var reudigLevel = GetReudigLevel(userId);
-            return GetUsername(name, inRudes, reudigLevel);
+            if (!RudeStatus.ContainsKey(userId))


Braces please.

main-- · 2017-01-18T13:50:51Z

Rude/Rude.cs

-            var reudigLevel = GetReudigLevel(userId);
-            return GetUsername(name, inRudes, reudigLevel);
+            if (!RudeStatus.ContainsKey(userId))
+                RudeStatus[userId] = new RudeStatus


Do you even thread safety? There is absolutely no synchronization here and this is a normal Dictionary.

I hate multithreading. Why can't everything be as cool as JS?

main-- · 2017-01-18T13:53:35Z

Rude/Rude.cs

-        {
-            return GetKickRudes(userId).Count();
-        }
+            // Remove potential old prefix


FYI this was meant to be a temporary workaround. Now that rude supports guests it's actually even broken because guests may choose a name that already has a rude-prefix.

Yeah, but at the moment I don't want to keep track of applied prefixes. It's on my todo-list though.

main-- · 2017-01-18T13:59:34Z

Rude/RudeStatus.cs

+            await Task.Delay(RudePlugin.RudeCooldown);
+            lock (RudeLock)
+            {
+                if (RudeIteration != currentRudeInteration)


Might be easier to just use a CancellationToken in Task.Delay instead of this version number. Has the added bonus of freeing this right away instead of once the timer expires.

Maybe, but meh. I changed this anyways.

main-- · 2017-01-18T13:59:52Z

Rude/RudeStatus.cs

+                    return;
+
+                RudeLevel = RudeLevel - 1;
+                RudeIteration++;


Why use ++ but not --?

main-- · 2017-01-18T14:00:40Z

Rude/Rude.cs

-
-        private static readonly List<RudeEntity> Rudes = new List<RudeEntity>();
-        private static readonly RandomNumberGenerator SecureRng = RandomNumberGenerator.Create();
+        public Dictionary<int, RudeStatus> RudeStatus = new Dictionary<int, RudeStatus>();


Why is this public?

No good reason.

main-- · 2017-01-18T14:03:44Z

Rude/Rude.cs

-            if(count == 0)
-                return null;
+        private readonly string[] Prefixes = { "[Rude] ", "[Ruderer] ", "[Rüdiger] " };
+        public static readonly TimeSpan RudeCooldown = TimeSpan.FromMinutes(30);


FYI the concept of cooldowns in the previous implementation meant that a given user could only rude once every 15 mins or so. This is IMO a very desirable feature as it prevents you from doing things like instakicking somebody or mass-ruding everyone on the server.

The old cooldown was 1 minute. If you ruded someone having already ruded in the last minute you ruded yourself instead.

this delta is vs the old design by me, not what was actually implemented/tested

Yep, that feature should be added. Agree. Should be rather easy to implement, will do.

main-- · 2017-01-18T14:06:09Z

Rude/Rude.cs

+            if (!RudeStatus.ContainsKey(userId))
+                RudeStatus[userId] = new RudeStatus
+                {
+                    OnRudeLevelDecrease = async () =>


I don't like this. You're never actually using this abstraction - you're just using it as a crutch for your data structures. Design them properly (add userid and rudeplugin to rudestatus) and you can drop this entirely.

moritzuehling · 2017-01-19T07:15:45Z

@main-- How about now?

main-- · 2017-01-19T16:28:24Z

Rude/RudePlugin.cs

+        public const double MinRudeTime = 45;
+        public const double MaxRudeTime = 120;
+        public const double MinRudeActionTimeout = 3;
+        public const double MaxRudeActionTimeout = 18;


Using TimeSpan here might make this a little more obvious.

main-- · 2017-01-19T16:31:37Z

Rude/RudePlugin.cs

+        private ConcurrentDictionary<int, RudeStatus> RudeStatus = new ConcurrentDictionary<int, RudeStatus>();
+        private ConcurrentDictionary<int, RudeStatus> RudeActorStatus = new ConcurrentDictionary<int, RudeStatus>();
+
+        private Random rand = new Random();


The primary reason why all code in fancyauth everywhere either instantiates a new Random for every method invocation or just uses System.Security.Cryptography.RandomNumberGenerator right away is that Random is not thread-safe!

(Also, is this really called rand? Please...)

main-- · 2017-01-19T16:35:48Z

Rude/RudePlugin.cs

+
+        private void EnsureRudeStatus(int userId)
+        {
+            if (!RudeStatus.ContainsKey(userId))


This check is pointless. Not only is it TOCTTOU-racy but it also accomplishes nothing at all - TryAdd already does this for you.

main-- · 2017-01-19T16:35:55Z

Rude/RudePlugin.cs

+                });
+            }
+
+            if (!RudeActorStatus.ContainsKey(userId))


main-- · 2017-01-19T16:39:15Z

Rude/RudePlugin.cs

+            await user.SaveChanges();
+        }
+
+        private void EnsureRudeStatus(int userId)


tbh this whole function is ill-designed

because let's face it are you ever going to call this and then not use a RudeStatus?

Just call GetOrAdd on both and return them as out parameters or something. AFAICS there is no removal code so it's no big deal but if there was any, you would have to do this because your code is racy (A:ensure, B:remove, A:use).

main-- · 2017-01-19T16:52:27Z

Rude/RudePlugin.cs

+            var rude = RudeStatus[target.UserId];
+            var rudeActor = RudeActorStatus[actor.UserId];
+
+            if (rudeActor.RudeLevel > 0)


Did you even stop and think about this for a second? Because unless I'm dumb this is utterly broken: The first level here means that your rude is on cooldown and lasts for the cooldown period. But as implemented, attempting to rude with your rude on cooldown adds additional levels for the full rude duration as cooldown. Also this is completely independent and never actually shows up anywhere, wtf.

main-- · 2017-01-19T16:55:42Z

Rude/RudePlugin.cs

+            if (rude.RudeLevel > Prefixes.Length)
+            {
+                rude.Reset();
+                await target.Kick("You have been too rude.");


FYI in the context of guests this leaks memory as every new guest has a new userid. The best solution would be to write this to the database but in any case, guests' data has to be freed (for members keeping it is acceptable but still not pretty).

main-- · 2017-01-19T17:04:58Z

Rude/RudePlugin.cs

+            {
+                RudeActorStatus.TryAdd(userId, new RudeStatus
+                {
+                    OnRudeLevelDecrease = () => { }


My opinion stands. Reusing this data structure for cooldowns feels very bolted-on so this callback still carries no value IMO. The class's one big feature is the delay mechanism where you can run code after a timeout and abstracting that might even make sense if you do it on a general layer (where it's no longer specific to rude) but that still doesn't give you a valid reason to use it here and without that, it's kinda pointless.

Simplify and improve Rude.

0c7717a

moritzuehling added the enhancement label Jan 18, 2017

moritzuehling self-assigned this Jan 18, 2017

moritzuehling requested review from main-- and oberien and removed request for oberien January 18, 2017 01:48

oberien reviewed Jan 18, 2017

View reviewed changes

main-- requested changes Jan 18, 2017

View reviewed changes

moritzuehling added 2 commits January 19, 2017 16:39

Rename Rude to RudePlugin

edbe81a

Improve Rude

805c932

moritzuehling force-pushed the gitgud branch from 16202a5 to 805c932 Compare January 19, 2017 16:39

main-- requested changes Jan 19, 2017

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Simplify and improve Rude. #1

Simplify and improve Rude. #1

moritzuehling commented Jan 18, 2017

oberien Jan 18, 2017

moritzuehling Jan 18, 2017

main-- Jan 18, 2017

moritzuehling Jan 18, 2017

main-- left a comment

main-- Jan 18, 2017

main-- Jan 18, 2017

moritzuehling Jan 18, 2017

main-- Jan 18, 2017

moritzuehling Jan 18, 2017

main-- Jan 18, 2017

moritzuehling Jan 18, 2017 •

edited

Loading

main-- Jan 18, 2017

moritzuehling Jan 19, 2017

main-- Jan 18, 2017

moritzuehling Jan 19, 2017

main-- Jan 18, 2017

main-- Jan 18, 2017

moritzuehling Jan 18, 2017

main-- Jan 18, 2017

oberien Jan 18, 2017

main-- Jan 18, 2017

moritzuehling Jan 18, 2017

main-- Jan 18, 2017

moritzuehling commented Jan 19, 2017

main-- Jan 19, 2017

main-- Jan 19, 2017

main-- Jan 19, 2017

main-- Jan 19, 2017

main-- Jan 19, 2017

main-- Jan 19, 2017 •

edited

Loading

main-- Jan 19, 2017

main-- Jan 19, 2017

Simplify and improve Rude. #1

Are you sure you want to change the base?

Simplify and improve Rude. #1

Conversation

moritzuehling commented Jan 18, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

main-- left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

moritzuehling Jan 18, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

moritzuehling commented Jan 19, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

main-- Jan 19, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

moritzuehling Jan 18, 2017 •

edited

Loading

main-- Jan 19, 2017 •

edited

Loading