Issues deleting a value from a mutlimap.

[voidflux]

I'm having an issue deleting from a map where the KEY is the Serial data member and i'm trying to delete where the KEY is somthing and the PackID is equal to a value.

here's me trying to do it:

When I run the following test:
public class MainTest 
{
	
	public static void main(String [] args)
	{
		MessageDB testDB = new MessageDB();
		Message msg = null;
		
		
		for(int i = 0; i < 2; i++)
		{
			msg = new Message();
			msg.Serial = "333";
			msg.Identifer = "UPDATE " + i;
			testDB.add(msg);
		}
		testDB.delete(msg, 1);
		testDB.print();
	}
 
}

It will print out both messages, without deleting
the message with Serial 333, and PacketID = 1.
Here's the whole class including the add function:

package tester;
 
 
 
import java.util.ArrayList;
import java.util.HashMap;
import java.util.Iterator;
import java.util.List;
import java.util.Map;
import java.util.Set;
import java.io.*;
 
//this class should store all the Messages or "Events" and
//you can access them based on their Serial key.
 
public class MessageDB {
	//database to hold the information
	//	holds the Alerts/messages
	Map<String, List><Message>> AlertMap;
 
	//Constructor
	MessageDB() {
		AlertMap = new HashMap<String, List><Message>>();
	}
 
	//print, outputs the contents of the hashMap
	public void print() {
		//want to print out the Key and all the Messages
		//associated with that key
		//print, outputs the contents of the hashMap
		if(AlertMap.isEmpty())
			System.out.println("Map Is Empty");
		
			for (String key : AlertMap.keySet())
				System.out.println("\n\nSerial (key): " + key
						+ "\nValues in Map: \n" + AlertMap.get(key));
	}
 
	
	
	void delete(Message msg)
	{
		
		for (String key : AlertMap.keySet())
		{
			if(msg.Serial.equals(key))
			{
				System.out.println("Successfully removed: " + key);
				AlertMap.remove(key);
			}
				
		}
	}
	
	
	void delete(Message msg, int packetID)
	{
		
		for (String key : AlertMap.keySet())
		{
			if(msg.Serial.equals(key) && msg.PacketID == packetID)
			{
				System.out.println("Successfully removed: " + key + "with PacketID: " + packetID);
				AlertMap.remove(key);
				break;
			}
			else
				System.out.println("Couldn't find!");	
		}
	}
 
	void add(Message msg) {
 
		//getting the position of the List by EntityID if avaiable
		List<Message> AlertList = AlertMap.get(msg.Serial);
 
		//checking to see if there is a unique Key already in the Map.
		if (AlertList == null) {
			//if there isnt a key in the map, add a new key, and a new List mapping 
			//to the key EntityID;
 
			AlertList = new ArrayList<Message>();
			AlertMap.put(msg.Serial, AlertList);
			AlertList.add(msg);
		} else {
			//adding message to List
			AlertList.add(msg);
		}
 
	}
	
	
 
}

Here's where I set the packetID, anytime a new
message is created it will automatically increment the number.

package server;

//this class will hold the Event/Message 
import java.util.List;
import java.util.HashMap;
import java.util.Map;
import java.util.ArrayList;
import java.util.Set;
import java.util.Iterator;

public class Message {

	String Identifer;

	String Serial;

	String Node;

	String NodeAlias;

	String Manager;

	String Agent;

	String AlertGroup;

	String AlertKey;

	String Severity;

	String Summary;

	String StateChange;

	String FirstOccurance;

	String LastOccurance;

	String InternalLast;

	String EventId;

	String LocalNodeAlias;

	//these are datamembers that will be used during 
	//the sending of the message to the reciever
	//PacketID should be incremented each time a new message is created 
	
	int SizeOfPacket;

	String PacketType;
	
	
	
	private int PacketID;
	private static int nextPacketID  = 1;
 
	private static synchronized int getNextId() 
	{
		return nextPacketID++;
	}
 
   

	

	Message() {
		Identifer = "";
		Serial = "";
		Node = "";
		NodeAlias = "";
		Manager = "";
		Agent = "";
		AlertGroup = "";
		AlertKey = "";
		Severity = "";
		Summary = "";
		StateChange = "";
		FirstOccurance = "";
		LastOccurance = "";
		InternalLast = "";
		EventId = "";
		LocalNodeAlias = "";
		SizeOfPacket = 0;
		PacketType = "";
		
		//increment the message count
		PacketID = getNextId();

	}

	void print() {
		System.out.println("\nIdentifer: " + this.Identifer + '\n' + "Serial: "
				+ this.Serial + '\n' + "Node: " + this.Node + '\n'
				+ "NodeAlias: " + this.NodeAlias + '\n' + "Manager: "
				+ this.Manager + '\n' + "Agent: " + this.Agent + '\n'
				+ "AlertGroup: " + this.AlertGroup + '\n' + "AlertKey: "
				+ this.AlertKey + '\n' + "Severity: " + this.Severity + '\n'
				+ "Summary: " + this.Summary + '\n' + "StateChange: "
				+ this.StateChange + '\n' + "FirstOccurance: "
				+ this.FirstOccurance + '\n' + "LastOccurance: "
				+ this.LastOccurance + '\n' + "InternalLast: "
				+ this.InternalLast + '\n' + "EventId: " + this.EventId + '\n'
				+ "LocalNodeAlias: " + this.LocalNodeAlias + '\n'
				+ "PacketType: " + this.PacketType + '\n' + "SizeOfPacket: "
				+ this.SizeOfPacket + '\n' + "PacketID: " + this.PacketID
				+ "\n");
	}
	
	void printV2()
	{
		System.out.println("Identifer: " + this.Identifer + "," + "Serial: "
				+ this.Serial + "," + "Node: " + this.Node + ","
				+ "NodeAlias: " + this.NodeAlias + "," + "Manager: "
				+ this.Manager + "," + "Agent: " + this.Agent + ","
				+ "AlertGroup: " + this.AlertGroup + "," + "AlertKey: "
				+ this.AlertKey + "," + "Severity: " + this.Severity + ","
				+ "Summary: " + this.Summary + "," + "StateChange: "
				+ this.StateChange + "," + "FirstOccurance: "
				+ this.FirstOccurance + "," + "LastOccurance: "
				+ this.LastOccurance + "," + "InternalLast: "
				+ this.InternalLast + "," + "EventId: " + this.EventId + ","
				+ "LocalNodeAlias: " + this.LocalNodeAlias + ","
				+ "PacketType: " + this.PacketType + "," + "SizeOfPacket: "
				+ this.SizeOfPacket + "," + "PacketID: " + this.PacketID
				+ "\n");
	}

	public String toString() {
		return ("PacketType: " + this.PacketType + " , "  + "SizeOfPacket: "
				+ this.SizeOfPacket + " , "  + "PacketID: " + this.PacketID
				+ ", " + "Identifer: " + this.Identifer + " , "  + "Serial: "
				+ this.Serial + " , " + "Node: " + this.Node + " , " 
				+ "NodeAlias: " + this.NodeAlias + " , "  + "Manager: "
				+ this.Manager + " , "  + "Agent: " + this.Agent + " , " 
				+ "AlertGroup: " + this.AlertGroup + " , "  + "AlertKey: "
				+ this.AlertKey + " , "  + "Severity: " + this.Severity + " , " 
				+ "Summary: " + this.Summary + "," + "StateChange: "
				+ this.StateChange + " , "  + "FirstOccurance: "
				+ this.FirstOccurance + " , "  + "LastOccurance: "
				+ this.LastOccurance + " , "  + "InternalLast: "
				+ this.InternalLast + " , " + "EventId: " + this.EventId + " , " 
				+ "LocalNodeAlias: " + this.LocalNodeAlias);
	}

}

Any help would be great!

[dlorde]

Have you tried stepping through the code by hand?

I ask because the delete method doesn't seem to do anything sensible at all.

AlertMap is map of lists keyed by unique strings. The messages are stored in the lists. To delete a message you need to find the list of messages in the map keyed by the message serial, and then delete the list entry that matches the message packetId.

When you call the delete method, you are passing it two arguments - the last message entered (which has a packet Id of 2), and a hard-coded packet Id of 1. Then inside the delete method, you compare the packet Id of the message you passed in (2) with the hard-coded packet Id you passed in (1). They will never match because they never change!

Again inside the delete method, you get the key set of the AlertMap and iterate over it, presumably to find the correct message list. But the map keys are unique, if you know the key of the item you want, you can just call Map.get(key). If you want to know if a key is in the map, you can call Map.containsKey(key). You don't have to iterate over a Map if you have a key.

Once the message list has been found in the map, you then have to delete the message from it. Your code tries to delete the whole message list from the map - if that was what you wanted, why not just pass the list key to the delete method? The method name "delete" doesn't give a clue, but since you pass a message to it, it is natural to assume you want to delete that message rather than the whole message list. The name 'deleteMessage' would be clearer, or 'deleteMessageList' if you really want to delete the whole list.

I think you need to clarify exactly what the delete method is going to do and how it is going to work. Unfortunately your post didn't actually say what you were trying to delete...

If you want it to delete the whole message list for a particular message, just pass it the message serial and use that to remove the list from the map.

If you want it to delete a particular message from its list, pass it the message and use the message serial to get the list. Then you can find the message with the correct packet Id in the list and remove it.

If you want to remove a message from its list given a serial and a packet Id (i.e. you don't have the message itself), pass the serial and packet Id to the delete method.

If you do want to delete just a single message, you might want to provide both the last two options above, by implementing one delete method that takes the serial and packet id and deletes the corresponding message, and another delete method that takes a message to be deleted, extracts the serial and packet Id and passes them to the first method. This gives you some flexibility without much extra code.

Computer Science is a science of abstraction -creating the right model for a problem and devising the appropriate mechanizable techniques to solve it...
A. Aho & J. Ullman

[voidflux]

Thanks for the help! With your explanation I figured it out!

My intial code was quite messed up I'm not sure what I was thinking. What I wanted it to do was once the database was already populated I just wanted to Delete the message that had the Serial given by user, and also the packet iD. Because its a multi map, the KEY maps several values (Lists), so the only thing that is now making the values unquie is their PacketID which is incremented each time a new message object is created.

Heres my code

boolean delete(String Serial, int PacketID)
	{
		if(AlertMap.isEmpty())
			System.out.println("\nDatabase is empty!");
		
	
		List<Message> AlertList = AlertMap.get(Serial);
		System.out.println("Getting the Messages with Serial: " + Serial);
	       
		Iterator it = AlertList.iterator();
		
		while(it.hasNext())
		{
		//	System.out.println("Element: " + it.next());
			Object o = it.next();
			Message m = (Message)o;
			if(m.PacketID == PacketID)
			{
				it.remove();
				System.out.println("Removed Messge with Packet ID: " + PacketID);
				return true;
			}
		}
		System.out.println("Couldn't find the message with Packet ID: " + PacketID);
		return false;
	     
	}


import java.util.Scanner;

public class MessageParser 
{
	public static boolean isValid(Scanner scanner) 
	{
		int i = 0;
		scanner.useDelimiter(",");

		
		while (scanner.hasNext()) 
		{
			if(scanner.next().equals("END"));
			{
				return true;
			}
			
		}
		scanner.close();
		return false;
		

	}



}

[dlorde]

Yup, that looks OK.

BTW, you don't have to cast the item you get out of the iterator if you make the iterator generic, like the list:

Iterator<Message> it = AlertList.iterator();

Remember that Iterator.remove() is an 'optional' method and may not always be available - iterators in general are not expected to support anything more than next() and hasNext().

If you may have a lot of messages per serial, you might want to use a map for the messages (keyed by packetId) instead of a List.

Try to use the Java Naming Convention (uppercase letter to start a class name, lowercase letter to start variable and method names). Everyone expects this convention to be used, and it saves a lot of confusion.

Don't worry about people stealing your ideas. If your ideas are any good, you'll have to ram them down people's throats...
H. Aiken