Refactor short code snippet

General discussions related to php

Moderators: egami, macek, gesf

Post Reply
User avatar
SophieLins
New php-forum User
New php-forum User
Posts: 1
Joined: Tue Feb 05, 2019 10:43 am

Tue Feb 05, 2019 10:48 am

Good afternoon guys, I have two code snippets, I'm studying good programming practices and I would like to know if anyone has any tips on how to refactor and improve the following code snippets.

Code: Select all

if(isset($_SESSION['loggedin']) && $_SESSION['loggedin'] == true){
	header("Location: http://www.google.com");
	exit();
}elseif(isset($_COOKIE['Loggedin']) && $_COOKIE['Loggedin'] == true){
	header("Location: http://www.google.com");
	exit();
}
the other part is this:

Code: Select all

class MyUserClass
{
	public function getUserList()
	{
		$dbconn = new DatabaseConnection('localhost','user','password');
		$results = $dbconn->query('select name from user');

		sort($results);

		return $results;
	}
}

Thank you very much for the help.

Sophie Lins

User avatar
hyper
php-forum Fan User
php-forum Fan User
Posts: 800
Joined: Mon Feb 22, 2016 5:52 pm

Tue Feb 05, 2019 12:14 pm

If you want to improve your code, just keep writing it rather than analysing every line as soon as you've written four. You'll find that you will change the way you code as you go along and read others code.


So far as programming practices is concerned, here are some guidelines which might prove useful

As to your code:
Listing 1-
I don't understand the point of testing to see if someone is logged on and then re-directing if they are? You could end up continually re-loading the same page?

Do NOT store logged on status in a cookie, ever. Do NOT store any user information (anything that uniquely identifies a user) in a cookie: cookies are stored on the users computer and can very easily be faked, so relying on this information can lead to major problems.

It can be safer to use === (triple equals signs) when testing for Boolean true / false

Code: Select all

$logged_on = 1000;
if ($logged_on == true) { # this will evaluate as true
  ...
if ($logged_on === true) { # this will evaluate as false as $logged_on is 1000 NOT true
=== means that the test is for the same data type as well as value (which can only be true or false)

Comment your code, at the start to describe what the purpose of the function / class is doing and then just little notes to remind you what you did and why

Code: Select all

/**
 * An example of bad and dangerous practice using cookies
 *
 * note to self:
 * I must remember not to write any user data to cookies
 * it can really compromise my system
 *
 */
if(isset($_COOKIE['Loggedin']) && $_COOKIE['Loggedin'] == true){ // very dangerous
	header("Location: http://www.google.com"); # this seems silly if user is logged in
	exit(); // # /* Always a good idea to exit after a header() statement */
}

I don't know what you expect from listing 2, it cannot do anything apart from create errors and warnings?

Post Reply