Mejorar codigo estructura y procesos.

[BKsiragon]

Hola saludos, escribo para que que mejoras u optimizaciones le pueden hacer a este programita para que tenga una mejor estructura y un mejor procesamiento de los datos.
aqui les dejo el codigo con el enunciado.

Código [Seleccionar] Expandir

/*
Desarrollar un programa que registre los datos
de un grupo de personas (cedula, nombre, apellido, teléfono).
El programa debe ser capaz de mostrar el registro completo de
        una persona de acuerdo a la cedula ingresada.

*/


#include <stdio.h>
#include <conio.h>
#include <iostream>
#include <string>
using namespace std;

int a=0;
class Persona {
   public:
   string nombre[50],apellido[50];
   int cedula[50],telefono[50];
   void registrar();
   void mostrar();
};

  void Persona::registrar() {
  cout<<"ingrese su nombre:"<<endl;
  cin>>nombre[a];
  cout<<"ingrese su apellido:"<<endl;
  cin>>apellido[a];
  cout<<"ingrese su cedula:"<<endl;
  cin>>cedula[a];
  cout<<"ingrese su telefono:"<<endl;
  cin>>telefono[a];
  cout<<"sus datos han sido registrados correctamente:"<<endl;
  system("pause");
  system("cls");
 
  a++;                   
}
void Persona::mostrar(){
      int ci=0,i=0,encontrado=0;
      cout<<"\ningrese su cedula para mostrar sus datos:"<<endl;
      cin>>ci;
      for (i=0;i<51;i++){
          if (ci==cedula[i]){
              cout<<"nombre: "<<nombre[i]<<endl;
              cout<<"apellido: "<<apellido[i]<<endl;
              cout<<"cedula: "<<cedula[i]<<endl;
              cout<<"telefono: "<<telefono[i]<<endl;
              system("pause");
              system("cls");
              encontrado=1;
              break;
              }
          else
              encontrado=0;
      }
      if (encontrado==0)
      {cout<<"No se encuentra nadie registrado con esta cedula"<<endl;}
      system("pause");
      system("cls");
         
     
}
main() {
   Persona p;
   int opc;   
    cout<<endl;
    cout<<"\n\nPor favor elija una opcion: \n1.-registrarse en el sistema \n2.-mostrar sus datos"<<endl;
    cin>>opc;
    while(opc){
       switch(opc){
          case 1:{
             p.registrar();
             break;}
          case 2:{
             p.mostrar();
             break;}
       }
    cout<<"\n\nPor favor elija una opcion: \n1.-registrarse en el sistema \n2.-mostrar sus datos"<<endl;
    cin>>opc;
    }
   cin.get();
   cin.get();
   return 0;
}

[0x98364]

El código es correcto, temas de optimización...para estos pequeños programas yo diría que no existe mas optimización 

[eferion]

Un código de este calibre suele ser dificilmente optimizable... no se por qué la gente tiende tanto a preocuparse por la optimización cuando está aprendiendo. Creo sinceramente que es mucho más importante centrarse en hacer las cosas bien que en optimizar código.

Una vez que ya has adquirido una base sólida puedes plantearte nuevos retos, como conseguir que el código fluya más rápido... pero es mejor acotar los frentes abiertos, sobretodo al principio... si no es fácil que los problemas te desborden.

Revisando un poco tu código yo te daría los siguientes consejos para mejorar su diseño. De ti depende aplicarlos o no.

Un saludo. ( va por adelantado para no guarrear lo que viene a continuación )

1. Separar conceptos

Quizás sería más conveniente tener una instancia de "Persona" por cada registro en vez de una sola instancia de "Persona" para gestionar los 50 registros.

Código (cpp) [Seleccionar] Expandir


class Persona
{
  public:
    string nombre;
    string apellido;
    int cedula;
    int telefono;
};

class ListaPersonas
{
 public:
   void registrar();
   void mostrar();

 private:

   std::vector<Persona> personas;
};

El uso de la clase es más natural e intuitivo, ya que cada instancia de "Persona" te proporciona la información de un único registro.

2. Separación de responsabilidades

En tu código, el método "registrar" se encarga de pedir al usuario los datos y de crear la entrada correspondiente... eso son dos responsabilidades en la misma función... malo.

Lo normal es que a registrar se le pase o bien una instancia de "Persona" para que la añada a la lista, o bien que reciba directamente los parámetros nombre, apellido, cedula y teléfono. Luego, obviamente, necesitas en otro sitio una función que se encargue de pedirle al usuario los datos.

Separar este código en dos capas te aporta independencia y aislamiento, y eso mejora mucho la usabilidad del código. Un ejemplo de ello es que podrías cambiar la consola por una ventana gráfica y la función "registra" no sufriría ningún cambio.

3. Si se usa POO, se usa con todas las consecuencias

La clase "Persona" proporciona acceso directo a todas sus variables miembros... malo.

Una de las principales características de la POO es que proporciona mecanismos para controlar el acceso a los miembros de una clase... aprovéchalo. Se que es algo más de código el tener que crear un getter y un setter para cada variable... pero es un hábito muy sano... si luego resulta que un setter hay que modificarlo para añadir ciertas comprobaciones, el código que acceda a ese setter no se va a enterar... mientras que si permites un acceso directo y luego tienes que cambiarlo por un setter te toca modificar todas las llamadas una a una.

Por definición, todas las variables miembro de una clase deberían ser privadas ( o protegidas, según el caso ), nunca públicas... eso rompe el principio de encapsulación.

4. Aprovecha los tipos de c++

C++ dispone del tipo bool para operaciones booleanas... usar un bool para evaluar un si/no es mucho más claro que tener usar un int a la usanza de c:

Código (cpp) [Seleccionar] Expandir


void Persona::mostrar()
{
 bool encontrado = false;
 int ci;

 cout<<"\ningrese su cedula para mostrar sus datos:"<<endl;
 cin>>ci;
 for (int i=0;i<51;i++)
 {
   if (ci==cedula[i])
   {
     cout<<"nombre: "<<nombre[i]<<endl;
     cout<<"apellido: "<<apellido[i]<<endl;
     cout<<"cedula: "<<cedula[i]<<endl;
     cout<<"telefono: "<<telefono[i]<<endl;
     encontrado = true;
     break;
   }
 }

 if ( !encontrado )
 {
   cout<<"No se encuentra nadie registrado con esta cedula"<<endl;
 }

 system("pause");
 system("cls");
}

5. No uses system

system no es portable, es lento y escapa al control de tu programa ( no puedes depurar lo que sucede dentro de esa llamada ). Acostúmbrate a usar otras alternativas, como crearte una función propia que haga exactamente lo mismo.

6. En C++ procura no usar includes de C

Como he comentado más arriba, si usas C++, procura usarlo con todas las consecuencias. Además dispone de una librería muy completa que te hace no depender de las librerías de C.

Entiendo que el código C es compatible con el código C++... pero usar C++ puro sin características de C acaba proporcionando un código más claro y te permitirá conocer C++ en mayor profundidad, lo que te permite sacarle más provecho... Además te ayuda a superar varios vicios adquiridos al aprender C ( yo he pasado por ello ).

[nolasco281]

Muy acertado lo que comenta eferion no te preocupes por optimizar el código si no porque tu código sea más robusto ese es el objetivo de C++.

[rir3760]

Un error se encuentra en el bucle para buscar la cédula:

Código (cpp) [Seleccionar] Expandir

string nombre[50],apellido[50];
int cedula[50],telefono[50];
void registrar();
void mostrar();

// ...

void Persona::mostrar(){

// ...

   for (i=0;i<51;i++){
      if (ci==cedula[i]){
Al declarar los arrays indicas que tienen 50 elementos y debes acceder a ellos mediante los indices 0 .. 49 pero en el bucle el ultimo elemento que se puede verificar es "cedula[50]", no existe tal elemento y por ello el programa puede reventar (comportamiento no definido).

Sin embargo no tiene caso corregirlo, solo tienes que seguir las recomendaciones de eferion y ese error desaparece por si solo.

Un saludo