3

Estoy trabajando en un proyecto y en un controlador tengo a mi parecer exceso de código que se puede reducir, pero no se como reducirlo.

public function crearAbonado(NuevoServicioRequest $request){
    $id = $request->input('id');
    $solicitud = Solicitud::find($id);
    $abonado = new Abonado;
    $IdAbonado = $request->input('IdAbonado');
    $medidor = new Medidor;
    $IdMedidor = $request->get('medidor');

    // Se traen los datos que hay en solicitud
    $cedula = $solicitud->Cedula;
    $nombre = $solicitud->Nombre;
    $apellido1 = $solicitud->Apellido1;
    $apellido2 = $solicitud->Apellido2;
    $zonaServicio = $solicitud->ZonaServicio;
    $direccion = $solicitud->Direccion;
    $telefono = $solicitud->Telefono;
    $correo = $solicitud->Correo;
    $usoServicio = $solicitud->UsoServicio;
    $numeroFinca = $solicitud->NumeroFinca;
    $numeroPlanoC = $solicitud->NumeroPlanoCatastro;
    $numeroNiveles = $solicitud->NumeroNiveles;
    $numeroMiembrosF = $solicitud->NumMiembrosFam;
    $areaTerreno = $solicitud->AreaTerreno;
    $img_cedula = $solicitud->img_cedula;

    // Asignar valores a abonado
    $abonado->IdAbonado = $IdAbonado;
    $abonado->Cedula = $cedula;
    $abonado->Nombre = $nombre;
    $abonado->Apellido1 = $apellido1;
    $abonado->Apellido2 = $apellido2;
    $abonado->ZonaServicio = $zonaServicio;
    $abonado->Direccion = $direccion;
    $abonado->Telefono = $telefono;
    $abonado->Correo = $correo;
    $abonado->UsoServicio = $usoServicio;
    $abonado->NumeroFinca = $numeroFinca;
    $abonado->NumeroPlanoCatastro = $numeroPlanoC;
    $abonado->NumeroNiveles = $numeroNiveles;
    $abonado->NumMiembrosFam = $numeroMiembrosF;
    $abonado->AreaTerreno = $areaTerreno;
    $abonado->img_cedula = $img_cedula;

    $abonado->save();

    $solicitud->IdEstado = 5;
    $solicitud->update();

    DB::table('tblmedidor')
        ->where('IdMedidor', $IdMedidor)
        ->update(['IdAbonado' => $IdAbonado]);

        SweetAlertController::solicitudRealizada('Nuevo Servicio');

        return redirect()->route('solicitudes.index');
}

La función de este método es enviar los datos que vienen de una tabla llamada solicitud a otra que se llama abonado.

Me gustaría saber si es necesario crear algún otro archivo, o funciones diferentes para poder optimizar el código, ya que como claramente se ve, esta muy cargado de código.

1
  • Bienvenido a StackOverflow en Español, te invito a que hagas el Recorrido y consigas tu primera medalla!, deberías leer Qué preguntas debo evitar, me parece que tu pregunta es algo amplia y basada principalmente en opiniones, ya que, cada persona tiene su forma de organizar y programar, y por lo tanto no tiene una respuesta fija, sí quieres replantear tu pregunta también ten en cuenta Cómo preguntar.
    – DaxTter77
    el 3 sep. 2019 a las 15:53

3 respuestas 3

2

Hay muchas cosas que se le pueden mejorar a dicho código, explicaré brevemente una mejora rápida que hice, sin tocar temas avanzados:

public function crearAbonado(NuevoServicioRequest $request){

    $solicitud = Solicitud::find($request->id);
    $IdAbonado = $request->IdAbonado;
    $IdMedidor = $request->medidor;

    $abonado = Abonado::create([
        'IdAbonado' => $IdAbonado,
        'Cedula' => $solicitud->cedula,
        'Nombre' => $solicitud->nombre,
        ...
    ]);

    $solicitud->fill(['IdEstado' => 5]);

    DB::table('tblmedidor')
        ->where('IdMedidor', $IdMedidor)
        ->update(['IdAbonado' => $IdAbonado]);

    SweetAlertController::solicitudRealizada('Nuevo Servicio');

    return redirect()->route('solicitudes.index');
}
  1. Utilizar el método create() en vez del largo proceso de save(). Create acepta un array con los atributos y en una sola línea, no es necesario inicializar un modelo, definir sus propiedades y luego llamar a un método.
  2. No veo necesario inicializar el una instancia de Medidor, no es utilizada en ninguna parte.
  3. Las variables del request se pueden llamar directamente como propiedades, no es necesario usar el método input.
  4. Usar fill() en vez de update(). La explicación es la misma de create() vs. save().
  5. Para que sea más homogéneo el proyecto, y específicamente el método, deberías usar siempre Eloquent o el Query Builder, no mezclarlos.
1
  • Muchas gracias también por esta solución, las explicaciones son muy precisas y claras. Tendré muy en cuenta el punto 5, ya que tienes razón.
    – ScriptKash
    el 4 sep. 2019 a las 16:49
0

Viendo el codigo esta es mi idea para optimizarlo y tener menos lineas, siempre en esto se puede mejorar mucho mas.

Los cosas que se cambiaron fueron las siguientes:

1) Uni la primera asignacion de variables con la segunda, con lo cual se reduce considerablemente la lineas de codigo.

2) La variable $id ahora esta ingregada en la $solicitud

public function crearAbonado(NuevoServicioRequest $request){

    $solicitud = Solicitud::find($request->input('id'));
    $abonado = new Abonado;
    $IdAbonado = $request->input('IdAbonado');
    $medidor = new Medidor;
    $IdMedidor = $request->get('medidor');

    // Asignar valores a abonado y se traen los datos
    $abonado->IdAbonado = $IdAbonado;
    $abonado->Cedula = $solicitud->Cedula;
    $abonado->Nombre = $solicitud->Nombre;
    $abonado->Apellido1 = $solicitud->Apellido1;
    $abonado->Apellido2 = $solicitud->Apellido2;
    $abonado->ZonaServicio = $solicitud->ZonaServicio;
    $abonado->Direccion = $solicitud->Direccion;
    $abonado->Telefono = $solicitud->Telefono;
    $abonado->Correo = $solicitud->Correo;
    $abonado->UsoServicio = $solicitud->UsoServicio;
    $abonado->NumeroFinca = $solicitud->NumeroFinca;
    $abonado->NumeroPlanoCatastro = $solicitud->NumeroPlanoCatastro;
    $abonado->NumeroNiveles = $solicitud->NumeroNiveles;
    $abonado->NumMiembrosFam = $solicitud->NumMiembrosFam;
    $abonado->AreaTerreno = $solicitud->AreaTerreno;
    $abonado->img_cedula = $solicitud->img_cedula;
    $abonado->save();

    $solicitud->IdEstado = 5;
    $solicitud->update();

    DB::table('tblmedidor')
        ->where('IdMedidor', $IdMedidor)
        ->update(['IdAbonado' => $IdAbonado]);

        SweetAlertController::solicitudRealizada('Nuevo Servicio');

        return redirect()->route('solicitudes.index');
}

Espero que pueda ser de ayuda...

0

Posiblemente esta pregunta caiga en basada en opiniones ya que es algo subjetivo y puede tener multiples respuestas válidas, no obstante considero que puedes acortar bastante código eliminando cosas innecesarias, como duplicar la asignación de variable por ejemplo.

Podrías hacer algo así:

public function crearAbonado(NuevoServicioRequest $request){
    // esto lo puedes pasar directamente ya que no lo utilizas en nada más
    /* $id = $request->input('id'); */
    $solicitud = Solicitud::find($request->input('id'));
    $IdAbonado = $request->input('IdAbonado');
    // ya que solo lo utilizas una vez nos podemos ahorrar esta asignación
    /* $IdMedidor = $request->get('medidor'); */

    $abonado = new Abonado;
    // esto no lo estas utilizando en el método
    /* $medidor = new Medidor; */

    // Se traen los datos que hay en solicitud
    // esto es innecesario ¿para que utilizar una variable intermedia?
    /*
    $cedula = $solicitud->Cedula;
    $nombre = $solicitud->Nombre;
    $apellido1 = $solicitud->Apellido1;
    $apellido2 = $solicitud->Apellido2;
    $zonaServicio = $solicitud->ZonaServicio;
    $direccion = $solicitud->Direccion;
    $telefono = $solicitud->Telefono;
    $correo = $solicitud->Correo;
    $usoServicio = $solicitud->UsoServicio;
    $numeroFinca = $solicitud->NumeroFinca;
    $numeroPlanoC = $solicitud->NumeroPlanoCatastro;
    $numeroNiveles = $solicitud->NumeroNiveles;
    $numeroMiembrosF = $solicitud->NumMiembrosFam;
    $areaTerreno = $solicitud->AreaTerreno;
    $img_cedula = $solicitud->img_cedula;
    */

    // Asignar valores a abonado
    // asignamos los valores directamente mejor
    $abonado->IdAbonado = $IdAbonado;
    $abonado->Cedula = $solicitud->Cedula;
    $abonado->Nombre = $olicitud->Nombre;
    $abonado->Apellido1 = $solicitud->Apellido1;
    $abonado->Apellido2 = $solicitud->Apellido2;
    $abonado->ZonaServicio = $solicitud->ZonaServicio;
    $abonado->Direccion = $solicitud->Direccion;
    $abonado->Telefono = $solicitud->Telefono;
    $abonado->Correo = $solicitud->Correo;
    $abonado->UsoServicio = $solicitud->UsoServicio;
    $abonado->NumeroFinca = $solicitud->NumeroFinca;
    $abonado->NumeroPlanoCatastro = $solicitud->NumeroPlanoCatastro;
    $abonado->NumeroNiveles = $solicitud->NumeroNivele;
    $abonado->NumMiembrosFam = $solicitud->NumMiembrosFam;
    $abonado->AreaTerreno = $solicitud->AreaTerreno;
    $abonado->img_cedula = $solicitud->img_cedula;
    $abonado->save();

    // al método update le puedes pasar el valor directamente
    /* $solicitud->IdEstado = 5; */
    $solicitud->update(['IdEstado' => 5]);

    DB::table('tblmedidor')
        ->where('IdMedidor', $request->get('medidor'))
        ->update(['IdAbonado' => $IdAbonado]);

    SweetAlertController::solicitudRealizada('Nuevo Servicio');

    return redirect()->route('solicitudes.index');
}
4
  • Muchas gracias por la explicación con los comentarios, me quedo muy claro y lo aplicaré esa lógica también para otros métodos.
    – ScriptKash
    el 3 sep. 2019 a las 16:17
  • Yo tambien pense en la misma solucion, bien hecho! el 3 sep. 2019 a las 16:31
  • @ScriptKash No entiendo el negativo.
    – Xerif
    el 25 sep. 2019 a las 15:00
  • @Xerif Hola, al parecer fue otra persona, ya que yo no tenía la opción para votar y que se mostrase.
    – ScriptKash
    el 25 sep. 2019 a las 15:09

Tu Respuesta

By clicking “Publica tu respuesta”, you agree to our terms of service and acknowledge you have read our privacy policy.

¿No es la respuesta que buscas? Examina otras preguntas con la etiqueta o formula tu propia pregunta.