wood burning stoves 2.0*
The moose likes Java in General and the fly likes Please review this code Big Moose Saloon
  Search | Java FAQ | Recent Topics | Flagged Topics | Hot Topics | Zero Replies
Register / Login


Win a copy of Java 8 in Action this week in the Java 8 forum!
JavaRanch » Java Forums » Java » Java in General
Bookmark "Please review this code" Watch "Please review this code" New topic
Author

Please review this code

Sindhu Kodoor
Ranch Hand

Joined: Sep 03, 2010
Posts: 65
HI ,

The below code is used to check the elements in a list and validate if the value is "1" or value is "2", then it should take that value and print in the last { x value } otherwise it should catch the null pointer exception and make x value as empty and print that , please let me know if I can modify the code and make it any better?

Sindhu Kodoor
Ranch Hand

Joined: Sep 03, 2010
Posts: 65
Please reply anyone
Raymond Tong
Ranch Hand

Joined: Aug 15, 2010
Posts: 230
    
    2

What problem do you want to solve ?
Instead of explaining what your code do, what do you want to achieve?

If you need to call array.get(i) several times, it would be better to use local variable.
You didn't change the content of list, it is not necessary to use a while loop.
If you want to check x is null or not , you can write (x == null) without having to catch NullPointerException.
Freddy Wong
Ranch Hand

Joined: Sep 11, 2006
Posts: 959

Patience is a Virtue.

This looks like a homework question to me. Anyway, to give you some inputs.
- The while loop is meaningless because the if/else statement inside always calls break.
- Catching NullPointerException isn't a good idea.
- x.equals(null) isn't right. You should use x == null instead.

There are many ways to improve your code, but I leave it to you as an exercise. Good luck


SCJP 5.0, SCWCD 1.4, SCBCD 1.3, SCDJWS 1.4
My Blog
David Newton
Author
Rancher

Joined: Sep 29, 2008
Posts: 12617

To be honest, I can't figure out what your code is trying to do. It seems like it's maybe three times too long for what the problem statement it.
Sindhu Kodoor
Ranch Hand

Joined: Sep 03, 2010
Posts: 65
ok , the problem is , there is a list whose size is unknown, and out of the list I need to only check for 2 values , say 1 and 2 , if either of 1 or 2 is present I need to print that value, if not present then it should print empty string. The reason I used a while loop is because when I just use the for loop , it is not iterating , it says "dead code" for "i++" in the for loop , hence I used the while , and moreover , I agree break should exit but it is not exiting from the loop , I have change the code to x==null
David Newton
Author
Rancher

Joined: Sep 29, 2008
Posts: 12617

Wouldn't you just want to iterate over list items, if it's not null and is "1" or "2" print it out, otherwise keep going?
Sindhu Kodoor
Ranch Hand

Joined: Sep 03, 2010
Posts: 65
You mean to say use something like this :



I am still using for loop as I do not know at which index of the list the value "1" or "2" appears
David Newton
Author
Rancher

Joined: Sep 29, 2008
Posts: 12617

In your problem statement it doesn't seem necessary to care what the index is, but that's neither here nor there--iterating over the list is iterating over the list, regardless of how it's done.

But no, I don't mean anything like what you just posted--why do you think there needs to be two iterations over the list?
Gavin Tranter
Ranch Hand

Joined: Jan 01, 2007
Posts: 333
Have you tried starting from the point of view of trying to loop through the List and printing every value?
Raymond Tong
Ranch Hand

Joined: Aug 15, 2010
Posts: 230
    
    2

Sindhu Kodoor wrote:You mean to say use something like this :



I am still using for loop as I do not know at which index of the list the value "1" or "2" appears

Post the code that using for-loop without using while
we would help you to resolve the problem.
It is really not necessary to use the while block.
Sindhu Kodoor
Ranch Hand

Joined: Sep 03, 2010
Posts: 65
Raymond Tong
Ranch Hand

Joined: Aug 15, 2010
Posts: 230
    
    2

You use "break" when you want to exit the iteration (for / while).
If you want to iterate all elements in your list, you don't need it.

Tutorial about for loop
David Newton
Author
Rancher

Joined: Sep 29, 2008
Posts: 12617

What is the purpose of those "break" statements?
Sindhu Kodoor
Ranch Hand

Joined: Sep 03, 2010
Posts: 65
If I don't put the break then when it has "1" value in consider the 3rd place in the list , then the expected output is "First x=1" but it prints "Third null First x=1"
David Newton
Author
Rancher

Joined: Sep 29, 2008
Posts: 12617

Oh, so you're trying to print the first index of both "1" and "2"? This was not clear from your original problem statement.
Sindhu Kodoor
Ranch Hand

Joined: Sep 03, 2010
Posts: 65
Now, I changed the whole thing and it works :

Sindhu Kodoor
Ranch Hand

Joined: Sep 03, 2010
Posts: 65
Does the above code look better?
Raymond Tong
Ranch Hand

Joined: Aug 15, 2010
Posts: 230
    
    2

Sindhu Kodoor wrote:Does the above code look better?

Why do you remove the element "1" from your list.
We still don't know what you are trying to do with your code.
Keep some example of inputs and expected outputs.
Campbell Ritchie
Sheriff

Joined: Oct 13, 2005
Posts: 36575
    
  16
Sindhu Kodoor wrote:Does the above code look better?
Only slightly. I would suggest you go through the code with a fine-toothed comb and find which parts are never actually used.
Sindhu Kodoor
Ranch Hand

Joined: Sep 03, 2010
Posts: 65
Problem is : I have a list say its of type String and values "3,4,5,6,1,7" , I need to iterate through this list and check if the list contains either value "1" or "2", but both will not be present at a time. So , I am using if else statements, and In case "1" is present I need to replace a String value x which is initially empty or null to "1", and if 1 is not present but 2 is present then replace x value to 2, if both are not present x value will returned as empty string.

This is a homework for a real problem scenario, the list is in a metadata where it appears as a tree stucture, like this :




So I check if These methods I get from an internal API(CLient specified) and if this link name returns true then I fetch this URI or else if same thing I check for value "2" then I fetch that uri. or else return an empty URI.

why m I doing this check is , if the uri I get from this list is empty then I need to reject this xml ( the above code is a property or metadata file of an xml) coming from the marklogic server otherwise write this entry into a file. I hope this time I am clear in explaining the problem scenario?
Raymond Tong
Ranch Hand

Joined: Aug 15, 2010
Posts: 230
    
    2

Sindhu Kodoor wrote:Problem is : I have a list say its of type String and values "3,4,5,6,1,7" , I need to iterate through this list and check if the list contains either value "1" or "2", but both will not be present at a time. So , I am using if else statements, and In case "1" is present I need to replace a String value x which is initially empty or null to "1", and if 1 is not present but 2 is present then replace x value to 2, if both are not present x value will returned as empty string.

If this is your requirement, you don't have to loop through all the elements in your list.
As soon as you find "1" or "2", you can exit the loop and got the updated value of x.

The code above seems meaningless, it doesn't change anything.
Even better, You can use list.contains("1") and list.contains("2") to check whether your list got "1" or "2".
Sindhu Kodoor
Ranch Hand

Joined: Sep 03, 2010
Posts: 65
As I said , that was just a rough work for the actual scenario , in the actual scenario , list "Links" consists of number of Lists "Link" which inturn consist of number of Lists "LinkName" and "URI", and the occurrence of the LinkName as "1" or "2" can be in any position :



In such cases I will have to loop through right? this will not fetch me the required value of URI :

Raymond Tong
Ranch Hand

Joined: Aug 15, 2010
Posts: 230
    
    2

Ok. So you iterate the elements, but as soon as you find "1" or "2", you can skip the others.
Sindhu Kodoor
Ranch Hand

Joined: Sep 03, 2010
Posts: 65
are you suggesting instead of the for loop to use iterator.hasNext()?
Raymond Tong
Ranch Hand

Joined: Aug 15, 2010
Posts: 230
    
    2

Sindhu Kodoor wrote:are you suggesting instead of the for loop to use iterator.hasNext()?

What is the purpose of those println if you want to assign value to x ?
If your list contains "1" or "2" you don't care other elements, right ?
Sindhu Kodoor
Ranch Hand

Joined: Sep 03, 2010
Posts: 65
yes, the println statements are only used for debugging purpose, I don't need to print anything.
Raymond Tong
Ranch Hand

Joined: Aug 15, 2010
Posts: 230
    
    2

Sindhu Kodoor wrote:yes, the println statements are only used for debugging purpose, I don't need to print anything.


So, this part is not meaningful, you agree?

Let say you have 7 elements (0,1,2,3,4,5,6),
you check the first element, "0", you move on.
you check the second element, "1", assign x = 1
you don't need to check the 3rd, 4th, 5th, 6th elements, because you found "1" in your list, agree ?
Sindhu Kodoor
Ranch Hand

Joined: Sep 03, 2010
Posts: 65
yes I absolutely agree, thats why I had used the break statements earlier, instead of break how do I get out of the loop?
Raymond Tong
Ranch Hand

Joined: Aug 15, 2010
Posts: 230
    
    2

Sindhu Kodoor wrote:yes I absolutely agree, thats why I had used the break statements earlier, instead of break how do I get out of the loop?

If you want to exit iteration (for / while), you should break.
You used it in your original code but you have removed it.
 
Don't get me started about those stupid light bulbs.
 
subject: Please review this code
 
Similar Threads
Null pointer Exception
parsing standard input
Application not accessing attached JAR file
Three Dimensional ArrayList?
Comparing ArrayList and double array[]