GML Need a shorter way to write this

[OnLashoc]

Hello GM'ers!

So I need a better / short way to write this. This code is currently working as intended but it's super ugly and super long. Is it already as good as it gets or can it be written better and still remain within the draw event of my oCursor object?

if instance_exists(oGUI) && !instance_exists(oTech) && !instance_exists(oArmy) && !instance_exists(oCouncil) && !instance_exists(oVillages) && !instance_exists(oMap) && (mouse_x > oGUI.x - 463 && mouse_x < oGUI.x + 235) && (mouse_y > oGUI.y - 340 && mouse_y < oGUI.y + 250) && (hoverNode != noone){
    draw_sprite (sSelected, -1, gridX * GRID_SIZE, gridY * GRID_SIZE);
}

Basically, when the oGUI object is present, and the smaller menu items oTech, oArmy, oCouncil, oVillages, and oMap (smaller windows within the oGUI window) are not present, & a node (grid square) is being hovered over, I need the selection square to be drawn. When the mouse is over the the oGUI I don't want the selection sprite drawn because it was drawing over the oGUI. With the code above, it is no longer drawing over the oGUI or the other windows as stated above so it's working, but in my mind probably not the most efficient use of GML lol, it's ugly long but it works, but could it be shortened?

Can multiple objects be passed through the !instance_exists(); statement? i.e. !instance_exists(oTech || oArmy || oCouncil || oVillages || oMap) ? *EDITED TO ADD Nope this doesn't work...



I'm sure there is the professional smartest guy in the room way, but Im self taught and still learning after a year + some months lol. Thanks in advance!

Onnie

 

[SupernaturalCow]

I mean, that's one big ass ugly list of conditions. I usually try to figure things out so it doesn't have to get to that. States are great for this. For example, you might have a game controller determining the game state according to what instances are currently active - game_playing, in_town, on_map_screen, etc. Then you could just run this code when you're in (I'm assuming) 'on_inventory_screen'.

But, to the matter at hand, usually according to best practices, code length shouldn't go past 100 chars (I like to follow the AirBnB code guide, which requires that). Since you have so many checks, and they're all 'and and and and', you can set up an array of them all, and check each one in a for loop. It adds more lines of code, but imo helps spread the checks out, and lets you see at a glance what you're expecting the conditions to be. Some example code:

// List of conditions.
var _checks = [ instance_exists(oGUI),
                !instance_exists(oTech),
                !instance_exists(oArmy),
                !instance_exists(oCouncil),
                !instance_exists(oVillages),
                !instance_exists(oMap),
                mouse_x > (oGUI.x - 463),
                mouse_x < (oGUI.x + 235),
                mouse_y > (oGUI.y - 340),
                mouse_y < (oGUI.y + 250),
                (hoverNode != noone)
            ],
    _count = 0,
    _len = array_length_1d(_checks),
    ;
// Check each condition.
for (var i=0; i<_len; i++) {
    if _checks[i] _count++
    else break;  // If any check fails, we don't need to keep going.
}
// If we passed them all, do the thing.
if (_count == _len) {
    draw_sprite (sSelected, -1, gridX * GRID_SIZE, gridY * GRID_SIZE);
}

The other thing is the mouse boundary checks can be done with a collision function (either built-in or your own). Oftentimes, refactoring is just a matter of breaking the problem down into smaller bit-sized functions.

 

[OnLashoc]

Thank you so very much!!

Yea I know it's ugly and some of the tutorials I've followed especially said "If it works then it's not stupid" lol, but even I know that line of code I wrote is awful!!

 

[the_dude_abides]

It can be shrunk to be shorter, but it wouldn't reduce the need to check all those things, so whether its "better" is debatable......

var is_gui = instance_exists(oGUI);
var is_tech = instance_exists(oTech);
var is_army = instance_exists(oArmy);
var is_council = instance_exists(oCouncil);
var is_villages = instance_exists(oVillages);
var is_map = instance_exists(oMap);
var is_mouse_x = false;
if (mouse_x > oGUI.x - 463 && mouse_x < oGUI.x + 235)
{is_mouse_x = true;} 
var is_mouse_y = false;
if (mouse_y > oGUI.y - 340 && mouse_y < oGUI.y + 250)
{is_mouse_y = true;}

if is_gui && !is_tech && !is_army && !is_council && !is_villages && !is_map && is_mouse_x && is_mouse_y && (hoverNode != noone)
{
draw_sprite (sSelected, -1, gridX * GRID_SIZE, gridY * GRID_SIZE);
}

That is, uh, shorter...but only in a pedantic kind of way. It's still exactly the same thing.

I mean, that's one big ass ugly list of conditions. I usually try to figure things out so it doesn't have to get to that. States are great for this. For example, you might have a game controller determining the game state according to what instances are currently active - game_playing, in_town, on_map_screen, etc. Then you could just run this code when you're in (I'm assuming) 'on_inventory_screen'.

But, to the matter at hand, usually according to best practices, code length shouldn't go past 100 chars (I like to follow the AirBnB code guide, which requires that). Since you have so many checks, and they're all 'and and and and', you can set up an array of them all, and check each one in a for loop. It adds more lines of code, but imo helps spread the checks out, and lets you see at a glance what you're expecting the conditions to be. Some example code:

// List of conditions.
var _checks = [ instance_exists(oGUI),
                !instance_exists(oTech),
                !instance_exists(oArmy),
                !instance_exists(oCouncil),
                !instance_exists(oVillages),
                !instance_exists(oMap),
                mouse_x > (oGUI.x - 463),
                mouse_x < (oGUI.x + 235),
                mouse_y > (oGUI.y - 340),
                mouse_y < (oGUI.y + 250),
                (hoverNode != noone)
            ],
    _count = 0,
    _len = array_length_1d(_checks),
    ;
// Check each condition.
for (var i=0; i<_len; i++) {
    if _checks[i] _count++
    else break;  // If any check fails, we don't need to keep going.
}
// If we passed them all, do the thing.
if (_count == _len) {
    draw_sprite (sSelected, -1, gridX * GRID_SIZE, gridY * GRID_SIZE);
}

I don't think that would work. Unless GMS 2 is different to GMS 1, if I put that into an array I wouldn't expect anything other than the result of that expression being tested. By adding "instance_exists(oGUI)" to an array entry - it doesn't place that command as a function to be run when the array is accessed. It just stores the end result (true / false) of the function within that array position.

// Check each condition.
for (var i=0; i<_len; i++) {
    if _checks[i] _count++

would equate to:
for (var i=0; i<_len; i++) {
if _checks _count++ // if true _count++ OR if false _count++.......but what is true? what is false? it has a Boolean value, but no sense of what it applies to

 

[OnLashoc]

That is correct, all the checks must be made if all are correct then display the selection square, if not then don't display.

I eventually will include something along the lines of Cow's code into a game controller type object, but for now it's set to the checks to be made by the oCursor object. My brain is troubled to compute and the ugly long ass line of code was the only way my brain was understanding what was going on and needed to happen to work correctly.

I also added below:

var is_gui = instance_exists(oGUI);
var is_tech = instance_exists(oTech);
var is_army = instance_exists(oArmy);
var is_council = instance_exists(oCouncil);
var is_villages = instance_exists(oVillages);
var is_map = instance_exists(oMap);
var is_mouse_x = false;
if instance_exists(oGUI) && (mouse_x > oGUI.x - 463 && mouse_x < oGUI.x + 235)
{is_mouse_x = true;}
var is_mouse_y = false;
if instance_exists(oGUI) && (mouse_y > oGUI.y - 340 && mouse_y < oGUI.y + 250)
{is_mouse_y = true;}

if is_gui && !is_tech && !is_army && !is_council && !is_villages && !is_map && is_mouse_x && is_mouse_y && (hoverNode != noone)
{
draw_sprite (sSelected, -1, gridX * GRID_SIZE, gridY * GRID_SIZE);
}

For that to work, it assumes that oGUI is already in existence when the game is loaded up when in fact it isn't created until you move past the main menu, then game setup screen, then the tribe selection screen, then finally the game room.

 

[YellowAfterlife]

Suppose you could make a script called instance_exists_any

for (var i = 0; i < argument_count; i++) {
    if (instance_exists(argument[i])) return true;
}
return false;

and then

if (!instance_exists_any(oGUI, oTech, oArmy, oCouncil, oVillages, oMap)
&& point_in_rectangle(mouse_x, mouse_y, oGUI.x - 463, oGUI.y - 340, oGUI.x + 235, oGUI.y + 250)
&& hoverNode != noone) {
   draw_sprite (sSelected, -1, gridX * GRID_SIZE, gridY * GRID_SIZE);
}

if this is used in multiple spots, make a combination script that checks for all those instances in one place to reduce code duplication

 

[OnLashoc]

Oh yea I didn't even think of making a script, I'm still wadding into the pool of knowledge but understanding it more and more. Thank you for that!

In a lot of tutorials, they create a script and tell you what to put in there, but don't really tell you why and how it works so I've been hesitant to make them.

 

[SupernaturalCow]

I don't think that would work. Unless GMS 2 is different to GMS 1, if I put that into an array I wouldn't expect anything other than the result of that expression being tested. By adding "instance_exists(oGUI)" to an array entry - it doesn't place that command as a function to be run when the array is accessed. It just stores the end result (true / false) of the function within that array position.

You're right - the expression is run before it is attributed to the array. You can see this happening in the Debugger if you follow along with F11. It will run through each item in the list, resolving each function before moving to the next. So in this way, all you have is an array of bools. This is effectively the same thing as stringing together a list of bool checks, you just now have it in array form. You still have to run this entire code every time you run the check (e.g. per step). We're not storing the results and expecting it to check by reference or anything later on, all we're doing is just compartmentalizing the process a bit.

would equate to:
for (var i=0; i<_len; i++) {
if _checks _count++ // if true _count++ OR if false _count++.......but what is true? what is false? it has a Boolean value, but no sense of what it applies to

Hm, I'm not sure your point got entirely across to me here.. but I'll do my best to address what I think you have an issue with: the For Loop. It's just there to get through the checks. If the bool is true, count goes up. If it's false, we break the loop. I suppose you're also right in the sense that you don't need to check whether our count is equal to the length of the checks, as in this case we have no 'OR' checks. If any of them are false, the sprite drawing shouldn't happen. Regardless, we're just using a for loop as a way of condensing down the number of manual checks we have to do. If you go into a code block in GM and try this code out (using checks that will turn true in your context), you'll get the same result as if you'd just strung all the checks together in one big if statement.

Edit: Oh and 'if _checks' is just a shorter way of saying 'if _checks == true'.

Hope that helps clear a few things up

 

[SupernaturalCow]

Oh yea I didn't even think of making a script, I'm still wadding into the pool of knowledge but understanding it more and more. Thank you for that!

In a lot of tutorials, they create a script and tell you what to put in there, but don't really tell you why and how it works so I've been hesitant to make them.

You've got the right attitude. Be open-minded to new things and don't expect yourself to have known them already, and you'll figure this all out faster than you realize

 

[the_dude_abides]

@SupernaturalCow
Where I misunderstood is expecting:

!instance_exists(oTech),
!instance_exists(oArmy),
!instance_exists(oCouncil),
!instance_exists(oVillages),
!instance_exists(oMap),
etc
to show as false, except that within that expression if they don't exist then it is true. That confused me a little.....

Probably badly explained even my reasoning there, but I think I see now how you use that in the array.

 

[SupernaturalCow]

@SupernaturalCow to show as false, except that within that expression if they don't exist then it is true. That confused me a little.....

Gotcha. Yeah the not operator (!) can really mess with you. I think the snag here is making sure you've compartmentalized the process of getting the behaviour you want, and then shortening it into something. Lists in code come in many forms, and an array is just another way of expressing OP's lengthy list of checks. If you copy the expressions exactly as they are in the if statement, they will play out to be what you expect in an array. I usually like to spread things out like in my above example, as it means when it all gets really messy, you can go to the Debugger and step through everything one line at a time. If it's all in a single line like OP, it gets really hard to figure out exactly which one of those expressions might be failing.

var _bool = true,
    _cool = !(_bool),
    _dool = !(_cool),
    _eool = !(_dool),
    _fool = !(_eool),
    ;
if _fool game = over_man;