Ticket #831 (closed planned_task: obsolete)

Opened 14 years ago

Last modified 13 years ago

TEXT_RESOURCE_HTML_R0

Reported by: Astea Owned by: diana
Priority: 3 Milestone: M05_PRE5
Component: TEXT_RESOURCES Version: 2.0
Keywords: Cc:
Category: MAIN Effort: 1
Importance: 0 Ticket_group: GROUP_PLAIN_AND_RTF_RESOURCES_R0
Estimated Number of Hours: Add Hours to Ticket:
Billable?: Total Hours:
Analysis_owners: diana, dido Design_owners:
Imp._owners: Test_owners:
Analysis_reviewers: deyan Changelog:
Design_reviewers: meddle Imp._reviewers:
Test_reviewers: Analysis_score: 3.5
Design_score: 2 Imp._score: 0
Test_score: 0

Description

wiki page: TEXT_RESOURCE_HTML_R0 - effort: 1d

Change History

comment:1 Changed 13 years ago by dido

  • Status changed from new to s1a_analysis_started
  • Analysis_score set to 0
  • Test_score set to 0
  • Design_score set to 0
  • Owner changed from Astea to dido
  • Imp._score set to 0
  • Analysis_owners set to diana, dido

comment:2 Changed 13 years ago by diana

  • Status changed from s1a_analysis_started to s1b_analysis_finished

comment:3 Changed 13 years ago by deyan

  • Status changed from s1b_analysis_finished to s1c_analysis_ok
  • Analysis_score changed from 0 to 3.5

comment:4 Changed 13 years ago by diana

  • Analysis_reviewers set to deyan

comment:5 Changed 13 years ago by diana

  • Owner changed from dido to diana
  • Status changed from s1c_analysis_ok to s2a_design_started

comment:6 Changed 13 years ago by diana

  • Status changed from s2a_design_started to s2b_design_finished

comment:7 Changed 13 years ago by meddle

  • Status changed from s2b_design_finished to s1c_analysis_ok
  • Design_score changed from 0 to 2
  • Design_reviewers set to meddle
  • About the code:
    • The assertEquals(buf.toString(),this.text); method has special arguments the first one is the EXPECTED one the second the ACTUAL, your case is not that one.
    • The indent your code, it's hardly readable that way.
    • Write your JavaDoc to be readable, put one new line between it's overview and parameters, indent it, write it carefully to be helpful, the good JavaDoc can be used as design documentation as well.
    • I think you should test your LogicR3 handler as well with SystemTest.
    • The current test of yours is not good, may be it will be no automatic, that method -> AppLogic.get().userCreateNewBook(mw.appView().get()); will make you use your BookPropertiesDialog, that's not good, in the SystemTest class there is a method for creating new book without it, you can use it in the SystemTest that you will write, and it's code when fixing this test that will test only the HtmlUtility logic and not use this many modules.
    • Are you diana or Diana, the author identificator is important thing.
    • Blocks like bodies of "if"s even if they are one row are surrounded with curly brackets.
    • Think of better names for your variables, one letter names are bad practice...
    • Flying JavaDoc is bad thing
    • I know that you are familiar with that page GoodCodeExamples, I don't know why don't you want to write your code using it, the other people do it and their reviews pass easily.
    • Why you write this twice : htmlResource.text().set(text);
  • About the design:
    • So I explained why your test is a dummy test above.
    • Why you named this HtmlLogic.USER_ADD_HTML_FRAME, the user will import HTML to a text frame, may be HtmlLogic.USER_IMPORT_HTML is the better name.
    • Somebody have to fix the fact that the text related frames continue to be created by the FrameFactory, instead like the other resources, why don't you do it (optional).
    • Other thing like that is the repeating "for" cycle for finding the next contentLocation of the new frames, all the people just copy it. It can be extracted to a helper method.
    • Why the originUrl is a field and no property, if you want to continue that way you must explain why.
    • I think the methods of this HtmlUtility should be static, do you need special instantiation of it every time when you execute the html import?
    • What about the getCorrespondingVal method isn't it better for it to use enumeration of the html styles and switch operator. I mean this set of if-else statements is too large. I mean you can create enumeration for the possible (for now) html view attributes, it will be better.
    • You use the javax.swing.text library, you can explain more in details it's use and give a link to it's JavaDoc.
  • And again you are the design owner, write your name in the ticket field, take more seriously our process it is important for the good product and documentation.

2p (1h)

comment:8 Changed 13 years ago by diana

  • Ticket_group set to GROUP_PLAIN_AND_RTF_RESOURCES_R0

comment:9 Changed 13 years ago by deyan

  • Status changed from s1c_analysis_ok to closed
  • Resolution set to obsolete

Batch update from file query-obsoleted.csv

Note: See TracTickets for help on using tickets.