wiki:CODE_SMELLS
Last modified 11 years ago Last modified on 01/12/09 19:57:30

Error: Macro BackLinksMenu(None) failed
compressed data is corrupt

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