Co byście zmienili w tym kodzie?

0

Jak w temacie co byście w tym kodzie zmienili tylko nie oceniajcie kodu java script bo wiem, że ten kod java script jest marny.

http://87.205.76.191/moja_strona.zip

0

Teraz wrzuciłem na Githuba.
tu jest link

https://github.com/tomi0001/strona

2

Tym razem uprzedze Cie @Shalom :D
user image

5

Myślę, że kod daje radę. Szczególnie ten fragment muszę sobie gdzieś zapisać:

if ($czas_dodania < 60) $this->data = "Mniej niż minutę temu";
    else if ($czas_dodania < 120) $this->data = "Mniej niż 2 minuty temu";
    else if ($czas_dodania < 180) $this->data = "Mniej niż 3 minuty temu";
    else if ($czas_dodania < 240) $this->data = "Mniej niż 4 minuty temu";
    else if ($czas_dodania < 300) $this->data = "Mniej niż 5 minut temu";
    else if ($czas_dodania < 360) $this->data = "Mniej niż 6 minut temu";
    else if ($czas_dodania < 480) $this->data = "Mniej niż 8 minut temu";
    else if ($czas_dodania < 600) $this->data = "Mniej niż 10 minut temu";
    else if ($czas_dodania < 900) $this->data = "Mniej niż 15 minut temu";
    else if ($czas_dodania < 1200) $this->data = "Mniej niż 20 minut temu";
    else if ($czas_dodania < 1500) $this->data = "Mniej niż 25 minut temu";
    else if ($czas_dodania < 1900) $this->data = "Około pół godziny temu";
    else if ($czas_dodania < 2400) $this->data = "Około 40 minut temu";
    else if ($czas_dodania < 2700) $this->data = "Około 45 minut temu";
    else if ($czas_dodania < 3000) $this->data = "Około 50 minut temu";
    else if ($czas_dodania < 3300) $this->data = "Około 55 minut temu";
    else if ($czas_dodania <= 3600) $this->data = "Około Godzinę temu";

4

To ja spróbuję dać jakiś konstruktywny komentarz.

index.php
tutaj najbardziej rzuca się w oczy ten kod https://github.com/tomi0001/strona/blob/master/index.php#L116 Serio nie można zrobić tego choćby w pętli? Zrób sobie klasę, w której będziesz trzymać informacje o Menu. (jaki url -> jaki plik ładujemy).
https://github.com/tomi0001/strona/blob/master/index.php#L31 Nie prościej tak?

if (!$nowy_user->sprawdz_czy_zalogowany())

https://github.com/tomi0001/strona/blob/master/index.php#L28 nie używaj mysql_escape_string tylko http://php.net/manual/pl/function.mysql-real-escape-string.php a najlepiej przerzuć się na PDO -> https://pl.wikibooks.org/wiki/PHP/Biblioteka_PDO

panel.php
https://github.com/tomi0001/strona/blob/master/strony/panel.php#L9 czytelniej jest zamknąć PHP ?> i kodu HTML nie wyświetlać w ten sposób.

https://github.com/tomi0001/strona/blob/master/strony/zaloguj.php#L2 "czy"? Co to znaczy?

Unikaj takich ifów -> https://github.com/tomi0001/strona/blob/master/clasy/data.php#L12 to TRAGICZNIE wygląda. Spokojnie możesz to zastąpić jakąś pętlą.

To tak z grubsza. Więcej mi się nie chce :)

PS. Folder o naszwie clasy? Serio? Po jakiemu to? Używaj tylko angielskiego.

1

od razu, rzucił mi się w oczy ponglish: "clasy" oraz "dodaj_kome".

2

Krótka odpowiedź: Zmieniłbym ** dużo ** :D

Długa odpowiedź:

print to funkcja, więc powinno się ją pisać z nawiasami.
zamień

$nowy_user = new user;
$data = new data;
$mysql = new baza;

na

$nowy_user = new user();
$data = new data();
$mysql = new baza();

bo nikt nie będzie wiedział o co chodzi.

Biedny @pol90 wszyscy się na niego rzucają a on nie wie o co chodzi.
Takie rzeczy jak na przykłąd od tąd do linii 185 mają MASĘ powtórzeń. W programowaniu nie lubimy powtórzeń. Trzeba je eliminować, np

można by z łatwością zrobić funkcję

function checkPozycja($name, $displayName) {
    if ($_GET["pozycja"] == $name) {
        include ("./strony/$name.php");
        print ("<title>$displayName</title>");
    }
}

i teraz całe te Twoje powtózenia można zapisać tak

checkPozycja('aktu', 'Aktualności');
checkPozycja('logowanie', 'Logowanie');
checkPozycja('zaloguj', 'Logowanie');
checkPozycja('wyloguj', 'Wylogowywanie');
checkPozycja('user', 'Użytkownicy');
checkPozycja('panel', 'Panel Administracyjny');
checkPozycja('dodaj_zdje', 'Zapisz zdjęcie');
checkPozycja('gale', 'Galeria');
checkPozycja('gale_kome', 'Komentarze');
checkPozycja('statyu', 'Aktualności');
checkPozycja('dodaj_aktu1', 'Aktualności');
checkPozycja('dodaj_aktu2', 'Aktualności');
checkPozycja('dodaj_aktu_kome', 'Aktualności');
checkPozycja('czat', 'Czat');

Po prostu ** nie wyświetlanie ** linka do panelu admina raczej nie jest zbyt dobrym zabezpieczeniem, nie sądzisz? :) (na razie może zostać, tylko żebyś wiedział, nie? :D )

if ($nowy_user->typ == "root") {
      print ("<a href=index.php?pozycja=panel>");

Java to nie JavaScript ;)

<SCRIPT TYPE="text/javascript" SRC="java.js"></SCRIPT>

Jeżeli chciałbyś popracować nad czytelnością plików które mają dużo html a mało php, może mógłbyś się zainteresować inną formą osadzania kodu (php ma tak jakby dwie) Jedna ta w której piszesz a druga np taka
http://pastebin.com/1ZRFMrBs
Gdzie używa się dwukropków zamiast klamerek (można tego użyć do if,elseif / for / foreach), może Ci się spodoba gdybyś chciał coś więcej.
Jak chcesz żeby Twoja strona przechodziła przez walidatory html to niestety samo to nie wystarczy

<link href="./style.css" rel="stylesheet" type="text/css" />
<meta content="text/html; charset=utf-8" http-equiv="content-type">

Spróbuj osadzić kod w takich znacznikach html

<!doctype html>
<html>
<head>
    <title>Moja strona</title>
    <link href="./style.css" rel="stylesheet" type="text/css" />
    <meta content="text/html; charset=utf-8" http-equiv="content-type">
</head>
<body>
  <!-- Tutaj Twój html (czyli w sumie wszystko co masz w index.php) -->
</body>
</html>

Ten kod:

private function oblicz_dzien_tygodnia($dzien_tygodnia) {
    if ($dzien_tygodnia == "Monday") return "Poniedziałek";
    else if ($dzien_tygodnia == "Tuesday") return "Wtorek";
    else if ($dzien_tygodnia == "Wednesday") return "Środę";
    else if ($dzien_tygodnia == "Thursday") return "Czwartek";
    else if ($dzien_tygodnia == "Friday") return "Piątek";
    else if ($dzien_tygodnia == "Saturday") return "Sobotę";
    else return "Niedziele";
  }

Można by fajnie zamienić na taki

private function oblicz_dzien_tygodnia($dzien) {
	return strstr($dzien, [
		"Monday" => "Poniedziałek",
		"Tuesday" => "Wtorek",
		"Wednesday" => "Środę",
		"Thursday" => "Czwartek",
		"Friday" => "Piątek",
		"Saturday" => "Sobotę",
		"Sunday" => "Niedziele"
	]);
 }

Jezus maria co to? :O

$baza->query("insert into galeria(sciezka,data) values ('$nazwa_pliku','$data') ");

Wiesz co to sql injection? Pomyśl co by było gdyby ktoś do Twojej zmiennej $nazwa_pliku wpisał takie coś:
dupa'); DROP TABLE galeria; --.

Twoje query wyglądałoby tak:

$baza->query("
  insert into galeria(sciezka,data) values ('dupa'); 
  DROP TABLE galeria; 
  --','$data') 
");

Nie muszę chyba mówić jakie są konsekwencje? Dla przypomnienia -- w SQL to komentarz.

A ten kod

 if($rozszerzenie !== "jpg" &&
    $rozszerzenie !== "gif" &&
    $rozszerzenie !== "png") {
      return false;
    }
// ...
if($imageFileType != "jpg" && $imageFileType != "png" && $imageFileType != "jpeg"
    && $imageFileType != "gif" ) {
      return 5;
}

Można by zamienić na

if (!in_array($rozszerzenie, ['jpg', 'gif','png'])) {
	return false;
}
// ...
if(!in_array($imageFileType, ["jpg", "png", "jpeg", "gif"]) {
      return 5;
}

Zamienić

if(imagejpeg($canvas, $plik2, 70)) {
   return true;
} else {
   return false;
}

na to

return imagejpeg($canvas, $plik2, 70);

Noi jeszcze, taki refactor szybki, pokazuje jak można by lepiej zrobić Twoją klasę os.php.

<?php

class os 
{
	function os($text) 
	{
		$text = strtolower($text);
		return $this->findInArray($text, $this->operatingSystemNames, "Nieznany");
	}
  
	function browser($text) {
		$text = strtolower($text);
		return $this->findInArray($text, $this->browserNames, "Nieznana");
	}
	
	private $operatingSystemNames = [
		"android" => "Linux Android",
		"linux" => "Linux",
		"windows nt 10" => "Windows 10",
		"windows nt 6.3" => "Windows 8.1",
	    "windows nt 6.2" => "Windows 8",
		"windows nt 6.1" => "Windows 7",
		"windows nt 6.0" => "Windows Vista",
		"windows nt 5.2" =>  "Windows 2003 Server",
		"windows nt 5.1" => "Windows XP",
		"windows nt 5.0" => "Windows 2000",
		"windows nt 4" =>  "Windows NT 4",
		"win98 " => "Windows 98",
		"windows 98" =>  "Windows 98",
		"win95" => "Windows 95",
		"windows 95" =>  "Windows 95",
		"win9x4.9" =>  "Windows ME",
		"freebsd" =>  "FreeBSD",
		"desktopbsd" =>  "DesktopBSD",
		"solaris" =>  "Solaris",
		"netbsd" =>  "NetBSD",
		"qnx" =>  "QNX";
		"mac os" => "Mac OS",
		"macos" => "Mac OS",
	];
	
  
	private $browserNames = [
		'opera' => "Opera",
		'firefox' => 'Mozilla Firefox',
		'msie' => 'Internet Exploler',
		'applewebkit' => 'AppleWebKit'
		'links' => 'Links',
		'chrome' => 'Chrome',
		'konqueror' => 'Konqueror'
	];
  
  
	private function findInArray($thisName, $array, $notFound)
	{
		foreach ($array as $key => $value) {
		    if (strsrt($thisName, $key)) return $value;
		}
		return $notFound;
	}
}
?>

Klasy baza w mysql.php nawet nie chce mi się czytać bo jest tak brzydka że jap**** :C
Dzieki za komentarz @mad_penguin Jasne że tak, chociaż nie wiem czy to faktycznie poprawi czytelność.

checkMany([
	'aktu' => 'Aktualności',
	'logowanie' => 'Logowanie',
	'zaloguj' =>  'Logowanie',
	'wyloguj' =>  'Wylogowywanie',
	'user' =>  'Użytkownicy',
	'panel' =>  'Panel Administracyjny',
	'dodaj_zdje' =>  'Zapisz zdjęcie',
	'gale' =>  'Galeria',
	'gale_kome' =>  'Komentarze',
	'statyu' =>  'Aktualności',
	'dodaj_aktu1' =>  'Aktualności',
	'dodaj_aktu2' =>  'Aktualności',
	'dodaj_aktu_kome' =>  'Aktualności',
	'czat' =>  'Czat'
]);

function checkMany($all) 
{
	foreach ($all as $name => $displayName) {
		if ($_GET["pozycja"] == $name) {
			include ("./strony/$name.php");
			print ("<title>$displayName</title>");
		}
    }
}

Co byście zmienili w tym kodzie?

1 użytkowników online, w tym zalogowanych: 0, gości: 1

Praca dla programistów

Forum dyskusyjne

Sprawy administracyjne

O nas

Skontaktuj się z nami