Module talk:Flags

Code change

Hello I saw your module on it.wiki, and I would like to make some changes to the code:

1. declare local all variables, they are more efficient and quick to access and you dont pollute global namespace
2. Why do you use a for loop to check if a key is in the table like:

for flagParameter,commonsFile in pairs(master.twoLetter) do 
    if flagParameter == territory.args[1]  then 
       commonsName = commonsFile 
       tempLink = commonsFile
     end
end

is simpler and quicker try to assign and check for nil

commonsFile = master.twoLetter[territory.args[1]]
if commonsFile then --check for nil
    commonsName = commonsFile 
    tempLink = commonsFile
end

3. Why check for if territory.args[2] ~= "{{{2}}}" then ? AFAIK a paremeter call will be resolved before call the module and substituded by its value.

I will upload my changes to the sandbox for test.--Moroboshi (talk) 11:14, 22 July 2013 (UTC)

Thank you for the interest and the good advice! This is not only my first attempt with Lua programming, it is also my most serious attempt to program anything out of HTML/CSS. For instance, I wasn't aware that by not declaring variables explicitly local the module would pullute the global namespace (something that sounds obvious now that you say it).

Your approach in 2) is definitely smarter and I'm happy to take it after testing that there are no side effects (I don't see why there should be any).

if territory.args[2] ~= "{{{2}}}" then is the way I found to check whether a parameter for variants i.e. "1945" has been defined, after asking at mediawiki.org. Improvements welcome.

Now I comprend you check for a parameter not definited. I think its cleaner to set the parameter to {{{2|}}} in the template and check for null string "" in the module.--Moroboshi (talk) 18:01, 22 July 2013 (UTC)

The same for {{{link}}} or {{{text}}}, set them to {{{link|}}} or {{{text|}}} and check for null string in the module. I uploaded the modified flags to Module:Flags/Sandbox with some comments about my changes. Apparently it works with the example test in the doc.--Moroboshi (talk) 18:49, 22 July 2013 (UTC)

I had a first look at you version in the sandbox. Looks good, and you are saving almost 20 LOC! But I want to be cautious taking the changes since the template is already transcluded in 600 articles at ca.wiki. Where is git when you need it?  ;) I'm starting my holidays tomorrow, flying and then having less time in front of the laptop. Still, I should be able to connect in the evenings. Feel free to "merge" your changes, just trying to do it edit by edit with comments for posterity. Welcome to the team!  :)

Just curious: why are you changing the sequence of the script? It makes comparing code a bit more difficult. I had the idea of going for 1 Name; 2 Variant; 3 Size; 4 Link; 5 Label - but if there are good reasons to change it I'm all ears.

I had trouble with other approaches for {{{link}}} or {{{text}}}. Null string is actually different from no parameter defined. Users can set link= or text= to give specific instructions to the template (no link and same label as name of flag).