Rewrite a Java method

Here's a method that's part of an auction class that I want to rewrite so that it doesn't rely on a lot with a particular number being stored at index (number -1) in the collection.  Any suggestions?  Thanks, SP


public Lot getLot(int lotNumber)
   {
        if((lotNumber >= 1) && (lotNumber < nextLotNumber)) {
            // The number seems to be reasonable.
            Lot selectedLot = lots.get(lotNumber - 1);
            // Include a confidence check to be sure we have the
            // right lot.
            if(selectedLot.getNumber() != lotNumber) {
                System.out.println("Internal error: Lot number " +
                                   selectedLot.getNumber() +
                                   " was returned instead of " +
                                   lotNumber);
                // Don't return an invalid lot.
                selectedLot = null;
            }
            return selectedLot;
        }
        else {
            System.out.println("Lot number: " + lotNumber +
                               " does not exist.");
            return null;
        }
    }
}

Open in new window

spower22Asked:
Who is Participating?
 
CEHJCommented:
i.e.
public Lot getLot(int lotNumber)
{
	boolean found = false;
	Lot result = null;	
	Iterator<Lot> i = lots.iterator();
	while(!found && i.hasNext())
	{
		Lot temp = i.next();
		found = (temp.getNumber() == lotNumber);
		if (found)
		{
			result = temp;
		}
	}
	return result;
}

Open in new window

0
 
CEHJCommented:
If it's a Collection, you need to iterate it until iterator.next().getNumber() == lotNumber
0
 
spower22Author Commented:
CEJH, thanks for the input, to be honest it's a little complex for my level of understanding of java (attempting to teach myself same), is there a simpler way to achieve the desired result?
0
Free Tool: Site Down Detector

Helpful to verify reports of your own downtime, or to double check a downed website you are trying to access.

One of a set of tools we are providing to everyone as a way of saying thank you for being a part of the community.

 
lilian-arnaudCommented:
small enhancement to CEHJ code
public Lot getLot(int lotNumber)
{
        Lot result = null;      
        Iterator<Lot> i = lots.iterator();
        while(i.hasNext())
        {
                Lot temp = i.next();
                found = (temp.getNumber() == lotNumber);
                return result;
        }
        return null;
}

Open in new window

0
 
lilian-arnaudCommented:
hooops
public Lot getLot(int lotNumber)
{
        Lot result = null;      
        Iterator<Lot> i = lots.iterator();
        while(i.hasNext())
        {
                Lot temp = i.next();
                if (temp.getNumber() == lotNumber) {
                   return temp;
                }
        }
        return null;
}

Open in new window

0
 
objectsCommented:
change lots to be a Map

public Lot getLot(int lotNumber)
{
   return lots.get(lotNumber);
}

and to add a lot you would use:

lots.put(lotNumber, lot);

0
 
CEHJCommented:
>>it's a little complex for my level of understanding of java

What is it you don't understand?
0
 
spower22Author Commented:
Pretty much all of it, I'm still at the else, if, for loop statement stage!
0
 
objectsCommented:
>  is there a simpler way to achieve the desired result?

absolutely, see the code I posted above :)

0
 
objectsCommented:
If you can only change that method then the code lilian-arnaud posted is the next best thing


0
 
CEHJCommented:
Firstly what kind of collection is it? Is it a List, a Set or what?
0
 
spower22Author Commented:
OK, having looked at it, it's clearer now, maybe you could explain where the i and temp references (in CEJH and Lillian's code) come into it and what they're doing?  
0
 
CEHJCommented:
'i' iterates over the collection and 'temp' is just to temporarily hold the current Lot. Did you try the code?
0
 
spower22Author Commented:
yes I tried it and it works perfectly, I'm just trying to understand what it's doing so I can learn from it...
0
 
CEHJCommented:
btw, lilian-arnaud - that's not an 'enhancement' - it's a corruption ;-)

My code deliberately defines a single exit point, which is more readable and more maintainable IMO ;-)
0
 
objectsCommented:
if you cannot change the collection type then have a look at this:

public Lot getLot(int lotNumber)
{
        Lot result = null;
        // Iterate through the collection until you find a match    
        Iterator i = lots.iterator();
        while(result==null && i.hasNext())
        {
                // Get next Lot in collection
                Lot temp = (Lot) i.next();
                // check if lot number matches the one we are after
                if (temp.getNumber() == lotNumber) {
                   // found a match
                   result = temp;
                }
        }
        // return what we found
        return result;
}
0
 
lilian-arnaudCommented:
CEHJ : I'm so sorry !

I changed my code to follow the rule :-)
public Lot getLot(int lotNumber)
{
        Lot result = null;      
        Iterator<Lot> i = lots.iterator();
        while(i.hasNext())
        {
                Lot temp = i.next();
                if (temp.getNumber() == lotNumber) {
                   result = temp;
                   break;
                }
        }
        return result;
}

Open in new window

0
 
objectsCommented:
> CEHJ : I'm so sorry !

no need to be :)

0
 
spower22Author Commented:
Thanks for all your help, in order to be fair I'm going to split the marks as I've learned something from all three inputs and in their own way each is right.....  SP
0
Question has a verified solution.

Are you are experiencing a similar issue? Get a personalized answer when you ask a related question.

Have a better answer? Share it in a comment.

All Courses

From novice to tech pro — start learning today.