Creating map from ids

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 ids)
{
Object oName, cName, startDate, endDate;
String header = "";
String footer = "";
String rowStart = "";
String rowEnd = "";
String columnStart = " ";
String columnEnd = "";
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 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).dateVa

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("");
sBuild.append(sbLoop);
sBuild.append("");

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);

StackExchange Code Review Q#46533, answer score: 6