Solved

Feedback on effectiveness and efficiency of code

Posted on 2014-04-21
7
290 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
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 220 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
Master Your Team's Linux and Cloud Stack

Come see why top tech companies like Mailchimp and Media Temple use Linux Academy to build their employee training programs.

 
LVL 35

Accepted Solution

by:
girionis earned 280 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 280 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

Master Your Team's Linux and Cloud Stack

Come see why top tech companies like Mailchimp and Media Temple use Linux Academy to build their employee training programs.

Question has a verified solution.

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

Suggested Solutions

Title # Comments Views Activity
servlet doXXX methods 3 62
servlet filter example 37 71
CSV file parsing thru Java 13 34
What browser will run Java? 7 56
Introduction This article is the second of three articles that explain why and how the Experts Exchange QA Team does test automation for our web site. This article covers the basic installation and configuration of the test automation tools used by…
Introduction This article is the last of three articles that explain why and how the Experts Exchange QA Team does test automation for our web site. This article covers our test design approach and then goes through a simple test case example, how …
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…
The viewer will learn the benefit of using external CSS files and the relationship between class and ID selectors. Create your external css file by saving it as style.css then set up your style tags: (CODE) Reference the nav tag and set your prop…

856 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