?
Solved

Feedback on effectiveness and efficiency of code

Posted on 2014-04-21
7
Medium Priority
?
296 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
[X]
Welcome to Experts Exchange

Add your voice to the tech community where 5M+ people just like you are talking about what matters.

  • Help others & share knowledge
  • Earn cash & points
  • Learn & ask questions
  • 3
  • 2
  • 2
7 Comments
 
LVL 75

Assisted Solution

by:Michel Plungjan
Michel Plungjan earned 880 total points
ID: 40016071
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
ID: 40016796
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 880 total points
ID: 40016984
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
Independent Software Vendors: We Want Your Opinion

We value your feedback.

Take our survey and automatically be enter to win anyone of the following:
Yeti Cooler, Amazon eGift Card, and Movie eGift Card!

 
LVL 35

Accepted Solution

by:
girionis earned 1120 total points
ID: 40017231
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
ID: 40024371
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 1120 total points
ID: 40026627
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
ID: 40026633
cheers
0

Featured Post

Independent Software Vendors: We Want Your Opinion

We value your feedback.

Take our survey and automatically be enter to win anyone of the following:
Yeti Cooler, Amazon eGift Card, and Movie eGift Card!

Question has a verified solution.

If you are experiencing a similar issue, please ask a related question

Preface This is the third article about the EE Collaborative Login Project. A Better Website Login System (http://www.experts-exchange.com/A_2902.html) introduces the Login System and shows how to implement a login page. The EE Collaborative Logi…
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…
The viewer will learn how to create and use a small PHP class to apply a watermark to an image. This video shows the viewer the setup for the PHP watermark as well as important coding language. Continue to Part 2 to learn the core code used in creat…
HTML5 has deprecated a few of the older ways of showing media as well as offering up a new way to create games and animations. Audio, video, and canvas are just a few of the adjustments made between XHTML and HTML5. As we learned in our last micr…
Suggested Courses

650 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