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 getting List ids and by using those ids I am creating a Map by fetching related data of id from database, here I am checking prevId with currId if these are not equal then I am putting them into oCMap.

Can this code be optimized? Or are there any improvements (like is there any better algorithm)? Please offer suggestions.

  public void populateMap(List<Number> ids)
  {
    Object oName, cName, startDate, endDate;
    String header = "<html><table>";
    String footer = "</table></html>";
    String rowStart = "<tr>";
    String rowEnd = "</tr>";
    String columnStart = "<td>&nbsp;";
    String columnEnd = "</td>";
    SimpleDateFormat displayDateFormat = new SimpleDateFormat ("dd/MM/yyyy");
    String convertedDateString;
    Number prevId = null;
    Number currId = null;    
    StringBuilder sBuild = new StringBuilder();
    StringBuilder sbLoop = new StringBuilder();    

    for(int j = 0; j < ids.length; j++)
    {

      Row row = getRow(ids[j]);
      currId  = (Number)row.getAttribute("OwnerId");      
      oName   = row.getAttribute("oName");
      cName = row.getAttribute("ContracttypeName");
      startDate    = row.getAttribute("StartDate");
      endDate      = row.getAttribute("EndDate");

      if(prevId != null && !currId.equals(prevId))
      {
        if(sbLoop.length() > 0)
        {
          sBuild.append(header);
          sBuild.append(sbLoop);
          sBuild.append(footer);
        }        
        _oCMap.put(prevId, sBuild); //HashMap declared at class level

        sBuild = new StringBuilder();
        sbLoop = new StringBuilder();
      }

      sbLoop.append(rowStart);
      if(oName != null)
      {
        sbLoop.append(columnStart + oName + columnEnd);
      }
      else
      {
        sbLoop.append(columnStart + columnEnd);
      }
      sbLoop.append(columnStart + cName + columnEnd);
      if(startDate != null)
      {
        convertedDateString = displayDateFormat.format(((Date)startDate).dateValue());
        sbLoop.append(columnStart + convertedDateString + columnEnd);
      }
      else
      {
        sbLoop.append(columnStart + columnEnd);
      }
      if(startDate != null && endDate != null)
      {
        sbLoop.append(columnStart + " t/m" + columnEnd);
      }
      else
      {
        sbLoop.append(columnStart + columnEnd);
      }
      if(endDate != null)
      {
        convertedDateString = displayDateFormat.format(((Date)endDate).dateValue());
        sbLoop.append(columnStart + convertedDateString + columnEnd);
      }
      else
      {
        sbLoop.append(columnStart + columnEnd);
      }
      sbLoop.append(rowEnd);
      prevId = currId;

      if(j ==  ownerContract.length - 1)  //code for putting into map for last id.
      {
        if(sbLoop.length() > 0)
        {
          sBuild.append(header);
          sBuild.append(sbLoop);
          sBuild.append(footer);
        }
        _oCMap.put(prevId, sBuild); //HashMap declared at class level
      }
    }
  }
share|improve this question
    
shall I move all the constant local variable to class level and declare them as static or its ok if I keep it there only... which one is the better way? –  eatSleepCode Apr 9 at 5:10

1 Answer 1

up vote 6 down vote accepted

Indentation
Java's indentation convention is 4 spaces, and not 2 as in your code. You may choose the align = signs between grouped lines, but if you do - be consistent - in your assignment block there are 3 different indentations...

Variables, constants and literals
Variables, which are dynamic and may change as the method progresses, should be named in camelCase, and should be declared as close to their use as possible (no need to declare them at the beginning of the method).
Constants, which do not change when the code is run, should be declared as constants outside the method, and be named in ALL_CAPS.
Personally I think that you should consider dropping the constants altogether, and use literals, since they are less ambiguous then using constants in this case:

sBuild.append("<html><table>");
sBuild.append(sbLoop);
sBuild.append("</table></html>");

reads better than:

sBuild.append(HEADER);
sBuild.append(sbLoop);
sBuild.append(FOOTER);

which reads better than:

sBuild.append(header);
sBuild.append(sbLoop);
sBuild.append(footer);

Make operations close to where they are used
Same as variable declaration, it is better to calculate a variable value close to where it is used:

Object oName = row.getAttribute("oName");
if(oName != null)
{
    sbLoop.append(columnStart + oName + columnEnd);
}
else
{
    sbLoop.append(columnStart + columnEnd);
}

Naming
You should give variable names which convey information about their purpose and usage. oName and cName are borderline, if they are part of the domain's nomenclature, but sBuild and sbLoop can definitely be improved.

Proper usage of StringBuilder
So, you've decided to use StringBuilder to optimize string concatenations. That's good, but then you use + to concatenate strings before you append them to your StringBuilder! #append method returns this exactly so you can avoid this:

sbLoop.append(columnStart).append(oName).append(columnEnd);

You use sBuild only to wrap your sbLoop with a header and a footer, I suggest you consider simply prepend sbLoop with the prefix, and append it the suffix without using a temp object:

if(prevId != null && !currId.equals(prevId))
{
    if(sbLoop.length() > 0)
    {
        sbLoop.insert(0, header);
        sbLoop.append(footer);
    }        
    _oCMap.put(prevId, sbLoop); //HashMap declared at class level

    sbLoop = new StringBuilder();
}

I also think that you should consider not save the StringBuilder in your map, but rather a toString() of it.

Less boilerplate
Your code has a repeating pattern which is something like this:

if(something != null)
{
    sbLoop.append(columnStart + something + columnEnd);
}
else
{
    sbLoop.append(columnStart + columnEnd);
}

I think a better pattern might be:

sbLoop.append(columnStart);
if(something != null)
{
    sbLoop.append(something);
}
sbLoop.append(columnEnd);
share|improve this answer
    
shall I move all the constant local variable to class level and declare them as static or its ok if I keep it there only... which one is the better way? –  eatSleepCode Apr 9 at 5:09
    
I believe you should inline most of them (simply use literals). Although it is less ubiquitous, you may use local constants intead of class constants, as long as you name them appropriately –  Uri Agassi Apr 9 at 6:24

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.