Skip to main content

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] [List Home]
Re: [udig-devel] uDig - Code Review on DocumentPropertyPage

Remaining feedback; and then I will do a pull request.

Aside: this is stage 2 of our documentation RFC, Stage 3 is when we will ask for a pull review of the docs and so forth.

Thanks thanks for the detailed review thus far… no need to waste time saying "suggest" as I have thick skin and take correction :-P
Code – General:
Suggest to clean up warnings like unused imports, non-externalized strings, etc.
Suggest to standardize spacing between variables, methods, etc. to increase readability and ease maintenance
done 
Code – DocumentPropertyPage:
Externalize strings used in the UI
Suggest to remove unused code. Eg. doComputeSize()
Suggest to rename variables to more meaningful and descriptive names making it easier to maintain. Eg. HotlinkDescriptor created = prompt.getDescriptor();
Suggest to set relative column widths to enable auto-adjustment when window size is adjusted
Refactor duplicate code used in performDefaults() and createContents(Composite parent) to ease code maintenance
 
Code – DocumentPropertyPage:
Suggest to refactor and extract the hotlink descriptor delimiter to a variable to ease maintenance
 
Code – HotlinkDescriptor:
Suggest to refactor and extract the hotlink descriptor definition value delimiter to a variable to ease maintenance
Check if it is safe to assume that type = WEB if only the attribute name is specified
 
Functionality:
On Add, check if attribute to be added already exists in list
I think we can have more than one Action, so I am going to leave that alone for a bit...
On Edit, check if attribute to be assigned already exists in list
Suggest to add a “Label” or “Name” value to allow user to input a label for the hotlink
Enable Hotlink checkbox seems to extend to the whole length of the window as it allows clicking outside of the button or text’s area
Clarification: Should unchecking “Enable Hotlink” and applying remove all the attribute settings or just simply disable hotlinking?
Clarification: Is the checkbox necessary? Or can we just check if “ hotlink attributes” count > 0?
I had it based on if the key exists at all, if the key does not exist at all then we would not resolve to an HotlinkSource, and the feature would not list itself in the DocumentView.

Cheers,
Jody 


Back to the top