Creating an ArrayList of Maps

Question

I am making an ArrayList that contains Map values. itemData is my ArrayList that contains itemselected, which is a map. Then, because I want to send this array to my PHP code, I am converting itemData into a JSONArray. Then parsing the array in my PHP script and inserting those individual values into a MySQL database.

private ArrayList<Map<String, String>> itemData = new ArrayList<>();
private Map<String, String> itemselected = new HashMap<>();
JSONArray itemSelectedJson = new JSONArray();

 private void selectedItems() {
        if(invEstSwitch.isChecked())
        {
            billType = textViewEstimate.getText().toString();
        }else{
            billType = textViewInvoice.getText().toString();
        }

        itemselected.put("custInfo",custSelected.toString());
        itemselected.put("invoiceNo", textViewInvNo.getText().toString());
        itemselected.put("barcode", barCode.getText().toString());
        itemselected.put("desc", itemDesc.getText().toString());
        itemselected.put("weight", weightLine.getText().toString());
        itemselected.put("rate", rateAmount.getText().toString());
        itemselected.put("makingAmt", makingAmount.getText().toString());
        itemselected.put("net_rate", netRate.getText().toString());
        itemselected.put("itemTotal", itemtotal.getText().toString());
        itemselected.put("vat", textViewVat.getText().toString());
        itemselected.put("sum_total", textViewSum.getText().toString());
        itemselected.put("bill_type", billType);
        itemselected.put("date", textViewCurrentDate.getText().toString());

        //Add the map to the Array
        itemData.add(index, itemselected);
        itemSelectedJson= new JSONArray(Arrays.asList(itemData));
        index++;
    }

The code is pretty straightforward, but I am interested in any possible improvement. I am sure there is a better way to do it.

Answer 1

Just some convention points:

• The indentation is off in the first couple of lines. Is this due to copy-paste? I have to assume so, as the rest of your code seems fine.

• The following if statement:

  if(invEstSwitch.isChecked())
  {
      billType = textViewEstimate.getText().toString();
  }else{
      billType = textViewInvoice.getText().toString();
  }
  

  is inconsistent. You put a newline before the beginning brace at the if, and not at the else.

  It is also bad formatting. There should be a space both left and right of the else, and a space after if:

  if (invEstSwitch.isChecked()) {
      billType = textViewEstimate.getText().toString();
  } else {
      billType = textViewInvoice.getText().toString();
  }
  

• The if statement can be simplified into a ternary expression, as it is assigning the same thing (assuming textViewEstimate and textViewInvoice are the same type):

  billType = (invEstSwitch.isChecked() ? textViewEstimate : textViewInvoice)
          .getText().toString();
  

• Try not to use ArrayList. As I said in this answer:

  It is almost always a poor option, as it is pretty much an array with helper functions. If you have an ArrayList of 10000 integers, and then add another integer, ArrayList will (if its current array it's storing the values in is of size 10000) create a new array, move all the values of the old array in the new array, then add the integer to the end. Sounds inefficient? Certainly does.

  My opinion is to use LinkedList. LinkedList is faster, because it works like this:

  • Each value in the list is stored in a Node.
  • Each Node points to the next Node.
  • Adding something to the end is as simple as creating a new Node and linking it to the Node chain.
  • Removing and inserting is as simple as changing some links around. The problem would be to find that link.

  Worst-case time complexity comparison:

          LinkedList        ArrayList
  Get:       O(n)              O(1)
  Add:       O(n)              O(n)
  Insert:    O(n)              O(n)
  

  Best-case time complexity comparison:

          LinkedList        ArrayList
  Get:       O(1)              O(1)
  Add:       O(1)              O(1)
  Insert:    O(1)              O(n) <- assuming not inserting at end of list
  

  Yes, LinkedList is losing, but in this case, you don't use get often. On the other hand, memory is an issue for ArrayList:

          LinkedList        ArrayList
  Get:       O(1)              O(1)
  Add:       O(1)              O(n)
  Insert:    O(1)              O(n)
  

• Your method is fairly inefficient. You create a new JSONArray each time you call the method. Instead, use the given put() method (this also means you don't need an ArrayList):

  private Map<String, String> itemselected = new HashMap<>();
  JSONArray itemSelectedJson = new JSONArray();
  
  private void selectedItems() {
      billType = (invEstSwitch.isChecked() ? textViewEstimate : textViewInvoice)
              .getText().toString();
  
      itemselected.put("custInfo",custSelected.toString());
      itemselected.put("invoiceNo", textViewInvNo.getText().toString());
      itemselected.put("barcode", barCode.getText().toString());
      itemselected.put("desc", itemDesc.getText().toString());
      itemselected.put("weight", weightLine.getText().toString());
      itemselected.put("rate", rateAmount.getText().toString());
      itemselected.put("makingAmt", makingAmount.getText().toString());
      itemselected.put("net_rate", netRate.getText().toString());
      itemselected.put("itemTotal", itemtotal.getText().toString());
      itemselected.put("vat", textViewVat.getText().toString());
      itemselected.put("sum_total", textViewSum.getText().toString());
      itemselected.put("bill_type", billType);
      itemselected.put("date", textViewCurrentDate.getText().toString());
  
      // Add the map to the Array
      itemSelectedJson.put(itemselected);
      index++;
  }