My KDA system (kills manager)

Help and Feedback Code Review
, ,
#1

I made a script to check who was responsible for your death or the death of another player / npc:


    local Victim = {}


local module = {}

     function module.New(caract)
        local Data = {
            ["Agressor"] = nil;
            ["Time"] = 0
        }
        Victim[caract] = Data
     end

     function module.Delet(caract)
       Victim[caract] = nil
     end

     function module.Get(caract)
        return Victim[caract]["Agressor"]
     end

     function module.Set(caract,aggressor)
        Victim[caract]["Time"] = 5
        Victim[caract]["Agressor"] = aggressor
     end

     function module.DeletAgressor(caract,agressor)
        for count = 1,5 do
            Victim[caract]["Time"] = Victim[caract]["Time"] - 1
               if agressor ~= module.Get(caract) or Victim[caract]["Time"] == 5 then
                  break
               else
                  module.Set(caract,"")
               end
           wait(1)
        end
     end

     function module.attackedPlayer(caract,aggressor)
         module.Set(caract,aggressor)
         module.DeletAgressor(caract,aggressor)
     end


return module

The system saves the victim and aggressor using their Character as a reference, after 5 seconds without being attacked the system removes the aggressor, in case the aggressor changes or he attacks you again, the time resets to 5 again, that is, if you kill yourself in combat the aggressor will receive the kill

4 Likes
#2

Very clean and compact, great job IMO.

1 Like
#3

I hope this is helpful :slight_smile: A lot of it is subjective, but I really think these suggestions would make your module a lot better.

Variable naming

You shorten variable names like “Delet” or “caract”, which I’m personally against. The drop in readability is not worth the tiny amount of time saved typing, especially with auto completion.

The names of the methods are unclear. KillsManager.New(character) sounds like you’re creating a new KillsManager, but actually it’s more like you want to start tracking kills for that character. I’d rename that method to “TrackCharacter” or “StartTracking”. I’d rename Delet to “StopTracking”. The Get method specifically “gets” the aggressor for the given character, so I’d rename it “GetAggressor”. Same with the Set method. The DeletAggressor method seems like it should reset the data for the given character, but looking at the code it seems like it starts the countdown for the aggressor to be reset. I’d rename it to “StartResetCountdown”. Finally I’d rename attackedPlayer to “AttackedPlayer”, to keep the capitalization scheme the same. Since you’re passing characters to most of these methods instead, I’d rename AttackedPlayer to AttackedCharacter, assuming this supports NPC characters being tracked.

Solution

The AttackedPlayer method delegates everything to two other methods, Set and DeletAgressor. If the module is supposed to handle setting the aggressor and keeping track of the time, then it’d make more sense to have Set and DeletAgressor be private instead, so the only interface that’s exposed is New, Delet, Get and AttackedPlayer.

Right now, the only point in time where knowing the aggressor of a character is relevant is when some outside source asks for the information. Instead of having a countdown, you could just calculate whether 5 seconds have passed at the moment where the aggressor is requested. E.g.:

function module.GetAggressor(character)
	local timeSinceAggression = tick() - Victim[character].Time
	if timeSinceAggression <= 5 then
		return Victim[character].Agressor
	else
		return nil
	end
end

That would change the Set method, e.g.:

function module.SetAggressor(character, aggressor)
	Victim[character].Time = tick()
	Victim[character].Aggressor = aggressor
end

This way, it keeps track of the time where the aggressor was last set instead of keeping track of how long is left.

You could make it all more user friendly by not requiring users to manually register characters to be tracked. Instead, check if they’re registered in the Set method, and register them if they aren’t. That way you wouldn’t have to expose the New or Delet methods, so the only things the user needs to know are PlayerAttacked and GetAggressor.

Also, replace the magic number 5 with a variable, to make it easier to change in the future.

3 Likes
#4

Thanks i will improve man, very thanks to give suggestions