GROUP_SCENES_R1

[gogov]

GROUP_SCENES_R1

Ticket	Summary	Effort	Status
No tickets found

[meddle]

I think this is good analysis for the both aspects and so I have no bad note and stuff :)

4.5p (10m)

[pap]

• I am passing it although there are problems.
• Algorithmic
  • No need of setUp/tearDown methods that just call the super method (QueryCacheTest and HasherTest)
  • This "assert count >= 0:"dasjldnasj" + count;" in the Hash class is not nice
  • No braces in ifs in the equals method of Hash class. If you use autogeneratted methods be sure to fix them.
  • Test for the immutability of Hash - arrays as parameters give possibility for rep exposure and this should be verified. This is important!!
  • Bad spacing in QueryCache - put a space between "if" keyword and its opening bracket.
  • It may be nice to put an @Immutable annotation to the immutable classes even if you don't suppose they'll be used in with pro-lib
  • You could use 3.5f instead of (float)3.5 - it is shorter (HasherTest)
  • Fake JavaDoc in HasherTest - I understand it is almost(or fully) meaningless but you could just write everywhere "initial value".
  • Plese remove the Thumbs.db file in modules/org.sophie2.base.scene/src/test/resources/distrib/Thumbs.db
  • Shouldn't the Sprite class have a @Immutable ?
  • I am not really sure but is it needed the cache member of the Sprite class to be publicly accessible? I think it may better be private or protected
  • Perhaps there is more
• Hardware
  • Things like " catch (IOException e) {

e.printStackTrace();

}" in your code are not nice.

• Obviusly you are missing some points like effects but I think they can be added in the next revision of the task
• I still don't see the deployment resolved but as this is a more general issue for this revision you may have some wooden solution that covers only some platforms with soem long wierd instructions.
• As we discussed this solution makes the scenes totally inextensible but I think we'd better have a fully working second solution and later on think how to split scene engines to reach extensibility.

• Good luck and please do not make the perfect solution now(especially the hardware part). You can improve it the next week.

[dido]

There are major changes inside the design so we'll return it to an_ok state

[pap]

• I pass it especially when it is already in the branch.
• Lots of commented out code (mainly clipping).
• Unnecessary tearDown methods(ImageSpriteTest, BlendSpriteTest, )
• Throw UnsupportedOperationException for methods that are not implmeneted/supported (applyTo in effects).
• I find it a bit strange that you have a ColorEffect as a member in the ShadowEffect class instead of an ImmColor.
• No JavaDoc on some of SceneEffect's methods.
• Probably you know that but the methods that apply effects with the OpenGL library probably need aditional parameters :)
• I think that it may be nice that immutable classes have an @Immutable annotation (actually I find this annotation so nice that it should be included in the JDK) - TilePos, *Effect.
• I don't like "getInstance" as a name in the EmptySprite class. While I was reading its usage I thought it was something lika factroy rather than a singleton.
• As Sprite is an @Immutable it is nice that its subclasses should have that annotation too
• In BlendSprite the method createBlend could just be called just create or make. Same with ClipSprite.
• I find the Tile class quite strange. First it has representation exposure. Second it uses a BufferedImage instead of ImmImage. And I don't understand "FOR INTERNAL USE ONLY! DO NOT MODIFY THE ARGUMENT!". It seems that the method is for internal use only :)
• The holy Meddle(may his sacred light shine upon us and light our path) says it is nice to have a space between "if" and "(" (TextureRegistry). Same with "for" and "(".
• I think that when you are throwing an exception as a result of catching another one it is nice to supply the cause in the constructor of the new exception (SceneUtil.readPixels).
• I would be very glad if you correct most of these problems. You may wish to do this along with the next revision of this task or separately.

[deyan]

Batch update from file query-obsoleted.csv