How to use CRUD in a efficient way

Hi,

That's a lot of code to tackle, but I'll try to offer some general advice. One of the first things I noticed is that you are doing a lot of

Model::where('id', '=', $id)

These can be compacted simply to

Model::find($id);

I'm not sure how you are returning a collection (using 'first()') on the results of those queries since you should probably be using ids as unique identifiers. If not then there should be a good reason why multiple 'suspects' etc have the same ID. Perhaps you should look over the docs on how to do basic inserts, retrieves, updates etc for Models. http://laravel.com/docs/5.1/eloquent

If you aren't using Eloquent, you can still abstract a lot of this away in your own model classes, but using raw database queries as above instead of eloquent query builder.

I would also recommend you take a look at this package: http://fractal.thephpleague.com That will allow you to format the response within its own object (you have a lot going on in your controllers) and decouple it from your raw SQL queries. This will save you from doing things like this:

...->get([
            'crime_reports.*', //to get ids and timestamps
            'ct.crime_type as crime_type',
            'cn.crime_description as crime_name',
            's.firstname as suspect_name',
            'v.firstname as victim_name'
        ]);

so that you can do something like the following, without caring about how it should look (since you should ask yourself if the controller is responsible for making sure all the field names are a certain way? I would bet it's a no):

$reports = CrimeReport::all();
$collection = new League\Fractal\Resource\Collection($reports);
return response($fractalManager->createData($collection)->toArray(), 200);

So on the same line of thought, you should really move a lot of your DB code to at least the Model class, since why would the controller be responsible for retrieving all of the model information? I would also perhaps recommend a Mapper layer of some sort, so that you can put the request data in to the mapper object and get a plain old array or object returned that you know will be the format for the rest of your application (so that your input is converted to a standard format at the point of entry).

Sorry if the reply seems a bit incoherent, there's a lot of stuff here that could be worked on but I hope you'll find some of my advice useful :)

Hi again @jearson ,

I thought I'd go over the class structure for your model as well. Is there a reason why crimeName, crimeType, suspect, victim etc is not encapsulated within a 'Crime' or 'Incident' object?

I can imagine the following scenario:

• A Crime has zero, one or many suspect
• A Crime has zero, one or many victims
• A Crime has zero or one CrimeReports
• A Crime has an associated 'name' and 'type' - properties of the crime? It is possible you could have Type being a polymorphic relation if you have different behaviour of the crime depending on the type of crime, but I expect having it as a normal property is what you are after.

This all adds up to the following class structure for me:

<?php

namespace App;
use CrimeReport;
use Victim;
use Suspect;

class Crime extends Model {

  protected $fillable = [
      'name', //these are properties of the 'Crime' object, and you would have these as columns on your 'crimes' table
      'type',
  ];

  //Assuming that a crime has only one possible report, but the report belongs to the crime,
  // not that the crime necessarily belongs to the report (i.e. so you can make a crime before a report)
  public function report(){
    return $this->hasOne(CrimeReport::class);
  }

  //Assuming that a crime has zero, one or many victims, and that victims can be associated with many crimes
  public function victim(){
    return $this->belongsToMany(Victim::class);
  }

  //A similar idea, assuming that you can have zero, one or many suspects,
  //and that suspects can be associated with many crimes
  public function suspect(){
    return $this->belongsToMany(Suspect::class);
  }
}

@Clmrie Thank you for your suggestion. I think I have a CrimeName Model for that. But is it not enough to have a CrimeReport Model and fetch all the possible incidents of crimes using left join? Can you comment also on my CRUD because it seems that the code is too long.

@Clmrie Im doing Model::where('id' ='id') because I'm getting an error not finding the values related to that id findOrFail(id). That's why im relating it to the model. Do you have any suggestion for that.Also in the edit form I'm using the old()function and the names of the inputs are not the same with the attributes in the victimprofile migration.

Hi @jearson

The point was that 'CrimeName' doesn't really map to a real-world object. It is 'part' of the 'Crime'. So that's why i would personally have everything under 'Crime' unless you're doing some fancy logic with the name of a crime, which would be interesting to see.

In terms of the CRUD, I think the problems are borne from a lack of awareness of what Eloquent can do for you. http://laravel.com/docs/5.1/eloquent-relationships

Have you looked at the 'Fractal' package I recommended to you? A lot of the logic you seem to be trying to do is pulling information out of the database and renaming the fields (formatting) at the same time.

If you used Fractal you could do something like this (as said in my previous reply) which would return the exact same index method, but be more reusable and less coupled to the DB:

$reports = CrimeReport::all();
$collection = new League\Fractal\Resource\Collection($reports);
return response($fractalManager->createData($collection)->toArray(), 200);

The fractalManager would be instantiated with a CrimeReportTransformer that you would define (check the fractal docs). For instance:

class CrimeReportTransformer extends TransformerAbstract {

  public function defaultIncludes = [
    'victims',
    'suspects',
  ];

    public function transform(CrimeReport $report)
    {
        return [
        'type' => $report->type,
        'description' => $report->description,
        ];
    }

    public function includeSuspects(CrimeReport $report){
        return $this->collection($report->suspects, new SuspectTransformer());
    }
    //and so on for victims etc, defining a transformer for each object type
}

Are you a subscriber of laracasts? If so I will pull a few links together that I think would help you.

Sorry if the above doesn't really answer what you're looking for, you posted a massive request with lots of code, so it's a lot to take in on both ends. If you want something more specific can you be specific on what it is exactly you are having issues with? I do hope you'll consider my advice though as your code is currently very unmaintainable and bug-prone, with few signs of good OOP design. I'm not trying to put you in a bad mood or be rude, but I am sure the reason you posted here in the first place is because that type of coding is giving you a bit of a headache trying to keep it all together.

:)

@Clmrie yeah Im reading through it. Im a subscriber and I really enjoy learning through laracasts and also with you. My issues is that I when I changed VictimProfile::where('id','=',$crime_update->victim_id)->first(); to VictimProfile::find($id) Im having an error I think because it is passing the id of the CrimeReport model not the id of the VictimProfile.

@jearson

Great. I would recommend starting with these videos/series. I recommend watching through each and making sure you keep going over bits that look useful but you're struggling to understand:

• https://laracasts.com/series/object-oriented-bootcamp-in-php — this is a must
• https://laracasts.com/lessons/solid-single-responsibility-principle — this is a must
• https://laracasts.com/series/laravel-5-fundamentals

Sorry about that VictimProfile error, I meant it in general. If you want to find the VictimProfile in that context, you would do:

VictimProfile::find($crime_update->victim_id);

Does that work?

@Clmrie Thanks. It works. It seems that I keep on repeating myself using that find in most of my function. I s there any good way to solve that?

@jearson

I don't think you even need to use the VictimProfile::find() method in this case, as you have a related object. Since you have 'victim' as a relation on your CrimeReport object, you should be able to simply do:

$crime = CrimeReport::findOrFail($id);
$victim = $crime->victim; //this will give the same information :)

You can find more about that in the Eloquent videos in the Laravel fundamentals series I linked in my previous post.

There is also an option for helping to reduce the number of CrimeReport::find()s that you are doing. It's a little more advanced but you can often clean up your code using what is known as Route-Model Binding.

• http://laravel.com/docs/5.1/routing#route-model-binding
• https://laracasts.com/series/laravel-5-fundamentals/episodes/18 - this is one of the videos in the series I recommended in my last post.

Keep throwing any more questions you have at me :)

@Clmrie Yeah it's the same output. I m reading on the transformer, it seems I cannot yet fully understand how would I change my query using that transformer. I have this question what if Im going to transform the query below into an eloquent ORM. But is it okay to mix eloquent into controller?Or there is another way?Sorry but Iam still learning laravel but im using it on my project.

$results = CrimeReport::query()

    ->leftjoin('crime_types as ct','ct.id', '=', 'crime_reports.crime_type_id')
    ->leftjoin('crime_name as cn','cn.id', '=', 'crime_reports.crime_name_id')
    ->leftjoin('suspect_profile as s','s.id', '=', 'crime_reports.suspect_id')
    ->leftjoin('victim_profile as v','v.id', '=', 'crime_reports.victim_id')
    ->get([
        'crime_reports.*', //to get ids and timestamps
        'ct.crime_type as crime_type',
        'cn.crime_description as crime_name',
        's.firstname as suspect_name',
        'v.firstname as victim_name'
    ]);

@jearson

No problem at all. I always find learning on the job is the fastest way. I don't know which properties/columns you have on crime_reports table so can't really comment on the output that crime_reports.* should give.

Can you tell me what each of your tables columns are? e.g. in this format:

suspects table

• id
• first name
• secondname

The point of the Fractal transformer package is to nest all of these related objects in to one output while keeping it understandable and easy to change, so it definitely fits what you're trying to do. We can go through a few cases of not using the Fractal package once you have given me the information it needs to return in '$results'.

@Clmrie I just applied the model binding. That was nice. Thanks for the link. Jeffrey is so good.

@Clmrie Here is my additional information: I dont know why my project teamate uses string for id's instead of integers.

//crime_reports

public function up()
{
    Schema::create('crime_reports', function (Blueprint $table) {
        $table->increments('id');
        $table->string('crime_type_id');
        $table->string('crime_name_id');
        $table->string('suspect_id');
        $table->string('victim_id');
        $table->timestamps();
    });
}

//suspect_profile

     public function up()
{
    Schema::create('suspect_profile', function (Blueprint $table) {
        $table->increments('id');
        $table->string('firstname');
        $table->string('middlename');
        $table->string('lastname');
        $table->string('house_no');
        $table->string('street');
        $table->string('barangay_id');
        $table->string('city');
        $table->string('gender');
        $table->string('height');
        $table->string('weight');
        $table->string('birthdate');
        $table->string('status');
        $table->string('nationality');
        $table->string('crime_type_id');
        $table->string('distinguising_marks');
        $table->string('contact_no');
        $table->timestamps();
    });
}

//victim_profile

     public function up()
{
    Schema::create('victim_profile', function (Blueprint $table) {
        $table->increments('id');
        $table->string('firstname');
        $table->string('middlename');
        $table->string('lastname');
        $table->string('house_no');
        $table->string('street');
        $table->string('barangay_id');
        $table->string('birthdate');
        $table->string('gender');
        $table->string('contact_no');
        $table->timestamps();
    });
}

//crime_name

      public function up()
{
    Schema::create('crime_name', function (Blueprint $table) {
        $table->increments('id');
        $table->string('section_no');
        $table->string('crime_type_id');
        $table->string('crime_description');
        $table->timestamps();
    });
}

//crime_type

    public function up()
{
    Schema::create('crime_types', function (Blueprint $table) {
        $table->increments('id');
        $table->string('crime_type');
        $table->timestamps();
    });
}

You might find a lot of improvements here. Just let me know so I can I adjust them.

@Clmrie While reading the eloquent I updated my insert with this $victiminfo = VictimProfile::create([ 'firstname'=>$request->victim-name ]); This is a nice solution or there are better ways than this?Thanks

@jearson

Ok, based on your tables your output can be produced with Fractal using the following classes as an example:

<?php

class CrimeReportTransformer extends TransformerAbstract
{

    protected $defaultIncludes = [
        'suspects',
        'victims',
    ];

    public function transform(CrimeReport $report)
    {
        return [
            'id' => $report->id,
        ];
    }

    public function includeSuspects(CrimeReport $report){
        return $this->collection($report->suspects, new SuspectTransformer());
    }

    public function includeVictims(CrimeReport $report){
        return $this->collection($report->victims, new VictimTransformer());
    }
}

class SuspectTransformer extends TransformerAbstract
{


    public function transform(Suspect $suspect)
    {
        return [
            'id' => $suspect->id,
            'firstName' => $suspect->firstname,
            'secondName' => $suspect->lastname,
            //...
            'contactNumber' => $suspect->contact_no,
        ];
    }
}

class VictimTransformer extends TransformerAbstract
{


    public function transform(Victim $victim)
    {
        return [
            'id' => $victim->id,
            'firstName' => $victim->firstname,
            //...
            'gender' => $victim->gender,
            'contact_no' => $victim->contact_no,
        ];
    }
}

If you output it in JSON this would output:

{
  "data": {
    "id" : 1,
    "suspects" : {
      "data" : {
        "id" : 1,
        "firstName" : "John",
        "secondName" : "Doe",
        //...
        "contactNumber" : "123456789"
      }
    },
    "victims" : {
      "data" : {
        "id" : 1,
        "firstName" : "Jane",
        "secondName" : "Doe",
        //...
        "contactNumber" : "123456789"
      }
    }
  }
}

(I have used example values for id etc but you get the idea).

---

In terms of better solution for:

$victiminfo = VictimProfile::create([ 'firstname'=>$request->victim-name ]);

If you are already using Request objects and assigning it so that you have a $request object with $victim-name etc, then you could clean it up further. Instead of your $request object having victim-name, suspect-name etc, just add it to an array that is compatible with your database.

i.e. in the request object, just set it on a $data array that the request object has as a property: $data['firstname'] = Request::get('victim_firstname'); $data['lastname'] = Request::get('victim_lastname');

Then when you come to doing the creation you can do this:

$victiminfo = VictimProfile::create($request->data);

If you don't go through and assign each to the 'data' object and instead pass through Request::all() you may end up with mass assignment exceptions being thrown if you post anything that is not allowed. This way filters it out.

If you can show me your VictimProfileRequest class I can offer more specific advice if the above is confusing.

@Clmrie Do you think it is better to change the names of the field in the migrations for victim_profile and suspect_profile to reflect on the names of input fields in the form?For easier implementation of CRUD?

@Clmrie Sorry but where do I place the classes? In the Model section?Is it separate classes or Inner Classes?

@jearson

Regarding keeping the names the same as the migrations - it doesn't really matter in my opinion. Yes, if you keep the names the same it will be easier because you can just throw data from one object to another (input straight to model, or output straight to html etc) but there may be a time you want to change the name of the field on the database backend but keep the same output response. If you enforce them to be the same at the moment you will have a lot of code to change should you ever want to rename a column. Does that make sense? If you don't foresee this every happening then I guess it would be okay.

On to your second point: In this case Fractal is separate from the framework (it can actually be used with any PHP code, not just Laravel). If you follow the instructions for setting it up with laravel on their website you will be able to use the classes I gave you and modify them as you need.

In my case I normally have a 'Models' directory in 'app' , but I know from your namespace that you just have them in the default location, which is absolutely fine. In your case I would just create a 'Transformers' folder in the 'app' folder and then place them in their own separate files there. Remember to set the namespace on each of the classes.

e.g.

<?php
namespace App\Transformers;

class CrimeReportTransformer extends TransformerAbstract {
//...

@Clmrie I like the way you explain the concept thanks. Yeah I've already set the fractal. Okay then I'll do this first and update you again.

@Clmrie Actually I dont have VictimProfileRequest.

@jearson

No problem, looking back at your original code I can see that now. I would suggest this is a bit of a code smell and should tell you something about your structure. You have 'CrimeNewReportRequest' dealing with the creation of a CrimeReport, Victim, Suspect etc.

If you were to follow the single responsibility principle (which is definitely recommended) you would separate this process out so that you have controllers for each Model type. e.g.

• CrimeReportController
• VictimController
• SuspectController

Each of these would deal with listing, storing, updating etc of each of the models. You would have separate views to create each victim, suspect etc.

You would then have your separate request types that only deals with the input that is relevant to it. So for your VictimController you might have one like 'VictimProfileRequest' and that deals with the input for that.

If you do want to have it so that you create the whole system (report, victims, suspects etc) at the same time from one controller (the reason this is a code smell is that it can't be reused for another purpose) then you could instead create several request objects and type hint them in your methods like before.

e.g.

public function store(VictimProfileRequest $victimRequest, SuspectProfileRequest $suspectRequest, ReportRequest $reportRequest) {
    $victim = Victim::create($victimRequest->data);
    $suspect = Suspect::create($suspectRequest->data);
    $report = $reportRequest::create($reportRequest->data);

    $suspect->report()->associate($report); //this relation assumes Report HasMany Suspects , and Suspect belongsTo Report
    $suspect->save();
    
    $victim->report()->associate($report); //same relation as above, but for victims instead
    $victim->save();
}

I'm not 'entirely' sure the above will work as I haven't had a use case like this myself, but I believe Laravel does some magic in the background and you can have multiple request objects that do different things with the original input data, allowing you to filter it using these objects like I suggested in my previous post.

Does this all make sense?

@Clmrie I m having hard time with the fractal. Im reading more about it. In my CrimeReportTransformer where did the suspects and victims from the $defaultIncludes came from? And how do I implement this? Where will I place the code:

$reports = CrimeReport::all(); $collection = new League\Fractal\Resource\Collection($reports); return response($fractalManager->createData($collection)->toArray(), 200); ?

@jearson

Fractal has its own code that sorts stuff out for you, in a similar way to Laravel. the $defaultIncludes array is a convenient way to add nested models to your output without knowing how they are transformed. For each entry in $defaultIncludes, you have to create a corresponding method, with the main object as the method parameter. e.g. for your example:

class CrimeReportTransformer extends TransformerAbstract {

  public $defaultIncludes = [
    'victims',
    'suspects',
  ];

    public function transform(CrimeReport $report)
    {
        return [
            'id' => $report->id,
        ];
    }

    public function includeSuspects(CrimeReport $report){
        return $this->collection($report->suspects, new SuspectTransformer());
    }
    
   public function includeVictims(CrimeReport $report) {
    return $this->collection($report->victims, new VictimTransformer());
  }
}

I offered example Transformers to use a of couple replies ago.

When in your controller you can then do this:

$reports = CrimeReport::all();
$collection = new League\Fractal\Resource\Collection($reports, CrimeReportTransformer()); 
$fractalManager = new League\Fractal\Manager();

return response($fractalManager->createData($collection)->toArray(), 200);

The fractal package then knows that the transformer has a 'transform()' method on it, and applies that on each of the $collection.

On each of these it loops through each entry in $defaultIncludes and looks for their corresponding methods 'includeSuspects' , 'includeVictims' and then appends the output of each of those transformers to the output as well...and so on. By doing this you allow the system to automatically search down all possible 'rabbit holes' without you getting yourself in a tangled mess. It breaks down to 'what do I want each model to look like when it is output' - then implement that transformer. Then if you know you want 'victims' anywhere else, you can just include that transformer and reference the victims you need transformed.

Obviously I'm not going to be able to walk you through each step of building your application so I would recommend searching for articles on Fractal, Laravel etc using Google that might help you. For instance, this might be helpful: http://laravelista.com/laravel-fractal/

Just remember that Fractal doesn't have anything to do with Laravel, I'm just recommending you use it as it will help you clean up your output code quite a bit.

@Clmrie ah okay. Hehe yeah Im reading that article.

@jearson

Great. I just realised the output of that is what I would do for an API. If you're using a normal Blade View Template you can do this instead:

$reports = CrimeReport::all();
$collection = new League\Fractal\Resource\Collection($reports, CrimeReportTransformer()); 
$fractalManager = new League\Fractal\Manager();

return view('yourviewname')->with(['data' => $fractalManager->createData($collection)->toArray()]);

Your output would then be available from the $data variable in your view. If you want to see what that looks like just do this in your view:

    <pre>
        <?php print_r($data); ?>
    </pre>

@Clmrie I figured that a while back because Im returning twice and it will ignore my view, that's why i got confused.

@Clmrie Im getting an error :

    ErrorException in Scope.php line 280:
    Invalid argument supplied for foreach()

@jearson

Is Scope.php a file you created? If so, can you show me the code of Scope.php? or at least the method that line 280 is in. When did the error occur? If it's not your code (Scope.php) then it is likely what you are passing in to it that is the problem.

@Clmrie No I didnt created that file.Its a built in file.

@Clmrie I look for that Scope under Fractal it seems the value being passed has some error. Heres the method in line 280 foreach ($data as $value) { list($transformedData[], $includedData[]) = $this->fireTransformer($transformer, $value); }