Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I am just trying to convert each list of values to String by index one by one. My Code is running fine. But I am thinking that I can write better code than this. I Need your good coding possibilities here to convert from list to String.

I am using the code below to convert:

CarComment[] list = form.getCarDataJaxb().getCarCommentList();
JSONObject obj = new JSONObject();
JSONArray comments = new JSONArray();
for (int i=0;i<list.length;i++){
    if (notEmpty(list[i].getText())){
        JSONArray commObj = new JSONArray();

        String str = list[i].getText();
        str = str.replaceAll("<", "&lt;");
        commObj.put(str);

        comments.put(commObj);
    }
}
obj.put("carcomm", comments);
response.setContentType("text/json");
response.getWriter().write(obj.toString());

I am trying to write good coding technique while converting from list to String at line String str = list[i].getText();

share|improve this question

2 Answers 2

up vote 5 down vote accepted

There actually isn't so much to improve here (at least not that I can think of).

Instead of using an index to iterate by, you can use a Java for-each loop. Since you only used the iterator variable to read the index of the list, it is usually more readable to use a for-each loop.

You were not indenting your code correctly (before the edit of your question), which I also fixed below.

And, below I use the opposite way of checking if the text is empty, by using continue if the if-statement is not true, you can avoid having to indent the code an extra step.

CarComment[] list = form.getCarDataJaxb().getCarCommentList();
JSONObject obj = new JSONObject();
JSONArray comments = new JSONArray();
for (CarComment carComment : list) {
    if (!notEmpty(carComment.getText()))
       continue;
    JSONArray commObj = new JSONArray();
    commObj.put(carComment.getText().replaceAll("<", "&lt;"));
    comments.put(commObj);
}
obj.put("carcomm", comments);
response.setContentType("text/json");
response.getWriter().write(obj.toString());

Preferably, to avoid double-negation, you should use if (isEmpty(...)) instead of if (!notEmpty(...))

However, there's no way of having to do a method call to getText (or a similar method call) on your line.

String str = list[i].getText();

What I did was to skip the declaration of a local variable and process the string (by calling replaceAll) directly when adding it to it's destination.

share|improve this answer
    
Hi Simon, i didn't use double-negation..! –  Mdhar9e Nov 21 '13 at 14:54
2  
@Mdhar9e basically isEmpty is available in java's String api. Use it when it is applicable and as told by Simon(the negation thing). Otherwise do you have the code which you wrote for notEmpty or its just a pseudo code ? –  Ravi Kumar Nov 21 '13 at 14:59
1  
@Mdhar9e I know you didn't, but I did. And I'm telling you to not do it ;) –  Simon André Forsberg Nov 21 '13 at 15:06

I have modified the loop. Here is the code --

for (CarComment carComment : list) {
  String text = carComment.getText();
  if (isEmpty(text)) { continue; }

  JSONArray commObj = new JSONArray();
  commObj.put(text.replaceAll("<", "&lt;"));

  comments.put(commObj);
}

you can provide CarComment class to see how it looks like and how is the getText? Those can be improved by looking the original code.

share|improve this answer
1  
it is an interface with setters and getters. –  Mdhar9e Nov 21 '13 at 15:14
1  
would you please explain your answer? –  Malachi Nov 22 '13 at 4:04

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.