HashSet and LinkedList

[Tomy Alexsander]

Write a method in CatHome called getVaccinated. This method finds all Cat
objects in the CatHome object’s LinkedList whose vaccinated field has the value
true. It stores all matching Cat objects in a new HashSet that is then returned from the
method. The HashSet must only contain those Cat objects that were present in the list
on that specific call of the method. You must ensure that it is clear to any code calling this
method that the HashSet contains zero or more Cat objects.
The method must also remove all the matching Cat objects from the LinkedList of
CatHome. If there are no matching cats then an empty HashSet must be returned.

So i did something like this but i do not know if I am moving in the correct way or not.
Somebody pleas help me?

  public HashSet getVaccinated()
    {
        int counter =0;
        map = new HashSet<>();
        for(Cat cat : cats){
            if(cat.getVaccinated() == true){
                counter++;
                return map.add(counter);
            }
        }

    } 

[Dave Tolls]

You're not far off.
My reading of the requirements says that the HashSet should be declared in the method (ie it's a local variable), so line 4 needs a little bit more to it (it also probably ought to be called vaccinatedCats, rather than just map).
Also, about the Set:
"You must ensure that it is clear to any code calling this
method that the HashSet contains zero or more Cat objects."
Your returned value, according to the method signature, does not do that.

In line 6, since getVaccinated is already a boolean then you don't need the '== true'.

Finally...does it compile?
(After the changes).

[Tomy Alexsander]

So i got this in my code.
But where i am returning the counter i got error message "it can not be converted to the String"

private HashSet<String> vaccinatedCats;

public HashSet getVaccinated()
    {
        int counter = 0;
        vaccinatedCats = new HashSet<>();
        for(Cat cat : cats){
            if(cat.getVaccinated()){
              counter++;
              return vaccinatedCats.add(counter);
            }
        }
        return null;
    }

[Tomy Alexsander]

Tomy Alexsander wrote:So i got this in my code.
But where i am returning the counter i got the error message "it can not be converted to the String"

because i am stuck on this part and i do not know where to move.

private HashSet<String> vaccinatedCats;

public HashSet getVaccinated()
    {
        int counter = 0;
        vaccinatedCats = new HashSet<>();
        for(Cat cat : cats){
            if(cat.getVaccinated()){
              counter++;
              return vaccinatedCats.add(counter);
            }
        }
        return null;
    }

[Campbell Ritchie]

Tomy Alexsander wrote:. . . ensure that it is clear to any code calling this method that the HashSet contains zero or more Cat objects. . . .

Not sure I understand that bit. Does that mean to write documentation comments or to make the return value a Set<Cat>. The latter sounds more like it. I wouldn't have made the method return HashSet but Set<Cat>? That way the compiler knows you have a Set of Cats. But I am not sure why you are exchanging a List for a Set.
The method name reads strangely; if somebody says, “getVaccinated(),” to me, I roll my sleeve up. I would query that with my teacher, but if you have been told to use that name, you will have to use it.

    public HashSet getVaccinated()
    {
        int counter =0;
        map = new HashSet<>();
        for(Cat cat : cats){
            if(cat.getVaccinated() == true){
                counter++;
                return map.add(counter);
            }
        }

    } 

Thank you for finding the code button and indenting the code well; unfortunately the tags lost the spaces before the method header. I added a hard space to my copy and it improved the formatting.
Don't use the field map, but declare a new local variable. Don't call a Set “map”. Make the name reflect the purpose. Try vaccinatedCatsSet or similar instead.
Line 6. Never write == true nor == false which are both poor style and error‑prone if you mistakenly change the == to =.
Never if (b == true) ... please.
Always if (b) ... please.
Never if (b == false) ... please.
Always if (!b) ... please.

Why have you get a method called getVaccinated() in the Cat object, too. The convention is for methods returning a boolean to be called isXXX() or hasXXX() or canXXX() (isXXX() is the commonest name). It is confusing to have two methods with the same name. Are you allowed to change the Cat method to isVaccinated() or hasBeenVaccinated() instead?
Why are you counting anything? Did it say anything about counting in the instructions?
Don't put return in your loop. That will terminate the method.
Read the documentation for the Set#add() method and see what its return type is. If you try returning that from this method, what is going to happen? You want to add that element to the Set and return the Set at the end of the method.
And where are you removing anything from the List? A couple of things there:-

1: I worry about that side‑effect. You are not simply returning vaccinated cats, but you are deleting them from somewhere else. It is iffy design to have a method do two things. If that is what you have been told, please query it but you will again have to do what you were told.

2: It is only efficient to remove something from a linked list (you haven't explained why you need a linked list) if you have its location, and the way to get its location is to use an explicit Iterator.

Anyway, cats are irredeemably and incorrigibly evil. And...welcome to the Ranch

[Campbell Ritchie]

Dave and I seem to agree: don't declare the vaccinated Set as a field.

[Tomy Alexsander]

Ok. I got another class  Cat. Which i connected to the

 private HashSet<Cat> vaccinatedCats;  cats = new LinkedList<Cat>(); 

where i am storing cat name cat breed and boolean vaccinated. i am not allowed to use a different method name.  Yes, I need to delete them from the LinkedList and then put that object to the HashSet and return it. but i do not know how to do it.

[Campbell Ritchie]

Move the declaration of the vaccinated set into the method, so it will be a local variable (and delete private).
Go through this Java™ Tutorials section and see whether it says anything about Iterators. You can use an Iterator to remove an element from a “source” Collection List and add the same element to a “destination” Collection Set. Alternatively, your book may have something about iterating Collections Lists with an Iterator.

[Campbell Ritchie]

Oh.....and give your List private access.

[Tomy Alexsander]

Am I in the correct way or no?

 public HashSet<Cat> getVaccinated()
    {
        HashSet<Cat> vaccsinated = new HashSet<Cat>();
        for(int counter=0; counter < cats.size(); counter++)
        {
            if(cats.get(counter).getVaccinated())
            {
                vaccsinated.add(cats.get(counter));
                cats.remove(counter);
                counter--;
            }
        }
        return vaccsinated;
    }

[Carey Brown]

 public HashSet<Cat> getVaccinated()

Should be

 public Set<Cat> getVaccinated()

HashSet<Cat> vaccsinated = new HashSet<Cat>();

Should be

Set<Cat> vaccinated = new HashSet<>();

[Campbell Ritchie]

Tomy Alexsander wrote:Am I in the correct way or no? . . .

No. Afraid not. I told you earlier it's best to use an Iterator on linked lists. Didn't you find a link in the tutorials showing you an example with an Iterator? I found it easily enough. You are risking going into quadratic time complexity (i.e. slow execution) on a linked list. (You won't get that problem with an array list.)
Another problem you may get with any sort of list is that if you have two consecutive vaccinated cats, you remove the first and the second moves to the recently vacated index and is skipped in the next iteration of the loop. So you don't correctly remove all cats. Remember the ideal loop removes all cats from the whole world. Some people may get the impression I don't like cats. An Iterator wouldn't have that problem.

[Addition]I see you have addressed my second problem with counter--; which I missed at first (sorrry ). That will produce the requisite removals, but I look on that as an awkward correction when an Iterator woudn't require it.

[Piet Souris]

Actually, you do not need an iterator per se, although that makes things easy.
Another way is to put the vaccinated dogs in a Set without removing, and then have a look at the lists method 'removeAll'.

[Campbell Ritchie]

If you are going to do it that way, I would use a Stream<Cat>. An Iterator allows you to remove Cats from the list and add them to the set simultaneously, whereas a Stream shouldn't suffer removal from its source.

[Tomy Alexsander]

So in this way, it will be better? but when I debug my first version was working.

 public HashSet<Cat> getVaccinated1(){

        HashSet<Cat> toBeVaccinated = new HashSet<>();
        Iterator<Cat> it = cats.iterator();
        while (it.hasNext()){
            Cat theCat = it.next();
            if(theCat.getVaccinated()){
                toBeVaccinated.add(theCat);
                it.remove();
            }

        }
        return toBeVaccinated;
    }

[Campbell Ritchie]

Tomy Alexsander wrote:So in this way, it will be better?

A lot better, yes.
But it would still benefit from a few improvements: I am not convinced you have got the right name for the set. I think it should be something like vaccinatedCats or vaccinatedCatSet, both of which names have (I think) appeared earlier. Also declare the set (line 3) and the return type as Set<Cat> rather than a specific type of Set, as people have mentioned earlier, too.
I presume you will be removing the 1 from the method name.

but when I debug my first version was working. . . .

Yes, as I said, it would produce the right result, but the correction to the index was awkward code. Look how much neater and simpler your code is now Well done.
Consider a for loop instead, as in the Java™ Tutorials link I was hinting about. That is something you won't see in the books, but it has the advantage that the scope of the Iterator variable is restricted to the loop. You can have problems if you let the Iterator remain in scope after the end of the loop and try to manipulate the Collection.

/**
 * Creates a Set of Cats already vaccinated, and removes them from
 * the collection of cats in this object. All Cats that have already been
 * vaccinated are collected into a Set which this method returns, and it
 * has the side-effect of removing those Cat objects from the collection
 * maintained by this object.
 *@return A Set of Cat objects, already vaccinated.
 */
public Set<Cat> getVaccinated() {
    Set<Cat> vaccinatedCts = new HashSet<>();
    for(Iterator<Cat> it = cats.iterator(); it.hasNext(); /* empty */) {
        Cat theCat = it.next();
        if(theCat.getVaccinated()){
            VaccinatedCats.add(theCat);
            it.remove();
        }
    }
    return vaccinatedCats;
}

The update part of the for loop header can be “/* empty */” because the loop variant is changed by the next() call in line 12. Put one space before any {s. Note the importance of the documentation comment.

[salvin francis]

I like how Campbell has used the for loop without an increment expression !!
I just wanted to point out that I would never expect a getter method to modify data in the background. A code like this in professional environment would raise a lot of eye brows for sure.

[Campbell Ritchie]

salvin francis wrote:I like how Campbell has used the for loop without an increment expression !!

Thank you But I thought that is how you always write a for loop including an Iterator.

. . . I would never expect a getter method to modify data in the background. . . .

I have already called it iffy design. There are many things about that assignment that I would call iffy.

[Campbell Ritchie]

A few hours ago, I wrote:. . . I would use a Stream<Cat>. . . .

And that would look like this:-

/**
 * Creates a Set of Cats already vaccinated. All Cats that have already
 * been vaccinated are collected into a Set which this method returns.
 *@return A Set of Cat objects, already vaccinated.
 */
public Set<Cat> getVaccinated() {
    return cats.stream()
               .filter(Cat::getVaccinated)
               .collect(Collectors.toSet());
}

Note that I have removed the side‑effect, and also that I am too tired to explain that code any further. Sorry.

[Mike Simmons]

If we accept the side effect as a requirement that we should actually implement, then a nice alternative is

    public Set<Cat> removeVaccinated() {
        var map = cats.stream().collect(Collectors.partitioningBy(Cat::getVaccinated, Collectors.toSet()));
        cats = map.get(false);
        return map.get(true);
    }

Where I've changed getVaccinated to removeVaccinated because I, too, despise side effects on a get method.  But if your instructor requires it, go ahead and call it getVaccinated().  We just won't like it. ;)

[salvin francis]

isVaccinated sounds better

[Mike Simmons]

Agreed.  For the no-side-effect version, at least.  There's also some confusion here because outside of JavaBean naming conventions, to "get vaccinated" actually means something that does have a side effect.  But that can't be what the assignment author meant, since the method never changes the vaccination status of any of the cats.  Anyway, our hopes for having logical method names in this assignment are probably futile - the original poster is probably stuck with the assignment as is.

[Campbell Ritchie]

Mike Simmons wrote:. . . I, too, despise side effects on a get method. . . .

Nice solution, but you have added another side effect. You have changed the List (field) to a Set. The requirement to take a List and return a Set is another iffy feature of this assignment.

Another over‑specified assignment, and as so often happens, the instructor denies the students the opportunity to learn design by providing a suboptimal design. But you have to follow the assignment's instructions to get full makrs.

[Campbell Ritchie]

Let's try and explain last night's offering. A Stream is an object usually hidden in the background. It takes all the elements in the source (it needs a source, except for a few methods (range(), rangeClosed(), iterate(), and generate()), which provide their own sources. That source provides all the elements the Stream requires; in this case you create the Stream with the stream() method.
Then I used filter(), which filters IN, (=retains) only those elements for which the test evaluates to true. In this case I can simply instruct it to use a method of the Cat object. More information in the Java™ Tutorials. That method call creates a second Stream object, manipulating that “filtered” subpopulation of elements.
The collect() call takes a Collector reference, which you can write yourself in three parts, like All Gaul. But if you go through the Collector documentation and find the link to Collectors, which has a method supplying a ready‑made Collector doing just what you want.

Only it creates a Set (generic=interface) rather than specifically a hash set (specific=implementation). That is why we said to declare everything as List not linked list and Set not hash set.

[Mike Simmons]

Campbell Ritchie wrote:

Mike Simmons wrote:. . . I, too, despise side effects on a get method. . . .

Nice solution, but you have added another side effect. You have changed the List (field) to a Set. The requirement to take a List and return a Set is another iffy feature of this assignment.

Ah, I overlooked that.  But it's an easy fix:

    public Set<Cat> removeVaccinated() {
        var map = cats.stream().collect(Collectors.partitioningBy(Cat::getVaccinated));
        cats = map.get(false);
        return new HashSet<>(map.get(true));
    }

or

    public HashSet<Cat> removeVaccinated() {
        var map = cats.stream().collect(Collectors.partitioningBy(Cat::getVaccinated));
        cats = new LinkedList<>(map.get(false));
        return new HashSet<>(map.get(true));
    }

(If it really needs to be a LinkedList and a HashSet...)

I agree on all your other points about the assignment.

[Campbell Ritchie]

Mike Simmons wrote:. . . But it's an easy fix: . . .

It so often is easy, isn't it.

[Piet Souris]

Campbell Ritchie wrote:
Only it creates a Set (generic=interface) rather than specifically a hash set (specific=implementation).

Becauseof the use of Collectors.toSet().
Using toCollection() delivers you any Collection you want. And whether you give the result an interface type or a specific type, doesn't matter now we have the 'var' keyword.