Tu programa tiene unos cuantos errores. Veamos:
Usas variables sin inicializar
int can;
valores=new int[can];
Si tu intención es que la capacidad de valores
se adapte al valor de can
... no lo estás haciendo bien. La instrucción new
se ejecuta en el mismo momento en el que la invocas, y ahí coge el valor de can
(variable no inicializada) y usará ese valor para calcular la capacidad de la reserva de memoria. Una vez hecha la reserva, la misma será autónoma y no va a cambiar por mucho que modifiques can
.
El puntero valores
debes inicializarlo después de conocer el valor de can
:
cout<<"¿Cuantos clientes ingresara a la plataforma?"<<endl;
cin>>can;
int * valores = new int[can];
Separar responsabilidades
Una cosa es una lista y otra los nodos de la lista. Por tu propia salud, no deberías exponer la lógica de la lista a todo el programa. Entiendo que estás empezando a programar en C++ y aun no has visto lo que sería la creación de objetos, pero eso no es impedimento para intentar hacer las cosas bien.
La estructura nodo
puedes dejarla como está:
struct nodo{
int nuc;
string nom,ape;
nodo *siguiente = nullptr;
};
Sin embargo, todo quedaría más bonito y legible si creases una estructura para gestionar la lista:
struct lista {
nodo * root = nullptr;
};
Parece un cambio absurdo, pero cuando empieces a programar objetos, este diseño te permitirá aislar completamente el diseño de la lista. Además, al poseer C++ un tipado fuerte, te permitirá distinguir las funciones que manipulan la lista de las funciones que manipulan nodos ... no podrás mezclar las funciones por error puesto que el tipo nodo
no puede ser usado como si fuese el tipo lista
y viceversa.
A modo de ejemplo, la función insertar
quedaría ahora así:
void insertar(lista & lista, int nu, string no, string ap)
{
nodo * nvo_nodo = new nodo;
nvo_nodo -> nuc=nu;
nvo_nodo -> nom=no;
nvo_nodo -> ape=ap;
if (lista.root == nullptr)
{
lista.root = nvo_nodo;
}
else
{
nodo * aux = lista.root;
while (aux->siguiente != nullptr)
{
aux = aux->siguiente;
}
aux->siguiente = nvo_nodo;
}
cout << "\t Cliente " << nu << " insertado a la lista correctamente\n";
}
Cálculos incorrectos
int longitud=sizeof(valores)/sizeof(*valores);
Este mecanismo te permite calcular el tamaño de un array. El problema es que valores
no es un array sino un puntero.
sizeof
es una función que se evalúa en tiempo de compilación, es decir, mientras se está creando el código binario. En ese momento el compilador no puede saber qué valor va a introducir el usuario (tiempo de ejecución).
Lo que sí se conoce en tiempo de compilación es el número de bytes que ocupa un puntero. Así, en arquitecturas de 64 bits, los punteros ocupan típicamente 8 bytes. Pues bien, dado que sizeof(*valores)
devuelve el tamaño de un int
, longitud
acabará valiendo 2
:
int longitud = 8 / 4 = 2;
Y esto será así siempre, independientemente del número que introduzcas al ejecutar el programa.
Esto es un problema, ya que utilizas este valor para ordenar los resultados.
Si te fijas, verás que ya tienes el número de elementos, ese valor se encuentra en la variable can
:
int longitud = can;
Nombres de variables
No debes racanear a la hora de darle nombre a las variables. Tienes que dar a las variables y a las funciones nombres concisos que indiquen claramente cual es su responsabilidad.
Si no sigues esta regla, acabarás escribiendo código ofuscado, dificil de leer y de seguir. Mientras recuerdes exactamente cómo está estructurado tu programa no será un problema, pero después será algo que te va a dar dolores de cabeza.
Nombres como can
, nuc
, nom
no son nada recomendables. En el código binario resultante no hay nombres de variables, por lo que da igual cuan largo sea el nombre que le des a una variable ... no va a tener ningún impacto en la forma en la que se ejecute tu programa y, sin embargo, si que puede facilitarte o complicarte la vida a la hora de modificar tu código.
Acceso incorrecto a índices
if(nuc[i>nuc[j]])
Este condicional no es correcto y el motivo es que has metido una comparación en el cálculo del índice. Un código equivalente sería el siguiente:
int index = i>nuc[j] ? 1 : 0
bool value = nuc[index] != 0;
if (value)
Lo que pretendías hacer yo creo que se parece más a esto:
if(nuc[i]>nuc[j])
No liberas la memoria dinámica
Como norma general, por cada llamada que hagas a new
, debe haber otra llamada a delete
en tu código. En tu caso esta simetría no se cumple. No liberas absolutamente ninguno de los nodos de la lista.
En tu código no pasa nada porque tu programa es muy sencillo y esa memoria se liberará automáticamente al finalizar el programa, pero si modificas tu programa para que, por ejemplo, se puedan sacar elementos de la lista, entonces ya si puedes empezar a tener un problema. La memoria asociada a los nodos que saques de la lista no se liberará y por ello la memoria que necesita tu programa para funcionar se irá incrementando paulatinamente. Con el número adecuado de iteraciones, tu programa podría llegar a consumir toda la memoria del equipo.
Aunque parezca algo innecesario, libera la memoria de la lista antes de finalizar el programa. Es mejor acostumbrarse a hacerlo siempre a arriesgarse a que tu algoritmo tenga lagunas de memoria.
Tu programa pierde memoria
void mostrar(nodo *lista)
{
nodo *actual= new nodo();
actual = lista;
Estás inicializando el puntero actual
para que direccione a un objeto de tipo nodo
que acabas de crear para, a continuación, modificar ese puntero, haciendo que apunte a lista
. La memoria reservada en la primera línea se ha perdido y ya no serás capaz de liberarla (no sabes su dirección de memoria)
Aquí vemos un caso en el que separar los conceptos de pila y lista puede ayudarte a programar mejor.
Para empezar, no se espera que esta función modifique la lista, luego se le puede pasar el parámetro como constante para asegurarnos:
void mostrar(lista const& lista)
{
nodo * aux = lista.root;
while(aux !=NULL){
cout << aux->nuc << '\n'
<< aux->nom << '\n'
<< aux->ape << '\n';
aux = aux->siguiente;
cout<<"_________________________________________\n";
}
}
Si te fijas, tiendo a eliminar los usos de std::endl
. Esto lo suelo hacer porque endl
fuerza a que se vacie el buffer de cout
. Esta operación es bastante lenta, por lo que usar endl
alegremente puede llegar a impactar negativamente en los tiempos de ejecución de tu programa.