Could I get some feedback on this?

[mdmartiny]

I am in the process of writing a class to get the ip address and the referring site to store in a database. I am not all that familiar with writing classes and want to make sure that I am on the right path. If I am doing something wrong in it please explain it to me so I can learn

class browser_ip_ref{
		
		private $_db;
		private $_ip;
		private $_url;

	    public function __construct($db,$ip,$url){
	        $this->_db = $db;
	        $this->_ip = ip2long($_SERVER["REMOTE_ADDR"]); 
	        $this->_url = $_SERVER['HTTP_REFERER'];
	    }

	    function getIp(){
	        if($this->_ip){
	            if (!empty($_SERVER['HTTP_CLIENT_IP'])) {
	                $this->_ip = $_SERVER['HTTP_CLIENT_IP'];
	            } elseif (!empty($_SERVER['HTTP_X_FORWARDED_FOR'])) {
	                $this->_ip = $_SERVER['HTTP_X_FORWARDED_FOR'];
	            }
	            return $this->_ip;
	        }
	    }

	    function insertInfo($this->_ip, $pageID, $browser, $version){
	    	$country_code = $this->_db->query("SELECT `ci` from `ip` WHERE ".$this->_ip." BETWEEN `start` AND `end`");
	        $country_code = $country_code->fetch_array(MYSQLI_ASSOC);
	        $countryID = $country_code['ci'];

	        $ipCount = $this->_db->("SELECT COUNT(*) AS count from `hit_counter_info` WHERE `longIP` = ".$this->_ip." AND `pageID` = $pageID");
	        $total = $ipCount->fetch_array(MYSQLI_ASSOC);
	        $num_rows =  $total['count'];

	        if ($num_rows < 1) {
	            //if not in the database add it.
	            $this->_db->("INSERT INTO `hit_counter_info` (`longIP`,`pageID`,`cc`,`browser`,`version`) VALUES ('".$this->_ip."', '".$pageID."', '".$countryID."', '".$browser."', '".$version."')");
	        }
	    }

	    function insertRef(){
	        $url_parts = parse_url($this->_url);
	        $host_parts = explode( '.', $url_parts['path']);
	        $tld = end($host_parts);
	        $good_tld = array('com','org','edu','uk','net','ca','mil','de','jp','fr','au','us','ru','ch','it','nl','se','no','es');

	        if (empty($url)) {
	            $this->_db->("UPDATE `hit_counter_referrers` SET `ref_count` = ref_count + 1 WHERE `ref_url` = 'bookmarked'");
	        } else if (in_array($tld, $good_tld)) {
	            $rowCount = $this->_db->("SELECT COUNT(*) AS count from `hit_counter_referrers` WHERE `ref_url` = '$this->_url'");
	            $total = $rowCount->fetch_array(MYSQLI_ASSOC);
	            $num_rows =  $total['count'];

	            if ($num_rows > 0) {
	                $this->_db->("UPDATE `hit_counter_referrers` SET `ref_count` = ref_count + 1 WHERE `ref_url` = '$this->_url'");
	            } else {
	                $this->_db->("INSERT INTO `hit_counter_referrers` (`ref_url`,`ref_count`) VALUES ('$this->_url', 1)");
	            }
	        }
	    }
	} 

[requinix]

public function __construct($db,$ip,$url){
	$this->_db = $db;
	$this->_ip = ip2long($_SERVER["REMOTE_ADDR"]); 
	$this->_url = $_SERVER['HTTP_REFERER'];
}

You accept an $ip and $url but don't use them. If you want them as defaults that's one thing,

public function __construct($db,$ip=null,$url=null){
	$this->_db = $db;
	$this->_ip = ip2long($ip ?: $_SERVER["REMOTE_ADDR"]);
	$this->_url = ($url ?: $_SERVER['HTTP_REFERER']);
}

but if not then don't have them at all.

 

function getIp(){
	if($this->_ip){
		if (!empty($_SERVER['HTTP_CLIENT_IP'])) {
			$this->_ip = $_SERVER['HTTP_CLIENT_IP'];
		} elseif (!empty($_SERVER['HTTP_X_FORWARDED_FOR'])) {
			$this->_ip = $_SERVER['HTTP_X_FORWARDED_FOR'];
		}
		return $this->_ip;
	}
}

Think you got your logic a bit messed up. Plus you're setting $_ip in the constructor so most of this is irrelevant, or even better should be moved into the constructor instead.

 

function insertInfo($this->_ip, $pageID, $browser, $version){

"$this->_ip" doesn't belong in there. (Didn't look at the rest of the code in there.)

 

 

A point about objects: initialize everything you can in the constructor, and pass in as arguments whatever information is needed to do that. Here that would be the database connection at a minimum, and IP address and URL are optional (as I mentioned earlier).