Re: [udig-devel] uDig - Code Review on DocumentPropertyPage

[Jody Garnett <jody.garnett@xxxxxxxxx>]

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