Criticize this bit of code please

03-05-2004, 02:08 PM

I submitted this to the JASS vault, but I seem to be getting 0 criticizm of it over there. Here is the function as long as the description that comes with it. Please offer constructive criticism on it so that I can optomize it.

Code:

function UnitReplaceItemsById takes unit who, integer oldId, integer newId, integer howMany returns nothing
	local integer x = 0
	local integer y = 1
	local boolean array isKey
	local item temp
	loop
		exitwhen x == 6
		set isKey[x] = false
		if (GetItemTypeId(UnitItemInSlot(who,x)) == 'kysn') then
			set isKey[x] = true			
		elseif (GetItemTypeId(UnitItemInSlot(who,x)) == oldId) and (y <= howMany) then
			set y = y + 1
			set temp = UnitItemInSlot(who,x)
			call UnitRemoveItem(who,temp)
			call RemoveItem(temp)
			call UnitAddItemById(who,newId)
		elseif (UnitItemInSlot(who,x) == null) then
			call UnitAddItemById(who,'kysn')
		endif
		set x = x + 1
	endloop
	set x = 0
	loop
		exitwhen x == 6
		if (GetItemTypeId(UnitItemInSlot(who,x)) == 'kysn') and (isKey[x] == false) then
			set temp = UnitItemInSlot(who,x)
			call UnitRemoveItem(who,temp)
			call RemoveItem(temp)
		endif
		set x = x + 1
	endloop
	set temp = null
endfunction

Quote:

This nifty little function will replace any number of items of the same type in a unit or hero's inventory while keeping slot status the same.
*Explanation of parameters*
takes:
unit who = Unit whom you wish to replace items on
integer oldId = The item Id of the old item you wish to replace
integer newId = The item Id of the new item you wish to replace
integer howMany = The amount of items you wish to switch. Ex. Unit has 4 items of type oldId, if you pass 1 as the howMany variable, it will only replace 1 of the items, whereas if you pass 6 it will replace all.

03-05-2004, 02:26 PM

Cubasis

It is VERY proccessing heavy.

Not only becouse it's running alot of code, loops and stuff. But it's also using Blizzard.j functions, AND the fact that it is recursive and does the same code again and again.

I would reccomend replacing the BJ functions you can, with their internal working. Here is a pseudocode of how you could do this:

Code:

set Current = 0
set i = 1
loop
   exitwhen i == 6
   if InventorySlot(i) == empty then
      add item('kysn')
   elseif InventorySlot(i) == oldId and Current < howMany then
      remove Item in InventorySlot(i)
      add item('newId')
      set Current = Current + 1
   endif
endloop

*another loop to remove all fluff items*

Somethins like this would be much more efficient i think, all in one loop. And atleast much less code. There it would also do all the items in one loop instead of Recursiving the thing.

Cubasis

03-05-2004, 10:13 PM

Narwanza

Cubasis, believe me, I tried to simplify this dang trigger as much as possible. The problem is, the item API just sucks. The thing you suggested wouldn't work because I would either have to remove all sunkeys from inventory, and that wouldn't be cool. I could try to find a workaround and add sun keys back, I may do that still, but it seems crazy. I cannot get item arrays to work at all. I was barely able to get that bit of code working. I took a good 4 or 5 hours of debugging to get that thing to work. It is so crazy. Try and make something like it. It will drive you insane.

Oh, and why does everyone always try to stray from BJ functions? I mean, if they were bad why would Blizzard go and make a 9000+ line text file full of useful functions? I know they may be slower than natives, but they shouldn't be tabooed.

[edit]Okay, I got a much more optomized version posted above. Check it out.

Do booleans leak?

03-07-2004, 11:22 AM

Grater

The reason blizzard.j functions are inferior are mostly to do with object cleanup, blizzard.j functions often take objects, like locations, when you can do the same more effecientely with pure reals and avoid the creation and cleanup of a location. Blizzard.j functions also set other variables, like bj_lastCreatedUnit, which you don't really need because you can just do set u = CreateUnit(...), obviously every "set" takes time, finally Blizzard.j functions can suffer from handle leaks.

Pratically, it's not going to really matter unless the function runs very often, or is used in other intensive calculations or manipulations. Once familliar with both common.j and Blizzard.j the common.j functions are as easy to use, in most cases (except some unit state stuff and the math utility functions). There is pratically no reason to not use them.

03-07-2004, 03:29 PM	#5
AIAndy	Well, there are many Blizzard.j functions that just switch the order of the parameters and call a native. Using those is a waste.

03-08-2004, 12:37 AM

Narwanza

Right, and UnitItemInSlotBJ is going from a base 1 wheras UnitItemInSlot is going from a base 0. I got really pissed when my function wasn't working and then I finally realized that I was using UnitItemInSlotBJ and UnitItemInSlot at different places. It annoyed me. So now i see why most of the time it is better to use common.j functions. Anyways, do you guys have any more criticizm of the code? Any more optimization I could do?