I don't see any flaws.
However, tab your code properly, it's good for your health. It's also good to use meaningful variable names, e.g. deaths instead of var.
That said, I think this is definitely a case where you should not be defining the functions anonymously, inside the event; this looks a little cluttered, especially to have two.
At the very least, make the Died event separate:
local deaths = 0
function characterDied( character )
deaths = deaths + 1
end
function newCharacter( character )
local humanoid = character:WaitForChild("Humanoid")
if humanoid then
humanoid.Died:connect(function() characterDied(character) end)
end
end
function newPlayer( player )
player.CharacterAdded:connect(newCharacter)
end
As to your comments:
var++
Lua does not allow these statements. First, Lua doesn't use statements-as-expressions like Lua, Java, JavaScript, etc. use. That is, in most of those languages, you can say
x = 5;
y = (x = x + 1);
Without that, the power of x++ and ++x sort of fades. It would also not be clear what it would mean with metatables (while strictly C++ has this ambiguity too, I don't think many would argue it works out well for it). Would they have to provide two new metamethods? Or it would just silently invoke + and then assign the variable? What about when you use ++ on a table's field? Does that call __index and __newindex?
WaitForChild vs FindFirstChild
In this case, I would say WaitForChild is better. It may be that FindFirstChild is sufficient, but I'm not sure you can seriously convince yourself that the Humanoid will always be present the moment CharacterAdded is fired. In any case, using WaitForChild when you at least expect there to be a result isn't any worse than FindFirstChild.
Because you are using WaitForChild, it can't possibly return nil (that would mean it should have waited longer). That means the check if humanoid then isn't necessary.
BONUS: Globals and Hiding Mutable State
This is an advanced aside. Feel free to skip reading it.
Your variable which I named deaths is an example of global state (though it's defined as local so it isn't a global variable).
The problem with global state is that things can change without it being obvious why they're changing.
For instance, wouldn't it be strange if calling :Remove() on an object in the Lighting renamed a brick in the Workspace? We want to eliminate the chance that things like that will be done.
One solution is to aggressively use packages / modules, but that isn't really a solution in this case.
The other option is to make a closure over a local variable:
do
local deaths = 0
function characterDied( character )
deaths = deaths + 1
end
function showDeaths()
local m = Instance.new("Message", workspace)
m.Text = deaths .. " have perished."
wait(5)
m:Destroy()
end
end
Now, only things in this do block can use deaths. This means that you can better make sure that only the right things are tampering with it.
While this is a sort of 'industry' 'best practice', it isn't very important to follow. I rarely see this done on ROBLOX. Most scripts are small enough, and written by one author, so you don't need to be so defensive.
1 comment
Looks like the code will work, and is as efficient as it will be. You can not use var++ unless I am wrong (BlueTaslem is most likely the one to correct me on that).