HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavaMinor

JPA connection : is this code is efficient enough

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
thisefficientcodeenoughconnectionjpa

Problem

By reading some tutorial i have written some peace of code to do crud operation . I just want to know how is this code is efficient or how can i make better ?

Here i am giving code of 3 class
1. EntityManagerProvider : get connection from DB and do crud operation
2. BaseDao : A generalised Dao which uses EntityManagerProvider

3. TargetGroupsDao : A specific Dao

```
import java.util.Collection;

import javax.persistence.EntityExistsException;
import javax.persistence.EntityManager;
import javax.persistence.EntityManagerFactory;
import javax.persistence.EntityTransaction;
import javax.persistence.Persistence;
import javax.persistence.PersistenceException;
import javax.persistence.Query;
import javax.persistence.RollbackException;
import javax.persistence.TransactionRequiredException;

public class EntityManagerProvider {

private static EntityManagerFactory emf;
private static boolean lock;
private static String persistentUnitName = "TMS_PU";
static {
EntityManagerProvider.emf = Persistence.createEntityManagerFactory(persistentUnitName);
}
private static final ThreadLocal localEm = new ThreadLocal() {
@Override
protected EntityManager initialValue() {
if (emf == null || !emf.isOpen()) {
emf = Persistence
.createEntityManagerFactory(persistentUnitName);
}
return emf.createEntityManager();
}
};

@Override
protected void finalize() throws Throwable {
EntityManager em = getEntityManager();
if (em != null && em.isOpen())
em.close();
super.finalize();
}

public EntityManagerProvider() {
}

public static EntityManager getEntityManager() {
EntityManager em = localEm.get();
if (em == null || !em.isOpen()) {
em = emf.createEntityManager();
localEm.set(em);
}
return em;
}

public static void beginTransaction() {

Solution

1.This sample is really ugly :) You can extract method isEmptyTrim() at least.

if( targetGroupsDto.getTgDesc()!= null && !targetGroupsDto.getTgDesc().trim().equals(""))
    map.put("tgDesc", targetGroupsDto.getTgDesc());
if( targetGroupsDto.getTgId()!= null && !targetGroupsDto.getTgId().trim().equals(""))
    map.put("tgId", targetGroupsDto.getTgId());
if( targetGroupsDto.getTgName()!= null && !targetGroupsDto.getTgName().trim().equals(""))
    map.put("tgName", targetGroupsDto.getTgName());
if( targetGroupsDto.getOwnershipType()!= null && !targetGroupsDto.getOwnershipType().trim().equals(""))
    map.put("ownershipType", targetGroupsDto.getOwnershipType());
if( targetGroupsDto.getNoOfMembers()!= null)
    map.put("noOfMembers", targetGroupsDto.getNoOfMembers());


2.Cycle over keyset is a bad practice.

for (String str : var.keySet()) {


You should use

for (Entry entry : var.entrySet()) {

Code Snippets

if( targetGroupsDto.getTgDesc()!= null && !targetGroupsDto.getTgDesc().trim().equals(""))
    map.put("tgDesc", targetGroupsDto.getTgDesc());
if( targetGroupsDto.getTgId()!= null && !targetGroupsDto.getTgId().trim().equals(""))
    map.put("tgId", targetGroupsDto.getTgId());
if( targetGroupsDto.getTgName()!= null && !targetGroupsDto.getTgName().trim().equals(""))
    map.put("tgName", targetGroupsDto.getTgName());
if( targetGroupsDto.getOwnershipType()!= null && !targetGroupsDto.getOwnershipType().trim().equals(""))
    map.put("ownershipType", targetGroupsDto.getOwnershipType());
if( targetGroupsDto.getNoOfMembers()!= null)
    map.put("noOfMembers", targetGroupsDto.getNoOfMembers());
for (String str : var.keySet()) {
for (Entry<String, Object> entry : var.entrySet()) {

Context

StackExchange Code Review Q#19177, answer score: 3

Revisions (0)

No revisions yet.