| | 1 | = Bad code samples = |
| | 2 | |
| | 3 | 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! |
| | 4 | {{{ |
| | 5 | |
| | 6 | if (source instanceof RtfBookResource) { |
| | 7 | RtfBookResource toCopy = (RtfBookResource) source; |
| | 8 | copiedBookResource = toCopy.clone(); |
| | 9 | try { |
| | 10 | copiedBookResource.setMetaInfo(toCopy.getMetaInfo()); |
| | 11 | } catch (Exception e) { |
| | 12 | throw new RuntimeException(e); |
| | 13 | } |
| | 14 | target.resources().add(copiedBookResource); |
| | 15 | } |
| | 16 | if (source instanceof BufferedImageBookResource) { |
| | 17 | BufferedImageBookResource toCopy = (BufferedImageBookResource) source; |
| | 18 | copiedBookResource = toCopy.clone(); |
| | 19 | try { |
| | 20 | copiedBookResource.setMetaInfo(toCopy.getMetaInfo()); |
| | 21 | } catch (Exception e) { |
| | 22 | throw new RuntimeException(e); |
| | 23 | } |
| | 24 | target.resources().add(copiedBookResource); |
| | 25 | } |
| | 26 | if (source instanceof MediaBookResource) { |
| | 27 | MediaBookResource toCopy = (MediaBookResource) source; |
| | 28 | copiedBookResource = new MediaBookResource(toCopy.getName(), toCopy |
| | 29 | .getMedia()); |
| | 30 | try { |
| | 31 | copiedBookResource.setMetaInfo(toCopy.getMetaInfo()); |
| | 32 | } catch (Exception e) { |
| | 33 | throw new RuntimeException(e); |
| | 34 | } |
| | 35 | target.resources().add(copiedBookResource); |
| | 36 | } |
| | 37 | |
| | 38 | |
| | 39 | Dont use deprecated API: |
| | 40 | if(currentBook.getContainerFile()!=null) { |
| | 41 | lastModified=new Date(currentBook.getContainerFile().lastModified()).toString(); |
| | 42 | bookSize=currentBook.getContainerFile().length(); |
| | 43 | } |
| | 44 | |
| | 45 | |
| | 46 | Flag, int, etc are bad names: |
| | 47 | boolean flag = true; |
| | 48 | can be |
| | 49 | boolean success = true; |
| | 50 | or |
| | 51 | boolean result = true; |
| | 52 | |
| | 53 | Lots of array expose or redundancy |
| | 54 | public Color[] HIGHLIGHT_COLORS = {Color.BLUE, Color.CYAN, Color.GREEN, Color.MAGENTA, Color.ORANGE, Color.PINK, Color.RED, Color.YELLOW}; |
| | 55 | |
| | 56 | Lots of lazy multi loading... in gui... |
| | 57 | |
| | 58 | Bad JavaDoc - it should be focused on what is it doing, not how, or the details. |
| | 59 | Initializes XXX - is bad for lazy loading setter. |
| | 60 | |
| | 61 | readerMode is a bad name for checkbox... |
| | 62 | |
| | 63 | |
| | 64 | |
| | 65 | Many people did not get the idea of the Logic... |
| | 66 | public void actionPerformed(final ActionEvent arg0) { |
| | 67 | |
| | 68 | final int currentPageIndex = currentPageIndex(); |
| | 69 | int pageToGo; |
| | 70 | |
| | 71 | try { |
| | 72 | pageToGo = Integer |
| | 73 | .parseInt(arg0.getActionCommand()); |
| | 74 | } catch (final Exception e) { |
| | 75 | // incorrect input |
| | 76 | JOptionPane.showMessageDialog( |
| | 77 | swingRepresentation(), |
| | 78 | "Invalid input. Enter a number between 0 and " |
| | 79 | + model().get().pages().size(), |
| | 80 | "Error", JOptionPane.ERROR_MESSAGE); |
| | 81 | BookView.this.currentPageField().get().setText( |
| | 82 | "" |
| | 83 | + model().get().currentPage().get() |
| | 84 | .getIndex()); |
| | 85 | return; |
| | 86 | } |
| | 87 | |
| | 88 | int maxPage = model().get().pages().size() |
| | 89 | - (isInReaderMode() ? 1 : 0); |
| | 90 | |
| | 91 | if ((pageToGo < 0) || pageToGo > maxPage) { |
| | 92 | // out of bounds |
| | 93 | JOptionPane.showMessageDialog( |
| | 94 | swingRepresentation(), |
| | 95 | "Invalid page number. Enter a number between 0 and " |
| | 96 | + maxPage, "Error", |
| | 97 | JOptionPane.ERROR_MESSAGE); |
| | 98 | pageToGo = currentPageIndex; |
| | 99 | } |
| | 100 | |
| | 101 | controller().get().toPage(pageToGo); |
| | 102 | |
| | 103 | assert model().get().currentPage().get() != null; |
| | 104 | BookView.this.currentPageField().get().setText( |
| | 105 | "" |
| | 106 | + model().get().currentPage().get() |
| | 107 | .getIndex()); |
| | 108 | } |
| | 109 | }); |
| | 110 | |
| | 111 | |
| | 112 | |
| | 113 | Splitting a big initialization into methods solves nothing! |
| | 114 | public FrameView(final PageView parent, final Frame<?> frame, |
| | 115 | final Class<? extends FrameController> subController) { |
| | 116 | super(parent, frame, subController); |
| | 117 | |
| | 118 | swingRepresentation(); |
| | 119 | |
| | 120 | initSizeAndLocation(); |
| | 121 | |
| | 122 | this.frameHaloMenu = new FrameHaloMenu(this); |
| | 123 | |
| | 124 | this.frameHaloMenu.lockTo(swingRepresentation(), |
| | 125 | RelativePosition.TOP_LEFT, RelativePosition.BOTTOM_LEFT); |
| | 126 | |
| | 127 | detachHaloMenu(); |
| | 128 | |
| | 129 | initialize(); |
| | 130 | addListeners(); |
| | 131 | |
| | 132 | getParent().swingRepresentation().setLayer(swingRepresentation(), |
| | 133 | getModel().getZorder()); |
| | 134 | getParent().swingRepresentation().add(swingRepresentation()); |
| | 135 | |
| | 136 | Logger.getRootLogger().info("Frame view created."); |
| | 137 | |
| | 138 | updateLayer(); |
| | 139 | } |
| | 140 | |
| | 141 | massive use of linked lists with no reason: |
| | 142 | List<T> res = new LinkedList<T>(); |
| | 143 | |
| | 144 | lots of instance things that should be static (like arrays of data). |
| | 145 | |
| | 146 | reader mode code was extreme.... |
| | 147 | |
| | 148 | Bad naming (and style) |
| | 149 | public String setPropertiesText(Book currentBook) |
| | 150 | |
| | 151 | Hack the model for the purpose of the view.. |
| | 152 | example - the zOrder + 1 hack |
| | 153 | |
| | 154 | Some people should ask more... especially when they think something they are |
| | 155 | going to do is going to get ugly. |
| | 156 | |
| | 157 | Bad comments: |
| | 158 | /** |
| | 159 | * @author Presley |
| | 160 | * |
| | 161 | */ |
| | 162 | is not nice comment for a class |
| | 163 | |
| | 164 | Non-static nested classes dont need reference to outer class.. |
| | 165 | they have it implicitely |
| | 166 | public BooksButton(final Book book, |
| | 167 | final OpenBooksPalette openBooksPalette) { |
| | 168 | super(book.getTitle()); |
| | 169 | |
| | 170 | No sub statements without {} |
| | 171 | if (getTab().getFlap().getAppView().currentBookView().get() == null) |
| | 172 | getTab().getFlap().getAppView().userNewBook(); |
| | 173 | |
| | 174 | |
| | 175 | Assign repeated expressions to locals variables |
| | 176 | if (getTab().getFlap().getAppView().currentBookView().get() == null) |
| | 177 | getTab().getFlap().getAppView().userNewBook(); |
| | 178 | int index = getList().locationToIndex(e.getPoint()); |
| | 179 | switch (index) { |
| | 180 | case 0: |
| | 181 | getTab().getFlap().getAppView().getCurrentChild() |
| | 182 | .getCurrentChild().userDropFrame(x, y, |
| | 183 | FrameKind.Text); |
| | 184 | break; |
| | 185 | case 1: |
| | 186 | getTab().getFlap().getAppView().getCurrentChild() |
| | 187 | .getCurrentChild().userDropFrame(x, y, |
| | 188 | FrameKind.Image); |
| | 189 | break; |
| | 190 | case 2: |
| | 191 | getTab().getFlap().getAppView().getCurrentChild() |
| | 192 | .getCurrentChild().userDropFrame(x, y, |
| | 193 | FrameKind.Media); |
| | 194 | break; |
| | 195 | default: |
| | 196 | ErrorHandlingUtilities.showErrorMessageDialog( |
| | 197 | getList(), |
| | 198 | "Error while trying to create frame.", |
| | 199 | "Frames Palette Error"); |
| | 200 | break; |
| | 201 | } |
| | 202 | |
| | 203 | Sequence of ifs, or switch is a bad thing. It is strange that |
| | 204 | someone created class ComponentsPaletteItem, but instead moving |
| | 205 | the functionallity there, had things like: |
| | 206 | case 0: |
| | 207 | curPage.userDropFrame(x, y, FrameKind.Text); |
| | 208 | break; |
| | 209 | case 1: |
| | 210 | curPage.userDropFrame(x, y, FrameKind.Image); |
| | 211 | break; |
| | 212 | case 2: |
| | 213 | curPage.userDropFrame(x, y, FrameKind.Media); |
| | 214 | break; |
| | 215 | |
| | 216 | Do not inherit, unless really necessary. Composition is usually better... |
| | 217 | public class ComponentsPaletteItem implements Transferable |
| | 218 | |
| | 219 | |
| | 220 | ListPalette was refactored... for good. ListPaletteItem finally introduced |
| | 221 | |
| | 222 | Search is refactored. |
| | 223 | |
| | 224 | BookTemplates are refactored. |
| | 225 | |
| | 226 | If you need the old code -- 2423 is the last stable revision. |
| | 227 | |
| | 228 | create methods are supposed to create something (and return it) |
| | 229 | private void createList(){ |
| | 230 | list().get().setSelectionMode(ListSelectionModel.SINGLE_SELECTION); |
| | 231 | createModel(); |
| | 232 | list().get().setCellRenderer(new ImageRenderer()); |
| | 233 | list().get().setDragEnabled(true); |
| | 234 | list().get().setTransferHandler(new PageTemplatesTransferHandler()); |
| | 235 | } |
| | 236 | |
| | 237 | -> initList is better name |
| | 238 | |
| | 239 | All menus were refactored... Now they are in the MainMenuBar class instead |
| | 240 | the AppView. |
| | 241 | |
| | 242 | FileChoser instance moved to AppView |
| | 243 | |
| | 244 | ImageFrameView - we need to discuss the strange painting |
| | 245 | |
| | 246 | This final everywhere is not needed. It is ok (and good) for |
| | 247 | fields, but for local variable and arguments (which are not |
| | 248 | used in inner classes) it is too much. |
| | 249 | |
| | 250 | Avoid code duplication... it leads to out of date copies: |
| | 251 | } catch (Exception e) { |
| | 252 | // incorrect input |
| | 253 | JOptionPane.showMessageDialog(bookView.swingFrame().get(), |
| | 254 | "Invalid input. Enter a number between 1 and " |
| | 255 | + bookView.model().get().pages().size(), |
| | 256 | "Error", JOptionPane.ERROR_MESSAGE); |
| | 257 | bookView.navigationPanel().get().currentPageField().get().setText("" + currentPageIndex); |
| | 258 | return; |
| | 259 | } |
| | 260 | |
| | 261 | int maxPage = bookView().get().model().get().pages().size() |
| | 262 | - (bookView().get().inReaderMode().get() ? 1 : 0); |
| | 263 | |
| | 264 | if ((pageToGo < 0) || pageToGo > maxPage) { |
| | 265 | // out of bounds |
| | 266 | JOptionPane.showMessageDialog(bookView().get().swingFrame().get(), |
| | 267 | "Invalid page number. Enter a number between 0 and " |
| | 268 | + maxPage, "Error", JOptionPane.ERROR_MESSAGE); |
| | 269 | pageToGo = currentPageIndex; |
| | 270 | } |
| | 271 | |
| | 272 | |
| | 273 | |
| | 274 | |
| | 275 | Don not prefix the static members related to the class... they are already scoped: |
| | 276 | bad (class DecreaseBorderButton): |
| | 277 | |
| | 278 | private static final String DECREASE_BORDER_TOOL_TIP = "Decrease the border of this frame."; |
| | 279 | better (class DecreaseBorderButton): |
| | 280 | private static final String TOOL_TIP = "Decrease the border of this frame."; |
| | 281 | |
| | 282 | |
| | 283 | }}} |
