[JASS] Avoid redundant code - here possible ?

[Ardenian]

Can I somehow avoid having to different functions that differ only in one line here ?

Code

function GridCenterX takes integer i returns real
    local integer i2 = R2I((udg_SUG_GridLength[i]*udg_SUG_GridBreadth[i])*0.5 +0.5)
    local location p
    local real x 
    set p = LoadLocationHandle( udg_SUG_Hashtable, i, i2)
    set x = GetLocationX( p)
    call RemoveLocation( p)
    set p = null
    return x    
endfunction
function GridCenterY takes integer i returns real
    local integer i2 = R2I((udg_SUG_GridLength[i]*udg_SUG_GridBreadth[i])*0.5 +0.5)
    local location p
    local real y 
    set p = LoadLocationHandle( udg_SUG_Hashtable, i, i2)
    set y = GetLocationY( p)
    call RemoveLocation( p)
    set p = null
    return y    
endfunction

function GridCenter takes integer i returns location
    return Location( GridCenterX(i), GridCenterY(i)) 
endfunction

I have a function GridCenter that takes an integer ( let it be 1) and returns a location. However, I want the X and Y location be seperatly be call-able, so I could call them to receive the real and not the location, if needed.

But, besides these lines:

set x = GetLocationX( p)
//and 
set y = GetLocationY( p)

the two functions are completely the same. Do you have any suggestion how I could cut away some code ?

Requirements:

• GridCenter has to take an integer and return a location
• The coordinates of the location returned by GridCenter should be able to be called in another way, so you can call them as reals.

Example:

local real x = GridCenterX(1)

 

[edo494]

are you able to use JassHelper(vJass)? if so, you could make a struct that stores real x and real y, and return that from one single function.

 

[Ardenian]

are you able to use JassHelper(vJass)? if so, you could make a struct that stores real x and real y, and return that from one single function.

Nope.

My solution would be to take a string in the function and check it for "X" and "Y" in an ITE and then perform the correct action. Is this a wise solution ?

 

[edo494]

well it is A solution, you could also return location

Any of the mentioned would work, but it really depends on you, none of these have severe advantages over any other, excluding destruction of objects

 

[Ardenian]

well it is A solution, you could also return location

Hm, do you mean I would load the location in GridCenter, take it into another function and GetLocationX or GetLocationY then ? This could work, thank you!

 

[PurgeandFire]

I don't think your function works at the moment. I didn't test it, but iirc LoadLocationHandle only returns the location that was stored, not a new one. So when you use RemoveLocation() on it, you're actually invalidating the location that is stored in the hashtable. So GridCenterX() will probably return the correct value, but GridCenterY() will probably return 0.

Since you are working with locations already, you could consolidate them:

function GridCenter takes integer i returns location
    local integer i2 = R2I((udg_SUG_GridLength[i]*udg_SUG_GridBreadth[i])*0.5 +0.5)
    return LoadLocationHandle( udg_SUG_Hashtable, i, i2)    
endfunction

In this function, I just returned the location in the hashtable directly. You should only remove this location when you know you don't need it anymore. If you want just the X and the Y, then it is actually rather easy:

function GridCenter takes integer i returns location
    local integer i2 = R2I((udg_SUG_GridLength[i]*udg_SUG_GridBreadth[i])*0.5 +0.5)
    return LoadLocationHandle( udg_SUG_Hashtable, i, i2)    
endfunction

function GridCenterX takes integer i returns real
    return GetLocationX(GridCenter(i))
endfunction

function GridCenterY takes integer i returns real
    return GetLocationY(GridCenter(i))
endfunction

 

[Ardenian]

Thanks a lot!

It amuses ( and scares) me, how much I forgot while working on it furthermore.

I have this now, as I need CenterX and CenterY elsewhere, too:

function CenterX takes integer i, real x returns real
    set x = x - (((udg_SUG_GridBreadth[i]*0.5)+0.5)*udg_SUG_Distance)
    return x
endfunction
function CenterY takes integer i, real y returns real
    set y = y + ((udg_SUG_GridLength[i]*0.5)+0.5)*udg_SUG_Distance
    return y
endfunction


function GridCenter takes integer i returns location
    local location p
    local real x
    local real y
    local integer i2 = udg_SUG_GridLength[i]*udg_SUG_GridBreadth[i] 
    
    set p = LoadLocationHandle( udg_SUG_Hashtable, i, i2)
    set x = GetLocationX( p)
    set x = CenterX(i, x)
    set y = GetLocationX( p)
    set y = CenterY(i, y)
    return Location( x, y) 
endfunction

Can I short it to this ?

function CenterX takes integer i, real x returns real
    return x - (((udg_SUG_GridBreadth[i]*0.5)+0.5)*udg_SUG_Distance)
endfunction
function CenterY takes integer i, real y returns real
    return y + ((udg_SUG_GridLength[i]*0.5)+0.5)*udg_SUG_Distance
endfunction

function GridCenter takes integer i returns location
    local location p
    set p = LoadLocationHandle( udg_SUG_Hashtable, i, udg_SUG_GridLength[i]*udg_SUG_GridBreadth[i])
    return Location( CenterX(i, GetLocationX( p)), CenterY(i, GetLocationX( p))) 
endfunction

 

[PurgeandFire]

Yep! But there are two slight issues. The first issue is that you should use GetLocationY when you call CenterY(). The second issue is a bit longer: you have to null p, or else there will be a reference leak. You can read more about it here (link). The TL;DR version of it is this: when you set a local variable to point to something, make sure you null it before the end of the function or before any return statement. This only applies to variables that extend agent, so you don't need to worry about it for reals, integers, code, string, or booleans, or any regular "handle" variables.

In your case, it is a bit tricky. If we null 'p' before that, then the return statement won't work. To get around this, we'll typically use a global variable. Since you aren't using vJASS, I'll assume that you have a variable named "TempLoc" in the variable editor:

function GridCenter takes integer i returns location
    set udg_TempLoc = LoadLocationHandle( udg_SUG_Hashtable, i, udg_SUG_GridLength[i]*udg_SUG_GridBreadth[i])
    return Location( CenterX(i, GetLocationX(udg_TempLoc)), CenterY(i, GetLocationY(udg_TempLoc))) 
endfunction

 

[Anitarf]

It is unclear to me why you are even storing locations in the hashtable. Can't you just calculate them each time based on grid coordinates? Also, I don't understand the logic behind the index you're using when reading from the hashtable. Can you explain this a bit more?