SCENE_COMMONS_R1

[Astea]

wiki page: SCENE_COMMONS_R1 - effort: 1.5d

[deyan]

Adding category

[gogov]

It's generally ok, though I have the following comments:

• You could try being a bit more specific with the task requirements, so the designer of the task doesn't get confused what you mean, though in this case it kinda clear
• You could add writing simple unit tests as a requirement
• You could be more specific about the first implementation idea, because I'm almost sure what you mean, though it sounds a bit unclear

No matter I'm leaving it analysis ok, it's good improve this one a bit.

[deyan]

The requirements are now clear enough

[pap]

• There are nice ideas in this design but also there are several flaws
• The most important thing is that what it describes is based upon changing code that doesn't look so for more than a month
• When you propose removing some constructors you should take the persistence system in mind
• The methods about converting to the preffered format and multiplying an image are already in the ImmImage class. This refactoring was done with the #1674 (GROUP_FILLING_R0) task.
• There is a class WeakHashMap in java so perhaps you can use it, although I am not sure how hekoful it will be.
• It is better if the static variable is declared to be of the interface type Map rather than the concrete implementataion.
• Make sure the equals and hashCode methods of ImmColor are consistent.
• I don't really understand why do you need the second argument to the constructor with BufferedImage argument
• You should also deal with the Icon cache.
• Make sure you do no break the fillings.
• I doubt a bit about the sense of the url caching.
• It is very bad that you use such an old codebase. You can make subbranches of your private folder that are using fresh trunk copies.
• Also I may propose that the scene may be sped up if we ommit some of the scalings. Perhaps we can use some MipMaps.

[pap]

• Two more things :
  • Tests are mandatory for a design and there are things to test in this task.
  • Don't screw the wiki page of the task. The sections in the template have their purpose so please bring back the "Testing" and "Comments" secitions.

[pap]

• I like some part of the implementation but there are some things that cannot be merged into the trunk
• First of all you have added a base.commons package to the org.sophie2.base.scene module. This is incorrect.
• You are referring to some class MyHashMap. There is no such class in the given code and also this is quite a bad name.
• I cannot understand why you are so afraid to write automatic tests. I can't let a tesk pass without any tests written. They are a part of the requirements for a successful task and there is a reason for this requirement.

[deni]

• I had added the new package by mistake and I removed it.
• I did not intend to use MyHashMap, sorry. I changed it to HashMap.
• And I still can't think of any tests for this task...

[pap]

• It is good that you fixed the HashMap issue.
• But as I have written it is unacceptable that there is no automatic test.
• The ImmImage has things that can be tested - whether it is really immutable, whether it really caches the things that it is supposed to.
• You made serious changes to the ImmImage class so you should add yourself to the list of authors.
• Automatic tests are very important because they verify that the application, its modules and classes behave the way we want them to. And they do it without the need of thousands of mouse clicks.

[deyan]

Batch update from file query-obsoleted.csv