help with simple condition

[jig7c]

how do i reduce the bottom code to make it more suitable
i want the condition to be

killing unit is a hero and random integer 1 to 100 less than or equal to 10....

10% chance the killing hero will benefit from the actions...

 private function Conditions takes nothing returns boolean
    if ( not ( IsUnitType(GetKillingUnit(), UNIT_TYPE_HERO) == true ) ) then
        return false
    endif
    if ( not ( GetRandomInt(1, 100) <= 10 ) ) then
        return false
    endif
    return true
endfunction

 

[chobibo]

private function Conditions takes nothing returns boolean
    return IsUnitType(GetKillingUnit(), UNIT_TYPE_HERO) and (GetRandomInt(1, 100) <= 10)
endfunction

 

[jig7c]

u know i did that exact same thing, and it started giving me errors... let me try ur script...

edit:
sure enough it works...

what about this one.. anyone?

function Trig_Illusion_Conditions takes nothing returns boolean
    if ( not ( GetUnitTypeId(GetAttacker()) == 'Ekgg' ) ) then
        return false
    endif
    if ( not ( IsUnitType(GetAttackedUnitBJ(), UNIT_TYPE_STRUCTURE) == false ) ) then
        return false
    endif
    if ( not ( IsUnitType(GetAttackedUnitBJ(), UNIT_TYPE_HERO) == false ) ) then
        return false
    endif
    if ( not ( IsUnitType(GetAttackedUnitBJ(), UNIT_TYPE_MECHANICAL) == false ) ) then
        return false
    endif
    if ( not ( IsUnitType(GetAttackedUnitBJ(), UNIT_TYPE_PEON) == false ) ) then
        return false
    endif
    return true
endfunction

 

[chobibo]

function Trig_Illusion_Conditions takes nothing returns boolean
    local boolean b
    set b = (GetUnitTypeId(GetAttacker()) == 'Ekgg')
    set b = b and (not IsUnitType(GetTriggerUnit(), UNIT_TYPE_HERO))
    set b = b and (not IsUnitType(GetTriggerUnit(), UNIT_TYPE_PEON))
    set b = b and (not IsUnitType(GetTriggerUnit(), UNIT_TYPE_STRUCTURE))
    set b = b and (not IsUnitType(GetTriggerUnit(), UNIT_TYPE_MECHANICAL))
    return b
endfunction

 

[Sevion]

Don't use so many if not then return false statements. That's ugly and bad. It's Blizzard JASS.

Not sure what kind of impact [ljass]set[/ljass] has on performance, but you can just inline it all in a single return statement.

function Cond takes nothing returns boolean
    return GetUnitTypeId(GetAttacker()) == 'Ekgg' and not (IsUnitType(GetTriggerUnit(), UNIT_TYPE_HERO) and IsUnitType(GetTriggerUnit(), UNIT_TYPE_PEON) and IsUnitType(GetTriggerUnit(), UNIT_TYPE_STRUCTURE) and IsUnitType(GetTriggerUnit(), UNIT_TYPE_MECHANICAL))
endfunction

Seeing as there are so many calls to GetTriggerUnit(), you can store that in a unit variable, but that'd leak because you're not nulling the variable. So you store the boolean in a variable as well:

function Cond takes nothing returns boolean
    local boolean b = false
    local unit u = GetTriggerUnit()
    set b = GetUnitTypeId(GetAttacker()) == 'Ekgg' and not (IsUnitType(u, UNIT_TYPE_HERO) and IsUnitType(u, UNIT_TYPE_PEON) and IsUnitType(u, UNIT_TYPE_STRUCTURE) and IsUnitType(u, UNIT_TYPE_MECHANICAL))
    set u = null
    return b
endfunction

Give a man a fish and you feed him for a day. Teach a man to fish and you feed him for a lifetime.

 

[jig7c]

conditions is something i just don't understand in jass... its the hardest part for me.. i just write the conditions in GUI, convert it to custom text and then write my actions without BJ/leakless/efficiently

thank you guys for help me out

 

[chobibo]

Seeing as there are so many calls to GetTriggerUnit(), you can store that in a unit variable, but that'd leak because you're not nulling the variable. So you store the boolean in a variable as well:

You're right, but that is a constant function so I decided not to use any variables anymore. You're example is indeed better.

Don't use so many if not then return false statements

Actually, those if statements aren't really that bad, it just looks bad, it's unreadable. Using well placed ifs are actually better than what's on mine and your example.

// This looks ugly but evaluates less than our first example, it also doesn't use a boolean variable.

function Trig_Illusion_Conditions takes nothing returns boolean
    local unit target
    if (GetUnitTypeId(GetAttacker()) == 'Ekgg') then
      set target = GetTriggerUnit()
      if IsUnitType(target, UNIT_TYPE_HERO) or IsUnitType(target, UNIT_TYPE_PEON) or IsUnitType(target, UNIT_TYPE_STRUCTURE) or IsUnitType(target, UNIT_TYPE_MECHANICAL) then
        set target = null
        return false
      endif
      set target = null
      return true
    endif
    return false
endfunction

That's ugly and bad. It's Blizzard JASS.

He converted a trigger to jass, it really is Blizzard's jass.

Not sure what kind of impact set has on performance, but you can just inline it all in a single return statement.

I wanted to make it readable. Also, inlining doesn't necessarily mean it performs faster, it just takes lesser space.

If we use ifs properly, it's possible to make the function more efficient on runtime, but it would require more storage space (file getting bigger). So what we really should be thinking about is what to risk, memory or processing power.

I also didn't think it was necessary to point out the details stated above, since I was assuming he was just starting out and it would overcomplicate things.

Anyway, Sevion's example is indeed better.

 

[Sevion]

It looks like my copy/paste when I copied code for my second example failed me... >_> Lol.