A recent post in the Struts forum asked a question about frames, which ends up being the least of the problems. The code snippet posted looked like this, spacing preserved:
<body>
<table width="100%" border="0" cellspacing="0" cellpadding="0">
<tr class="topbanner">
<td height="81">
<%
String contextPath = (String)request.getContextPath();
IDfCollection cabinetList = (IDfCollection)session.getAttribute("allCabinets");
try{
DocumentBuilderFactory docBuilderFactory = DocumentBuilderFactory.newInstance();
DocumentBuilder docBuilder = docBuilderFactory.newDocumentBuilder();
Document doc = docBuilder.parse (new File("tree.xml"));
doc.getDocumentElement().normalize();
NodeList listOfCabinets = doc.getElementsByTagName("branch");
int noOfCabinets = listOfCabinets.getLength();
for(int i=0; i<noOfCabinets; i++){
Node cabinet = listOfCabinets.item(i);
NamedNodeMap attributeMap = cabinet.getAttributes();
for(int j=0; j<attributeMap.getLength(); j++){
Node attribute = attributeMap.item(j);
if(attribute.getNodeName().equalsIgnoreCase("id")){
String value = attribute.getNodeValue();
%>
<tr bgcolor="#CCD3D9">
<td height="1" colspan="2" class="helptextbold2">
<div class="trigger" onclick="javascript:showBranch(<%=value%>);swapFolder(<%=value%>)" id="<%=value%>">
<img src="images/closed.gif" border="0">
<table align="left" width="20%" border="0" cellspacing="0" cellpadding="0">
<tr>
<td bgcolor="white">
<html:frame>
<a
target="<%=contextPath%>/DocumentManagement.do?cabinetName=<%=value%>&methodName=getCabinetContents"
onclick="javascript:showBranch(<%=value%>);swapFolder(<%=value%>)">
<%=value%>
</a>
</html:frame>
<%
}
}
}
%>
</div>
<br>
</td></tr></table>
I wonder about the environment and mindset that allows code like this to exist. I'm not (merely) singling out the person that asked the original question; I see code like this all the time. I do, however, have problems understanding how, in 2009, there exist places where code like this can be written. Where are the mentors? Where are the examples that beat code like this into a bloody pulp and make even the thought of writing it unimaginable?
Code written by people that care (almost!) never looks like this. Attention to detail pervades well-written code: take indentation, for example. I don't know of any editors that make indentation impossible. Indentation matters: it's a drop-dead simple way of indicating structure and hierarchy. Indentation can reveal flaws in structure and hierarchy, the snippet above having several. Incorrect indentation is misleading, impedes understanding, and creates unnecessary cognitive overhead. Indentation doesn't matter to the compiler: a valid point--but it matters to *people*.
The indentation isn't the most disturbing part of the code above--once I corrected the indentation (as best as I could, considering the snippet's incompleteness) the real killer is the chunk of XML processing code sitting in the JSP. Ignore that it's parsing an XML file on every page load, ignore that it's iterating over a *map* to retrieve a single mapped value (the id).
Come on: even apps that used servlets and JSP can be written to remove the need for this kind of programming. And this is *Struts* (Struts 1, but even still)... why bother using Struts at all if this is the code you're going to write?
Do the XML processing in a *real* Java file, where it belongs. Put it in a service object so when caching is implemented all you need to do is change the service implementation used by the action.
Even *Java* has mechanisms that allow (relatively, let's be realistic here) concise JSP. Hide the inner per-cabinet HTML in a JSP-based custom tag. Use JSTL. Use JSP EL. Drop the monstrosity above down to this:
<table class="cabinets">
<tr>
<td>
<c:forEach var="cabinetName" items="${cabinetNames}">
<app:cabinetInfo cabinetName="${cabinetName}"/>
</c:forEach>
</td>
</tr>
</table>
I rag on Java a *lot*, and I have reasons to--but this is 2009, and people writing the same kind of apps that people were writing circa 1998-2000 is inexcusable. In what company would the original example have survived even a *minimal* code review? It's not even acceptable for a rapid prototype, at least nowhere that I've been associated with. So where did the breakdown occur? Is it this particular developer? No, because code like this exists all over the place. That said, *good* developers strive to understand and work with the environment they're in, and use it to the best advantage possible.
Is it the company the developer works for? *Somebody* approved this code. Yes, the approver might have been the original developer--but that leads to a host of associated problems. Code reviews should be an integral part of a good development process. Mentoring can be part of, or parallel to, a code review process. This code should not have existed for more than a single checkin.
Is it the sample code found in books and the internet? Let's be honest--book code is highly edited for space and educational purposes. Internet code is largely the same, although some sample code is clearly better than others. "Graduated" code samples, where the code is drillable, with the basics covered first but that allow access to a complete code sample (you know, error handling, I18N, whatever) should be encouraged. We can't make people *use* good code, but we can get it in people's faces much, much more than we are now.
Some thing, or things, are broken when code like this is posted and the question is anything other than "How can I make this code tolerable, readable, extensible, and debuggable?" I don't know how to fix it, or to make people indent their code, or to convert all existing examples to be more instructive about something other than just the laser-focused point the example is trying to make--but I know that code like the original snippet makes me wonder about several steps in the coding process at at least one programming house, and it makes me want to make sure that my code, and the code of places I'm associated with, never looks like it.
No comments:
Post a Comment