Solved

Feedback on effectiveness and efficiency of code

Posted on 2014-04-21
7
284 Views
Last Modified: 2014-04-27
Hello there,

I have this class which is implemented on the server side. I want to get help from experts and get their feedback/suggestions/recommendations on this piece of code I have written and tell me if it has been written with proper standard. specially the hibernate section where I am querying the database. I will appreciate your help. I have attached my code.

cheers
Zolf
zz.txt
0
Comment
Question by:zolf
  • 3
  • 2
  • 2
7 Comments
 
LVL 75

Assisted Solution

by:Michel Plungjan
Michel Plungjan earned 220 total points
Comment Utility
You for sure need to look at StringBuilder instead of + concatenation

This is also bad

if (dsRequest.getFieldValue("queryBatchNo") != null && "1".equals(dsRequest.getFieldValue("queryBatchNo").toString()))

instead save as string and test it twice
0
 

Author Comment

by:zolf
Comment Utility
mplungjan

thanks for your feedbacks. can you please tell me more about this comment of yours. I did not understand what you meant by test it twice
instead save as string and test it twice

regarding the concatenation did you mean my sql queries
0
 
LVL 75

Assisted Solution

by:Michel Plungjan
Michel Plungjan earned 220 total points
Comment Utility
yes I meant the queries


String queryBatchNo =  dsRequest.getFieldValue("queryBatchNo");

if (queryBatchNo!= null && "1".equals(queryBatchNo.toString()) )...

PS: There are many more considerations that Java experts can point out in your code. You have too many if statements and too big pieces of code inside each
0
Top 6 Sources for Identifying Threat Actor TTPs

Understanding your enemy is essential. These six sources will help you identify the most popular threat actor tactics, techniques, and procedures (TTPs).

 
LVL 35

Accepted Solution

by:
girionis earned 280 total points
Comment Utility
A few things you could improve on:

1) Always use a package.
2) Don't use a System.out.println (use a logger instead)
3) Do not instantiate Long objects with new Long. Use Long.valueOf instead (it is more memory friendly).
4) Don't throw Exception, instead throw specific exceptions.
5) Avoid printStackTrace, again, use a logger.
6) Your singleton is not thread-safe. If you use this class in a multi-threaded environment you might end up with more than one instance of you class.
0
 

Author Comment

by:zolf
Comment Utility
thanks you all for your comments

by: girionis

Your singleton is not thread-safe.

can you please tell me which code you meant.actually I will be using it in a multi-threaded environment and that was my concern and the reason for posting my code here to get experts like you all to help me improve my code.
0
 
LVL 35

Assisted Solution

by:girionis
girionis earned 280 total points
Comment Utility
I am talking about this bit of code

public static SupplierOrderCompleteBusinessLogic getInstance()
	{
		if (instance == null)
			instance = new SupplierOrderCompleteBusinessLogic();
		return instance;
	}

Open in new window

this is not thread safe (and as a side note, people can always create more instances since you provide an implicit default constructor). You must synchronise the getInstance() method by either doing

public static synchronized SupplierOrderCompleteBusinessLogic getInstance()
	{
		if (instance == null)
			instance = new SupplierOrderCompleteBusinessLogic();
		return instance;
	}

Open in new window

or use some synchronisation right before the instance creation.

But I would go with the initialisation on demand holder idiom solution if you can go with lazy loading,

        private SupplierOrderCompleteBusinessLogic() {}

        private static class Holder {
             private static final SupplierOrderCompleteBusinessLogic INSTANCE = new SupplierOrderCompleteBusinessLogic();
        }

	public static SupplierOrderCompleteBusinessLogic getInstance() {
             return Holder .INSTANCE;
        }

Open in new window

0
 

Author Closing Comment

by:zolf
Comment Utility
cheers
0

Featured Post

IT, Stop Being Called Into Every Meeting

Highfive is so simple that setting up every meeting room takes just minutes and every employee will be able to start or join a call from any room with ease. Never be called into a meeting just to get it started again. This is how video conferencing should work!

Join & Write a Comment

Browsers only know CSS so your awesome SASS code needs to be translated into normal CSS. Here I'll try to explain what you should aim for in order to take full advantage of SASS.
Java functions are among the best things for programmers to work with as Java sites can be very easy to read and prepare. Java especially simplifies many processes in the coding industry as it helps integrate many forms of technology and different d…
This tutorial covers a step-by-step guide to install VisualVM launcher in eclipse.
The viewer will receive an overview of the basics of CSS showing inline styles. In the head tags set up your style tags: (CODE) Reference the nav tag and set your properties.: (CODE) Set the reference for the UL element and styles for it to ensu…

743 members asked questions and received personalized solutions in the past 7 days.

Join the community of 500,000 technology professionals and ask your questions.

Join & Ask a Question

Need Help in Real-Time?

Connect with top rated Experts

18 Experts available now in Live!

Get 1:1 Help Now