Ocena przerobionego skryptu php

pytanie zadane 24 kwietnia 2017 w PHP przez Paweł123 Nałogowiec (33,500 p.)

Cześć, czy moglibyście ocenić przerobiony skrypt? Co byście w nim zmienili?

<?php

    session_start();

	if (isset($_POST['email']))
	{
		
        //Udana walidacja? Załóżmy, że tak!
		$wszystko_OK=true;
		
		//Sprawdź poprawność nickname'a
		$nick = $_POST['nick'];
		
		//Sprawdzenie długości nicka
		if ((strlen($nick)<3) || (strlen($nick)>20))
		{
			$wszystko_OK=false;
			$_SESSION['e_nick']="Nick musi posiadać od 3 do 20 znaków!";
		}
		
		if (ctype_alnum($nick)==false)
		{
			$wszystko_OK=false;
			$_SESSION['e_nick']="Nick może składać się tylko z liter i cyfr (bez polskich znaków)";
		}
		
		// Sprawdź poprawność adresu email
		$email = $_POST['email'];
		$emailB = filter_var($email, FILTER_SANITIZE_EMAIL);
		if ((filter_var($emailB, FILTER_VALIDATE_EMAIL)==false) || ($emailB!=$email))
		{
			$wszystko_OK=false;
			$_SESSION['e_email']="Podaj poprawny adres e-mail!";
        
        
		}	
		else
        {           
        
		
         // Podajesz tytuł jaki ma mieć ta wiadomość email
        $tytul = "Formularz";
        $dokogo = "admin@gmail.com";
        // Przygotowujesz treść wiadomości
        $wiadomosc = "";
        $wiadomosc .= "Imie: " . $nick . "\n";
        $wiadomosc .= "E-mail kontaktowy: " . $email . "\n";
        // Wysyłamy wiadomość
        $sukces = mail($dokogo, $tytul, $wiadomosc, "Od: <$odkogo>");

        // Przekierowywujemy na potwierdzenie
        if ($sukces){
          header('Location: potwierdzenie.php');
        }
        else{
          header('Location: error.html');;
        }
        }
		//Zapamiętaj wprowadzone dane
		$_SESSION['fr_nick'] = $nick;
		$_SESSION['fr_email'] = $email;
	}
	
	
?>

<!DOCTYPE HTML>
<html lang="pl">
<head>
	<meta charset="utf-8" />
	<meta http-equiv="X-UA-Compatible" content="IE=edge,chrome=1" />
	<title>Osadnicy - załóż darmowe konto!</title>
	
	<style>
		.error
		{
			color:red;
			margin-top: 10px;
			margin-bottom: 10px;
		}
	</style>
</head>

<body>
	
	<form method="post">
	
		Nickname: <br /> <input type="text" value="<?php
			if (isset($_SESSION['fr_nick']))
			{
				echo $_SESSION['fr_nick'];
				unset($_SESSION['fr_nick']);
			}
		?>" name="nick" /><br />
		
		<?php
			if (isset($_SESSION['e_nick']))
			{
				echo '<div class="error">'.$_SESSION['e_nick'].'</div>';
				unset($_SESSION['e_nick']);
			}
		?>
		
		E-mail: <br /> <input type="text" value="<?php
			if (isset($_SESSION['fr_email']))
			{
				echo $_SESSION['fr_email'];
				unset($_SESSION['fr_email']);
			}
		?>" name="email" /><br />
		
		<?php
			if (isset($_SESSION['e_email']))
			{
				echo '<div class="error">'.$_SESSION['e_email'].'</div>';
				unset($_SESSION['e_email']);
			}
		?>
		
		
		
		
		<br />
		
		<input type="submit" value="Zarejestruj się" />
		
	</form>

</body>
</html>

Link do strony.

1 odpowiedź

odpowiedź 24 kwietnia 2017 przez rafal.budzis Szeryf (85,260 p.)

Na pewno bym oddzielił szablon od logiki aplikacji. Jeden plik to bardzo złe podejście. Możesz poczytać o MVC to pewnie temat na trochę dalszy etap nauki ale warto zebyś miał w głowie ze trzeba sie kiedyś za to zabrać ;)

Warto też używać funkcji i instrukcji return do przerywania jej dzięki temu kaskade ifów mozemy zamienić na płaska strukturę.

Nawet jeśli nie rozdzielasz plików na szablony oraz logike warto przestawić się na to co powinno być miedzy HTMLem a czego tam nie powinno być. Miedzy znacznikami HTML mogą być if`y, pętle oraz wyświetlanie wartości. Nie powinno się tam dodawać żadnych instrukcji które zmieniają sesje, wykonują zapytania. tworzenie tam instrukcji unset jest bardzo złe unset a mozesz przepisać do góry aplikacji a przed unsetem zapisać dane z sesji do zwykłej zmiennej dzieki czemu bedzie mozna jej użyć w szablonie.

W szablonach znacznie lepiej wyglądają szybkie otwaracia i zamkniecia oraz alternatywny zapis pętli , ifów

if (warunek) : /*kod*/ endif

dlatego też prawidłowy kod dla mnei powinnien wyglądac tak :

<? if (isset($_SESSION['e_email'])) : ?>
     <div class="error">
          <?= $_SESSION['e_email'] ?>
     </div>
<? endif ?>

Zamiast

        <?php
            if (isset($_SESSION['e_email']))
            {
                echo '<div class="error">'.$_SESSION['e_email'].'</div>';
                unset($_SESSION['e_email']);
            }
        ?>

Powodzenia w dalszym rozwoju każdy zaczynał takimi kodami jak ty a nawet gorszymi ;)