| 38 | | 1. Work towards the goal! |
| 39 | | * No matter what this document defines, or how concrete your task is defined, there is no way to define exactly what should be good for our goal, and what should be bad. |
| 40 | | * You have to work towards the goal. |
| 41 | | * When you work on something, you have to understand why and how it is related to our goal. |
| 42 | | * You have to try to drive the whole team towards the goal (no one alone is capable of reaching it) |
| 43 | | 2. Be honest! |
| 44 | | * You have to present the situation to the leader as is. |
| 45 | | * Even if it does not look good. |
| 46 | | * For example, if you have no idea how to make something, don't tell "almost done" to the leader. |
| 47 | | 3. Be initiative! |
| 48 | | * If you find a way that, you can make something good to the team, don't wait, but propose it to be done. |
| 49 | | 4. Look around! |
| 50 | | * If you see problems in the code, in the product, in the organization, or whatever, announce them. |
| 51 | | * Try to keep an eye about what other people do (either for things you can learn from, or for things they are doing bad). |
| 52 | | * Try to get a global vision of what is happening in our team. |
| 53 | | 5. Seek improvements (for yourself, and for the team) |
| 54 | | * No matter how good you are, there are always more things you can learn, or skills you can get (note that knowledge and skills are different things..) |
| 55 | | * Improving and learning is your responsibility. |
| 56 | | * Not trying to understanding how things work, but learning that this does this, and that does that will lead you nowhere! Really! Even if you program for 20 years. I've seen such people. |
| 57 | | 6. Seek the most appropriate solution!. |
| 58 | | * There is no perfect code. There is no perfect solution also (or maybe there is, but it is not reachable). |
| 59 | | * There are no general good design rules - a design is good if it works well, it is easy to use (like if it is library, the clients need not to write much), is simple, has good performance, is modifiable/extensible, testable, etc. Nor the number of design patterns used solves this, nor a code convention... nothing. You have to find a design that is good. |
| 60 | | * Hacking around just to make things work is usually a time bomb. It is not acceptable to do this, because saving 1 day this month, usually costs 7 days the next month. |
| 61 | | * What we should do, is seeking something between just make it work, and perfect, but more close to perfect than just works. |
| 62 | | * That is, seek a good solution. |
| 63 | | 7. Quality is not negotiable! - but scope and resources are |
| 64 | | * If something can not be done in acceptable quality, given the resources (time) and scope (functionality) it has, the we either reduce the scope, or increase the resources. |
| 65 | | * We are not allowed to reduce the quality! |
| 66 | | 8. Accept the challenge! |
| 67 | | * We are not doing the next Store Information System, the next Lawyer's web site, nor the next database module for the big company X |
| 68 | | * What we are doing, is something that only few companies in the world can do, and even they are not good enough. |
| 69 | | * There is no "How to make the next generation desktop publishing software"! |
| 70 | | |
| 71 | | [edit] |
| 72 | | Discipline |
| 73 | | |
| 74 | | 1. You have to try to follow the process as much as possible. |
| 75 | | * Self discipline is needed. |
| 76 | | * If you don't have enough self discipline, then this team is not appropriate for you (you should go to a company which forces you to be disciplined by other means). |
| 77 | | 2. By default, our work-time for half-time working (all of us currently) is 11:00 to 15:00. |
| 78 | | * If you need to shift it, or not work some days, but work more other days, you need to warn the leader, or the appropriate person. |
| 79 | | * TODO: maybe specify that you can not shift more than the half of the working time? |
| 80 | | 3. By default you have to make 20 working hours per week. |
| 81 | | * This means 20 productive work hour. |
| 82 | | * Not 20 hours staying in the office. |
| 83 | | * You may stay at the office as much as you like, take breaks, study, play games, whatever, but you should make 20 productive hours (working on a task). |
| 84 | | 4. You may work from home, or from other locations, but unless you have approval from the leader (like when we had no electricity), at least 3/4 of your time you should work from the office. |
| 85 | | 5. Unless having approval from the leader, you may not work more than 8 hours per day. |
| | 22 | 1. Work towards the goal! |
| | 23 | * No matter what this document defines, or how concrete your task is defined, there is no way to define exactly what should be good for our goal, and what should be bad. |
| | 24 | * You have to work towards the goal. |
| | 25 | * When you work on something, you have to understand why and how it is related to our goal. |
| | 26 | * You have to try to drive the whole team towards the goal (no one alone is capable of reaching it) |
| | 27 | 2. Be honest! |
| | 28 | * You have to present the situation to the leader as is. |
| | 29 | * Even if it does not look good. |
| | 30 | * For example, if you have no idea how to make something, don't tell "almost done" to the leader. |
| | 31 | 3. Be initiative! |
| | 32 | * If you find a way that, you can make something good to the team, don't wait, but propose it to be done. |
| | 33 | 4. Look around! |
| | 34 | * If you see problems in the code, in the product, in the organization, or whatever, announce them. |
| | 35 | * Try to keep an eye about what other people do (either for things you can learn from, or for things they are doing bad). |
| | 36 | * Try to get a global vision of what is happening in our team. |
| | 37 | 5. Seek improvements (for yourself, and for the team) |
| | 38 | * No matter how good you are, there are always more things you can learn, or skills you can get (note that knowledge and skills are different things..) |
| | 39 | * Improving and learning is your responsibility. |
| | 40 | * Not trying to understanding how things work, but learning that this does this, and that does that will lead you nowhere! Really! Even if you program for 20 years. I've seen such people. |
| | 41 | 6. Seek the most appropriate solution!. |
| | 42 | * There is no perfect code. There is no perfect solution also (or maybe there is, but it is not reachable). |
| | 43 | * There are no general good design rules - a design is good if it works well, it is easy to use (like if it is library, the clients need not to write much), is simple, has good performance, is modifiable/extensible, testable, etc. Nor the number of design patterns used solves this, nor a code convention... nothing. You have to find a design that is good. |
| | 44 | * Hacking around just to make things work is usually a time bomb. It is not acceptable to do this, because saving 1 day this month, usually costs 7 days the next month. |
| | 45 | * What we should do, is seeking something between just make it work, and perfect, but more close to perfect than just works. |
| | 46 | * That is, seek a good solution. |
| | 47 | 7. Quality is not negotiable! - but scope and resources are |
| | 48 | * If something can not be done in acceptable quality, given the resources (time) and scope (functionality) it has, the we either reduce the scope, or increase the resources. |
| | 49 | * We are not allowed to reduce the quality! |
| | 50 | 8. Accept the challenge! |
| | 51 | * We are not doing the next Store Information System, the next Lawyer's web site, nor the next database module for the big company X |
| | 52 | * What we are doing, is something that only few companies in the world can do, and even they are not good enough. |
| | 53 | * There is no "How to make the next generation desktop publishing software"! |
| | 54 | |
| | 55 | |
| | 56 | '''Discipline''' |
| | 57 | |
| | 58 | 1. You have to try to follow the process as much as possible. |
| | 59 | * Self discipline is needed. |
| | 60 | * If you don't have enough self discipline, then this team is not appropriate for you (you should go to a company which forces you to be disciplined by other means). |
| | 61 | 2. By default, our work-time for half-time working (all of us currently) is 11:00 to 15:00. |
| | 62 | * If you need to shift it, or not work some days, but work more other days, you need to warn the leader, or the appropriate person. |
| | 63 | * you can not shift more than the half of the working time |
| | 64 | 3. By default you have to make 20 working hours per week. |
| | 65 | * This means 20 productive work hour not 20 hours staying in the office. |
| | 66 | * You may stay at the office as much as you like, take breaks, study, play games, whatever, but you should make 20 productive hours (working on a task). |
| | 67 | 4. You may work from home, or from other locations, but unless you have approval from the leader (like when we had no electricity), at least 3/4 of your time you should work from the office. |
| | 68 | 5. Unless having approval from the leader, you may not work more than 8 hours per day. |
| 133 | | * analyzing - task is waiting to be taken, and someone to determine What should be done? |
| 134 | | o transitions: |
| 135 | | + to designing when all the post-conditions are met |
| 136 | | + to analyzing when post-conditions are not met |
| 137 | | |
| 138 | | * designing - The implementor is designing it, that is, tries to decide How it should be done? |
| 139 | | o pre-conditions: |
| 140 | | + the post-conditions of analyzing |
| 141 | | o work: |
| 142 | | + designing is not only getting an idea of how to do it, but a detailed concept, |
| 143 | | + for many tasks the design phase should take most of the time |
| 144 | | o post-conditions - see below |
| 145 | | o transitions: |
| 146 | | + to implementing if the reviewers decide, that the task is designed well |
| 147 | | + to designing if the reviewers decide, that the task needs better design, or does not satisfy the requirements |
| 148 | | + to analyzing if the reviewers or implementors decide that the task can not be done in acceptable time or have no idea how to do it |
| 149 | | |
| 150 | | * implementing - The implementor makes an attempt to complete the task. |
| 151 | | o pre-conditions: |
| 152 | | + the post-conditions of designing |
| 153 | | * post-conditions - see below |
| 154 | | o transitions: |
| 155 | | + to done if the reviewers decide, that there are no any issues. |
| 156 | | + to done if |
| 157 | | # the reviewers decide, that there are very minor issues which should be fixed later |
| 158 | | # the issues are reported as bugs (or something) |
| 159 | | # the leader agrees on this |
| 160 | | # *note: it is usually better to face the reality, that it is just not done. |
| 161 | | + to implementing if the reviewers decide it does not fully satisfies the requirements |
| 162 | | + to designing if the reviewers decide, that the initial design will not do the work, and needs to be fixed. |
| 163 | | + to analyzing if the reviewers decide, that the task can not be done in acceptable time or quality. |
| 164 | | |
| 165 | | * done |
| 166 | | o when the task is really done |
| 167 | | |
| 168 | | |
| 169 | | * *Note that, failing a state (for example moving from implementing to analyzing because implementing failed) requires that the state of the product is reversed to the initial. |
| | 112 | * '''analyzing''' - task is waiting to be taken, and someone to determine What should be done? |
| | 113 | * to learn how to write good analysis, see [wiki:PLATFORM_STANDARDS_ANALYSIS_R0 Platform Standards Analysis] |
| | 114 | * transitions: |
| | 115 | * to designing when all the post-conditions are met |
| | 116 | * to analyzing when post-conditions are not met |
| | 117 | |
| | 118 | * '''designing''' - The implementer is designing it, that is, tries to decide How it should be done? |
| | 119 | * pre-conditions: |
| | 120 | * the post-conditions of analyzing |
| | 121 | * work: |
| | 122 | * designing is not only getting an idea of how to do it, but a detailed concept, |
| | 123 | * for many tasks the design phase should take most of the time |
| | 124 | * post-conditions - see below |
| | 125 | * transitions: |
| | 126 | * implementing if the reviewers decide, that the task is designed well |
| | 127 | * designing if the reviewers decide, that the task needs better design, or does not satisfy the requirements |
| | 128 | * analyzing if the reviewers or implementers decide that the task can not be done in acceptable time or have no idea how to do it |
| | 129 | |
| | 130 | * '''implementing''' - The implementer makes an attempt to complete the task. |
| | 131 | * pre-conditions: |
| | 132 | * the post-conditions of designing |
| | 133 | * post-conditions - see below |
| | 134 | * transitions: |
| | 135 | * done if |
| | 136 | * the reviewers decide, that there are no any issues. |
| | 137 | * the reviewers decide, that there are very minor issues which should be fixed later |
| | 138 | * the issues are reported as bugs (or something) |
| | 139 | * the leader agrees on this |
| | 140 | * *note: it is usually better to face the reality, that it is just not done. |
| | 141 | * implementing if the reviewers decide that the requirements are not fully satisfied |
| | 142 | * designing if the reviewers decide, that the initial design will not do the work, and needs to be fixed. |
| | 143 | * analyzing if the reviewers decide, that the task can not be done in acceptable time or quality. |
| | 144 | |
| | 145 | * '''done''' |
| | 146 | * when the task is really done |
| | 147 | |
| | 148 | * '''testing''' |
| | 149 | * pre-conditions: |
| | 150 | * the task is in '''done''' phase |
| | 151 | * post-conditions: |
| | 152 | * transitions: |
| | 153 | * done if |
| | 154 | * the reviewers decide, that there are no more tests to be added |
| | 155 | * the reviewers decide, that minor missing tests can be added later |
| | 156 | * the leader agrees on this |
| | 157 | * implementing if the reviewers decide that the requirements are not fully satisfied |
| | 158 | * designing if the reviewers decide, that the current tests need to be fixed |
| | 159 | * analyzing if the reviewers decide, that the task can not be done in acceptable time or quality. |
| | 160 | |
| | 161 | * *Note that, failing a state (for example moving from implementing to analyzing because implementing failed) requires that the state of the product is reversed to the initial. |
| 185 | | * code |
| 186 | | o The code style is correct to our convention. |
| 187 | | o The code is easily readable. |
| 188 | | o There are no fake JavaDocs (JavaDocs without useful information). |
| 189 | | o The java-doc is complete enough. |
| 190 | | + For each element, the JavaDoc should describe clearly what is it (if this is not easy, then probably the design is bad) |
| 191 | | o It causes 0 errors. |
| 192 | | o It causes 0 warnings. |
| 193 | | o The @SuppressWarnings annotation is used only if really needed. |
| 194 | | + I know 3 cases for it - |
| 195 | | # unused, when something is used only by reflection |
| 196 | | # synthetic-access, when you want to touch something internal with inner class (although in many cases this may be avoided) |
| 197 | | # unchecked, when you are doing something complex with generic (note that this is also avoidable in many cases) |
| 198 | | o No inaccurate, or unclear XXX and TODO tags |
| 199 | | o It should not break other code (ensured by their tests). |
| 200 | | o other suggestions? |
| 201 | | |
| 202 | | |
| 203 | | * design |
| 204 | | o It is documented |
| 205 | | + (not only in the code). |
| 206 | | o It is simple and understandable. |
| 207 | | o It is easy to use. |
| 208 | | + (using it requires less work / small code) |
| 209 | | o It is error proof (hard to use it in a wrong way) |
| 210 | | + The compiler prevents most errors. |
| 211 | | # (it is better to have an abstract getKind() instead of forcing implementors to call setKind at construction) |
| 212 | | # (final may also help) |
| 213 | | + Most of other errors cause an exception |
| 214 | | # (defensive programming helps here) |
| 215 | | o When a library is used, it is used correctly. |
| 216 | | o There are no representation exposures. |
| 217 | | o There are no GOD (very long) methods. |
| 218 | | o There are no GOD (very long) classes. |
| 219 | | o The coupling is low. |
| 220 | | o No magic numbers (and strings) |
| 221 | | o No array members. |
| 222 | | + (unless there is really a reason) |
| 223 | | o No public instance fields. |
| 224 | | o No public non final fields |
| 225 | | o It has high cohesion. |
| 226 | | + (see http://en.wikipedia.org/wiki/Cohesion_(computer_science) ) |
| 227 | | o It has low coupling. |
| 228 | | + (see http://en.wikipedia.org/wiki/Coupling_%28computer_science%29 ) |
| 229 | | o It is possible to state in one sentence what every class or member is/does. |
| 230 | | o Classes and members are named correctly. |
| 231 | | o Every class (or attribute) is self responsible and self complete. |
| 232 | | o No code duplication (copy paste). |
| 233 | | o It is easily testable. |
| 234 | | + (when not, testing helpers are provided to make it such). |
| 235 | | o No other kind of smells. |
| 236 | | * It has good package structure. |
| 237 | | * It is logging correctly. |
| 238 | | * It has adequate error handling. |
| 239 | | * No premature optimization. |
| 240 | | * No premature abstraction. |
| 241 | | |
| 242 | | * tests |
| 243 | | o It has automatic use case tests, before implementing. |
| 244 | | + (they are very good to demonstrate the design for a library) |
| 245 | | o If it has demos, the demos are actually tests with main method. |
| 246 | | + (demos should be done at design time!) |
| 247 | | o It has enough unit tests to ensure its correctness |
| 248 | | + (listed at design time, passing at implementation time) |
| 249 | | o It has enough integration tests |
| 250 | | + (listed at design time, passing at implementation time) |
| 251 | | o It has enough system tests |
| 252 | | + (listed at design time, passing at implementation time) |
| 253 | | o It has written scenarios for manual testing |
| 254 | | + (when it is an external feature or a bug) |
| 255 | | o The tests had code coverage |
| 256 | | + (good code coverage does not mean good tests, but bad code coverage means bad tests) |
| 257 | | |
| 258 | | * other: |
| 259 | | o Enough information should be provided in the backlog. |
| 260 | | |
| 261 | | [edit] |
| 262 | | Bad code samples |
| 263 | | |
| 264 | | 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! |
| 265 | | |
| 266 | | if (source instanceof RtfBookResource) { |
| 267 | | RtfBookResource toCopy = (RtfBookResource) source; |
| 268 | | copiedBookResource = toCopy.clone(); |
| 269 | | try { |
| 270 | | copiedBookResource.setMetaInfo(toCopy.getMetaInfo()); |
| 271 | | } catch (Exception e) { |
| 272 | | throw new RuntimeException(e); |
| 273 | | } |
| 274 | | target.resources().add(copiedBookResource); |
| 275 | | } |
| 276 | | if (source instanceof BufferedImageBookResource) { |
| 277 | | BufferedImageBookResource toCopy = (BufferedImageBookResource) source; |
| 278 | | copiedBookResource = toCopy.clone(); |
| 279 | | try { |
| 280 | | copiedBookResource.setMetaInfo(toCopy.getMetaInfo()); |
| 281 | | } catch (Exception e) { |
| 282 | | throw new RuntimeException(e); |
| 283 | | } |
| 284 | | target.resources().add(copiedBookResource); |
| 285 | | } |
| 286 | | if (source instanceof MediaBookResource) { |
| 287 | | MediaBookResource toCopy = (MediaBookResource) source; |
| 288 | | copiedBookResource = new MediaBookResource(toCopy.getName(), toCopy |
| 289 | | .getMedia()); |
| 290 | | try { |
| 291 | | copiedBookResource.setMetaInfo(toCopy.getMetaInfo()); |
| 292 | | } catch (Exception e) { |
| 293 | | throw new RuntimeException(e); |
| 294 | | } |
| 295 | | target.resources().add(copiedBookResource); |
| 296 | | } |
| 297 | | |
| 298 | | |
| 299 | | Dont use deprecated API: |
| 300 | | if(currentBook.getContainerFile()!=null) { |
| 301 | | lastModified=new Date(currentBook.getContainerFile().lastModified()).toString(); |
| 302 | | bookSize=currentBook.getContainerFile().length(); |
| 303 | | } |
| 304 | | |
| 305 | | |
| 306 | | Flag, int, etc are bad names: |
| 307 | | boolean flag = true; |
| 308 | | can be |
| 309 | | boolean success = true; |
| 310 | | or |
| 311 | | boolean result = true; |
| 312 | | |
| 313 | | Lots of array expose or redundancy |
| 314 | | public Color[] HIGHLIGHT_COLORS = {Color.BLUE, Color.CYAN, Color.GREEN, Color.MAGENTA, Color.ORANGE, Color.PINK, Color.RED, Color.YELLOW}; |
| 315 | | |
| 316 | | Lots of lazy multi loading... in gui... |
| 317 | | |
| 318 | | Bad JavaDoc - it should be focused on what is it doing, not how, or the details. |
| 319 | | Initializes XXX - is bad for lazy loading setter. |
| 320 | | |
| 321 | | readerMode is a bad name for checkbox... |
| 322 | | |
| 323 | | |
| 324 | | |
| 325 | | Many people did not get the idea of the Logic... |
| 326 | | public void actionPerformed(final ActionEvent arg0) { |
| 327 | | |
| 328 | | final int currentPageIndex = currentPageIndex(); |
| 329 | | int pageToGo; |
| 330 | | |
| 331 | | try { |
| 332 | | pageToGo = Integer |
| 333 | | .parseInt(arg0.getActionCommand()); |
| 334 | | } catch (final Exception e) { |
| 335 | | // incorrect input |
| 336 | | JOptionPane.showMessageDialog( |
| 337 | | swingRepresentation(), |
| 338 | | "Invalid input. Enter a number between 0 and " |
| 339 | | + model().get().pages().size(), |
| 340 | | "Error", JOptionPane.ERROR_MESSAGE); |
| 341 | | BookView.this.currentPageField().get().setText( |
| 342 | | "" |
| 343 | | + model().get().currentPage().get() |
| 344 | | .getIndex()); |
| 345 | | return; |
| 346 | | } |
| 347 | | |
| 348 | | int maxPage = model().get().pages().size() |
| 349 | | - (isInReaderMode() ? 1 : 0); |
| 350 | | |
| 351 | | if ((pageToGo < 0) || pageToGo > maxPage) { |
| 352 | | // out of bounds |
| 353 | | JOptionPane.showMessageDialog( |
| 354 | | swingRepresentation(), |
| 355 | | "Invalid page number. Enter a number between 0 and " |
| 356 | | + maxPage, "Error", |
| 357 | | JOptionPane.ERROR_MESSAGE); |
| 358 | | pageToGo = currentPageIndex; |
| 359 | | } |
| 360 | | |
| 361 | | controller().get().toPage(pageToGo); |
| 362 | | |
| 363 | | assert model().get().currentPage().get() != null; |
| 364 | | BookView.this.currentPageField().get().setText( |
| 365 | | "" |
| 366 | | + model().get().currentPage().get() |
| 367 | | .getIndex()); |
| 368 | | } |
| 369 | | }); |
| 370 | | |
| 371 | | |
| 372 | | |
| 373 | | Splitting a big initialization into methods solves nothing! |
| 374 | | public FrameView(final PageView parent, final Frame<?> frame, |
| 375 | | final Class<? extends FrameController> subController) { |
| 376 | | super(parent, frame, subController); |
| 377 | | |
| 378 | | swingRepresentation(); |
| 379 | | |
| 380 | | initSizeAndLocation(); |
| 381 | | |
| 382 | | this.frameHaloMenu = new FrameHaloMenu(this); |
| 383 | | |
| 384 | | this.frameHaloMenu.lockTo(swingRepresentation(), |
| 385 | | RelativePosition.TOP_LEFT, RelativePosition.BOTTOM_LEFT); |
| 386 | | |
| 387 | | detachHaloMenu(); |
| 388 | | |
| 389 | | initialize(); |
| 390 | | addListeners(); |
| 391 | | |
| 392 | | getParent().swingRepresentation().setLayer(swingRepresentation(), |
| 393 | | getModel().getZorder()); |
| 394 | | getParent().swingRepresentation().add(swingRepresentation()); |
| 395 | | |
| 396 | | Logger.getRootLogger().info("Frame view created."); |
| 397 | | |
| 398 | | updateLayer(); |
| 399 | | } |
| 400 | | |
| 401 | | massive use of linked lists with no reason: |
| 402 | | List<T> res = new LinkedList<T>(); |
| 403 | | |
| 404 | | lots of instance things that should be static (like arrays of data). |
| 405 | | |
| 406 | | reader mode code was extreme.... |
| 407 | | |
| 408 | | Bad naming (and style) |
| 409 | | public String setPropertiesText(Book currentBook) |
| 410 | | |
| 411 | | Hack the model for the purpose of the view.. |
| 412 | | example - the zOrder + 1 hack |
| 413 | | |
| 414 | | Some people should ask more... especially when they think something they are |
| 415 | | going to do is going to get ugly. |
| 416 | | |
| 417 | | Bad comments: |
| 418 | | /** |
| 419 | | * @author Presley |
| 420 | | * |
| 421 | | */ |
| 422 | | is not nice comment for a class |
| 423 | | |
| 424 | | Non-static nested classes dont need reference to outer class.. |
| 425 | | they have it implicitely |
| 426 | | public BooksButton(final Book book, |
| 427 | | final OpenBooksPalette openBooksPalette) { |
| 428 | | super(book.getTitle()); |
| 429 | | |
| 430 | | No sub statements without {} |
| 431 | | if (getTab().getFlap().getAppView().currentBookView().get() == null) |
| 432 | | getTab().getFlap().getAppView().userNewBook(); |
| 433 | | |
| 434 | | |
| 435 | | Assign repeated expressions to locals variables |
| 436 | | if (getTab().getFlap().getAppView().currentBookView().get() == null) |
| 437 | | getTab().getFlap().getAppView().userNewBook(); |
| 438 | | int index = getList().locationToIndex(e.getPoint()); |
| 439 | | switch (index) { |
| 440 | | case 0: |
| 441 | | getTab().getFlap().getAppView().getCurrentChild() |
| 442 | | .getCurrentChild().userDropFrame(x, y, |
| 443 | | FrameKind.Text); |
| 444 | | break; |
| 445 | | case 1: |
| 446 | | getTab().getFlap().getAppView().getCurrentChild() |
| 447 | | .getCurrentChild().userDropFrame(x, y, |
| 448 | | FrameKind.Image); |
| 449 | | break; |
| 450 | | case 2: |
| 451 | | getTab().getFlap().getAppView().getCurrentChild() |
| 452 | | .getCurrentChild().userDropFrame(x, y, |
| 453 | | FrameKind.Media); |
| 454 | | break; |
| 455 | | default: |
| 456 | | ErrorHandlingUtilities.showErrorMessageDialog( |
| 457 | | getList(), |
| 458 | | "Error while trying to create frame.", |
| 459 | | "Frames Palette Error"); |
| 460 | | break; |
| 461 | | } |
| 462 | | |
| 463 | | Sequence of ifs, or switch is a bad thing. It is strange that |
| 464 | | someone created class ComponentsPaletteItem, but instead moving |
| 465 | | the functionallity there, had things like: |
| 466 | | case 0: |
| 467 | | curPage.userDropFrame(x, y, FrameKind.Text); |
| 468 | | break; |
| 469 | | case 1: |
| 470 | | curPage.userDropFrame(x, y, FrameKind.Image); |
| 471 | | break; |
| 472 | | case 2: |
| 473 | | curPage.userDropFrame(x, y, FrameKind.Media); |
| 474 | | break; |
| 475 | | |
| 476 | | Do not inherit, unless really necessary. Composition is usually better... |
| 477 | | public class ComponentsPaletteItem implements Transferable |
| 478 | | |
| 479 | | |
| 480 | | ListPalette was refactored... for good. ListPaletteItem finally introduced |
| 481 | | |
| 482 | | Search is refactored. |
| 483 | | |
| 484 | | BookTemplates are refactored. |
| 485 | | |
| 486 | | If you need the old code -- 2423 is the last stable revision. |
| 487 | | |
| 488 | | create methods are supposed to create something (and return it) |
| 489 | | private void createList(){ |
| 490 | | list().get().setSelectionMode(ListSelectionModel.SINGLE_SELECTION); |
| 491 | | createModel(); |
| 492 | | list().get().setCellRenderer(new ImageRenderer()); |
| 493 | | list().get().setDragEnabled(true); |
| 494 | | list().get().setTransferHandler(new PageTemplatesTransferHandler()); |
| 495 | | } |
| 496 | | |
| 497 | | -> initList is better name |
| 498 | | |
| 499 | | All menus were refactored... Now they are in the MainMenuBar class instead |
| 500 | | the AppView. |
| 501 | | |
| 502 | | FileChoser instance moved to AppView |
| 503 | | |
| 504 | | ImageFrameView - we need to discuss the strange painting |
| 505 | | |
| 506 | | This final everywhere is not needed. It is ok (and good) for |
| 507 | | fields, but for local variable and arguments (which are not |
| 508 | | used in inner classes) it is too much. |
| 509 | | |
| 510 | | Avoid code duplication... it leads to out of date copies: |
| 511 | | } catch (Exception e) { |
| 512 | | // incorrect input |
| 513 | | JOptionPane.showMessageDialog(bookView.swingFrame().get(), |
| 514 | | "Invalid input. Enter a number between 1 and " |
| 515 | | + bookView.model().get().pages().size(), |
| 516 | | "Error", JOptionPane.ERROR_MESSAGE); |
| 517 | | bookView.navigationPanel().get().currentPageField().get().setText("" + currentPageIndex); |
| 518 | | return; |
| 519 | | } |
| 520 | | |
| 521 | | int maxPage = bookView().get().model().get().pages().size() |
| 522 | | - (bookView().get().inReaderMode().get() ? 1 : 0); |
| 523 | | |
| 524 | | if ((pageToGo < 0) || pageToGo > maxPage) { |
| 525 | | // out of bounds |
| 526 | | JOptionPane.showMessageDialog(bookView().get().swingFrame().get(), |
| 527 | | "Invalid page number. Enter a number between 0 and " |
| 528 | | + maxPage, "Error", JOptionPane.ERROR_MESSAGE); |
| 529 | | pageToGo = currentPageIndex; |
| 530 | | } |
| 531 | | |
| 532 | | |
| 533 | | |
| 534 | | |
| 535 | | Don not prefix the static members related to the class... they are already scoped: |
| 536 | | bad (class DecreaseBorderButton): |
| 537 | | |
| 538 | | private static final String DECREASE_BORDER_TOOL_TIP = "Decrease the border of this frame."; |
| 539 | | better (class DecreaseBorderButton): |
| 540 | | private static final String TOOL_TIP = "Decrease the border of this frame."; |
| 541 | | |
| 542 | | |
| 543 | | |
| 544 | | |
| 545 | | |
| 546 | | General |
| | 176 | * code |
| | 177 | * The code style is correct to our convention. |
| | 178 | * The code is easily readable. |
| | 179 | * There are no fake JavaDocs (JavaDocs without useful information). |
| | 180 | * The java-doc is complete enough. |
| | 181 | * For each element, the JavaDoc should describe clearly what is it (if this is not easy, then probably the design is bad) |
| | 182 | * It causes 0 errors. |
| | 183 | * It causes 0 warnings. |
| | 184 | * The @SuppressWarnings annotation is used only if really needed: |
| | 185 | * unused, when something is used only by reflection |
| | 186 | * synthetic-access, when you want to touch something internal with inner class (although in many cases this may be avoided) |
| | 187 | * unchecked, when you are doing something complex with generic (note that this is also avoidable in many cases) |
| | 188 | * No inaccurate, or unclear XXX and TODO tags |
| | 189 | * It should not break other code (ensured by their tests). |
| | 190 | * There some bad code examples here - [wiki:CODE_SMELLS] |
| | 191 | |
| | 192 | |
| | 193 | * design |
| | 194 | * It is documented |
| | 195 | * (not only in the code). |
| | 196 | * It is simple and understandable. |
| | 197 | * It is easy to use. |
| | 198 | * (using it requires less work / small code) |
| | 199 | * It is error proof (hard to use it in a wrong way) |
| | 200 | * The compiler prevents most errors. |
| | 201 | * (it is better to have an abstract getKind() instead of forcing implementors to call setKind at construction) |
| | 202 | * (final may also help) |
| | 203 | * Most of other errors cause an exception |
| | 204 | * (defensive programming helps here) |
| | 205 | * When a library is used, it is used correctly. |
| | 206 | * There are no representation exposures. |
| | 207 | * There are no GOD (very long) methods. |
| | 208 | * There are no GOD (very long) classes. |
| | 209 | * The coupling is low. |
| | 210 | * No magic numbers (and strings) |
| | 211 | * No array members. |
| | 212 | * (unless there is really a reason) |
| | 213 | * No public instance fields. |
| | 214 | * No public non final fields |
| | 215 | * It has high cohesion. |
| | 216 | * (see http://en.wikipedia.org/wiki/Cohesion_(computer_science) ) |
| | 217 | * It has low coupling. |
| | 218 | * (see http://en.wikipedia.org/wiki/Coupling_%28computer_science%29 ) |
| | 219 | * It is possible to state in one sentence what every class or member is/does. |
| | 220 | * Classes and members are named correctly. |
| | 221 | * Every class (or attribute) is self responsible and self complete. |
| | 222 | * No code duplication (copy paste). |
| | 223 | * It is easily testable. |
| | 224 | * (when not, testing helpers are provided to make it such). |
| | 225 | * No other kind of smells. |
| | 226 | * It has good package structure. |
| | 227 | * It is logging correctly. |
| | 228 | * It has adequate error handling. |
| | 229 | * No premature optimization. |
| | 230 | * No premature abstraction. |
| | 231 | |
| | 232 | * tests |
| | 233 | * It has automatic use case tests, before implementing. |
| | 234 | * (they are very good to demonstrate the design for a library) |
| | 235 | * If it has demos, the demos are actually tests with main method. |
| | 236 | * (demos should be done at design time!) |
| | 237 | * It has enough unit tests to ensure its correctness |
| | 238 | * (listed at design time, passing at implementation time) |
| | 239 | * It has enough integration tests |
| | 240 | * (listed at design time, passing at implementation time) |
| | 241 | * It has enough system tests |
| | 242 | * (listed at design time, passing at implementation time) |
| | 243 | * It has written scenarios for manual testing |
| | 244 | * (when it is an external feature or a bug) |
| | 245 | * The tests had code coverage |
| | 246 | * (good code coverage does not mean good tests, but bad code coverage means bad tests) |
| | 247 | |
| | 248 | * other: |
| | 249 | * Enough information should be provided in the backlog. |
| | 250 | |
| | 251 | |
| | 252 | == General == |
| 561 | | * Implementing a new external feature (visible from the user). |
| 562 | | o analyzing - |
| 563 | | + should have a specification, and a manual testing scenario |
| 564 | | o designing - |
| 565 | | + write an automatic test/tests which verifies that your feature is working |
| 566 | | + look at the related code, |
| 567 | | + decide what needs to be added |
| 568 | | + if you add new library feature |
| 569 | | # write use case tests for it |
| 570 | | + You may also write skeleton types (only declaration) and demos. |
| 571 | | + write an outline of your design. |
| 572 | | o implementing |
| 573 | | + Make all the designed things work. |
| 574 | | + Ensure that all tests pass. |
| 575 | | + During the process, add more tests. |
| 576 | | o reviewers - one other developer and one QA. |
| 577 | | |
| 578 | | * Researching about a technology, library, whatever. |
| 579 | | o analyzing - specify what needs to be researched |
| 580 | | + what the research will solve, |
| 581 | | + what is needed to be solved (this is important) |
| 582 | | o designing |
| 583 | | + You can try things, etc, but... do not pollute the main source (for example with libraries). |
| 584 | | + If you need to use other libraries, do it in another project (or in another branch) |
| 585 | | + If you don't need other libraries, you can do it in a research package, but you should make sure that you don't introduce warnings, failing tests, etc. |
| 586 | | o implementing |
| 587 | | + You present the written results / conclusions of your research, demo codes, etc. |
| 588 | | o reviewers - 1-2 developers |
| 589 | | |
| 590 | | * Implementing a new internal feature (a library or something, not directly visible). |
| 591 | | o analyzing - what the library will provide |
| 592 | | o designing - should include |
| 593 | | + use case tests, |
| 594 | | + samples, |
| 595 | | + (in some cases demo), |
| 596 | | + design outline |
| 597 | | o implementing |
| 598 | | + the library should be implemented |
| 599 | | + enough tests should be written during the process |
| 600 | | o reviewers: 2 developers |
| 601 | | * Performing some structure changes, or refactoring. |
| 602 | | o analyzing - what the issues are |
| 603 | | o designing - understanding it, and providing an idea how to fix the issues |
| 604 | | o implementing - fixing the issues |
| 605 | | o reviewers: 1-2 developers |
| 606 | | * Writing a specification |
| 607 | | o analyzing - what needs to be specified |
| 608 | | o designing - brief ideas about the thing |
| 609 | | o implementing - writing the specification exactly |
| 610 | | o reviewers: - 1 qa + 1 developer |
| 611 | | * Doing documentation. |
| 612 | | o analyzing - what needs to be document |
| 613 | | o designing - what it will contain (outline) and understanding of the content |
| 614 | | o implementing - adding the appropriate content. |
| 615 | | o reviewers: - 1-2 team members |
| 616 | | * External testing (the usual testing task). |
| 617 | | o analyzing - has to state what the focus of the tests will be. |
| 618 | | o designing - has to select test scenarios, and make new if necessary |
| 619 | | o implementing - should apply testing scenarios and provide bugs and recommendations (things that probably the users will like/dislike) |
| 620 | | o reviewers: 1 developer |
| 621 | | * Fixing a bug. |
| 622 | | o analyzing - what the bug is. needs a manual testing scenario (in bugzilla) |
| 623 | | o designing - an automatic test (failing) should be present, idea what is causing it, more tests (which detect related internal bugs), idea how can be fixed |
| 624 | | o implementing - fixing the bug, making the tests pass, and some refactoring |
| 625 | | o reviewers: 1 QA and 1 developer |
| 626 | | * Something else? |
| 627 | | |
| | 266 | * Implementing a new external feature (visible from the user): |
| | 267 | * analyzing - |
| | 268 | * should have a specification, and a manual testing scenario |
| | 269 | * designing - |
| | 270 | * write an automatic test/tests which verifies that your feature is working |
| | 271 | * look at the related code, |
| | 272 | * decide what needs to be added |
| | 273 | * if you add new library feature |
| | 274 | * write use case tests for it |
| | 275 | * You may also write skeleton types (only declaration) and demos. |
| | 276 | * write an outline of your design. |
| | 277 | * implementing |
| | 278 | * Make all the designed things work. |
| | 279 | * Ensure that all tests pass. |
| | 280 | * During the process, add more tests. |
| | 281 | * reviewers - one other developer and one QA. |
| | 282 | |
| | 283 | * Researching about a technology, library, whatever. |
| | 284 | * analyzing - specify what needs to be researched |
| | 285 | * what the research will solve, |
| | 286 | * what is needed to be solved (this is important) |
| | 287 | * designing |
| | 288 | * You can try things, etc, but... do not pollute the main source (for example with libraries). |
| | 289 | * If you need to use other libraries, do it in another project (or in another branch) |
| | 290 | * If you don't need other libraries, you can do it in a research package, but you should make sure that you don't introduce warnings, failing tests, etc. |
| | 291 | * implementing |
| | 292 | * You present the written results / conclusions of your research, demo codes, etc. |
| | 293 | * reviewers - 1-2 developers |
| | 294 | |
| | 295 | * Implementing a new internal feature (a library or something, not directly visible). |
| | 296 | * analyzing - what the library will provide |
| | 297 | * designing - should include |
| | 298 | * use case tests, |
| | 299 | * samples, |
| | 300 | * (in some cases demo), |
| | 301 | * design outline |
| | 302 | * implementing |
| | 303 | * the library should be implemented |
| | 304 | * enough tests should be written during the process |
| | 305 | * reviewers: 2 developers |
| | 306 | |
| | 307 | * Performing some structure changes, or refactoring. |
| | 308 | * analyzing - what the issues are |
| | 309 | * designing - understanding it, and providing an idea how to fix the issues |
| | 310 | * implementing - fixing the issues |
| | 311 | * reviewers: 1-2 developers |
| | 312 | |
| | 313 | * Writing a specification |
| | 314 | * analyzing - what needs to be specified |
| | 315 | * designing - brief ideas about the thing |
| | 316 | * implementing - writing the specification exactly |
| | 317 | * reviewers: - 1 qa + 1 developer |
| | 318 | |
| | 319 | * Doing documentation. |
| | 320 | * analyzing - what needs to be document |
| | 321 | * designing - what it will contain (outline) and understanding of the content |
| | 322 | * implementing - adding the appropriate content. |
| | 323 | * reviewers: - 1-2 team members |
| | 324 | |
| | 325 | * External testing (the usual testing task). |
| | 326 | * analyzing - has to state what the focus of the tests will be. |
| | 327 | * designing - has to select test scenarios, and make new if necessary |
| | 328 | * implementing - should apply testing scenarios and provide bugs and recommendations (things that probably the users will like/dislike) |
| | 329 | * reviewers: 1 developer |
| | 330 | |
| | 331 | * Fixing a bug. |
| | 332 | * analyzing - what the bug is. needs a manual testing scenario (in bugzilla) |
| | 333 | * designing - an automatic test (failing) should be present, idea what is causing it, more tests (which detect related internal bugs), idea how can be fixed |
| | 334 | * implementing - fixing the bug, making the tests pass, and some refactoring |
| | 335 | * reviewers: 1 QA and 1 developer |
