Performing CRUD between 4 tables

Question

This code works perfectly fine but I have repeated many lines of code in this controller. I need some suggestions on how to optimize this code and remove repetition, with focus on the create and update methods.

<?php
 namespace App\Http\Controllers\PrivateJob;
 use Illuminate\Http\Request;
 use App\Http\Controllers\Controller;
 use App\Models\catagory;
 use App\Models\city;
 use App\Models\province;
 use App\Models\sector;
 use App\Models\private_jobadb;
 use App\Models\privatejobcity;
 use App\Http\Requests\storePrivateNewspaperFormValidation;
 use App\Models\catagory_private_jobadb;
 class privateJobController extends Controller
 {
 public function dropDownData()
 {
 $newspaper['catagory'] = catagory::all();
 $newspaper['province'] = province::all();
 $newspaper['sector'] = sector::all();
 $newspaper['city'] = city::all();
 return $newspaper;
}
/**
 * Display a listing of the resource.
 *
 * @return \Illuminate\Http\Response
 */
 public function index()
 {
 $private_job = private_jobadb::all();
 return view('admin.private_jobs.all_priate_jobs', compact('private_job'));
}
/**
 * Show the form for creating a new resource.
 *
 * @return \Illuminate\Http\Response
 */
 public function create()
 {
 $newspaper = $this->dropDownData();
 return view('admin.private_jobs.create_Private_jobs', compact('newspaper'));
 }
/**
 * Store a newly created resource in storage.
 *
 * @param \Illuminate\Http\Request $request
 * @return \Illuminate\Http\Response
 */
 public function store(storePrivateNewspaperFormValidation $request)
 {
 $values = $request->input();
 foreach ($values as $key => $result) {
 if ($key == 'city_id') {
 $city = array($key => $result);
 } elseif ($key == 'catagory_id') {
 $catagory = array($key => $result);
 }
 }
 unset($values['city_id']);
 unset($values['catagory_id']);
 if ($request->hasFile('image')) {
 $request->file('image');
 $filename = $request->image->getClientOriginalName();
 $originalfile['image'] = $request->image->storeAs('public/newpaper_jobs', $filename);
 $values['slug'] = str_slug($values['company_name'] . '-' . rand(1, 1000), '-');
 $data = array_merge($values, $originalfile);
 $private_job_ad = new private_jobadb($data);
 $private_job_ad->save();
 $insertedId = $private_job_ad->id;
 foreach ($city['city_id'] as $value) {
 $privatejobcity = new privatejobcity();
 $privatejobcity->fill(['city_id' => $value, 'private_jobabd_id' => $insertedId]);
 $privatejobcity->save();
 }
 foreach ($catagory['catagory_id'] as $value) {
 $catagory_private_jobadb = new catagory_private_jobadb();
 $catagory_private_jobadb->fill(['catagory_id' => $value, 'private_jobads_id' => 
 $insertedId]);
 $catagory_private_jobadb->save();
 }
 //a flash message shold be shown that data successfully created
 flash('Data successfully added')->success();
 return back();
 } else
 $values['slug'] = str_slug($values['company_name'] . '-' . rand(1, 1000), '-');
 $private_job_ad = new private_jobadb($values);
 $private_job_ad->save();
 $insertedId = $private_job_ad->id;
 foreach ($city['city_id'] as $value) {
 $privatejobcity = new privatejobcity();
 $privatejobcity->fill(['city_id' => $value, 'private_jobabd_id' => $insertedId]);
 $privatejobcity->save();
 }
 foreach ($catagory['catagory_id'] as $value) {
 $catagory_private_jobadb = new catagory_private_jobadb();
 $catagory_private_jobadb->fill(['catagory_id' => $value, 'private_jobads_id' => 
 $insertedId]);
 $catagory_private_jobadb->save();
 }
 //a flash message should be shown that image is't selected
 flash('Data successfully added')->success();
 return back();
 }
 public function show($id)
 {
 //
 }
/**
 * Show the form for editing the specified resource.
 *
 * @param int $id
 * @return \Illuminate\Http\Response
 */
 public function edit($id)
 {
 $data = $this->dropDownData();
 $private_job = private_jobadb::with('cities', 'catagory')->where('id', $id)->get()->first();
 $result = $private_job->cities->map(function ($data) {
 return $data['id'];
 })->all();
 $single_catagory = $private_job->catagory->map(function ($data) {
 return $data['id'];
 })->all();
 return view('admin.private_jobs.edit_private_job', compact('data', 'private_job', 'result', 
'single_catagory'));
}
/**
 * Update the specified resource in storage.
 *
 * @param \Illuminate\Http\Request $request
 * @param int $id
 * @return \Illuminate\Http\Response
 */
 public function update(Request $request, $id)
 {
 $values = $request->input();
 foreach ($values as $key => $result) {
 if ($key == 'city_id') {
 $city = array($key => $result);
 } elseif ($key == 'catagory_id') {
 $catagory = array($key => $result);
 }
 }
 unset($values['city_id']);
 unset($values['catagory_id']);
 if ($request->hasFile('image')) {
 $request->file('image');
 $filename = $request->image->getClientOriginalName();
 $originalfile['image'] = $request->image->storeAs('public/private_job', $filename);
 $data = array_merge($values, $originalfile);
 $updated_this_id = private_jobadb::findOrFail($id);
 $updated_this_id->fill($data);
 $updated_this_id->save();
 catagory_private_jobadb::where('private_jobads_id', $id)->delete();
 foreach ($catagory['catagory_id'] as $value) {
 $catagory_private_jobadb = new catagory_private_jobadb();
 $catagory_private_jobadb->fill(['private_jobads_id' => $id, 'catagory_id' => $value]);
 $catagory_private_jobadb->save();
 }
 privatejobcity::where('private_jobabd_id', $id)->delete();
 foreach ($city['city_id'] as $value) {
 $privatejobcity = new privatejobcity();
 $privatejobcity->fill(['city_id' => $value, 'private_jobabd_id' => $id]);
 $privatejobcity->save();
 }
 //a flash message shold be shown that data successfully updated
 flash('Data successfully updated')->success();
 return back();
 } else
 //a flash message should be shown that data is updated without image
 flash('Data successfully updated without new image old image willbe used')->success();
 $updated_this_id = private_jobadb::findOrFail($id);
 $updated_this_id->fill($values);
 $updated_this_id->save();
 return back();
 }
/**
 * Remove the specified resource from storage.
 *
 * @param int $id
 * @return \Illuminate\Http\Response
 */
 public function destroy($id)
 {
 $deletedRows = private_jobadb::where('id', $id)->delete();
 if ($deletedRows) {
 flash('Data successfully added')->success();
 return back();
 } else {
 flash('Data not deleted')->error();
 redirect()->back();
 }
 }
}

• Sᴀᴍ Onᴇᴌᴀ: what does $fillable look like in the private_jobadb model? does it contain city_id and catagory_id? if not, is there a reason to unset those keys in $values before calling fill()?
• arfa imtiaz: private_jobadb does not contain city_id and catagory_id they both are in separate table associative entity. Because i m using mass assignment so i have to unset these keys so i can insert data into private_jobadb table and store city_id and catagory_id in separete array before unset and from that array i insert data to their table @SamOnela
• Alankar More: which version of Laravel are you using?
• Alankar More: There are several changes you need to do related to your code. - Change the names of the model classes you are using. Model name should start with the Capital Letter e.g. City instead city. - Do not use _ in model names instead use like: private_jobadb would be changed to PrivateJobadb - Controller are not the place to call the database queries, they are just used to route your request and represents views by passing some data. - Models can have common queries Here you can use service and repository pattern just to avoid the code repetition

Answer 1

I need some suggestions on how to optimize this code and remove repetition, with focus on the create and update methods.

If you aren't familiar with the principle Don't repeat yourself I recommend reading about it:

The Don’t Repeat Yourself (DRY) principle states that duplication in logic should be eliminated via abstraction;

Duplication is Waste

Adding additional, unnecessary code to a codebase increases the amount of work required to extend and maintain the software in the future. Duplicate code adds to technical debt. Whether the duplication stems from Copy Paste Programming or poor understanding of how to apply abstraction, it decreases the quality of the code. Duplication in process is also waste if it can be automated. Manual testing, manual build and integration processes, etc. should all be eliminated whenever possible through the use of automation.¹

Looking at the store and update methods I see a lot of redundancy. I would abstract out code to save related data - e.g.

private function _savePrivateJobCities($cities, $id) 
{ 
 foreach ($cities as $value) {
 $privatejobcity = new privatejobcity();
 $privatejobcity->fill(['city_id' => $value, 'private_jobabd_id' => $id]);
 $privatejobcity->save();
 }
}
private function _saveCatagoryPrivateJobadbAssociations($catagoryIds, $jobadbId) 
{
 foreach ($catagoryIds as $value) {
 $catagory_private_jobadb = new catagory_private_jobadb();
 $catagory_private_jobadb->fill(['catagory_id' => $value, 'private_jobads_id' => $jobadbId]);
 $catagory_private_jobadb->save();
 }
}

Then those methods can be called in the update and store methods. Those methods also begin with the same 15 lines (taking the result of calling $request->input(), saving the city_id and catagory_id values in separate variables and then calling unset() on those elements. Perhaps the goal is to avoid passing those elements to the call to ->fill() on the model variable. I tested passing excess fields to ->fill() and saw no error. But to ensure no excess fields are passed, one could get the list of fillable fields from the model using ->getFillable(). Then find the intersection using array_intersect_key() and array_flip().

$fillValues = array_intersect_key($values, array_flip($updated_this_id->getFillable());
$updated_this_id->fill($fillValues);

Also - in update(), there are no curly brackets following the else... Is the goal to only call flash() in the else case, or that plus updating by $id?

 } else
 //a flash message should be shown that data is updated without image
 flash('Data successfully updated without new image old image willbe used')->success();
$updated_this_id = private_jobadb::findOrFail($id);

Presuming there should be curly braces around the update lines as well, that code can be abstracted to another method as well:

private function _updatePrivatejobadb($values, $id) {
 $updated_this_id = private_jobadb::findOrFail($id);
 $updated_this_id->fill($values);
 $updated_this_id->save();
}

Then update() can be simplified as follows:

/**
 * Update the specified resource in storage.
 *
 * @param \Illuminate\Http\Request $request
 * @param int $id
 * @return \Illuminate\Http\Response
 */
 public function update(Request $request, $id)
 {
 $values = $request->input();
 if ($request->hasFile('image')) {
 $request->file('image');
 $filename = $request->image->getClientOriginalName();
 $originalfile['image'] = $request->image->storeAs('public/private_job', $filename);
 $data = array_merge($values, $originalfile);
 $this->_updatePrivatejobadb($data, $id);
 catagory_private_jobadb::where('private_jobads_id', $id)->delete();
 $this->_saveCatagoryPrivateJobadbAssociations($values['catagory_id'], $id);
 privatejobcity::where('private_jobabd_id', $id)->delete();
 $this->_savePrivateJobCities($values['city_id'], $id);
 //a flash message shold be shown that data successfully updated
 flash('Data successfully updated')->success();
 return back();
 } else
 //a flash message should be shown that data is updated without image
 flash('Data successfully updated without new image old image willbe used')->success();
 $this->_updatePrivatejobadb($values, $id);
 return back();
 }

the store method has a lot of duplicate code in the two if...else blocks. The else after the extra code to store the image could be removed. Then it can be simplified like below:

/**
 * Store a newly created resource in storage.
 *
 * @param \Illuminate\Http\Request $request
 * @return \Illuminate\Http\Response
 */
 public function store(storePrivateNewspaperFormValidation $request)
 {
 $values = $request->input();
 if ($request->hasFile('image')) {
 $request->file('image');
 $filename = $request->image->getClientOriginalName();
 $originalfile['image'] = $request->image->storeAs('public/newpaper_jobs', $filename);
 }
 $values['slug'] = str_slug($values['company_name'] . '-' . rand(1, 1000), '-');
 $private_job_ad = new private_jobadb($values);
 $private_job_ad->save();
 $insertedId = $private_job_ad->id;
 $this->_savePrivateJobCities($values['city_id'], $insertedId);
 $this->_saveCatagoryPrivateJobadbAssociations($values['catagory_id'], $insertedId);
 //a flash message should be shown that image is't selected
 flash('Data successfully added')->success();
 return back();
 }

---

¹_{http://deviq.com/don-t-repeat-yourself/}

• Sᴀᴍ Onᴇᴌᴀ: @mickmackusa It seems others agree for you on the whole of recent edit suggestions - if this doesn’t answer your question you could post on meta or in chat- e.g. the 2nd