Feedback/optimering av flerfunktionellt php script

Använd en bra editor eller IDE som löser det åt dig. Jag rekommenderar PHPStorm, dock inte gratis.

Så länge du inte använder eval så är det lugnt, PHP vet att saker som ligger i en variabel inte är kod utan data.

Blir det fel ordning på HTML koden?
Hur det presenteras ska du inte bry dig om, det är nått du ska lösa med CSS.
Utan det är bara att HTML dokumentet blir rätt som är viktigt.

Uppifrån och ner, så länge du inte har med loopar, funktionsanrop och liknande.
Då följer PHP givetvis dom

Som sagt: använd en bra editor. Det är en tidsinvestering väl värd insatsen. Jag ser att det fortfarande är en blandning av tabbar och mellanslag för indentering .

Ja, ska man hårdra det så ska detta gälla egentligen all output som går till webbläsaren, och inte medvetet är en del av en tagg där den ska användas just som HTML-kod. Den instinktiva frågan ska snarare vara "Ska jag inte använda `htmlspecialchars()` här?", där man ska motivera varför man inte har det, än tvärtom. Tänker man inte så, så kommer någon person någon dag putta in ett `<` där man inte förväntar sig det och så ryker hela layouten.

Om du med "PHP-injections" menar att folk kan skriva in godtycklig just PHP-kod, så bara om du använder `eval()` eller någon liknande konstruktion på indata från användaren.

Vanligare är XSS och "vanlig" HTML- eller Javascriptinjektion, vilket är nog så illa. Har du exempelvis en "gästbok" och okritiskt inte använder `htmlspecialchars()` vid utskrift så är det inga problem för en användare att inkludera kod som körs av besökande klienters webbläsare, vilket inte är önskvärt.

Försök att skapa ett minimalt exempel som uppvisar samma beteende. Under tiden du utarbetar detta minimala exempel så kommer du själv märka när det som du anser är ett problem "försvinner", och då har du i det närmaste troligen redan löst problemet. Ha även för vana att titta i den genererade HTML-koden (vanligen Ctrl+u i webbläsare) för att se var utskrifter sker där, så att det inte bara är någon CSS-regel som "fintar dig".

	$no = 1;
	$i = 1;

Använd `$no = true;`, `$i = true;` i stället om du ändå bara tänkt använda variablerna som booleanska flaggor. Det gör koden tydligare. Använd dessutom mer beskrivande variabelnamn än `$no` och `$i` — dessa namn beskriver inte särskilt mycket mer än att den första kanske beskriver ett nummer (vilket ju inte ens är sant) eller "nej" (och vad betyder då `$no = 0`? "Inte nej"? Det blir krångligt), och den andra, tja, kan vara lite vad som helst. Instinkten när man ser ett `$i` är att det är en iterationsräknare, men det stämmer inte här.

	//Convert search input for MySQL
	$likedbA_ID = '%' . $dbA_ID . '%';
	$likedbquality = '%' . $dbquality . '%';
	$likedbyear = '%' . $dbyear . '%';
	$likedbtype = '%' . $dbtype . '%';
	$likedbcountry = '%' . $dbcountry . '%';
	$likedbisroll = '%' . $dbis_roll . '%';

	$query = "SELECT * FROM coin_bases WHERE A_ID LIKE ? AND quality LIKE ? AND year LIKE ? AND type LIKE ? AND country LIKE ? and is_roll LIKE ? ORDER BY row, A_ID";

`LIKE` är en speciell funktion för strängjämförelse som tar reguljära uttryck i SQL-syntax. Det känns som att den inte helt och hållet passar här, åtminstone inte i alla fält. Vad betyder exempelvis att angivet år är "LIKE" året i databasen? Vad skiljer det mot att det angivna året är exakt det som står i databasen?

		$stmt->bind_param(
		'ssssss',

Eftersom du lagt till `%`-tecken så är alla variabler nu strängar, men egentligen är ju vissa heltal, `i`. Kodens semantiska betydelse försvinner lite här i och med den påtvingade `LIKE`-konstruktionen för data där den egentligen inte hjälper.

			if ($i == 1)

Definierar man `$i = true;` som nämndes tidigare så blir det naturligt att skriva `if ($i)`, vilket tydligare beskriver vad man menar. Det går faktiskt att skriva så ändå här i och med PHP:s implicita typkonverteringar, men det skapar en onödig risk i att råka gå på en nit, när det finns tydligare lösningar tillgängliga.

				echo "
				<table class=\"CSSTableGenerator\">
				<tr>
				    <td width=\"110\">Ark-ID</td>
				    <td>Rad</td>
				    <td>År</td>
				    <td>Kvalité</td>
				    <td>Värde</td>
				    <td>Valör</td>
				    <td>Land</td>
				    <td>Notering</td>
				    <td>Ändra</td>
				 </tr>";

Minns "tricket" med att hoppa ur PHP-läge i stället för att pressa in långa stycken HTML-kod i `echo`. Då slipper man escapea citationstecken och annat, och en (bra) editor förstår vad som är HTML eller ej och kan hjälpa till med indentering, koduppmärkning, etc. Ovanstående hade alltså kunnat skrivas:

				?>
<table class="CSSTableGenerator">
  <tr>
    <td width="110">Ark-ID</td>
    <td>Rad</td>
    <td>År</td>
    <td>Kvalité</td>
    <td>Värde</td>
    <td>Valör</td>
    <td>Land</td>
    <td>Notering</td>
    <td>Ändra</td>
  </tr>
<?php

Gör man detta konsekvent så blir det åtminstone i mina ögon lättare att se strukturen, och det är en del i att separera logik från presentation. Jag brukar låta HTML-koden och PHP-koden ha separata logiska indenteringsnivåer i dokumenten; vissa brukar inte ha det. Jag uppskattar att även den resulterande HTML-koden blir korrekt indenterad då det underlättar felsökning i mina ögon, men men.

                if (!$value || $value == 0)

Vad betyder egentligen detta i ljuset av PHP:s implicita typkonverteringsregler? Faktum är att dina båda test säger samma sak med tanke på vilka värden `$value` kan ta.

			    echo "</td>

			    <td>$type</td>
			    <td>$country</td>
			    <td>$notes</td>
			    <td><a href=\"edit.php?id=$id\"><img src=\"images/page_white_edit.png\" alt=\"\" width=\"16\" height=\"16\" />Ändra</a></td>
			</tr>";

Som innan: var inte rädd för att skriva exempelvis:

			    ?>
    </td>

    <td><?=$type?></td>
    <td><?=$country?></td>
    <td><?=$notes?></td>
    <td><a href="edit.php?id=<?=$id?>"><img src="images/page_white_edit.png" alt="" width="16" height="16">Ändra</a></td>
  </tr>
<?php

En tom alt-text är dessutom ekvivalent med att inte ge attributet alls, så fyll på med något.

	//Now we calculate the SUM value
[…]
	$mysqli->close(); //Close connection to Mysql
}// END search Func.

Kommentarer ska snarare säga varför du gör saker än bara upprepa det som står i koden. Att `$mysqli->close();` betyder "Close connection to Mysql" är ju tydligt av bara kodraden i sig.

"Now we calculate the SUM value" — ja, koden visar ju direkt att vi beräknar ut ett värde, men vilket, och varför? Kommentaren skulle kanske snarare vara "Find entries that match the search criteria and print the sum of their values", dvs något som säger att vi summerar vissa värden, och vilka. Kanske är kommentaren till och med överflödig om koden är enkel nog; det finns vissa som går så långt som att säga att kod aldrig ska behöva kommentarer om den är tydlig nog, vilket är klart överdrivet (de skulle inte jag vilja arbeta med ), men det är viktigt att tänka på att kommentarerna har en annan funktion än koden i sig.

"END search Func" är också lite "tårta på tårta" när vi har slutparantesen precis där. Har man en massiv funktion så kan det finnas applikationer av sådana kommentarer, men oftast är det en sak där konsekvent indentering talar för sig själv.

I språk med doc-kommentarer (Javadoc, XML comments och liknande) så lever jag i princip efter den regeln.
Alla metoder och klasser ska ha doc-kommentarer, däremot är inline-kommentarer förbjudna.
Istället får man skriva om koden (mellanlagra uträkningar i namngivna variabler istället för en stor uträkning,
bryta ut en uträkning till en egen metod eller liknande). Jag gillar det då det gör att man blir tvingad att
skriva enkel och läsbar koden med korta metoder. Korta metoder gör inte bara koden mer läsbar utan det
minskar också upprepning och gör koden mer testbar.

EDIT: Förtydligande
Att helt utelämna alla typer av kommentarer är en helt annan sak och alla argument jag har hört för det
är bara dåliga ursäkter på att vara lat

Lite OT, men eftersom vi pratar en del om kodkvalité så kan det vara intressant

    //This is declared so that our while statement only will write the table head once
	$table_head = TRUE;

SKriv ut förslagsvis ut `$table_head` innan loopen, så kommer det ju bara ske en gång likväl.

    $conditions = array();
    $parameters1 = array();
    $parameters2 = array();
    if ($dbA_ID) {
        $conditions[] = "A_ID = $dbA_ID";
    }
    if ($dbquality) {
        $conditions[] = "quality = $dbquality";
    }
[…]

Det blir en del koddubblering här. Det är ofta något att undvika. Vi ser att du gör egentligen samma kontroll av en viss variabel och sedan lägger till en post till samma vektor. Vi ser ju också som du nämner att du har behövt överge "prepared statements" för att kunna söka på flera saker, vilket inte ska behövas.

Vi kan också notera att vi inte behöver deklarara en variabel som vektor på förhand i PHP, utan det är bara att skippa det steget, och det fungerar precis som nu ändå.

En approach för att ha en adaptiv sökning beroende på vilka termer som ges är att ge defaultvärden till funktionens inargument genom exempelvis:

function search($dbA_ID='', $dbquality='', $dbyear='', $dbtype='', $dbcountry='', $dbis_roll='')

(EDIT: Jag använde först `NULL` som defaultargument för att kunna just skilja den tomma strängen från "ingenting", men här är det nog enklare att använda den tomma strängen, för att slippa specialbehandla `$_POST`-variabeln innan funktionsanropet.)

PHP saknar tyvärr hantering av namngivna parametrar i funktionsanrop, vilket eventuellt hade kunnat vara smidigt här. Ett alternativ är att skriva funktionen så att den kallas med en associativ vektor med motsvarande nyckelnamn, men det blir likväl extra kod i hanteringen.

Därefter kan du kontrollera om dessa satts till något speciellt i funktionsanropet med t ex:

if ($dbA_ID !== '') {

osv.

I den kod du visade tidigare så adderade du till tre vektorer i varje `if`-sats likt:

        $conditions[] = 'A_ID = ?';
        $parameters1[0] = 'i';
        $parameters2[0] = $dbA_ID;

Du hade inte behövt hårdkoda indexet i dina två senare vektorer, utan de hade inkrementerats på samma sätt som den första. Dock så ser vi återigen att det blev en del koddubblering när samma kod skrevs om och om igen för varje fält, där bara variabelnamnen skiljer. Du dubblerade även suffixet ` = ?` för varje fält. Spontant så känner jag att det är läge för en funktion som tar en vektor av vektorer som input och utför dessa mekaniska steg själv, så att det enda som utåt behöver definieras är just kolumnnamn, datatyp och värde. `array_column()` kan vara av nytta för att emulera en sorts manuell matris i PHP.

En implementation av dessa bitar är pseudokodmässigt väldigt rättfram, men PHP har en förmåga att komma med vissa små "gotchas" så som hantering av referenser och bångstyrig syntax för vektoroperationer. Ett övertydligt kommenterat skelett till en sådan variant av sökfunktionen som använder prepared statements och åter fungerar med ett variabelt antal söktermer samtidigt skulle kunna vara:

/**
 * List database entries matching the search terms. Return NULL if no search
 * terms are given.
 */
function search($dbA_ID='', $dbquality='', $dbyear='', $dbtype='',
    $dbcountry='', $dbis_roll='')
{
    require 'db_connect.php';

    // Poor man's ENUM.
    defined('COLUMN_NAME') or define('COLUMN_NAME', 0);
    defined('DATA_TYPE') or define('DATA_TYPE', 1);
    defined('SEARCH_REF') or define('SEARCH_REF', 2);

    // Define database column names, types and corresponding in data variable.
    // Index 0: column name
    //       1: data type
    //       2: reference to search parameter
    $dbSearchEntries = [
        ['A_ID', 'i', &$dbA_ID],
        ['quality', 'i', &$dbquality],
        //…
    ];

    // Select search entries where a search term has been given.
    $dbParameters = array_filter($dbSearchEntries, function($dbSearchEntry) {
        return $dbSearchEntry[SEARCH_REF] !== '';});

    // If no search terms were given, return characteristic value.
    if (empty($dbParameters)) {return NULL;}

    // Create `WHERE` condition in the form of a prepared statement.
    $dbColumnQueries = implode(' AND ',
        array_map(
            function($s) {return $s .= ' = ?';},
            array_column($dbParameters, COLUMN_NAME)
        )
    );
    // Create `bind_param()` style data type parameter.
    $dbColumnTypes = implode(array_column($dbParameters, DATA_TYPE));

    $query = "
        SELECT *
        FROM coin_bases
        WHERE $dbColumnQueries
        ORDER BY row, A_ID";

    $stmt = $mysqli->prepare($query) or die('Failed to prepare statement.');

    // Dynamically bind parameters depending on given search terms.
    call_user_func_array(
        [$stmt, 'bind_param'],
        array_merge(
            [$dbColumnTypes],
            array_column($dbParameters, SEARCH_REF)
        )
    );

    $stmt->execute();
    //…
}

Tanken är att det ska räcka med att fylla på `$dbSearchEntries` med den faktiskt skiljande information som varje kolumn bär med sig, och sedan låta resten av funktionen böka runt det mekaniska.

I din kod så skapade du tidigare en kopia av värdet i `$parameters2` (tänk på att använda bättre variabelnamn för din egen skull) — ovan lagrar jag motsvarande värde som en referens. Det spelar ingen roll i minnesåtgång (om man inte förändrar värdet) då PHP använder en copy-on-write-implementation för variabler, utan främsta skälet att jag gör det är för att jag då kan använda `call_user_func_array()` direkt på resultatet.

I praktiken kanske det vore bättre att bryta ut själva sökningen och presentationen i olika delar, där `search()` bara returnerar ett `$stmt` som går att loopa över snarare än skriver ut HTML. Det skulle frikoppla logik och presentation.

EDIT: Mitt initiala vektorfilter:

    // Build up search parameters from the relevant given function arguments.
    $buildDbQuery = function($column, $type, &$value) use (&$dbParameters) {
        if ($value !== NULL) {$dbParameters[] = [$column, $type, &$value];}
    };
    foreach ($dbSearchEntries as $dbSearchEntry) {
        call_user_func_array($buildDbQuery, $dbSearchEntry);
    }

finns redan i standardbiblioteket som `array_filter()`… Använder den existerande funktionen i stället och sparar några rader .

Jag ändrar mig också gällande att använda `NULL` som default-inargument. Det är nog enklare att behandla `$_POST`-variabler ifall vi antar att den tomma strängen är "icke angivet värde", skulle jag säga.

Ah, ja, men det löser du bättre genom att kolla exempelvis

if ($stmt->num_rows) {
    // Vi har minst en träff, skriv ut header här.
    while ($stmt->fetch()) {
        // Loopa över raderna
    }
}

Jag använder `define()` för att undvika att behöva ha "magiska tal" senare i koden. `array_column($dbParameters, 1)` säger mig ingenting om vad jag gör, men `array_column($dbParameters, DATA_TYPE)` gör att jag direkt förstår vad som menas. Om jag i framtiden vill lägga till fler index eller ändra strukturen på vektorn så behöver jag ej heller jaga heltalsindex i koden, utan kan ändra på ett centralt ställe.

Att jag kollar om `COLUMN_NAME` redan är definierad innan jag försöker definiera det är för att man per konstruktionen inte får återdefiniera konstanter (då vore de inte mycket av "konstanter"), och eftersom definitionen är inuti funktionen så kommer jag annars få en varning om jag kallar på funktionen mer än en gång per session. Troligen kommer detta inte hända mer än i testsyfte.

En bättre lösning vore egentligen att lägga dessa definitioner på en annan plats, exempelvis precis innan funktionen definieras. Då behövs inte baletten med `defined(…) or define(…);`. Jag minns att jag hade någon initial tanke med att inte göra detta i exempelkoden jag postade, men jag minns den inte glasklart just nu .

EDIT: För att sväva ut lite så tror jag min tanke var att inte "förorena den globala namnrymden" genom att definiera konstanterna inuti funktionen, men det hjälper faktiskt inte överhuvudtaget här, eftersom de ändå registreras globalt vid funktionsanropet, vilket också är just anledningen till att jag behöver kolla med `defined()`. Det finns ingen anledning att ha dem inne i funktionen utöver att logiken samlas på ett ställe. Man hade kunnat låta dem vara lokala variabler, men en konstant är egentligen en mer naturlig datatyp för ändamålet.

I praktiken löser man vanligen sådana problem med namnrymder (nytt i PHP sedan 5.3) genom att sammanfläta dessa med en katalogstruktur, definiera klasser i separata filer och sedan ladda dessa automatiskt vid anrop via en autoloader, men det är initialt väl mycket att böka med, och inte en prioritet i ett mindre projekt. Det är mer värt att fokusera på annat till en början.

    // If no search terms were given, return characteristic value.
    if (empty($dbParameters)) {return NULL;}

Ifall alla söktermer var tomma, dvs "den tomma strängen", så har vi ju egentligen inga kriterier för sökningen. Ovanstående rad kollar om `$dbParameters` (som efter `array_filter` ovan enbart innehåller de fält som hade giltiga motsvarande söktermer) är tom, dvs om inga söktermer givits. Då avslutas hela `search()` och vi returnerar bara `NULL`, så där du kallar på `search()` i ditt skript så kan du kolla typ: `if (search([parametrar]) === NULL) {// Skriv något om att man måste ange något sökkriterium för att söka.}`. `false` kanske är bättre än `NULL`, men det kan bero på kontext. `false` skulle också kunna betyda "korrekt formaterad sökning, men inga träffar", eller tvärtom om man vill det.

En annan möjlighet vore att tolka "inga angivna sökkriterier" som "visa alla databasrader". I stället för att returnera `NULL` så skulle man då exempelvis kunna sätta `$dbColumnQueries` till `1 = 1` för att returnera alla rader (eller skriva om `$query` så att man skippar `WHERE`-raden helt, men det finns en fördel i att kunna använda samma kodskelett i båda fallen, och `1 = 1` är ett rätt vanligt idiom i SQL-kontext).

Snabba punkter:

<?php
    $count_10 = 0;
    $count_8 = 0;
    $count_6 = 0;
    $count_4 = 0;
    $count_2 = 0;

Du behöver inte förinitiera variabler här.

    $query= "SELECT count(quality) FROM coin_bases WHERE quality = 10;";
    if ($stmt = $mysqli->prepare($query))
    {
        $stmt->execute();
        $stmt->bind_result($quality);
        while ($stmt->fetch())
        {
            $count_10 = $quality;
        }
    }
…
…
…

Denna bit är ett typexempel på något att låta en loop göra. Du behöver ändå vektorn `[10, 8, 6, 4, 2]` senare, så skapa den innan detta stycke och loopa över den. Du kan exempelvis kanske låta talen vara nycklar i en associativ vektor, där värdena är motsvarande `$count`. `foreach()` och `array_keys()` kan vara till nytta.

3D-diagram må se flashigt ut, men det kan ofta distrahera från poängen och till och med förvränga resultatet. I detta fall gör perspektivet att de tårtbitar som hamnar längst bak ser mindre ut än de som är längst fram, vilket ger en felaktig representation av det faktiska förhållandet (ja, tidningar använder ibland liknande statistikframställning, men det gör det inte OK ). Tårtdiagrammet är inte nödvändigtvis fel här (även om ett stapeldiagram med faktiska värden hade gett mer och tydligare information på mindre plats), men enkel 2D är nog att föredra ur informationssynpunkt.

Tillägg, med mer tid:

if (isset($_GET['do']) == "quality")

Detta gör inte vad du föreställer dig. `isset()` kommer returnera sant eller falskt, inget annat. Du frågar nu PHP "är sant/falskt lika med strängen 'quality'?" — en sådan fråga skulle få många språk att kollapsa, men PHP kämpar livet ur sig för att försöka tolka detta ändå, och tänker "OK, en tom sträng är ju typ 'falsk', så då måste en icke-tom sträng vara 'sann', så användaren frågar mig egentligen bara om `$_GET['do']` är satt". Det du skriver ovan är alltså ekvivalent med att bara skriva `if (isset($_GET['do']))`, eftersom högerledet ändå alltid kommer evaluera till `true`.

    include 'db_connect.php';
    $count_10 = 0;
    $count_8 = 0;
    $count_6 = 0;
    $count_4 = 0;
    $count_2 = 0;

    $query= "SELECT count(quality) FROM coin_bases WHERE quality = 10;";
    if ($stmt = $mysqli->prepare($query))
    {
        $stmt->execute();
        $stmt->bind_result($quality);
        while ($stmt->fetch())
        {
            $count_10 = $quality;
        }
    }
    $query= "SELECT count(quality) FROM coin_bases WHERE quality = 8;";
    if ($stmt = $mysqli->prepare($query))
    {
        $stmt->execute();
        $stmt->bind_result($quality);
        while ($stmt->fetch())
        {
            $count_8 = $quality;
        }
    }
    $query= "SELECT count(quality) FROM coin_bases WHERE quality = 6;";
    if ($stmt = $mysqli->prepare($query))
    {
        $stmt->execute();
        $stmt->bind_result($quality);
        while ($stmt->fetch())
        {
            $count_6 = $quality;
        }
    }
    $query= "SELECT count(quality) FROM coin_bases WHERE quality = 4;";
    if ($stmt = $mysqli->prepare($query))
    {
        $stmt->execute();
        $stmt->bind_result($quality);
        while ($stmt->fetch())
        {
            $count_4 = $quality;
        }
    }
    $query= "SELECT count(quality) FROM coin_bases WHERE quality = 2;";
    if ($stmt = $mysqli->prepare($query))
    {
        $stmt->execute();
        $stmt->bind_result($quality);
        while ($stmt->fetch())
        {
            $count_2 = $quality;
        }
    }
    $mysqli->close();

Tumregel: varje gång du copy+pastear närpå identiska kodstycken, exceptionellt så ifall det är på rad på detta sätt, så skapar du framtida problem. Tänk på koden i termer om funktioner och data. Alla de kodstycken du skrev innan utför ju samma funktion, med enda skillnad att indata skiljer.

Den biten kan tydligare och med mindre koddubblering skrivas som exempelvis:

    include 'db_connect.php';

    $qualities = [10, 8, 6, 4, 2];
    // ... eller `$qualities = range(10, 2, 2);` om man vill vara rolig.

    $query = '
        SELECT COUNT(*)
        FROM coin_bases
        WHERE quality = ?
        ';
    $stmt = $mysqli->prepare($query);
    $stmt->bind_param('i', $quality);
    $stmt->bind_result($count);

    foreach ($qualities as $quality) {
        $stmt->execute();
        $stmt->fetch();
        $quality_count[$quality] = $count;
    }
    $stmt->close();

(Här ser man också en fördel i att kunna återanvända kompilerade SQL-frågor mot databasen med förändrad indata; det kan vara en märkbar prestandavinst när det handlar om många och stora frågor.)

Man hade med fördel kunnat bryta ner ovanstående i externa funktioner, för att inte skapa en enda monstersats som gör hundra saker. Låter man funktioner göra enkla och väldokumenterade saker så är det lättare att felsöka. En övning kan vara att skapa funktionen `coinQualityCount($array)` så att hela det ovanstående stycket kan skrivas:

    $quality_count = coinQualityCount([10, 8, 6, 4, 2]);

och låt den specifika implementationen av hur det räknas döljas i denna funktion, så att du kan fokusera på logikgången i skriptet i stället.

I vilket fall: `$quality_count` är nu en associativ vektor med nycklar `10`, `8`, `6`, `4`, `2` som innehåller motsvarande radantal i databasen. Detta har även den stora fördelen att du inte behöver återdefiniera dessa "magiska" värden på fler ställen i koden. Där du just nu senare skriver:

    $MyData->addPoints(array($count_10,$count_8,$count_6,$count_4,$count_2),"ScoreA");

kan du nu i stället skriva:

    $MyData->addPoints(array_values($quality_count), "ScoreA");

och där du skriver:

    $MyData->addPoints(array("10","8","6","4","2"),"Labels");

kan du nu enklare skriva:

    $MyData->addPoints(array_keys($quality_count), "Labels");

Det låter även rimligt att det skulle finnas en `addPoints`-variant som direkt tar en associativ vektor och listar ut vad som är nycklar och värden själv; kolla i dokumentationen för biblioteket.

På detta sätt så ser du att ditt kodstycke sköter sig själv i mycket större grad — du behöver inte gå in och pilla på fem ställen bara för att du vill lägga till en myntkvalitetspost, utan det räcker med att lägga till ett element i den enstaka vektor där du definierar dessa värden. Det gör det också mycket enklare att bryta upp logiken i funktioner som sköter sig själva när de får lite indata att jobba med.

SELECT count(quality)

`COUNT(*)` är ett vanligare idiom, som dessutom är kraftigt optimerat i hastighet. Du räknar ju rader, inte just fältet `quality`. Ifall ditt fält är definierat som `NOT NULL` så kommer mig veterligen MySQL ändå skriva om anropet till `COUNT(*)`, men annars så gör din räknare faktiskt troligen inte riktigt vad du tänker (även om det ger samma resultat, men det tar längre tid, och är onödigt). Se MySQL-manualen för `COUNT()`, eller denna hastighetstest för faktiska värden (användarkommentarerna innehåller en del felaktigheter; lita hellre på manualen).

                array_push($cc, $quality);

`array_push($a, $b);` är identiskt med `$a[] = $b;`, men det senare är med vana lättare att läsa (färre tecken, man ser direkt var tilldelningen sker), du slipper förinitiera vektorn (se manualen) och det är snabbare då det undviker ett funktionsanrop till förmån för en direkt språkegenskap.

I förbigående kan jag nämna att `$cc = [];` är ekvivalent med `$cc = array();` (sedan PHP 5.4), men färre tecken och behagligare om man är van vid exempelvis Python. Speciellt om man skapar en vektor inuti ett funktionsanrop så är `[]` att föredra då det inte bygger på teckensoppan så förbaskat. Notera dock att det är sällan man överhuvudtaget behöver förinitiera objekt i PHP på det sätt du gör, om man använder snabbsyntaxen för `array_push()`. För detaljens skull kan jag nämna att `[]` inte är snabbare än `array()` då det senare inte är en egentlig funktion i PHP utan en språkkonstruktion, precis som `[]`. Hastigheten är sällan eller aldrig viktig i sig, men det kan ändå guida vad som bör vara praxis när alla andra faktorer är desamma.

        while ($stmt->fetch())
        {
            if ($quality)
            {
                array_push($cc, $quality);
            }
            else
            {
                array_push($cc, 0);
            }

är (här) helt ekvivalent med

        while ($stmt->fetch()) {$cc[] = $quality;}

Du behöver inte krångla med "specialfallet" att `$quality` är 0 — du ska ju ändå bara lägga `$quality`:s värde till vektorn. I ditt exempel ovan så säger du i praktiken: "ifall $quality inte är 0, så lägg till $quality, men ifall $quality är 0 så lägg till 0", vilket är samma logisk som "lägg till $quality".

Du ska ej heller använda `while ($stmt->fetch())` här. Du vet ju på förhand att du bara har en fråga, som ger ett resultat. I stället kan du skriva:

        $stmt->fetch();
        $cc[] = $quality;

    $quality_value = $quality_value - 2;

Detta skrivs enklare

    $quality_value -= 2;

men mer konceptuellt rätt här tycker jag är att lista önskade element explicit i en vektor. Det råkar vara så att man kan använda `while`-konstruktionen på detta sätt, eller `range()` som jag skrev i en egentligen skämtsam kommentar i mitt exempel ovan, men det är egentligen bara en artefakt av ett visst specialfall snarare än något som beskriver något om datan i sig. Optimering som tar bort den semantiska betydelsen av vad man gör är ytterst sällan något man bör göra förrän man faktiskt märker att det behövs i hastighetssynpunkt.

Du återanvänder ju nu ej heller din databasfråga, vilket är en av poängerna med prepared statements och att binda parametrar. Kolla igenom mitt kodexempel i mitt senaste inlägg: det bör vara att kopiera rakt av och ska inte innehålla några tekniska lurigheter som det förra. Vi föriniterar variablerna, förbereder databasfråga, loopar över variablerna med `foreach()` och lagrar resultatet: inga konstigheter.

Om värdet är 0 så kan man argumentera för att det korrekta för ett cirkeldiagram är att rita ut area 0, dvs ingenting. Ifall du vill ha med denna information så är som du nämner just ett stapeldiagram kanske att föredra.

function coinQualityCount($get_array, &$quality_count)

Skapa en vektor som du returnerar i funktionen i stället; klart mindre bakvänt.

if (isset($_GET['do'])) {$do = $_GET['do'];}
if ($do === 'QualityQuota')

"Om `$_GET['do']` är satt, så sätt `$do` till `$_GET['do']`." — men vad händer om `$GET_['do']` inte är satt? Då sätts ju aldrig `$do`, men ändå antas det direkt på raden efter att den finns. Det håller inte, och därför klagar PHP på dig.

Ett idiomatiskt sätt är att låta `$_GET['do']` innehålla ett namn på den åtgärd du vill att skriptet ska utföra (så som den också gör här), och först kolla `isset($_GET['do'])`, och sedan gå in i en `switch`-sats. Om du bara har en möjlig `do`-åtgärd så räcker det visserligen med:

if (isset($_GET['do']) && $_GET['do'] === 'QualityQuota') {
    …

men har man bara ett enda möjligt värde så behöver man ju egentligen inte kontrollera värdet alls: är `$_GET['do']` satt så är det ju just detta värde. Att skriva det som en `switch`-sats kan underlätta inför om man lägger till funktionalitet i framtiden, vilket alltid är en ädel tanke, men inte ska gå till överdrift. Var balansen ligger är något man ständigt får revidera efter erfarenheter.

    $parameters = [
    [10, 8, 6, 4, 2],
    ["10", "8", "6", "4", "2"]
    ];

Varför behöver du två olika uppsättningar? PHP är dynamiskt typat, så strängen `"5"` och heltalet `5` går troligen precis lika bra att använda, så det bör inte finnas någon anledning att upprepa definitionen i vektorn. Även om du skulle behöva en uppsättning med heltal och en med motsvarande strängar så är det naturligare att omvandla detta på plats i stället för att spara redundant information.

    coinQualityCount($parameters[0], $array_result);

Använd som sagt `return` i funktionen i stället och skriv

    $array_result = coinQualityCount($parameters[0]);

eller motsvarande. Det är mer rättfram.

$a = [10, 8, 6, 4, 2];
$b = [10, 8, 6, 4, 2];
$repeat = 0;

$repeat_to = count($a) - 1;

while ($repeat <= $repeat_to)
{
    $c[] = $a[$repeat] + $b[$repeat];
    $repeat = $repeat + 1;

}
echo implode(" ", $c);

Jag är inte helt med på varför du behöver lägga ihop två vektorer. En tumregel är också att om standardbiblioteket saknar en funktion så kanske det betyder att man gör något som inte riktigt är meningen att man ska göra. Jag tror att problemet "försvinner" genom att titta närmre på frågeställningen. Bara för sakens skull så skulle ett kompakt sätt att addera två vektorer elementvis kunna vara:

$array_a = [1, 2, 3]; $array_b = [20, 30, 40];
$a_and_b_summed_elementwise = array_map(function ($a, $b) {return $a + $b;}, $array_a, $array_b);
var_dump($a_and_b_summed_elementwise);

vilket ger output:

array(3) {
  [0]=>
  int(21)
  [1]=>
  int(32)
  [2]=>
  int(43)
}

men jag undrar som sagt om det ska behövas. Det kan nog lösas bättre direkt i databasfrågan, eller något.

Just nu så skapar du en vektor som en parameter i funktionsanropet, som funktionen tar emot via referens och modifierar. Mer rättfram är att skapa en funktion som returnerar en vektor som man tilldelar sin lokala variabel.

Jag skrev att `switch`-satsen med fördel kan användas om du har olika åtgärder som du vill behandla. Här har du bara en: men då behövs egentligen inte den extra `if`-satsen överhuvudtaget. Är `$_GET['do']` satt så vet du ju vad den innehåller. Kanske man kunde använda ett variablenamn som tydligare klargör att det är ett boolenaskt värde, men men.

Det handlar i stort sett aldrig om hastighetsvinster, utan om att skriva kod som är lättläst och "idiomatisk", dvs som inte är "överraskande" för det den har tänkt göra. Vad är "idiomatiskt"? En definition är att man skriver på det sätt som vore det naturliga för någon som är väldigt väl bekant med språket. Sådana idiom utvecklas inte sällan med hastighetsaspekten på näthinnan (på samma sätt som språket i sig utvecklas till att låta idiomatiska uttryck vara just mest effektiva), så den biten kommer vanligen på köpet, men det är nästan aldrig anledningen till att skriva på ett visst sätt. Det handlar snarare om att konstruktionerna ska se naturliga ut. Ju mindre "överraskande" koden skulle vara för någon som är van, desto bättre åldras den.

Frågan är om dessa datatyper egentligen inte borde ligga i olika tabeller, när de ändå behöver använda olika kolumner.

    $q_count = 0;

    $query = '
        SELECT quantity
        FROM coin_bases
        WHERE quality = ? AND is_roll = 1
        ';
    $stmt = $mysqli->prepare($query);
    $stmt->bind_param('i', $quality);
    $stmt->bind_result($quantity);

        $stmt->execute();
        while ($stmt->fetch())
        {
            $q_count = $q_count + $quantity;
        }
        $quantity_sub_count[$quality] = $q_count;
    }
    $stmt->close();
    $mysqli->close();

Låt databasen summera i stället för att plocka ut en mängd data och loopa över den i PHP-kontext. Sätter du `SUM(quantity)` i din första databasfråga så ger MySQL direkt summan av träffande celler.

Det är ofta bra att låta databasen jobba; det är det den gör bäst. Ur hastighetssynpunkt så är det ofta gynnsamt att inte överföra stora datastrukturer, utan i stället låta databasen sköta stor del av logiken direkt och relatera ett så färdigt resultat som möjligt (inom rimlighet). Du skulle säkert kunna få den totala summan (mynt+rullar) i en enstaka fråga genom att använda lite mer komplicerad logik, typ `SELECT SUM(CASE WHEN is_roll = 1 THEN quantity ELSE 1 END) AS quality_sum FROM coin_bases WHERE quality = ?`, men om jag inte i inspirationens hetta lyckades dra fungerade SQL-syntax direkt ur min magiska databashatt utan att ens ha koll på tabellstrukturen så kan det krävas lite debuggande och tankeverksamhet. Ifall det vållar alltför mycket problem så är det något som kan vara ett senare projekt att fixa.

Ja, passar det så är det inga problem. I indatahantering skulle jag säga att är vanligt att en viss `case`-sats kallar en funktion som i sin tur efterhand kallar `header('Location: …')` som skickar iväg användaren, vilket i praktiken är likvärdigt.

$tinymce = 1;
$tabs = 1;
$jQuery = 1;
$tinymce = 1;
$hide = 1;

Om du ska använda variabler som booleanska "flaggor" så är det tydligare att ge dem värdet `true` i stället för `1`. Du behöver troligen inte ändra någonting alls i resten av din kod (eftersom `1` och `true` genom lös typning konverteras till varandra), men det underlättar logik i en del fall och är som sagt tydligare för vad som menas.

$id_help = '';
if (isset($_GET['id'])) {$id_help = $_GET['id'];}

Detta är ett typfall för att använda PHP:s ternary operator för att tydliggöra vad som händer. Ovanstående är ekvivalent (i praktiken sant i PHP >=5.4, men se fotnot * för en teknisk notis) med:

$id_help = isset($_GET['id']) ? $_GET['id'] : '';

Det gör det semantiskt tydligare att man vill sätta en viss variabel till något, och man slipper upprepa denna variabel fler gånger. Inte helt uppenbart att läsa förstå gången man ser det, men naturligt med vana. Man kan även nästla flera sådana satser, men bör nog generellt inte göra det av läsbarhetsskäl.

Det verkar bara som att du använder denna variabel på ett ställe (även om jag kanske inte ser hela koden), varpå det kanske är enklare att samla logiken vid funktionsanropet:

    case 'help_change':
        $id_help = isset($_GET['id']) ? $_GET['id'] : '';
        helpChange($id_help);
        break;

eller alternativt

    case 'help_change':
        helpChange(isset($_GET['id']) ? $_GET['id'] : '');
        break;

men som synes så blir det lätt svårare att läsa när man nästlar uttryck på det sättet. Att låta saker ske på olika rader ger också en fördel i felsökning, då ett radnummer från PHP-kompilatorn direkt säger vad felet är.

$action = '';
if (isset($_GET['do'])) {$action = $_GET['do'];}
switch($action){

Även här kan ?:-syntax förtydliga lite, men än bättre vore nog:

if (isset($_GET['do'])) {
    switch($_GET['do']) {
        …
    }
}

Att du just nu sätter `$action` till den tomma strängen är ju bara för att undvika att `switch`-satsen träffar något, men du ber ändå PHP-tolken att kontrollera variabeln mot varje `case`, även fast du vet att ingen kommer träffa. Det är onödigt jobb, och det förmörkar den tänkta logiken. Genom att avdela `switch`-satsen inuti ett `if`-block så säger du till PHP (och läsaren) att det är något vi gör ifall `$_GET['do']` är satt. Du får även möjligheten att lägga till en avslutande aktion på ett naturligt sätt efter att `switch`-satsen är klar, eller som alternativ till att `if`-satsen träffat. I ditt fall så vore det naturligt att skriva:

if (isset($_GET['do'])) {
    switch($_GET['do']) {
        …
        default:
            // Hantera att de angett en icke-giltig funktion.
    }
} else {
    systemMenu_content();
}

Ditt nuvarande `default`-block är egentligen inte tänkt att vara svaret på frågan "vad ska göras om `$_GET['do']` inte var lika med någon känd aktion?", utan snarare "vad ska göras om `$_GET['do']` inte var satt?", varpå det passar bättre som ovan.

        printAll_content();
…
        databaseBackup();
…
        help_content();

Det finns olika konventioner för hur man namnger filer/variabler/funktioner/klasser/konstanter/etc., och frånsett vissa generellt vedertagna standarder så är valet rätt fritt, men man ska sträva efter att vara konsekvent inom ett och samma projekt. De tre funktionerna ovan blandar mellan `lowerCamelCase`, `lower_underscore_separated` och en kombination av de båda. Håller man konsekvensen så blir det lättare att läsa, då hjärnan direkt kan koppla ett namn till ett koncept.

    default:
        systemMenu_content();
        break;

Du behöver ingen `break` i sista klausulen, och eftersom `default` alltid är sist så behövs den aldrig där. Det är ingen som tar skada av att det är där, men det är en kodrad utan nytta.

Ja, i det exemplet så var tanken att någonstans i bakgrunden fanns en klass som hette exempelvis `Site`, som hade instantierats till det lokala objektet `$site` genom anropet `$site = new Site();` någonstans tidigare. Poängen (en av dem) med att använda klasser är att funktioner som konceptuellt hör ihop kan samlas i en klass, vilket ger mer struktur till kodandet. Man kan säga att det är en utvidgning av det du redan börjat göra när du faktoriserar ut funktionalitet i separata funktionsfiler.

Om klassen definierats enligt:

class Site
{
    const SITE_NAME = 'Min fina sida';

    public function skrivHej()
    {
        echo "Hej!";
    }
}

så hade vi inifrån dokumentet kallat på funktionen genom `$site->skrivHej();`, där `->` kallas "objektoperatorn" i PHP (`T_OBJECT_OPERATOR`). En annan närbesläktad operator som du kan se är den som sköter statiska variabler och funktioner som man i stället kan kalla på direkt via klassnamnet genom `::`-operatorn likt: `Site::SITE_NAME`. `::` kallas internt i PHP-motorn obriljant nog för det hebreiska namnet för "dubbelkolon", vilket gör att en programmarere tror det är första april första gången man ser felmeddelandet "Parse error: syntax error, unexpected T_PAAMAYIM_NEKUDOTAYIM" (ytterligare trivia: det är till och med felstavat).

Meningen och fördelarna med objektorientering är ett massivt ämne, och den vanligaste reaktionen första gången någon ser det (mig inkluderat) är att vara rätt skeptisk, men det har definitivt stora fördelar inom många områden.

Just PHP var ursprungligen inte alls tänkt att vara objektorienterat, men detta byggdes på med tiden, och sedan åtminstone PHP 5 så är det ett ytterst naturligt sätt att använda PHP, och jag vågar säga att mer eller mindre alla större PHP-projekt är uppbyggda på det sättet. Om man strukturerar sina klasser rätt så undviker man spagettikodsfällan som är ytterst vanlig i övrigt när man ser PHP ("om man strukturerar sina klasser rätt" — med ett stort icke-trivialt "om").

En sak värd att nämna är att PHP kan använda en så kallad autoloader som automatiskt kan kontrollera vissa sökvägar för att leta upp en klass, så om man exempelvis skriver `$dbConnection = new MySite\DB\DbConnection();` så kommer PHP automatiskt leta upp klassdefinitionen i filen `include/MySite/DB/DbConnection.php` (om man konfigurerat sin autoloader på detta sätt). Man slipper då att explicit inkludera en harang av filer i början av varje fil, och kan lita på att PHP själv listar ut var de funktioner du kallar är någonstans. Detta är en ytterst smidig funktion när man väl fått kläm på det, och det kopplar även in i tänket med namnrymder som inkluderats i PHP >=5.3.

———

*: I PHP <5.4 så var ?:-konstruktionen inte ekvivalent med `if-else`-varianten internt, då `?:` alltid skapade en ren kopia av värdet som tilldelades i stället för att använda copy-on-write som språket vanligen gör vid tilldelning. Ifall det handlade om stora vektoroperationer kunde det skilja många storleksordningar i hastighet, men sedan PHP 5.4 har detta fixats (changelog säger Improved ternary operator performance when returning arrays).

I ditt javascript försöker du läsa ut "firstname" men din json är ju bara en array av 3 strängar?

Radera .firstname och ha bara kvar "obj[i]" så kan det nog kanske funka.