6
\$\begingroup\$

I had posted this here and was asked to move it to code review since my code works and am looking for the best possible solution : original post

NOTE: I have seen the JOIN in GAE related posts but GROUP BY in my requirements made me make a new post and I have made a solution of my own which I wanted to check how good is it.


Datastore Entities properties and types (database)

  1. Entity : PartyEntity let's call this as a

    • String partyName
    • String partyId
    • BlobKey image
  2. Entity : InsertEntity let's call this as b

    • String partyIdentifier
    • String name
    • String constituency

Objective

SELECT * FROM a JOIN b on a.partyId = b.partyIdentifier GROUP BY b.constituency;

The solution I made is as follows. Do suggest any changes or better ideas.

// SELECT * FROM a (PartyEntity holds unique partyId) and for each partyId SELECT * FROM b

private List<ListDisplay> initData() {

    DatastoreService datastore = DatastoreServiceFactory
            .getDatastoreService();

    List<ListDisplay> data = new ArrayList<ListDisplay>();

    Query query = new Query("PartyEntity");
    PreparedQuery preparedQuery = datastore.prepare(query);
    List<Entity> entities = preparedQuery.asList(FetchOptions.Builder
            .withDefaults());

    for (Entity entity : entities) {

        String PARTY_ID = (String) entity.getProperty("partyId");

        // fetch from another
        Query query2 = new Query("InsertEntity");
        Filter filterByPartyId = new Query.FilterPredicate(
                "partyIdentifier", FilterOperator.EQUAL, PARTY_ID);
        query2.setFilter(filterByPartyId);
        PreparedQuery preparedQuery2 = datastore.prepare(query2);

        List<Entity> entities2 = preparedQuery2.asList(FetchOptions.Builder
                    .withDefaults());

        for (Entity entity2 : entities2) {
            ListDisplay display = new ListDisplay();
            display.setPartyId((String) entity.getProperty("partyId"));
            display.setPartyName((String) entity.getProperty("partyName"));
            display.setImage((BlobKey) entity.getProperty("image"));
            display.setName((String) entity2.getProperty("name"));
            display.setConstituency((String) entity2
                    .getProperty("constituency"));
            data.add(display);
        } // end loop entity2

    } // end loop entity

    return data;
}

// GROUP BY constituency clause part

public HashMap<String, List<ListDisplay>> getData() {
    HashMap<String, List<ListDisplay>> data = new HashMap<>();
    List<ListDisplay> list = initData();

    for (ListDisplay d : list) {
        // if null add
        if (data.get(d.getConstituency()) == null) {
            List<ListDisplay> internal = new ArrayList<>();
            internal.add(d);
            data.put(d.getConstituency(), internal);
        }
        // else modify
        else {
            List<ListDisplay> hashlist = data.get(d.getConstituency());
            hashlist.add(d);
            data.remove(d.getConstituency());
            data.put(d.getConstituency(), hashlist);
        }
    }

    return data;
}
\$\endgroup\$
3
\$\begingroup\$

Just a few generic notes, I'm not familiar with Google App Engine:

  1. HashMap<...> reference types could be Map<...>:

    public Map<String, List<ListDisplay>> getData() {
    

    See: Effective Java, 2nd edition, Item 52: Refer to objects by their interfaces

  2. I'd rename this variable to result to express its purpose:

    List<ListDisplay> data = new ArrayList<ListDisplay>();
    
  3. remove is unnecesary here, put will override it:

    data.remove(d.getConstituency());
    data.put(d.getConstituency(), hashlist);
    
  4. Consider replacing the following with a Multimap:

    // if null add
    if (data.get(d.getConstituency()) == null) {
        List<ListDisplay> internal = new ArrayList<>();
        internal.add(d);
        data.put(d.getConstituency(), internal);
    }
    // else modify
    else {
        List<ListDisplay> hashlist = data.get(d.getConstituency());
        hashlist.add(d);
        data.remove(d.getConstituency());
        data.put(d.getConstituency(), hashlist);
    }
    

    The following uses Google Guava's Multimap and much simpler:

    final ArrayListMultimap<String, ListDisplay> data = ArrayListMultimap.create();
    
    for (ListDisplay d : list) {
        data.put(d.getConstituency(), d);
    }
    

    If you really need Map return type you can convert it back to Map with data.asMap() which returns a Map<String, Collection<ListDisplay>> but you cast it down safely to Map<String, List<ListDisplay>>. (ArrayListMultimap javadoc supports this as well as these Stack Overflow posts.)

  5. Variable names in Java usually camelCase.

    String PARTY_ID = (String) entity.getProperty("partyId");
    
  6. Comments on the closing curly braces are unnecessary and disturbing. Modern IDEs and editors could show blocks.

        } // end loop entity2
    

    “// …” comments at end of code block after } - good or bad?

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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