current community

your communities

more stack exchange communities

Stack Exchange
tour help
careers 2.0
Take the 2-minute tour ×
Stack Overflow is a question and answer site for professional and enthusiast programmers. It's 100% free, no registration required.
up vote 2 down vote favorite

I have written the following class for my program:

public class RefGen {

    private static String refNo;

    protected static void generate(){

         //Create array to store reference
         ArrayList<String> refList = new ArrayList<>();

         //Get date and set output format
         DateFormat dateFormat = new SimpleDateFormat("yyMMdd");        
         Date curDate = new Date();

          //Variables
          String clientKey = InGenUI.clientText.getText();
          String refDate = dateFormat.format(curDate); 
          String refType = InGenUI.typeCombo.getSelectedItem().toString();
          String userName = InGenUI.userCombo.getSelectedItem().toString();
          String ref;
          int n = 1;

          //Create Reference
          refNo = clientKey.toUpperCase() + "/" + refDate + "/" + refType + "/" + userName + "/" + Integer.toString(n);

          //Check to see if refNo already exists in array
          while (refList.contains(refNo)) {
              n = n + 1;
              refNo = clientKey.toUpperCase() + "/" + refDate + "/" + refType + "/" + userName + "/" + Integer.toString(n);
              refList.add(refNo);
          }

          refList.add(refNo);
          System.out.println(refList);
      }

      public static String reference(){

          return refNo;
      }
  }

The purpose of this class is to generate a unique reference number and store it in an array. Before doing so, it needs to check to see if the array already contains the value. If not, n is incremented by 1 until refNo becomes a unique value that does not exist in the array.

RefGen.reference() is called by genButton in InGenUI.java which output's refNo's value to clientLabel also in InGenUI.java:

    private void genButtonActionPerformed(java.awt.event.ActionEvent evt) {                                          
        RefGen.generate();
        String refNo = RefGen.reference();
        clientLabel.setText(refNo);
    }

The program generates the reference number but never increments the label value in InGenUI.java or the actually array itself in RefGen.java. It also seems that the array only holds a single value on each button click.

I think that refList stores the original refNo value generated but empties the array each time it does so. I suspect that each time I click genButton I am actually creating a new instance of refList and hence wiping out the old values. Is this correct? If so, how can I protect the instance of refList I created while keeping it in the RefGen.java class?

Thank you in advance.

share|improve this question
3  
I think you should juse HashSet instead of ArrayList. –  Stefan Beike Jul 29 at 9:53
    
I would agree with @Stefan Beike here. I don't know the actual size of your ArrayList but if it's big enough it makes a great difference checking if it contains each element. –  Eypros Jul 29 at 10:02

2 Answers 2

up vote 1 down vote accepted

Well, you could make refList a class variable, i.e. move it out of the generate() method.

I don't know what exactly you mean with "protect the list" but you can make the list a private/protected member and only allow manipulation via special methods.

Additionally, I'm not sure this part works as intended (neither currently nor after the suggested changes):

while (refList.contains(refNo)) {
          n = n + 1;
          refNo = clientKey.toUpperCase() + "/" + refDate + "/" + refType + "/" + userName + "/" + Integer.toString(n);
          refList.add(refNo); //<-- problems here
 }

Currently, your List won't contain refNo and thus it won't be added.
After the suggested changes the list would contain refNo but now you'd add it again. So you'd probably want to remove that add operation.

As already suggested you also might want to use a Set<String> instead and choose an implementation that fits your needs, e.g. TreeSet<String> of the reference numbers should be sorted of LinkedHashSet<String> if the order of (first) insertion does matter.

Using a set you could even keep re-adding the elements (it's still odd, but you could) since sets don't allow for duplicates.

You could also change your code to try and add a reference number and it that didn't succeed or the size of the set didn't change you increment the number and try again.

UPDATE:

I didn't realize that the method was static (thanks Stephen C), so I moved the parts concerning instance variables to here.

Generally static variables aren't that well recieved for storing data such as your reference list. There are multiple reasons, e.g. what if you'd want to use the same code for multiple lists, encapsulation and inheritance (e.g. you can't override static methods and having non-static methods write to static variables often isn't really meaningful), the possibility of memory leaks (if you don't manually set statics to null or unload classes yourself, they can't be collected) etc.

Thus you might consider changing two possibilities for changing your code:

  • keep the list outside of the class and pass it as a parameter
  • change the code to make the list and generate method non-static.
share|improve this answer
1  
Well yes, except that the OP is actually using static methods here, so the "instance variable" needs to be a static variable. –  Stephen C Jul 29 at 10:11
    
Thank you all, this worked perfectly. I also noticed the problem mentioned above was sending the program into an infinite loop. Noted the use of Hash for performance but I will complete the program first and think about changing over if necessary. The list will be very small at present, maybe up to 20 entries in a day. Thank you again for your help. –  user3645158 Jul 29 at 10:13
up vote 5 down vote

You seems to be creating a fresh array list every time you call generate:

     //Create array to store reference
     ArrayList<String> refList = new ArrayList<>();

You should declare it outside the method at class level.

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.