Ocena jakości kodu

Question

Cześć wszystkim, proszę was o przejrzenie o ocenienie mojego kodu, sam część poprawiłem, ale jeszcze zostało pare rzeczy, które bym poprawił, ale nie mam pomysłu jak to zrobić.

Więc tak to jest główna klasa php:

class ConnectToDatabase
{
	private $imgCheck='<img src="oppin.pl/img/check.gif" style="width:25px; height:25px;" alt="">';
	private $imgUncheck='<img src="oppin.pl/img/uncheck.gif" style="width:25px; height:20px;" alt="">';

	public $connection;

	public function ConnectWithDatabase($host,$db_user,$db_password,$db_name)
	{
		$this->connection=@new mysqli($host,$db_user,$db_password,$db_name);
		$this->connection->set_charset('utf8');

		if($this->connection->connect_errno!=0)
		{
			return false;
		}
		else
		{
			return true;
		}
	}

	public function SaveActivationsToTable()
	{
		for($i=1;$i<=10;$i++)
		{
			$activetodatabase[$i]=$_POST['active'.$i];
		}

		return $activetodatabase;
	}

	public function QueryToDatabase($query)
	{
		return $this->CheckQueryToDatabase($query);
	}

	private function CheckQueryToDatabase($sqlget)
	{
		if(@$this->connection->query($sqlget))
		{
			return true;
		}
		else
		{
			return false;
		}
	}

	public function SelectDataWhereEmailEaqul($sqlget)
	{
		$rezult=@$this->connection->query($sqlget);

		return $rezult->num_rows;
	}

	public function CheckIsUserOnThatEmail($email)
	{
		$informationaboutuser=$this->SelectDataWhereEmailEaqul("SELECT * FROM Aktywacje WHERE email='$email'");

		if ($informationaboutuser > 0) 
		{
			return true;
		}
		else
		{
			return false;
		}
	}
	public function GetInformationAboutUser($email)
	{
		$wiersz=@$this->connection->query("SELECT * FROM Aktywacje INNER JOIN daneklienta ON Aktywacje.email=daneklienta.email WHERE Aktywacje.email='$email'")->fetch_assoc();

		for($i=1;$i<=10;$i++)
		{
			$active[$i] = $wiersz['aktywacja'.$i];

			if($active[$i]==1)
			{
				$active[$i] = $this->imgCheck;
			}
			else if($active[$i]!=1)
			{
				$active[$i] = $this->imgUncheck;
			}
		}

		$active[11]=$wiersz['numerseryjny'];
		$active[12]=$wiersz['danezprocesora'];
		$active[13]=$wiersz['plec'];
		$active[14]=$wiersz['imie'];
		$active[15]=$wiersz['nazwisko'];
		$active[16]=$wiersz['company'];
		$active[17]=$wiersz['street'];
		$active[18]=$wiersz['zipcode'];
		$active[19]=$wiersz['miasto'];
		$active[20]=$wiersz['pelnanazwamiasta'];
		$active[21]=$wiersz['wojewodztwo'];
		$active[22]=$wiersz['kraj'];
		$active[23]=$wiersz['telefon'];
		$active[24]=$wiersz['fax'];
		$active[25]=$wiersz['nicknazwafirmy'];
		$active[26]=$wiersz['email'];
		$active[27]=$wiersz['datazakupu'];
		$active[28]=$wiersz['paragonfakturaNR'];

		return $active;
	}

}

class Emails{

	public function SendEmail($email,$file,$informationUser)
	{
		if($email!=" " && $file!=" ")
		{
			require '../SendEmails/phpmailer.php';

			return true;
		}
		else
		{
			return false;
		}
	}

	public function UpdateActivation()
	{
		for($i=1;$i<=10;$i++)
		{
			$active[$i]=$_POST['active'.$i];

			if($active[$i]==1)
			{
				$active[$i]="V";
			}
			else if($active[$i]!=1)
			{
				$active[$i]="X";
			}
		}

		return $active;
	}
}

class OperationsOnFiles
{

	public function CreateNewFile($namefile)
	{
		if($namefile!=" ")
		{
			return $uchwyt = fopen($namefile, "w");
		}
		else
		{
			return 0;
		}
	}

	public function SaveActivationsToCreatedFile($resource,$dane)
	{
		if($dane && $resource!= 0)
		{
			$data=fwrite($resource, $dane); 
			return true;
		}
		else
		{
			return false;
		}	
	}

}

 

Celowo zrobiłem tak żeby każda metoda coś zwracała bo wtedy taka metoda jest łatwiejsza do testowania, jedyne co mi się nie podoba w tej klasie to metoda "GetInformationAboutUser", chciałbym ja jakoś skrócić ale nie mam pomysłu jak to zrobić, jeśli macie jakiś pomysł, albo jest jakaś jeszcze zła praktyka albo błąd to mówcie:)

I wywołania tej klasy:

require_once "../connect.php";

require_once "ConnectTodatabase.php";

$sex=htmlspecialchars($_POST['sex']);
$firstname=htmlspecialchars($_POST['firstname']);
$lastname=htmlspecialchars($_POST['lastname']);
$company=htmlspecialchars($_POST['company']);
$street=htmlspecialchars($_POST['street']);
$zipcode=htmlspecialchars($_POST['zipcode']);
$city=htmlspecialchars($_POST['city']);
$fullcity=htmlspecialchars($_POST['fullcity']);
$state=htmlspecialchars($_POST['state']);
$country=htmlspecialchars($_POST['country']);
$phone=htmlspecialchars($_POST['phone']);
$fax=htmlspecialchars($_POST['fax']);
$regname=htmlspecialchars($_POST['regname']);
$email=htmlspecialchars($_POST['email']);
$saledate=htmlspecialchars($_POST['saledate']);
$paragon=htmlspecialchars($_POST['paragon']);

$connect= new ConnectToDatabase();

$ifconnected=$connect->ConnectWithDatabase($host,$db_user,$db_password,$db_name);

if($ifconnected==false)
{
	echo "Error: ".$polaczenie->connect_errno;
}
else
{
	$ifemail=$connect->SelectDataWhereEmailEaqul(sprintf("SELECT * FROM Aktywacje WHERE email='%s'"
		,mysqli_real_escape_string($connect->connection,$email)));

	if ($ifemail > 0) 
	{
		$ifemailindaneklienta=$connect->SelectDataWhereEmailEaqul(sprintf("SELECT * FROM daneklienta WHERE email='%s'"
		,mysqli_real_escape_string($connect->connection,$email)));

		if($ifemailindaneklienta>0)
		{
			$connect->QueryToDatabase("UPDATE daneklienta SET imie='$firstname', nazwisko='$lastname',plec='$sex',street='$street',zipcode='$zipcode',miasto='$city',pelnanazwamiasta='$fullcity',wojewodztwo='$state',company='$company',kraj='$country',telefon='$phone',fax='$fax',nicknazwafirmy='$regname',datazakupu='$saledate',paragonfakturaNR='$paragon' WHERE email='$email'");
		}
		else
		{
			$connect->QueryToDatabase("INSERT INTO daneklienta (id,imie,nazwisko,plec,street,zipcode,miasto,pelnanazwamiasta,wojewodztwo,	company,kraj,telefon,fax, nicknazwafirmy, email, datazakupu, paragonfakturaNR) VALUES (NULL, '$firstname', '$lastname','$sex','$street','$zipcode','$city','$fullcity','$state','$company','$country','$phone','$fax','$regname','$email','$saledate','$paragon')");
		}

		$sendemail= new Emails();

		$IsUser=$connect->CheckIsUserOnThatEmail($email);
		
		if($IsUser)
		{
			$InformationAboutUser=$connect->GetInformationAboutUser($email);

			$sendemail->SendEmail($email,"nothing",$InformationAboutUser);	
		}
	}
}

I z drugiego pliku:

require_once "../connect.php";

require_once "ConnectTodatabase.php";

$email=$_POST['email'];
$numseries=$_POST['numseries'];
$datamikropro=$_POST['datamikropro'];

$connect= new ConnectToDatabase();

$ifconnected=$connect->ConnectWithDatabase($host,$db_user,$db_password,$db_name);

if($ifconnected==false)
{
	echo "Error: ".$polaczenie->connect_errno;
}
else
{
	$ifemail=$connect->SelectDataWhereEmailEaqul("SELECT * FROM Aktywacje WHERE email='$email'");

	$activetodatabase=$connect->SaveActivationsToTable();

	if ($ifemail > 0) 
	{
		$connect->QueryToDatabase("UPDATE Aktywacje SET numerseryjny='$numseries', danezprocesora='$datamikropro', aktywacja1='$activetodatabase[1]', aktywacja2='$activetodatabase[2]', aktywacja3='$activetodatabase[3]', aktywacja4='$activetodatabase[4]', aktywacja5='$activetodatabase[5]', aktywacja6='$activetodatabase[6]', aktywacja7='$activetodatabase[7]', aktywacja8='$activetodatabase[8]', aktywacja9='$activetodatabase[9]', aktywacja10='$activetodatabase[10]' WHERE email='$email'");
	}
	else
	{
		$connect->QueryToDatabase("INSERT INTO Aktywacje (id,numerseryjny,danezprocesora,aktywacja1,aktywacja2,aktywacja3,aktywacja4,aktywacja5,aktywacja6,aktywacja7,aktywacja8,aktywacja9,aktywacja10, email) VALUES (NULL, '$numseries', '$datamikropro','$activetodatabase[1]','$activetodatabase[2]','$activetodatabase[3]','$activetodatabase[4]','$activetodatabase[5]','$activetodatabase[6]','$activetodatabase[7]','$activetodatabase[8]','$activetodatabase[9]','$activetodatabase[10]','$email')");
	}
}

$operation= new OperationsOnFiles();

$sendemail= new Emails();

$namefile="CheckedActivationsInDatabase.txt";

$newFile=$operation->CreateNewFile($namefile);

$sendActivation=$sendemail->UpdateActivation();

$data='Aktywacja 1: '.$sendActivation[1]." ".$_POST['activetextarea1']."\nAktywacja 2: ".$sendActivation[2]." ".$_POST['activetextarea2']."\nAktywacja 3: ".$sendActivation[3]." ".$_POST['activetextarea3']."\nAktywacja 4: ".$sendActivation[4]." ".$_POST['activetextarea4']."\nAktywacja 5: ".$sendActivation[5]." ".$_POST['activetextarea5']."\nAktywacja 6: ".$sendActivation[6]." ".$_POST['activetextarea6']."\nAktywacja 7: ".$sendActivation[7]." ".$_POST['activetextarea7']."\nAktywacja 8: ".$sendActivation[8]." ".$_POST['activetextarea8']."\nAktywacja 9: ".$sendActivation[9]." ".$_POST['activetextarea9']."\nAktywacja 10: ".$sendActivation[10]." ".$_POST['activetextarea10'];

$file=$operation->SaveActivationsToCreatedFile($newFile,$data);

$sendemail->SendEmail($email,$namefile,null);

W tych wywołaniach nie podobają mi się zapytania do bazy, nie są zabezpieczone.. macie jakiś pomysł jak je zabezpieczyć, tak żeby też jakoś w miare czytelnie to wyglądało? Funkcji real_escape_string nie będe używał bo ma swoje wady.

Więc jeśli macie jakieś pomysły to walcie śmiało:)

Answer 1

Pragnę zwrócić tylko uwagę, że o ile klasy powinny mieć jedną odpowiedzialność(single responsibility principle​​​​​​), o tyle Twoje klasy robią obiad.

Zainteresuj się kursem Arka, napisz kod od początku i wróć po feedback. 

Comments

Tego się obawiałem, wiem nazwy metod w stosunku do nazw klas są troche niedopasowanie... wiem na czym polegają zasady solid:)

---

Nie neguję tego, że wiesz. Mimo wszystko w kodzie tego nie zaprezentowałeś w proponuję Ci ogarnąć jakiś mały framework na początek. 

---

No cóź że nie widać tego w kodzie to trudno się nie zgodzić z tobą troche na szybko było to robione, ale bardzo łatwo to można poprawić a frameworka nie chce używać, znam z małych CodeIgnitera np ale mi zależy na wydajności aplikacji wszystko robi się szybciej z frameworkiem ale to wtedy aplikacja będzie więcej ważyć...

 

Answer 2

Namotałeś tego kodu. Można wiele rzeczy zrobić prościej ale mniej zrozumiale bo to 2 nie jest konieczne.
Zmienne POST/GET deklaruje się tylko w przypadku kiedy takie istnieją. Jeśli nie to porogram wywali błąd.
Gdy piszesz kwerendę do bazymysql to używaj "grawis" tak się nazywa ten znak ` Np: "SELECT `asdf` FROM"  Przy małych zapytaniach nie robią znaczenia, jednak przy multi zapytaniach może się coś pomieszać.
$_POST['active'.$i] nie została w rzaden sposób zabezpieczona ani przefiltrowana.
Żeby napisać dobry kod trzeba dużo doświadczenia bo człowiek uczy się tylko na błędach. Tak więc ćwicz i ćwicz.
PS: w src podaje się "http://" src="oppin.pl/img/check.gif"

Comments

Ano masz rację, ale sam wiem że można zrobić prościej, ale tak jak mówiłem dostosowywuje to to testów jednostkowych a zmienna $_POST['active'.$i] nie musi być zabezpieczona bo tylko ja będe z tej funkcjonalności korzystał, tam gdzie jest zastosowana funkcja htmlspecialchars tam są dane wpisane przez użytkowników i to je trzeba zabezpieczyć:)

PS dzięki za przypomnienie o grawisie, kiedyś z niego korzystałem ale już o nim zapomniałem.

---

No właśnie nie. Każda POST/GET musi być zabezpieczona. To nie ty będziesz z niej korzystał tylko użytkownik pamiętaj !
Ktoś bardziej znający się na kodzie będzie mógł wykorzystać tę lukę 

---

Ja bo to jest wykorzystywane w panelu admina który jest(będzie) zabezpieczony hasłem, jeśli chciałby skorzystać z tej zmiennej to musiałby najpierw złamać hasło, które będzie już dobrze zabezpieczone