Last modified 16 years ago
Last modified on 01/12/09 19:57:30
Bad code samples
Here are some bad code examples (far from complete...) I have collected. Please check it, and ask if you don't understand why something is bad. This is important!
if (source instanceof RtfBookResource) {
RtfBookResource toCopy = (RtfBookResource) source;
copiedBookResource = toCopy.clone();
try {
copiedBookResource.setMetaInfo(toCopy.getMetaInfo());
} catch (Exception e) {
throw new RuntimeException(e);
}
target.resources().add(copiedBookResource);
}
if (source instanceof BufferedImageBookResource) {
BufferedImageBookResource toCopy = (BufferedImageBookResource) source;
copiedBookResource = toCopy.clone();
try {
copiedBookResource.setMetaInfo(toCopy.getMetaInfo());
} catch (Exception e) {
throw new RuntimeException(e);
}
target.resources().add(copiedBookResource);
}
if (source instanceof MediaBookResource) {
MediaBookResource toCopy = (MediaBookResource) source;
copiedBookResource = new MediaBookResource(toCopy.getName(), toCopy
.getMedia());
try {
copiedBookResource.setMetaInfo(toCopy.getMetaInfo());
} catch (Exception e) {
throw new RuntimeException(e);
}
target.resources().add(copiedBookResource);
}
Dont use deprecated API:
if(currentBook.getContainerFile()!=null) {
lastModified=new Date(currentBook.getContainerFile().lastModified()).toString();
bookSize=currentBook.getContainerFile().length();
}
Flag, int, etc are bad names:
boolean flag = true;
can be
boolean success = true;
or
boolean result = true;
Lots of array expose or redundancy
public Color[] HIGHLIGHT_COLORS = {Color.BLUE, Color.CYAN, Color.GREEN, Color.MAGENTA, Color.ORANGE, Color.PINK, Color.RED, Color.YELLOW};
Lots of lazy multi loading... in gui...
Bad JavaDoc - it should be focused on what is it doing, not how, or the details.
Initializes XXX - is bad for lazy loading setter.
readerMode is a bad name for checkbox...
Many people did not get the idea of the Logic...
public void actionPerformed(final ActionEvent arg0) {
final int currentPageIndex = currentPageIndex();
int pageToGo;
try {
pageToGo = Integer
.parseInt(arg0.getActionCommand());
} catch (final Exception e) {
// incorrect input
JOptionPane.showMessageDialog(
swingRepresentation(),
"Invalid input. Enter a number between 0 and "
+ model().get().pages().size(),
"Error", JOptionPane.ERROR_MESSAGE);
BookView.this.currentPageField().get().setText(
""
+ model().get().currentPage().get()
.getIndex());
return;
}
int maxPage = model().get().pages().size()
- (isInReaderMode() ? 1 : 0);
if ((pageToGo < 0) || pageToGo > maxPage) {
// out of bounds
JOptionPane.showMessageDialog(
swingRepresentation(),
"Invalid page number. Enter a number between 0 and "
+ maxPage, "Error",
JOptionPane.ERROR_MESSAGE);
pageToGo = currentPageIndex;
}
controller().get().toPage(pageToGo);
assert model().get().currentPage().get() != null;
BookView.this.currentPageField().get().setText(
""
+ model().get().currentPage().get()
.getIndex());
}
});
Splitting a big initialization into methods solves nothing!
public FrameView(final PageView parent, final Frame<?> frame,
final Class<? extends FrameController> subController) {
super(parent, frame, subController);
swingRepresentation();
initSizeAndLocation();
this.frameHaloMenu = new FrameHaloMenu(this);
this.frameHaloMenu.lockTo(swingRepresentation(),
RelativePosition.TOP_LEFT, RelativePosition.BOTTOM_LEFT);
detachHaloMenu();
initialize();
addListeners();
getParent().swingRepresentation().setLayer(swingRepresentation(),
getModel().getZorder());
getParent().swingRepresentation().add(swingRepresentation());
Logger.getRootLogger().info("Frame view created.");
updateLayer();
}
massive use of linked lists with no reason:
List<T> res = new LinkedList<T>();
lots of instance things that should be static (like arrays of data).
reader mode code was extreme....
Bad naming (and style)
public String setPropertiesText(Book currentBook)
Hack the model for the purpose of the view..
example - the zOrder + 1 hack
Some people should ask more... especially when they think something they are
going to do is going to get ugly.
Bad comments:
/**
* @author Presley
*
*/
is not nice comment for a class
Non-static nested classes dont need reference to outer class..
they have it implicitely
public BooksButton(final Book book,
final OpenBooksPalette openBooksPalette) {
super(book.getTitle());
No sub statements without {}
if (getTab().getFlap().getAppView().currentBookView().get() == null)
getTab().getFlap().getAppView().userNewBook();
Assign repeated expressions to locals variables
if (getTab().getFlap().getAppView().currentBookView().get() == null)
getTab().getFlap().getAppView().userNewBook();
int index = getList().locationToIndex(e.getPoint());
switch (index) {
case 0:
getTab().getFlap().getAppView().getCurrentChild()
.getCurrentChild().userDropFrame(x, y,
FrameKind.Text);
break;
case 1:
getTab().getFlap().getAppView().getCurrentChild()
.getCurrentChild().userDropFrame(x, y,
FrameKind.Image);
break;
case 2:
getTab().getFlap().getAppView().getCurrentChild()
.getCurrentChild().userDropFrame(x, y,
FrameKind.Media);
break;
default:
ErrorHandlingUtilities.showErrorMessageDialog(
getList(),
"Error while trying to create frame.",
"Frames Palette Error");
break;
}
Sequence of ifs, or switch is a bad thing. It is strange that
someone created class ComponentsPaletteItem, but instead moving
the functionallity there, had things like:
case 0:
curPage.userDropFrame(x, y, FrameKind.Text);
break;
case 1:
curPage.userDropFrame(x, y, FrameKind.Image);
break;
case 2:
curPage.userDropFrame(x, y, FrameKind.Media);
break;
Do not inherit, unless really necessary. Composition is usually better...
public class ComponentsPaletteItem implements Transferable
ListPalette was refactored... for good. ListPaletteItem finally introduced
Search is refactored.
BookTemplates are refactored.
If you need the old code -- 2423 is the last stable revision.
create methods are supposed to create something (and return it)
private void createList(){
list().get().setSelectionMode(ListSelectionModel.SINGLE_SELECTION);
createModel();
list().get().setCellRenderer(new ImageRenderer());
list().get().setDragEnabled(true);
list().get().setTransferHandler(new PageTemplatesTransferHandler());
}
-> initList is better name
All menus were refactored... Now they are in the MainMenuBar class instead
the AppView.
FileChoser instance moved to AppView
ImageFrameView - we need to discuss the strange painting
This final everywhere is not needed. It is ok (and good) for
fields, but for local variable and arguments (which are not
used in inner classes) it is too much.
Avoid code duplication... it leads to out of date copies:
} catch (Exception e) {
// incorrect input
JOptionPane.showMessageDialog(bookView.swingFrame().get(),
"Invalid input. Enter a number between 1 and "
+ bookView.model().get().pages().size(),
"Error", JOptionPane.ERROR_MESSAGE);
bookView.navigationPanel().get().currentPageField().get().setText("" + currentPageIndex);
return;
}
int maxPage = bookView().get().model().get().pages().size()
- (bookView().get().inReaderMode().get() ? 1 : 0);
if ((pageToGo < 0) || pageToGo > maxPage) {
// out of bounds
JOptionPane.showMessageDialog(bookView().get().swingFrame().get(),
"Invalid page number. Enter a number between 0 and "
+ maxPage, "Error", JOptionPane.ERROR_MESSAGE);
pageToGo = currentPageIndex;
}
Don not prefix the static members related to the class... they are already scoped:
bad (class DecreaseBorderButton):
private static final String DECREASE_BORDER_TOOL_TIP = "Decrease the border of this frame.";
better (class DecreaseBorderButton):
private static final String TOOL_TIP = "Decrease the border of this frame.";
Comments
- A separate code box for each sample would make reading much easier. Are there no new code smells for the last 4 months? We should update this document more often or drop it at all. --boyan@2009-01-12
