what's wrong with my program?

[stonedviper]

Hi and Merry Christmas!

Here's the problem I wanted to make a program to output how many times there is a repeating string and at which positions of a list(it's the printer function).
and it worked.
but here's the problem my vector thinks it has 1 string when it should have all the repeating strings.
and no matter what method i try to get the strings I end up with 1 unless I hard code it to the number of strings that should be in there and it works as intended.
Why does it happen?

I haven't used the printer function because I wanted to find out where the problem is, but couldn't and that's why I'm asking here.

Thank you!

#include <iostream>
#include <list>
#include <vector>

void printer(std::string stringer,std::list<std::string> listofstring)
{
    std::vector<int> kyle;
    int repeat=0,position=0;

    std::list<std::string>::iterator it;
    for(it=listofstring.begin();it!=listofstring.end();it++)
    {
        if(stringer==*it)
        {
            repeat++;
            kyle.push_back(position+1);
        }
        position++;
    }

    std::cout<<"string "<<stringer<<" repeats "<<repeat<<" at ";
    for(int i=0; i<repeat;i++)
    {
        std::cout<<kyle[i]<<"\n";
    }
}

int main()
{
    std::string name;
    std::list<std::string> listofnames;
    std::list<std::string>::iterator it;
    std::vector<std::string> vectorofnames;
    std::vector<int> possave;
    bool a=NULL,b=NULL,c=NULL;
    int choice=0,position=0,repeat=0,luk=0;

    std::cout<<"Enter the number of strings you want to enter in the list! ";
    std::cin>>choice;

    for(int i=0;i<choice;i++)
    {
        a=false;
        std::cout<<"Enter string "<<i+1<<" ";
        std::cin>>name;
        for(it=listofnames.begin();it!=listofnames.end();it++)
        {
            if(name==*it)
            {
                vectorofnames.push_back(name);
                possave.push_back(position);
                a=true;
            }
        }
        //if(!a)
            listofnames.push_back(name);

        position++;
    }

    luk=vectorofnames.size();

    do
    {
        b=false;
        c=false;
        for(int i=0;i<vectorofnames.size();i++)
        {
            name=vectorofnames[i];
            for(int j=0;j<vectorofnames.size();j++)
            {
                if(name==vectorofnames[j])
                {
                    if(b)
                    {
                        vectorofnames.erase(vectorofnames.begin()+j);
                        i=luk;
                        j=luk;
                        c=true;
                    }
                    else b=true;
                }
            }
        }
    }
    while(c);


    while(!vectorofnames.empty())
    {
        std::cout<<vectorofnames.back()<<" ";
        vectorofnames.pop_back();
    }

}

[dhayden]

The problem must be elsewhere in your code. printer() works fine for me:

#include <iostream>
#include <list>
#include <vector>

void printer(std::string stringer,std::list<std::string> listofstring)
{
    std::vector<int> kyle;
    int repeat=0,position=0;

    std::list<std::string>::iterator it;
    for(it=listofstring.begin();it!=listofstring.end();it++)
    {
        if(stringer==*it)
        {
            repeat++;
            kyle.push_back(position+1);
        }
        position++;
    }

    std::cout<<"string "<<stringer<<" repeats "<<repeat<<" at ";
    for(int i=0; i<repeat;i++)
    {
        std::cout<<kyle[i]<<"\n";
    }
}

int main()
{
    std::list<std::string> listofnames;

    listofnames.push_back("apple");
    listofnames.push_back("pear");
    listofnames.push_back("apple");
    listofnames.push_back("orange");
    printer("apple", listofnames);
    printer("orange", listofnames);
    printer("kiwi", listofnames);
    return 0;
}

string apple repeats 2 at 1
3
string orange repeats 1 at 4
string kiwi repeats 0 at

Looking at the rest of your code, line 73 is probably wrong because it will always be tree when i==j and i!=0. If you're trying to remove duplicates then change line 73 to
if (i!= j && name == vectorofnames[j]) Then you don't need b at all.

[stonedviper]

It works!
Thank you!

but can you explain in depth because I don't understand your answer and what is the key difference?

Thank you though!

Edit:
while it does work without b it prints everything backwards if I want to print in order I have to use b can you explain please?

[stonedviper]

I figured it out and the answer was so obvious for my code and for yours.

with yours you check the next element and whit mine the bool of b is wrongly placed
thank you again

and for the suggestion I will use it in future

[TheIdeasMan]

Hi,

Could I add some other suggestions?

The parameters of the functions which are STL containers should be passed by reference.

If you are going to iterate an entire container, consider using a range based for loop instead of iterators:

void printer(const std::string& stringer, 
              const std::list<std::string>& listofnames)
{
    std::vector<std::size_t > kyle; // I prefer size_t here rather than int
    std::size_t repeat = 0;
    std::size_t position = 0;

    
   for(auto&& item  : listofnames) // uses forward reference, the best practise.
    {
        if(stringer == item)
        {
            repeat++;
            kyle.push_back(position+1);
        }
        position++;
    }

    std::cout << "string " <<stringer << " repeats " << repeat << " at ";
     // repeat is equal to size of kyle, so can use range based for as well
    for(auto&& val  : kyle)
    {
        std::cout << val << "\n";
    }
}

I also put some spaces in the code to relieve the density.

I am not so sure about setting bool values to NULL, why not false?

It is also better to declare 1 variable per LOC.

Does the container have to be a list? I don't see anything to preclude the use of a std::vector.

Pedantically, if using a do loop, always put the while part on the same line as the closing brace.

Hope this helps a little :+)

[stonedviper]

While your code looks more beautiful than mine.

some of the things I can't understand them like the auto&&, the for loop you have set up and the size_t.

Can you explain please explain it or point me to a documentation?

Also my task is to use a list and output the repeating strings and how many times they repeat and at which position.

Thank you!

[TheIdeasMan]

Hi,

The range based for loop:

http://en.cppreference.com/w/cpp/language/range-for

There are other ways of doing this, but not as safe:

for (const auto& item : container) // const lvalue reference with auto type deduction

for (const int item : container) // const with explicit type, pass by value

Note that one can't have :

[code]for (const auto&& item : container)

To have const with a forward reference, it seems to me that one must put the for loop in a function, and have the container variable as a const parameter.


Forward reference

http://en.cppreference.com/w/cpp/language/reference

Forward references enable perfect forwarding, they can handle const, non-const, lvalues, rvalues, so thus makes them preferable to the examples above.

https://eli.thegreenplace.net/2014/perfect-forwarding-and-universal-references-in-c/

std::size_t is usually the largest unsigned type the system has (typically unsigned 64 bit), and is the return type for size() functions in STL containers. One should use this type to prevent warnings about type mismatch, and avoid implicit casts from std::size_t to int say. I tend to use it anywhere an unsigned type is appropriate, I don't often have to worry about the 64 bit size versus a 32 bit int.

Good Luck !!

[stonedviper]

That was more than I asked...still thank you!