Authors CurseForge

Quote from Tekkub

wow... just... wow... NEVER DO THAT. Bad PIL, BAD!

Passing named args is not possible. What PIL describes there is passing a table, which is a horrible idea. Never EVER do that.

No offense, Tek, but I'm pretty sure Roberto has a fairly good notion of which parts of the language are good and which are bad. He's one of the inventors, after all.

I agree that this style of coding shouldn't be used in WoW addons. There's just no need for it, ever. But PiL wasn't written for WoW addon authors. As general coding practice goes, there's a time and a place for that style, even if you personally don't ever happen to be in that time and place. For instance, the "data files" example in PiL ch.12. As readability goes it's hard to beat.

I use this approach for code maintenance purposes. My addon, SpeakinSpell, has a main function called OnSpeechEvent which I call in close to 100 different places, and that function supports upwards of 100 optional arguments. inspired by my experience with kwargs in python, I architected my code to use named parameters with optional values using a table as described above (though I don't use that syntax style - I write it as two lines)

I used to use a named argument list and pass nil values where I wanted it to be unspecified / use a default (before I figured out how to use tables to achieve something like python's kwargs). But as I added more features to my addon, that meant adding more calls to that function, and more optional arguments... and that led me down a path where maintaining the older code to search/replace/update all the calls to OnSpeechEvent to add more parameters to a named parameter list in OnSpeechEvent( arg1, arg2, arg3, etc ) turned into a nightmare.

So I architected my code to use tables for passing optional named arguments like kwargs in python in order to make my code self-maintaining and easier to enhance. Now I use a lot of code fragments like the following...

I don't understand why you're saying this is such a bad idea, because I've never noticed a performance hit from doing this. But if I should be doing this a better way, please explain, because of course I'd always love to do better at this if I can...

-- this ValidateObject function provides a nice readable way to implement default values
-- if any key is missing from obj, it will be imported from DefaultObject
function SpeakinSpell:ValidateObject( obj, DefaultObject )
	for key,val in pairs(DefaultObject) do
		if obj[key] == nil then -- NOTE: don't replace "false" values
			if type( DefaultObject[key] ) == "table" then
				obj[key] = self:CopyTable(DefaultObject[key])
			else
				obj[key] = DefaultObject[key]
			end
		end
	end
end


-- This is the function I call in 100 places with different parameters
function SpeakinSpell:OnSpeechEvent( DetectedEventStub )
	local DefaultEvent = {
		name = "UNKNOWN",
		type = "EVENT",
		rank = "",
		target = UnitName("target"),
		caster = UnitName("player"),
-- this is pseudo-code.  There are more default parameters than this in my real code, and many are not validated with defaults like this - they just get passed down into subroutines and used in a string.gsub() function
	}

	local DetectedEvent = SpeakinSpell:ValidateObject( DetectedEventStub, DefaultEvent )
	
	-- do stuff with the DetectedEvent object here
end


-- The rest of these functions are RegisterEvent event handler functions
-- Note the variety of parameter lists I want to pass into OnSpeechEvent
-- and the flexibility that tables (kwargs) have given me this way

function SpeakinSpell:UNIT_SPELLCAST(event, caster, target, spellname, spellrank)

	-- process the spell
	local DetectedEventStub = {
		-- event descriptors
		type = event,
		name = spellname,
		rank = spellrank,
		-- event-specific data for substitutions
		caster = caster,
		target = target,
		spellid = self:GetSpellID(spellname, spellrank),
	}
	self:OnSpeechEvent( DetectedEventStub )
end

function SpeakinSpell:ACHIEVEMENT_EARNED(event, AchievementID)
	local IDNumber, Name, Points, Completed, Month, Day, Year, Description, Flags, Image, RewardText = GetAchievementInfo( AchievementID )
	local DetectedEventStub = {
		type = "ACHIEVEMENT",
		name = L["me"], -- DisplayName = "Achievement Earned by <name>"
		-- NOTE: we don't want the name of this achievement to be in the type or name of this stub, defined above, 
		--		because we want ALL differently-named achievements to share the same speech event settings
		--		so we'll use an event-specific custom data name to support substitution the name of this achievement in the speech
		achievement = Name,
		-- and the achievement link also relays the achievement name, via the standard <spelllink> substitution
		spelllink = GetAchievementLink( AchievementID ),
		-- and standard meaning for target and caster OF THE EVENT
		target = UnitName("player"),
		caster = UnitName("player"),
		-- and let's include (most of) the rest of the achievement info for substitutions, why not?
		points = Points,
		desc = Description,
		reward = RewardText,
	}
	self:OnSpeechEvent( DetectedEventStub )
end

function SpeakinSpell:BARBER_SHOP_OPEN()
	local DetectedEventStub = {
		type = "NPC",
		name = L["Enter Barber Chair"],
	}
	self:OnSpeechEvent( DetectedEventStub )
end

Make sure you do table.wipe(DefaultEvent) beforehand too if you have some keys that are sometimes set but not other times too.

OK that makes sense now, thanks :)

The purpose of my addon is to detect when a game event happens (originally when you cast a spell with UNIT_SPELLCAST_SENT, but enhanced over the past year to include additional events) and announce that spell casting or other event in the chat, like "/p <caster> cast <spellname> on <target>"


My DefaultEvent object is actually defined once in SpeakinSpell.DEFAULTS.DetectedEvent. I don't create it as a local in OnSpeechEvent as I showed there - I was oversimplifying my code above to show how I was passing function parameters.

I didn't set it up that way for GC because I didn't know that GCing a table was such a big deal. I did it that way so I only had to define the default values once in a single place that's easy to find and change - it makes sense for default values to be global)

As I understand it, the only issue with passing a table as a function parameter is not the way you pass the data to the function call - it's just that the table must be GCed eventually. If I created a table without passing it to a function, it would be the same issue, right? We're not talking about a problem that occurs from pushing the table pointer onto the stack when making the function call (unless I'm missing something?)

I assume you would raise an equal complaint about some of the other functions I've written that use a local table of functions to achieve something like a switch-case statement, instead of a series of if-else conditions... GCing that table costs CPU cycles that I would not lose from refactoring that code into a series of if-else conditions.

On a side note, a function that can take 100+ optional args? Ouch. I'd refactor that bitch into a handful of more specific functions. It'll be a hell of a lot easier to maintain too.

The 100+ optional args mostly drive a single generic subroutine that does string replacements out of a table using string.gsub.

For each Table.keyname = value, I replace substrings to change bracketed "<keyname>" pattern matches to the corresponding "value". Each call to OnSpeechEvent can specify custom string substitutions by setting DetectedEventStub.keyname = value. This includes string substitutions like <target> <caster> <spellname> <rank> etc. Info about the event that you can use in a string (a speech announcing the event in chat, ex. "/p <caster> cast <spellname> on <target>")

Many of those <substitutions> strings are things I want to guarantee have some value defined for any speech for any event, so I set up default values for those in SpeakinSpell.DEFAULTS.DetectedEvent. But if the blizzard API event notification provides more accurate info, I use that instead of the defaults. The blizz API event parameters vary a lot, which is the main reason I set up my string replacement code to operate on a table like this.

I don't have much other functional logic that looks at those optional values, like if-else conditions, so it doesn't make sense to split it into separate functions.

As I enhance it to be able to announce more kinds of game events, I use this optional parameters table to drive customizing the string substitutions for each event, with maximum shared code.

For example the most recent one I added was a handler for COMBAT_LOG_EVENT_UNFILTERED with SPELL_DAMAGE to detect when you deal a critical strike. My design made it easy to add DetectedEventStub.damage to support a speech like "/p My <spellname> just crit for <damage> damage - woot!". All I had to do was define DetectedEventStub.damage = damage, and no additional code was needed to implement the <damage> substitution because I built my framework to enable me to do that easily.

Plus, more importantly I think to the topic of the OP, I didn't have to go back and change any of the other event hooks that I already created to deal with my new damage parameter, because I set it up as an optional parameter using a table similar to the method described in the OP.

What I hear you saying about GC is that it's a bad performance hit on the CPU to create and delete a table - there's some overhead there. So I could optimize the performance of this design somewhat by reusing some of these tables more instead of making copies in a few places. I've never noticed this performance hit yet, but I assume it will become more evident as my addon grows to support more functions, so as defensive as I may be about my design, I do appreciate the criticism.

I'm intrigued. What's oUF?

It's a unit frame add-on: http://github.com/haste/oUF

The file you want to look at is elements/tags.lua. The basic idea is that we use a big fat function of doom to create closures that we can cache for later usage. This way it doesn't really matter how heavy the Tag() function is, as it's a one-time cost anyway.

No, it was actually the first time I'm browsing the WoWAce forums in months :p. Pure coincident.