1
\$\begingroup\$

enter image description here

I want to do an API or contract in Laravel focus when one person wants a contract or credit, but honestly it's for a scholarship but the process it's similar when I got a credit from a bank. Honestly I wrote this code and it works well, but I would like to improve my code, because I want to become a professional.

In my Class ConsultaSivi there are different methods- there are setters and getters, validation and it passes everything that can become a contract or credit. I want to mention that there are two data sources: one if it's a database and the second if it's using an API. ConsultaSivi gets information from another API and ConsultaCreded gets information from a database.

How can I improve my class ConsultaSivi and ConsultaCreded?

Classes Codeudor and Deudor are sub-classes of Class Persona.

Here's my class persona

<?php

namespace App\Libs\Domain;

/**
 * clase que contiene los datos de una persona dentro de un credito
 */
class Persona
{
    /**
     * contiene el numero de cedula de la persona
     * @var [string]
     */
    public $cedula;

    /**
     * contine el nombre de la persona
     * @var [string]
     */
    public $nombreCompleto;
}

My class credito (or credit):

<?php

namespace App\Libs\Domain;

/**
 * clase que alamacenara los daos de un prestamo/credito perteneciente a un prestatario
 */
class Credito
{
    /**
     * variable destinada a contener el obj de la clase deudor
     * @var [obj]
     */
    public $deudor;

    /**
     * variable destinada a contener el obj de la clase codeudores
     * @var [obj]
     */
    public $codeudores;

    /**
     * contiene el numero de un contrato o credito
     * @var [string]
     */
    public $numeroContrato;

    /**
     * contiene el monto total del prestamo
     * @var [float]
     */
    public $montoCredito;

    /**
     * contiene el monto de los pagos trimestrales
     * @var [float]
     */
    public $pagoTrimestral;

    /**
     * contiene la relacion de pago para el cobro del cheque
     * @var [string]
     */
    public $relacionPago;

    /**
     * almacena el numero del memo-resolucion del contrato
     * @var [string]
     */
    public $memo;

    /**
     * almacena el año inicial cuando empezo el contrato
     * @var [string]
     */
    public $anioInicial;

    /**
     * almacena el año final cuando finaliza el contrato
     * @var [string]
     */
    public $anioFinal;
}

This is my class ConsultaSivi:

<?php

namespace App\Libs;

use App\Libs\Domain\Codeudor;
use App\Libs\Domain\Credito;
use App\Libs\Domain\Deudor;
use App\Libs\Domain\Persona;
use App\Libs\Exceptions\SystemExceptions;
use GuzzleHttp\Client;
use GuzzleHttp\Psr7\Request;
use Validator;

class ConsultaSivi implements ConsultaInterface
{
    /**
     * Número de cédula del deudor del principal
     * @var [string]
     */
    public $cedulaDeudor;
    /**
     * Números de cédulas de los codeudores del credito
     * @var [string]
     */
    public $cedulasCodeudores;

    /**
     * Credito que esta siendo construido
     * @var [obj]
     */
    public $credito;

    /**
     * Usuario que inicio sesion
     */
    public $user;

    /**
     * Ip de máquina para hacer la consulta al servicio
     */
    public $ip;

    /**
     * método que asigna la cédula del deudor recibida por parametro
     * @param [string] $cedulaDeudor [recibe la cédula por entrada en teclado]
     */
    public function setCedulaDeudor($cedulaDeudor)
    {
        $isValid = $this->validarCedula($cedulaDeudor);

        if ($isValid) {
            $this->cedulaDeudor = $cedulaDeudor;
        }
    }

    /**
     * método que asigna la/s cédula/s del /los deudor/res recibida por parametro
     * @param [string] $cedulasCodeudores [recibe cédulas por entrada en teclado]
     */
    public function setCedulasCodeudores($cedulasCodeudores)
    {
        $cedulas = [];

        if (is_array($cedulasCodeudores)) {
            $isValid = $this->validarCedula($cedulasCodeudores);

            if ($isValid) {
                $this->cedulasCodeudores = $cedulasCodeudores;
            }
        } else {
            new SystemExceptions(401);
        }
    }

    /**
     * método que realiza la consulta al api de sivi-linux y lo retorna a la ruta
     * @return [type] [description]
     */
    public function consultar()
    {
        $this->credito = new Credito();
        $this->consultarDeudor();
        $this->consultarCodeudores();
    }

    /**
     * método que  crea Deudor
     * @return [type] [obj]
     */
    public function consultarDeudor()
    {
        $deudor = new Deudor();
        $deudor = $this->api($this->cedulaDeudor);
        $deudor = $this->concatenarNombreCompleto($deudor);

        return $this->credito->deudor = $deudor;
    }

    /**
     * método que crea codeudor
     * @return [type] [obj]
     */
    public function consultarCodeudores()
    {
        $codeudor = [];
        $codeudores = new Codeudor();

        foreach ($this->cedulasCodeudores as $cedulas) {
            $codeudores = $this->api($cedulas);
            $codeudores = $this->concatenarNombreCompleto($codeudores);
            array_push($codeudor, $codeudores);
        }

        return $this->credito->codeudores = $codeudor;
    }

    public function validarCedula($cedula)
    {
        $cedula = (array) $cedula;
        $rules = ['cedula' => 'required|required'];
        $i = 0;
        $isValidCedula = true;
        $cedulaCounter = count($cedula);

        while ($i < $cedulaCounter && $isValidCedula) {
            $input = ['cedula' => $cedula[$i]];
            $validator = Validator::make($input, $rules);

            if ($validator->fails()) {
                $isValidCedula = false;
            }
            $i++;
        }

        return $isValidCedula;
    }

    public function requestApi($method, $cedula, $user, $ip)
    {
        return new Request(
            'GET',
            $method,
            [
                'query' => [
                    'cedula'  => $cedula,
                    'usuario' => $user,
                    'ip'      => $ip,
                ],
            ]
        );
    }

    /**
     * Método que realiza la consulta a Sivi
     * @return [type] [obj]
     */
    public function api($cedula)
    {
        $url = \Config::get('app.sivi_api');
        $metodo = \Config::get('app.sivi_api_method');

        try {
            $client = new Client([
                'base_uri' => $url,
                'timeout'  => 3.14,
            ]);

            $request = $this->requestApi($metodo, $cedula, $this->user, $this->ip);
            $response = $client->send($request);

            return $this->parseJson($response->getBody());
        } catch (\Exception $e) {
            new SystemExceptions($e);
        }
    }

    /**
     * método que realiza la conversion de json a una array de php
     * @return [type] [obj]
     */
    public function parseJson($response)
    {
        return json_decode($response, true);
    }

    /**
     * método que concatena nombre completo
     * @return [type] [obj]
     */
    public function concatenarNombreCompleto($data)
    {
        $persona = new Persona();
        $persona->cedula = $data['cedula'];
        $apellido = $this->obtenerApellidoMaternoPaterno($data);
        $persona->nombreCompleto = $data['primer_nombre'] . ' ' . $apellido;
        $persona->nombreCompleto = $this->limpiarNombreCompleto($persona->nombreCompleto);

        return $persona;
    }

    public function obtenerApellidoMaternoPaterno($apellidos)
    {
        $paterno = $apellidos['apellido_paterno'];
        $materno = $apellidos['apellido_materno'];

        return (!isset($paterno) || trim($paterno === '') && !isset($materno)) ? $materno : $paterno;
    }

    /**
     * método que limpia nombre completo del deudor o codeudor de caracteres especiales y espacios
     * @return [type] [obj]
     */
    public function limpiarNombreCompleto($input)
    {
        return preg_replace("/[\d,\W,\s]+/", " ", $input);
    }

    /**
     * método que retorna el obj de credito
     * @return [obj] [credito]
     */
    public function getCredito()
    {
        $consultaSivi = new ConsultaSivi();
        if ($this->credito) {
            dd($this->credito);
            return $this->credito;
        }
    }
}

I am using those in the routes:

    <?php

use App\Libs\ConsultaCreded;
use App\Libs\ConsultaSivi;

/*
|--------------------------------------------------------------------------
| Web Routes
|--------------------------------------------------------------------------
|
| Here is where you can register web routes for your application. These
| routes are loaded by the RouteServiceProvider within a group which
| contains the "web" middleware group. Now create something great!
|
*/

Auth::routes();

// Planilla
Route::resource('planilla', 'PlanillaController');

// Home
Route::get('/', 'HomeController@index')->name('home');

// Tests
Route::get('/creded', function () {
    $consultaCreded = new ConsultaCreded();
    $consultaCreded->setCedulaDeudor('5-78-68');
    $consultaCreded->consultar();
    $credito = $consultaCreded->getCredito();
    dd($credito);
});

Route::get('/sivi', function () {
    $consultaSivi = new ConsultaSivi();
    $consultaSivi->user = \Auth::user()->username;
    $consultaSivi->ip = \Request::ip();
    $consultaSivi->setCedulaDeudor('1-728-2208');
    $consultaSivi->setCedulasCodeudores(['3-728-2208','1-728-2208']);
    $consultaSivi->consultar();
    $credito = $consultaSivi->getCredito();
});

My interface is here:

    <?php

namespace App\Libs;

/**
 * [Interfaz que desarrolla un punto en comun para la comunicacion con las diferentes fuentes de informacion del sistema]
 */
interface ConsultaInterface
{
    /**
     * [consultar Metodo que hace la consulta a la fuente de informacion]
     */
    public function consultar();

    /**
     * [setCedulaDeudor description]
     * @param [type] $cedula [description]
     */
    public function setCedulaDeudor($cedula);

    /**
     * [setCedulasCodeudores description]
     * @param [type] $cedulas [description]
     */
    public function setCedulasCodeudores($cedulas);

    /**
     * [getCredito description]
     * @return [type] [description]
     */
    public function getCredito();
}

class consulta creded- This is from my other partner:

<?php

namespace App\Libs;

use Illuminate\Support\Facades\DB;
use App\Libs\Domain\Credito;
use App\Libs\Domain\Deudor;
use App\Libs\Domain\Codeudor;
use App\Libs\Exceptions\SystemExceptions;

/**
 * Clase para consultar a la DB CREDED que implementa la interface consulta
 */
class ConsultaCreded implements ConsultaInterface
{
    /**
     * Numero de cedula del deudor principal del credito
     * @var [string]
     */
    public $cedulaDeudor;

    /**
     * almacena datos de la consulta a la base de datos
     * @var [obj]
     */
    public $consulta;

    /**
     * Credito que esta siendo construido
     * @var [obj]
     */
    public $credito;

    /**
     * Numeros de cedulas de los codeudores del credito
     * @var [string]
     */
    public $cedulasCodeudores;

    /**
     * metodo que asigna la cedula del deudor recibida por parametro
     * @param [string] $cedulaDeudor [recibe la cedula por entrada en teclado]
     */
    public function setCedulaDeudor($cedulaDeudor)
    {
        if (!$cedulaDeudor) {
            new SystemExceptions(101);
        }
        $this->cedulaDeudor = $cedulaDeudor;
    }

    /**
     * Metodo que realiza la consulta a la Fuente de Dato de CREDED
     */
    public function consultar()
    {
        try {
            $this->consulta = DB::connection('creded')
                ->table(env('CREDED_DB_TABLE'))
                ->where('cedula', '=', $this->cedulaDeudor)
                ->first();
        } catch (\Exception $e) {
            new SystemExceptions(103);
        }

        return $this->consulta;
    }

    /**
     * metodo que retorna el obj de credito
     * @return [obj] [credito]
     */
    public function getCredito()
    {
        if (!$this->configurarCredito()) {
            new SystemExceptions(104);
        }

        return $this->credito;
    }

    /**
     * asigna los valores de la consulta en sus respectivas variables dentro de sus clases
     * @return [type] [description]
     */
    public function configurarCredito()
    {
        if ($this->consulta) {
            $credito = $this->generarCredito($this->consulta);
            $credito->deudor = $this->generarDeudor($this->consulta);
            $credito->codeudores = $this->generarCodeudor($this->consulta);

            return $this->credito = $credito;
        }
    }

    public function generarCredito($consulta)
    {
        $credito = new Credito();

        $credito->numeroContrato = $consulta->sol_contracnum;
        $credito->montoCredito = $consulta->cal_montot;
        $credito->pagoTrimestral = $this->calcularPagoTrimestral($consulta->cal_totmes);
        $credito->anioInicial = $consulta->cal_fecini;
        $credito->anioFinal = $consulta->cal_fecfin;

        return $credito;
    }

    public function calcularPagoTrimestral($pagoMensual)
    {
        return $pagoMensual * 3;
    }

    /**
     * metodo que retorna el obj del Deudor
     * @return [obj] [deudor]
     */
    public function generarDeudor($consulta)
    {
        $deudor = new Deudor();
        $deudor->cedula = $consulta->cedula;
        $deudor->nombreCompleto = $this->limpiarNombre($this->consulta->nombre);

        return $deudor;
    }

    /**
     * metodo que retorna el obj del Codeudor
     * @return [obj] [deudor]
     */
    public function generarCodeudor($consulta)
    {
        $codeudor = new Codeudor();
        $codeudor->cedula = $this->consulta->sol_cedu_repre;
        $codeudor->nombreCompleto = $this->limpiarNombre($this->consulta->nomrepre);

        return $codeudor;
    }

    /**
     * reemplaza los espacios en blanco dobles del nombre por uno solo espacio y luego la funcion Trim() los elimina
     * @param  [string] $nombre [recibe el nombre de la persona]
     * @return [string]         [retorna el nombre sin espacios dobles]
     */
    public function limpiarNombre($nombreCompleto)
    {
        return preg_replace('/[\d,\W,\s]+/', ' ', trim($nombreCompleto));
    }

    /**
     * asigna la/s cedula/s del /los deudor/res recibida por parametro
     * @param [string] $cedulasCodeudores [recibe cedulas por entrada en teclado]
     */
    public function setCedulasCodeudores($cedulasCodeudores)
    {
    }
}
\$\endgroup\$
  • \$\begingroup\$ I changed the title to a different one that describes what the code does per site goals: "State what your code does in your title, not your main concerns about it.". Feel free to give it a different title if there is something more appropriate. \$\endgroup\$ – Sᴀᴍ Onᴇᴌᴀ Nov 27 '17 at 19:47
  • \$\begingroup\$ global state for accessing user input, don't use global state for validation, don't use global state for database connection, don't use active record, don't declare class parameters as public .. etc Improvement \$\endgroup\$ – herrera2307 Dec 12 '17 at 18:57
  • \$\begingroup\$ ಠ_ಠ Is that supposed to a response to my comment about changing the title? Or is that directed at something/someone else?? \$\endgroup\$ – Sᴀᴍ Onᴇᴌᴀ Dec 12 '17 at 19:01
  • \$\begingroup\$ yes it's directed at something else it's own notes, but i think it's correct the title \$\endgroup\$ – herrera2307 Dec 12 '17 at 19:02
  • \$\begingroup\$ i just curious how to improve my beta code hehe \$\endgroup\$ – herrera2307 Dec 12 '17 at 19:03
2
\$\begingroup\$

Overview

I think the interface and implementing classes looks fine. If there were common methods that both classes implementing the interface would use, then an abstract class might be a good route to go. For more context of when to use which, refer to this SO answer.

Router

It would be wise to consider using named controller methods in the routes instead of the basic routing approach using a closure. The two last routes current have 5-7 lines each. When new routes are added that may have similar lengths, that can make the route file very large.

Method critiques

public function setCedulaDeudor($cedulaDeudor)
{
    $isValid = $this->validarCedula($cedulaDeudor);
    if ($isValid) {
        $this->cedulaDeudor = $cedulaDeudor;
    }
}

Some might argue that storing the method call in the variable $isValid just adds an excess variable that is only used once. Unless the method name is long, it could just be condensed into the conditional:

if ($this->validarCedula($cedulaDeudor)) {

But I can understand how some would argue it improves readability to be clear where the values are stored.

public function setCedulasCodeudores($cedulasCodeudores)
{
    $cedulas = [];
    if (is_array($cedulasCodeudores)) {
        $isValid = $this->validarCedula($cedulasCodeudores);
        if ($isValid) {
            $this->cedulasCodeudores = $cedulasCodeudores;
        }
    } else {
        new SystemExceptions(401);
    }
}

$cedulas doesn't appear to be used after it is assigned to the empty array. I would hope there is not a method called after that which references that variable globally...Presuming that is the case, that variable can be removed.

I would question whether parseJson() really needs to be defined, since it really is just a shortcut to calling json_decode(). If there were more lines to that method, it was called more than once or the interface required it be implemented (and a different implementing class did something different) then it would obviously be good to keep.

\$\endgroup\$

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.