CODEWITHSUNDEEP
javamethodsarraylist
Mayfield
Mayfield

Reputation: 1

Am I writing this method the right way?

I've got an ArrayList called conveyorBelt, which stores orders that have been picked and placed on the conveyor belt. I've got another ArrayList called readyCollected which contains a list of orders that can be collected by the customer.

What I'm trying to do with the method I created is when a ordNum is entered, it returns true if the order is ready to be collected by the customer (thus removing the collected order from the readyCollected). If the order hasn't even being picked yet, then it returns false.

I was wondering is this the right way to write the method...

  public boolean collectedOrder(int ordNum)
  {
      int index = 0;
      Basket b = new Basket(index);
      if(conveyorBelt.isEmpty()) {
          return false;
      }
      else {
          readyCollected.remove(b);
          return true;
      }
  }

Upvotes: 0

Views: 103

Answers (4)

Michael Berry
Michael Berry

Reputation: 72294

As already pointed out, you most likely need to be using ordNum.

Aside from that the best answer anyone can give with the code you've posted is "perhaps". Your logic certainly looks correct and ties in with what you've described, but whether it's doing what it should depends entirely on your implementation elsewhere.

As a general pointer (which may or may not be applicable in this instance) you should make sure your code deals with edge cases and incorrect values. So you might want to flag something's wrong if readyCollected.remove(b); returns false for instance, since that indicates that b wasn't in the list to remove.

As already pointed out, take a look at unit tests using JUnit for this type of thing. It's easy to use and writing thorough unit tests is a very good habit to get into.

Upvotes: 0

Amir Afghani
Amir Afghani

Reputation: 38531

Based on your description, why isn't this sufficient :

  public boolean collectedOrder(int ordNum)   {         
      return (readyCollected.remove(ordNum) != null);
  }

Why does the conveyorBelt ArrayList even need to be checked?

Upvotes: 0

templatetypedef
templatetypedef

Reputation: 372814

You can solve this problem using an ArrayList, but I think that this is fundamentally the wrong way to think about the problem. An ArrayList is good for storing a complete sequence of data without gaps where you are only likely to add or remove elements at the very end. It's inefficient to remove elements at other positions, and if you have just one value at a high index, then you'll waste a lot of space filling in all lower positions with null values.

Instead, I'd suggest using a Map that associates order numbers with the particular order. This more naturally encodes what you want - every order number is a key associated with the order. Maps, and particularly HashMaps, have very fast lookups (expected constant time) and use (roughly) the same amount of space no matter how many keys there are. Moreover, the time to insert or remove an element from a HashMap is expected constant time, which is extremely fast.

As for your particular code, I agree with Brian Agnew on this one that you probably want to write some unit tests for it and find out why you're not using the ordNUm parameter. That said, I'd suggest reworking the system to use HashMap instead of ArrayList before doing this; the savings in time and code complexity will really pay off.

Upvotes: 1

Brian Agnew
Brian Agnew

Reputation: 272297

I'm a little confused since you're not using ordNum at all.

If you want to confirm operation of your code and generally increase the reliability of what you're writing, you should check out unit testing and the Java frameworks available for this.

Upvotes: 1

Related Questions