BASE_BOOK_COMMONS_R0

[Astea]

wiki page: BASE_BOOK_COMMONS_R0 - effort: 2d

[deyan]

Adding category

[orliin]

finished in 1h

[peko]

The analysis is quite good. Maybe the task requirements section contains too much things that can be done in 2 days but it is not unachievable.

[boyan]

taken for design

[boyan]

done in 5h

[peko]

Defining things about book commons is ok. But things that can be done with books are not all connected with the book model. The model is separated from the things that can be done with the model. In other words the model does not contain the View and the Logic since these are helper components that change the model and display it. In the design you have to state all the things that the model contains - frame connections, pages etc. And then should describe the hierarchy of classes that contain Books, Pages, Backgorunds, Connections etc. In implementation phase these should be implemented - or at last base classes, since this is huge to be done for the effort stated.

[boyan]

taken for improvement

[boyan]

redesigned in 2h

[peko]

Design is pretty good.

[boyan]

taken for implementation

[boyan]

Done in approximately 2.5d (total).

[pap]

Reviewed in 90m.
Things that are not OK:

• There are no connections between the classes in the uml diagram.
• Warnings in unit tests
• assert* methods usage is not good. Prefer assertEquals/assertSame to assertTrue( foo == bar )
• The test for current page is not good. You should test that if the page that is curent is removed from the book there is no urrent book and after readding the same page it becomes current
• If the book is not supposed to be inherited(this means having different book types) then the class Book should be declared final. The same applies to Page.
• The class (Book, Page) layout is not accoding to this convention ​http://java.sun.com/docs/codeconv/html/CodeConventions.doc2.html#1852 , which happens to be in the useful links page in thedev doc.
• I think that the method createDefaultBook(String title, ImmSize pageSize) is not a good idea. It could either be a private helper method or it shouldn't exist. But I don't think that there can be many kinds of default books.
• You should describe better the idea of lastCurrent
• The version property of the book is not needed. there will be a version provided when we make the book a resource.
• The delete method of the book class doesn't seem very sensible to me.
• The frames method has a check that's not necessary after the @Own annotation has been impelmented - the one whether the frame is already added
• There are unnecessary finals in the Page class
• The getMaxPageBorderThickness() of the Page class is more like a Book method if it is necessary at all
• There is at least a prototype for that clones ProObjects so it can be used for copying books.
• The shadow() method of the Page class and the delete methods of the Page and Book classes should be reviewed and removed if they are no needed
• PageIntegrationTest and BookIntegrationTest should be refactored. Remove the "@SuppressWarnings("all")" directive, write JavaDoc, refactor the test methods

[boyan]

taken for improvement

[boyan]

done in 1 eff.

[jani]

The remarks from the previous implementation review are taken into account. I see no new problems and the unit tests pass. 50m.

[deyan]

Batch update from file query-obsoleted.csv