4
\$\begingroup\$

I love to post here on codereview my code to have a feedback from developers that have more experience than me, to improve my coding skills. During these days I'm working on a webapp for a small business who need to manage internally the production cycle of handmade hats. While I'm analyzing all the clients needs, I've started writing code that will become a class to upload hats images on the server and informations to the database. I'm going deep to learn on how to write good PHP OOP code and on how to design a simple MVC (but i think that this word doesn't fit at all my work.) I've this code for now and it's separated into two file, one that will contain all the business logics, and another one that will only control all the $_POST,$_GET requests. I will add more feature soon, but for now i need to finish a fully working draft.

index.php

This is the "front controller", and the page that the user will use to manage CRUD operations from a GUI. Due to the fact that i want to make a one page app, if someone can suggest me how i can show the login form when the $_SESSION is not set, I will appreciate it much. I know that it can be done using php?

<div class="container" id="login">    
    <div class="row justify-content-center"> 
        <div class="col-sm-4" id="">
            <form method="POST" action="" id="loginForm">
                    <input type="text" class="form-control" name="username" placeholder="Username" id="user" />
                    <br>
                    <input type="password" class="form-control" name="password" placeholder="Password" id="password" />
                    <br>
                    <button type="submit" class="btn btn-primary btn-block" id="login">L</button>
            </form>
        </div>
    </div>
</div>    

<div class="container" id="app">
    <div class="row">
        <div class="col-sm-12 col-lg-6" id="">
        <form method="POST" enctype="multipart/form-data" action="" id="f">
            <div class="form-row">

                <div class="col-lg-6">
                <label>Material 1</label>    
                <input type="text" class="form-control" name="materialA" id="materialA" />    
                <small></small> 
                </div>

                <div class="col-lg-6">
                <label>Material 2</label>    
                <input type="text" class="form-control" name="materialB" id="materialB" />    
                <small></small> 
                </div>

                <div class="col-lg-6">
                <label>Material 3</label>    
                <input type="text" class="form-control" name="materialC" id="materialC" />    
                <small></small> 
                </div>

                <div class="col-lg-6">
                <label>Material 4</label>     
                <input type="text" class="form-control" name="materialD" id="materialD" /> 
                <small></small>    
                </div>

                <div class="col-lg-12">
                <label>Accessories</label>    
                <textarea class="form-control" name="accessories" id=""></textarea>  
                <small></small> 
                </div>

                <div class="col-lg-12">
                <label>Hat model image</label>    
                <input type="file" name="hatimg" id="hatImg">
                <small></small> 
                </div>

                <div class="col-lg-12">

                    <button type="submit" class="btn btn-success">Save</button>  

                </div>
            </div>
        </form>
        </div>
        <div class="col-sm-12 col-lg-6">
        <img class="img-fluid" src="#" id="previewHat">
        </div>
    </div>
</div>    

<script>
$(document).ready(function(){
   $('#f').submit(function(e){
       e.preventDefault();
            $.ajax({
                type: 'POST',
                url: 'do.php',
                data: new FormData(this),
                cache: false,
              //  dataType: false,
                processData: false,
                contentType: false,
                success: function(res){
                    console.log(res);
                }
        });
    });
readURL();    
});    

function readURL(input) {

  if (input.files && input.files[0]) {
    var reader = new FileReader();

    reader.onload = function(e) {
      $('#previewHat').attr('src', e.target.result);
    }

    reader.readAsDataURL(input.files[0]);
  }
}

$("#hatImg").change(function() {
  readURL(this);
});    

</script>

do.php

<?php
session_start();
require_once 'db.php';
require_once 'core.php';

$core = new sys($db);

if(isset($_FILES['hatimg'])){

    $data = array('tmp_name'=>$_FILES['hatimg']['tmp_name'],'name'=>$_FILES['hatimg']['name'],'size'=>$_FILES['hatimg']['size'],'error'=>$_FILES['hatimg']['error'],'materialA'=>$_POST['materialA'],'materialB'=>$_POST['materialB'],'materialC'=>$_POST['materialC'],'materialD'=>$_POST['materialD'],'accessories'=>$_POST['accessories']);

echo $core->insert($data);

}
?>

system.php

<?php

class sys{

private $db = null;  
private $extensions;    
protected static $stmt;    
public $data = array();


public function __construct(\PDO $db){

    $this->db = $db;
}    

public function insert(array $data){

    $this->extensions = array('image/jpeg','image/jpg','image/png');
    if(is_array($data)){
        if($data['error'] === UPLOAD_ERR_OK){

            $this->tmp_name = $data['tmp_name'];    
            $this->name = basename($data['name']);
            $this->size = $data['size'];  

            $this->finfo = finfo_open(FILEINFO_MIME_TYPE);    
                if(in_array(finfo_file($this->finfo, $this->tmp_name),$this->extensions)){
                    if(is_uploaded_file($this->tmp_name)){

                        if(move_uploaded_file($this->tmp_name, "img/$this->name")){

                            $stmt = $this->db->prepare('INSERT INTO hats_info (materialA,materialB,materialC,materialD,accessories,hat_image) VALUES (?,?,?,?,?,?)');  
                            return $stmt->execute(array($data['materialA'],$data['materialB'],$data['materialC'],$data['materialD'],$data['accessories'],$data['name']));
                            }
                        }
                } else {
                    return 'invalid file format';
            }
        }
    }      
}

?>
\$\endgroup\$
6
\$\begingroup\$

There are only two hard things in Computer Science: cache invalidation and naming things. -- Phil Karlton

For the life of me I wouldn't think of the $core->extensions property as a list of file extensions allowed to download. The only meaning I'd make is a list of active plugins.

So the main refactoring should be renaming sys class and $core variable to something more related to handling a particular download case.

The rest is just a few nitpicks.

  • if(is_array($data)){ is a tautology, you already have a type hint in the function definition.
  • making the sole function's argument an array, and especially such a big array is a bad idea, you are robbing yourself of useful argument hints.
  • error checking conditions without else statement are bad as well. Either add a code to handle an error (like in if($data['error'] === UPLOAD_ERR_OK){), or get rid of the condition at all (like with if(move_uploaded_file($this->tmp_name, "img/$this->name")){)
  • echoing a boolean value has some caveats. Better make an explicit value, for example by casting it to int.
  • returning the PDOStatement::execute()s result makes no sense as it will always be true, otherwise the execution won't reach this point at all being interrupted by the exception thrown. To handle database errors though AJAX you have to implement an error handler like one shown in my article.
  • return value should be always of the same type. Returning a boolean in case of one error and a string in case of another is a total mess. Make your code throw exceptions on error, catch them and report to the client. Whereas a correct ar execution don't have to return anything
  • I don't see a point in having $data, $name and $tmp_name class variables.
\$\endgroup\$
  • \$\begingroup\$ "making the sole function's argument an array, and especially such a big array is a bad idea, you are robbing yourself of useful argument hints." ok, but I need to pass the $_FILES and the $_POST informations, so an array was the first solution I've thinked. Maybe splitting the upload and the database insertion in two separate method is better than actually I've done? \$\endgroup\$ – user9741470 Jun 2 '18 at 11:31
  • 1
    \$\begingroup\$ You can make just separate arguments instead of array \$\endgroup\$ – Your Common Sense Jun 2 '18 at 14:47
  • \$\begingroup\$ I will evaluate this possibility, by modifying the method to accept two arguments. So it will become public function insert(array $data, array $file){}. I will also rename the array that actually holds the allowed mime type. If you jave any suggestions about uploaded files validations please don't hesitate to post it. \$\endgroup\$ – user9741470 Jun 2 '18 at 15:07
  • \$\begingroup\$ I would make a distinct function parameter for each variable you are passing. just like you did with array, but as function parameters. \$\endgroup\$ – Your Common Sense Jun 3 '18 at 8:03

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.