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> ";
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
}
}
}
-
\$\begingroup\$ 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? \$\endgroup\$eatSleepCode– eatSleepCode2014年04月09日 05:10:04 +00:00Commented Apr 9, 2014 at 5:10
1 Answer 1
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);
-
\$\begingroup\$ 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? \$\endgroup\$eatSleepCode– eatSleepCode2014年04月09日 05:09:41 +00:00Commented Apr 9, 2014 at 5:09
-
\$\begingroup\$ 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 \$\endgroup\$Uri Agassi– Uri Agassi2014年04月09日 06:24:00 +00:00Commented Apr 9, 2014 at 6:24