?
Solved

Feedback on effectiveness and efficiency of code

Posted on 2014-04-21
7
Medium Priority
?
294 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
CHALLENGE LAB: Troubleshooting Connectivity Issues

Goal: Fix the connectivity issue in the lab's AWS environment so that you can SSH into the provided EC2 instance.  

 
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

Get 15 Days FREE Full-Featured Trial

Benefit from a mission critical IT monitoring with Monitis Premium or get it FREE for your entry level monitoring needs.
-Over 200,000 users
-More than 300,000 websites monitored
-Used in 197 countries
-Recommended by 98% of users

Question has a verified solution.

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

Introduction Since I wrote the original article about Handling Date and Time in PHP and MySQL several years ago, it seemed like now was a good time to update it for object-oriented PHP.  This article does that, replacing as much as possible the pr…
Styling your websites can become very complex. Here I'll show how SASS can help you better organize, maintain and reuse your CSS code.
The viewer will learn how to count occurrences of each item in an array.
The viewer will learn the basics of jQuery including how to code hide show and toggles. Reference your jQuery libraries: (CODE) Include your new external js/jQuery file: (CODE) Write your first lines of code to setup your site for jQuery…
Suggested Courses
Course of the Month14 days, 7 hours left to enroll

771 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