3

Mi problema es que quiero eliminar todos objetos de mi vector que tengan el mismo modelo. El usuario selecciona de la lista uno de los objetos y cuando presiona eliminar modelo seleccionado se eliminaran todos los que tengan el mismo modelo. Solo me funciona el de eliminar un carro con el siguiente código:

    void eliminar(int pos)
        {
            arrCarros.erase(begin(arrCarros) + pos);
        }

Donde pos es el selectedindex de el listbox y me funciono. Para eliminar el mismo modelo aplique:

    void eliminarModelo(int pos)
    {
        char* x = new char[50];
        x = arrCarros[pos]->getModelo();
        for (int i = 0;i< arrCarros.size(); i++)
        {
            if (arrCarros[i]->getModelo() == x)
            {
                arrCarros.erase(begin(arrCarros) + i);
            }
        }

    }

Pero lo único que hace es eliminar el seleccionado y no los que tengan el mismo modelo.

CArrCarro.h

#ifndef __CARRCARROS_H__
#define __CARRCARROS_H__
#include <vector>
#include "CCarros.h"
using namespace std;
class CArrCarros
{
public:
    CArrCarros()
    {
    }

    ~CArrCarros()
    {
    }

    void agregar(CCarros* x)
    {
        arrCarros.push_back(x);
    }

    void eliminar(int pos)
    {
        arrCarros.erase(begin(arrCarros) + pos);
    }

    void eliminarModelo(int pos)
    {
        char* x = new char[50];
        x = arrCarros[pos]->getModelo();
        for (int i = 0;i< arrCarros.size(); i++)
        {
            if (arrCarros[i]->getModelo() == x)
            {
                arrCarros.erase(begin(arrCarros) + i);
            }
        }

    }

    vector<CCarros*> getCarros() { return arrCarros; }

private:
    vector<CCarros*> arrCarros;

};



#endif // !__CARRCARROS_H__

Este es mi formulario Formulario

1
  • Esta eliminarModelo() no se ve nada bien... Primero creas el puntero x para después asignarle lo que devuelva getModelo(). También deberías tener en cuenta que vector::erase reduce el size del vector.
    – user4717
    Commented el 5 jun. 2017 a las 5:20

3 respuestas 3

2

Fíjate bien en lo que haces:

void eliminarModelo( int pos ) {
  char* x = new char[50];
  x = arrCarros[pos]->getModelo( );

Creas un arreglo en memoria dinámica, pero le asignas el valor devuelto por getModelo( ).

Ya vamos mal. De momento, tienes pérdidas de memoria. Seguimos.

  for (int i = 0;i< arrCarros.size(); i++) {
    if (arrCarros[i]->getModelo() == x) {

Vamos empeorando. Comparamos x con el valor devuelto por getModelo( ). Eso fué precisamente lo que hicimos al obtener el primer valor de x. Y dicha variable es un puntero; es decir, esa comparación solo nos dará true para el índice pos.

Cualquier otra comparación, nos dará siempre false.

Una posible solución sería:

#include <string.h>

. . .

void eliminarModelo( int pos ) {
  char* x = new char[50];

  strcpy( x, arrCarros[pos]->getModelo( ) );

  for( int i = 0;i< arrCarros.size(); i++ )
    if( !strcmp( arrCarros[i]->getModelo( ), x ) )
      arrCarros.erase(begin(arrCarros) + i);

  delete[] x;
}

Esa solución depende de varias cosas. Principalmente, que tu función getModelo( ) devuelva un puntero a una cadena terminada en 0 y de tamaño <= 49 caracteres (necesitamos 1 para el 0 final).

7
  • Funciono, pero le falto el i--; de @eferion. Todavía estoy aprendiendo a programar y me espera un largo camino. Muchas gracias. Commented el 5 jun. 2017 a las 7:07
  • @JuanDiegoRomeroPeralta cierto, se me pasó por completo :-)
    – user23371
    Commented el 5 jun. 2017 a las 7:16
  • ¿<string.h>? O_o Commented el 6 jun. 2017 a las 7:15
  • @PaperBirdMaster si si, el header de C. Tal y como está el código, es lo suyo. Tendría que haber usado std::string ... pues si, pero es que me salió así :-)
    – user23371
    Commented el 6 jun. 2017 a las 7:27
  • En eliminarModelo ni siquiera hace falta copiar el retorno de arrCarros[pos]->getModelo(), dado que va a ser usado por strcmp y esta función no modifica los datos que se le facilitan, perfectamente x podría asignarse al retorno de arrCarros[pos]->getModelo() sin consecuencias negativas y nos ahorramos el new, el strcpy y el delete[]. Commented el 6 jun. 2017 a las 8:22
1

Aparte de lo comentado por @Dolmenes...

void eliminarModelo(int pos)
{
    char* x = new char[50];
    x = arrCarros[pos]->getModelo();
    for (int i = 0;i< arrCarros.size(); i++)
    {
        if (/* comparacion del modelo */)
        {
            arrCarros.erase(begin(arrCarros) + i);
        }
    }
}

Cada vez que se llama a arrCarros.erase se elimina un elemento del vector... y todos los siguientes se desplazan una posición... pero esta última circunstancia no la tienes en cuenta, es decir, si tienes el siguiente vector:

A A B

Sucedería lo siguiente:

  • Encuentras A y eliminas el elemento de la posición 0, queda: A B
  • Compruebas el elemento de la posicion 1, B, lo ignoras. El vector final será A B

Lo que tienes que hacer es evitar el incremento si sacas un elemento del vector:

for (int i = 0;i< arrCarros.size(); i++)
{
    if (/* comparacion del modelo */)
    {
        arrCarros.erase(begin(arrCarros) + i);
        i--;
    }
}
2
  • Bien visto. Se me pasó :-)
    – user23371
    Commented el 5 jun. 2017 a las 7:14
  • @Dolmenes en otras ocasiones sucede al revés :)
    – eferion
    Commented el 5 jun. 2017 a las 7:32
1

Puedes usar la cabecera de algoritmos (<algorithm>), en concreto la función para eliminar elementos de colecciones std::remove_if:

void eliminarModelo(int pos)
{
    const auto &x = arrCarros[pos]->getModelo();
    auto i = std::remove_if(arrCarros.begin(), arrCarros.end(),
    [&x](auto carro){ return std::strcmp(carro->getModelo(), x) == 0; });

    for (auto b = i, e = arrCarros.end(); b != e; ++b)
        delete *b;

    arrCarros.erase(i, arrCarros.end());
}

La función std::remove_if recibe dos parámetros de rango (inicio y final de la colección en que borrar) y un tercer parámetro que sería una función (llamada predicado) que debe devolver verdadero si el elemento ha de ser borrado.

La función std::remove_if en realidad no borra los elementos que cumplen con el predicado si no que los mueve al final de la colección y devuelve un iterador a lo que debería ser el nuevo final1 de la colección.

Dado que has usado memoria dinámica para alojar las instancias de CCarros, no se puede hacer el borrado directo sin provocar fugas de memoria, así que es necesario un bucle adicional para aplicar delete sobre los elementos antes de llamar a std::vector::erase.

Sin embargo, si cambias el tipo de arrCarros a std::vector<CCarros> la función eliminarModelo puede quedar como:

void eliminarModelo(int pos)
{
    const auto &x = arrCarros[pos].getModelo();
    auto i = std::remove_if(arrCarros.begin(), arrCarros.end(),
    [&x](auto carro){ return std::strcmp(carro.getModelo(), x) == 0; });

    arrCarros.erase(i, arrCarros.end());
}

Pero para que eso sea posible, la clase CCarros debe cumplir con el concepto Móvil-Asignable (MoveAssignable).

A tener en cuenta.

  1. Memoria: Dolmenes ya ha hablado de las fugas de memoria, pero yo insistiré aún más en ello. Para evitar las fugas de memoria debes usar punteros inteligentes o no usar punteros en absoluto, mi consejo es que no uses punteros a no ser que sea estrictamente necesario (y en tu caso no parece serlo), así te ahorras los new y delete además de no necesitar trabajo adicional en el destructor de CArrCarros (que ahora necesitas y no has puesto).
  2. Estilo: Los nombres usados para tus clases son confusos, no tengo nada en contra de la Notación Húngara para identificar clases, pero usar el plural para objetos que conceptualmente son únicos es confuso, cambia CCarros por CCarro.
  3. Seguridad: Devuelves una copia de tus datos internos en la función getCarros, esto no es malo per se aunque si ineficiente. Como devuelves una copia de un std::vector de punteros, estás dando acceso total a los elementos almacenados en CArrCarros rompiendo su encapsulación y permitiendo que procesos ajenos modifiquen (¡o borren!) el estado interno de la clase sin que esta tenga control sobre esos cambios. Este problema lo solucionas aplicando el consejo 1 y devolviendo el std::vector como constante, puedes evitar la copia si lo devuelves como referencia constante.

1En C++, los finales de las colecciones son un iterador al elemento posterior al último elemento de la colección.

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.