Page 1 of 1

I need an opinion its important....Thanks :D

Posted: Mon Oct 01, 2007 10:24 pm
by phpisgreat
feyd | Please use

Code: Select all

,

Code: Select all

and [syntax="..."] tags where appropriate when posting code. Your post has been edited to reflect how we'd like it posted. Please read:  [url=http://forums.devnetwork.net/viewtopic.php?t=21171]Posting Code in the Forums[/url] to learn how to do it too.[/color]


Hi, we have been using the following codes on our site for a year and I wanted to know if these codes are just spaghetti or its actual usable code. I know it can be bettered a lot I just need opinions if the codes can be used a while more until we reprogram everything as a complete CMS system. 

Thanks a lot! my superiors want to know because there is another coder that says these codes are just spaghetti.

There is more codes but i am able to access the database fine and everything else. Of course the codes can be bettered but i dont believe its just spaghetti!!!!! I used these codes because there was no need to reinvent the wheel. I apreciate your help!   

--------------------------------------------------
file name: SystemComponent.php
--------------------------------------------------

Code: Select all

<?php
class SystemComponent {

 var $settings;
 
 function getSettings() {
 
 // System Variables
 $settings['siteDir'] = 'path';
 
 // Database variables
 $settings['dbhost'] = 'dbhost';
 $settings['dbusername'] = 'dbusername';
 $settings['dbpassword'] = 'dbpassword';
 $settings['dbname'] = 'dbname';
 
 return $settings;
 
 }
 
}

?>
------------------------------------




file name: dbconnector.php

Code: Select all

<?php
////////////////////////////////////////////////////////////////////////////////////////
// Class: DbConnector
// Purpose: Connect to a database, MySQL version
///////////////////////////////////////////////////////////////////////////////////////
require_once 'SystemComponent.php';

class DbConnector extends SystemComponent {

var $theQuery;
var $link;

//*** Function: DbConnector, Purpose: Connect to the database ***
function DbConnector(){

 // Load settings from parent class
 $settings = SystemComponent::getSettings();

 // Get the main settings from the array we just loaded
 $host = $settings['dbhost'];
 $db = $settings['dbname'];
 $user = $settings['dbusername'];
 $pass = $settings['dbpassword'];

 // Connect to the database
 $this->link = mysql_connect($host, $user, $pass);
 mysql_select_db($db);
 register_shutdown_function(array(&$this, 'close'));

}

//*** Function: query, Purpose: Execute a database query ***
function query($query) {
 $this->theQuery = $query;
 return mysql_query($query, $this->link);
}

//*** Function: getQuery, Purpose: Returns the last database query, for debugging ***
function getQuery() {
 return $this->theQuery;
}

//*** Function: getNumRows, Purpose: Return row count, MySQL version ***
function getNumRows($result){
 return mysql_num_rows($result);
}

//*** Function: fetchArray, Purpose: Get array of query results ***
function fetchArray($result) {
 return mysql_fetch_array($result);
}

//*** Function: close, Purpose: Close the connection ***
function close() {
 mysql_close($this->link);
}


}
?>
---------------------------------------------
File Name: Sentry.php

Code: Select all

<?php
////////////////////////////////////////////////////////////////////////////////////////
// Class: sentry
// Purpose: Control access to pages
///////////////////////////////////////////////////////////////////////////////////////
class sentry {
 
 var $loggedin = false; // Boolean to store whether the user is logged in
 var $userdata;   //  Array to contain user's data
 
 function sentry(){
  session_start();
  header("Cache-control: private"); 
 }
 
 //======================================================================================
 // Log out, destroy session
 function logout(){
  unset($this->userdata);
  session_destroy();
  return true;
 }

 //======================================================================================
 // Log in, and either redirect to goodRedirect or badRedirect depending on success
 function checkLogin($user = '',$pass = '',$group = 10,$goodRedirect = '',$badRedirect = ''){

  // Include database and validation classes, and create objects
  require_once('DbConnector.php');
  require_once('Validator.php');
  $validate = new Validator();
  $loginConnector = new DbConnector();
  
  // If user is already logged in then check credentials
  if ($_SESSION['user'] && $_SESSION['pass']){

   // Validate session data
   if (!$validate->validateTextOnly($_SESSION['user'])){return false;}
   if (!$validate->validateTextOnly($_SESSION['pass'])){return false;}

   $getUser = $loginConnector->query("SELECT * FROM rusers WHERE user = '".$_SESSION['user']."' AND pass = '".$_SESSION['pass']."' AND thegroup <= ".$group.' AND enabled = 1');

   if ($loginConnector->getNumRows($getUser) > 0){
    // Existing user ok, continue
    if ($goodRedirect != '') { 
     header("Location: ".$goodRedirect."?".strip_tags(session_id())) ;
    }   
    return true;
   }else{
    // Existing user not ok, logout
    $this->logout();
    return false;
   }
   
  // User isn't logged in, check credentials
  }else{ 
   // Validate input
   if (!$validate->validateTextOnly($user)){return false;}
   if (!$validate->validateTextOnly($pass)){return false;}

   // Look up user in DB
   $getUser = $loginConnector->query("SELECT * FROM rusers WHERE user = '$user' AND pass = PASSWORD('$pass') AND thegroup <= $group AND enabled = 1");
   $this->userdata = $loginConnector->fetchArray($getUser);

   if ($loginConnector->getNumRows($getUser) > 0){
    // Login OK, store session details
    // Log in
    $_SESSION["user"] = $user;
    $_SESSION["pass"] = $this->userdata['pass'];
    $_SESSION["thegroup"] = $this->userdata['thegroup'];
        
    if ($goodRedirect) { 
     header("Location: ".$goodRedirect."?".strip_tags(session_id())) ;
    }
    return true;

   }else{
    // Login BAD
    unset($this->userdata);
    if ($badRedirect) { 
     header("Location: ".$badRedirect) ;
    }  
    return false;
   }
  }   
 }
} 
?>
-----------------------------------------

filename: validator.php

Code: Select all

<?php
require_once 'SystemComponent.php';
class Validator extends SystemComponent {

 var $errors; // A variable to store a list of error messages

 // Validate something's been entered
 // NOTE: Only this method does nothing to prevent SQL injection
 // use with addslashes() command
 function validateGeneral($theinput,$description = ''){
  if (trim($theinput) != "") {
   return true;
  }else{
   $this->errors[] = $description;
   return false;
  }
 }
 
 // Validate text only
 function validateTextOnly($theinput,$description = ''){
  $result = ereg ("^[A-Za-z0-9\ ]+$", $theinput );
  if ($result){
   return true;
  }else{
   $this->errors[] = $description;
   return false; 
  }
 }

 // Validate text only, no spaces allowed
 function validateTextOnlyNoSpaces($theinput,$description = ''){
  $result = ereg ("^[A-Za-z0-9]+$", $theinput );
  if ($result){
   return true;
  }else{
   $this->errors[] = $description;
   return false; 
  }
 }
  
 // Validate email address
 function validateEmail($themail,$description = ''){
  $result = ereg ("^[^@ ]+@[^@ ]+\.[^@ \.]+$", $themail );
  if ($result){
   return true;
  }else{
   $this->errors[] = $description;
   return false; 
  }
   
 }
 
 // Validate numbers only
 function validateNumber($theinput,$description = ''){
  if (is_numeric($theinput)) {
   return true; // The value is numeric, return true
  }else{ 
   $this->errors[] = $description; // Value not numeric! Add error description to list of errors
   return false; // Return false
  }
 }
 
 // Validate date
 function validateDate($thedate,$description = ''){

  if (strtotime($thedate) === -1 || $thedate == '') {
   $this->errors[] = $description;
   return false;
  }else{
   return true;
  }
 }
 
 // Check whether any errors have been found (i.e. validation has returned false)
 // since the object was created
 function foundErrors() {
  if (count($this->errors) > 0){
   return true;
  }else{
   return false;
  }
 }

 // Return a string containing a list of errors found,
 // Seperated by a given deliminator
 function listErrors($delim = ' '){
  return implode($delim,$this->errors);
 }
 
 // Manually add something to the list of errors
 function addError($description){
  $this->errors[] = $description;
 } 
  
}
?>
-------------------------------------------------
filename viewing.php

This is an example of viewing the information it works fine.

Code: Select all

<?php
// Require the database class
require_once('../includes/DbConnector.php');


// Create an object (instance) of the DbConnector
$connector = new DbConnector();

// Execute the query to retrieve the selected article
$result = $connector->query('SELECT * FROM vendors WHERE ID = '.$HTTP_GET_VARS['id']);

// Get an array containing the resulting record
$row = $connector->fetchArray($result);

?>
                    <?php echo $row['Name']; ?></span></p>
                  <p>
                    <?php 
      
      if ($row['photo'] == '') {
      echo "<b>No Photo!</b>";
      }
      else {
      $photo = $row['photo'];
      echo '<img src="images/vendors/'.$photo.'">'; 
                         }?>
                  </p>
                  <p><?php echo '<b>Address: </b>'.$row['address']; ?><br />
                      <?php echo '<b>Telephone: </b>'.$row['tel']; ?><br />
                      <?php echo '<b>Office: </b>'.$row['office']; ?><br />
                      <b>Email: </b><?php echo $row['email']; ?></p>

feyd | Please use

Code: Select all

,

Code: Select all

and [syntax="..."] tags where appropriate when posting code. Your post has been edited to reflect how we'd like it posted. Please read:  [url=http://forums.devnetwork.net/viewtopic.php?t=21171]Posting Code in the Forums[/url] to learn how to do it too.[/color]

Posted: Tue Oct 02, 2007 4:35 am
by aceconcepts
That's a lot of code to look at.

From my opinion, if the variable values are constant then I think its a good idea to declare a set of variables like you have.

I do not think they are spaghetti :D

Posted: Tue Oct 02, 2007 6:31 am
by onion2k
I can see a few things in there that are a little alarming. For example:

Code: Select all

$result = $connector->query('SELECT * FROM vendors WHERE ID = '.$HTTP_GET_VARS['id']);
An unescaped GET variable being accessed through the old construct with no validation or security. Yikes.

That said I don't think it's particularly 'spaghetti'ish code. It's object oriented so it's quite logical to have several files and plenty of separation. That's not the same as spaghetti though. To me spaghetti is code that's very hard to follow because it leaps around between objects, functions and stuff in an illogical manner. Have you asked the other developer to explain why he thinks it's spaghetti?

Descriptive Subjects

Posted: Tue Oct 02, 2007 10:09 am
by feyd
Please choose a more descriptive subject.
[url=http://forums.devnetwork.net/viewtopic.php?t=30037]Forum Rules[/url] Section 1.1 wrote:2. Use descriptive subjects when you start a new thread. Vague titles such as "Help!", "Why?" are misleading and keep you from receiving an answer to your question.

Posted: Tue Oct 02, 2007 4:15 pm
by RobertGonzalez
The only spaghetti I see in your code comes at the end, where the view and the model seem to merge. But that will always be that since at some point you need to get your code and your output to talk.

Could it be done cleaner? Sure. But the way it is now seems like it should do just fine without a whole lot of need for finding your way around.