6
\$\begingroup\$

I have a flight class that looks like this:

class Flight {
 public $id; // unique id
 public $structures = array(); // each "structure" indicates one fighter class assembled in this flight class
 public $dogfights = array() if the flight is dogfighting, this array holds the ID of each opposing flight we are dogfighting
}

class Fighter { // this is whats inside $Flight->structures
 public $id
 public $parentid 
}

If two opposing flights are close to each other, I initiate a dogfight. I determine if they are dogfighting like this:

public function initiateDogfights(){
    $dogfights = array();

    for ($i = 0; $i < sizeof($this->ships)-1; $i++){
        for ($j = $i+1; $j < sizeof($this->ships); $j++){
            $dist = Math::getDist2($this->ships[$i]->getCurrentPosition(), $this->ships[$j]->getCurrentPosition());
            if ($dist <= $this->ships[$i]->size / 2 + $this->ships[$j]->size / 2){
                $new = true;
                for ($k = 0; $k < sizeof($this->ships[$i]->dogfights); $k++){
                    if ($this->ships[$i]->dogfights[$k] == $this->ships[$j]->id){
                        $new = false;
                    }
                }
                if ($new){
                    $this->ships[$i]->dogfights[] = $this->ships[$j]->id;
                    $this->ships[$j]->dogfights[] = $this->ships[$i]->id;
                    $dogfights[] = array(0 => $this->ships[$i]->id, 1 => $this->ships[$j]->id);
                }
            }
        }
    }
    if (sizeof($dogfights)){
        DBManager::app()->insertDogfights($this->gameid, $this->turn, $dogfights);
    }
}

Basicly, I compare all ships versus all ships (aka flights), compare their distance to each other. If it's below a threshold, I push the opposing flights ID into the flights->dogfights array.

I check if the dogfights are new, because they might have been transferred from the DB and alas be initiated a turn ago. So, each flight's $dogfights prop holds the IDs of all opposing flights this flight is tackling. Now, this part is working correctly.

Now, here comes the tricky part:

Each flight has a certain amount of Fighters (in the $structures prop). The amount of dogfights a flight can participate in is unlimited. So, it's very much possible that one friendly flight is dogfighting two enemy flights. As the number of structures / fighters in a flight is unlimited, a single flight of 5 fighters could fight against 2 flights divided into 2 and 10 fighters.

My 5 fighters now need to determine which enemy fighter, or flight, they target, and I want it to be weighted evenly, depending on the amount of fighters in a flight. In the above example, engaging 2 and 10 fighters with 5 fighters, for each of my own fighters, there is a chance of 20%/80% to pick a target from the 2/10 man flight.

I determine this here:

public function createDogfightTargetData(){
    $fires = array();
    for ($i = 0; $i < sizeof($this->ships); $i++){
        $flights = array();
        $counts = array(0);
        if ($this->ships[$i]->flight && sizeof($this->ships[$i]->dogfights)){
            for ($j = 0; $j < sizeof($this->ships[$i]->dogfights); $j++){
                $count = $counts[sizeof($counts)-1];
                $flights[] = $this->getUnitById($this->ships[$i]->dogfights[$j]);
                for ($k = 0; $k < sizeof($flights[sizeof($flights)-1]->structures); $k++){
                    if (! $flights[sizeof($flights)-1]->structures[$k]->destroyed){
                        $count++;
                    }
                }
                $counts[] = $count;
            }
            $fires = array_merge($fires, $this->ships[$i]->createFireOrders($this->gameid, $this->turn, $flights, $counts));
        }
    }
    if (sizeof($fires)){
        DBManager::app()->insertFireOrders($this->gameid, $this->turn, $fires);
    }
    return true;
}

Basically, I'm setting up an array that holds the numbers of enemy units in each index and sum it up. In the above example, where I'm fighting 2 and 10 fighters, the array would like this:

$counts = [0, 2, 12]

I pass this $counts array AND the associated Flights array (which holds object pointers to each Flight class involved in dogfights with this Flight) to another function to determine which enemy flight each friendly fighter is going to hit.

public function createFireOrders($gameid, $turn, $flights, $counts){
    $fires = array();

    //echo $this->id; echo "</br>";
    //var_export($counts);

    for ($i = 0; $i < sizeof($this->structures); $i++){
        if (!$this->structures[$i]->destroyed){
            $roll = mt_rand(0, $counts[sizeof($counts)-1]);
            $index = -1;
            for ($j = sizeof($counts)-1; $j >= 0; $j--){
                if ($roll == 0){
                    $index = 0;
                }
                else if ($roll <= $counts[$j] && $roll > $counts[$j-1]){
                    $index = $j-1;
                }

                if ($index > -1){
                    $fires[] = array(
                        "gameid" => $gameid,
                        "turn" =>$turn,
                        "shooterid" => $this->id,
                        "targetid" => $flights[$index]->id,
                        "weaponid" => $this->structures[$i]->systems[0]->id,
                        "shots" => $this->structures[$i]->systems[0]->shots
                    );
                    break;
                }

            }
        }
    }
    return $fires;
}

Here, I'm checking my own flight for "intact" fighters and if found, roll a dice from count[0] to count(sizeof($count)1), then compare the dice to each index of the $count and henceforth associate the index in $count with the flight from $flights (passed parameter).

Example:

$targets = [0, 2, 12]
$flights = [$id4, $id6]
$roll 7 -> between $targets[1] [2] -> pick $flights[2]

This function is working flawlessly, however, it feels kinda awkward.

Can this be improved in a reasonable way, and if so, how?

\$\endgroup\$
9
  • 1
    \$\begingroup\$ Again, similar to other recent question post, I don't think there is enough context here. On what class(es) do initiateDogFights(), createDogfightTargetData() and createFireOrders() live? It is hard to give feedback on specific methods without context of what classes they are in. Again, my guess is that there are fundamental problems with overall class design that a broader review of class definitions/interfaces might help. It doesn't seem to make sense to me to hold arrays of "dogfights" on every single "Flight" instance you may have. \$\endgroup\$
    – Mike Brant
    Commented Feb 16, 2017 at 21:59
  • \$\begingroup\$ ... initiateDogFights and createTargetData are methods of the global Manager class that holds all gamedata. CreateFireOrders obviously takes place on a Flight class, as you can see by the methods code. Flights need to hold the IDs of all dogfights they are participating in because...its relevant information obviously... ? github.com/AncientSion/fire server/manager.php server/flight.php \$\endgroup\$
    – user431806
    Commented Feb 17, 2017 at 7:15
  • \$\begingroup\$ Also, im not looking for a broad review, but simply a means, to, if possible, minify the existing, last 2, methods, with the same result. \$\endgroup\$
    – user431806
    Commented Feb 17, 2017 at 7:22
  • 2
    \$\begingroup\$ You may be surprised at the results you get from a broader review. For example you say you are really only interested in reviewing the last two methods in this post, while just from a glance at the first, I can see you are using an O(n^2) complexity approach for detecting proximity, something that could probably be optimized (depending on your expected density of flights on the grid) by using a different proximity detection algorithm. It's like you are trying to put out a few small fires, while a raging forest fire goes on all around you. \$\endgroup\$
    – Mike Brant
    Commented Feb 17, 2017 at 14:57
  • 2
    \$\begingroup\$ Please don't take this as me being prickly. It's just the the true purpose of a code review is not just to look at one particular algorithm or section of logic, it is to look at all aspects of code quality - design, data structures, algorithms, security, syntax, etc. If you just put out wider context, you will likely get a lot better feedback. \$\endgroup\$
    – Mike Brant
    Commented Feb 17, 2017 at 15:00

1 Answer 1

4
\$\begingroup\$

I agree with Mike Brant's comments that it would be better to have the complete code posted for a broad review but that likely won't happen- if you do want to do so then I advise you to do so in a new post, since editing your post might invalidate the advice below. Nonetheless i see a few idiomatic PHP aspects that could be improved.

The code could be simplified using PHP’s foreach loops instead of for loops- e.g.

for ($i = 0; $i < sizeof($this->ships)-1; $i++){
    for ($j = $i+1; $j < sizeof($this->ships); $j++){

this could be simplified using foreach loops with the range() function:

foreach (range(0, sizeof($this->ships)-2) as $i) {
    foreach (range($i, sizeof($this->ships)-1) as $j) {

and also the loop in createFireOrders():

for ($i = 0; $i < sizeof($this->structures); $i++){
        if (!$this->structures[$i]->destroyed){

with foreach there is no need to do the bookkeeping on the counter variable:

foreach ($this->structures as $i => $structure) {

and instead of referring to $this->structures[$i] just use $structure.


The array on this line in initiateDogfights():

$dogfights[] = array(0 => $this->ships[$i]->id, 1 => $this->ships[$j]->id);

doesn't need to have the indexes explicitly set:

$dogfights[] = array($this->ships[$i]->id, $this->ships[$j]->id);
\$\endgroup\$
1
  • 1
    \$\begingroup\$ The question is from February 2017. Still, appreciated ;) \$\endgroup\$
    – user431806
    Commented Jul 31, 2019 at 8:58

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.