[roster-code-reviews] Comment on revision r344 in roster-dns-management

[roster-dns...@googlecode.com]

sharrell.purdue commented on revision r344 in project roster-dns-management.
Details are at
http://code.google.com/p/roster-dns-management/source/detail?r=344


Line-by-line comments:

File: /branches/web-interface/roster-web/htdocs/main.py (r344)
===============================================================================

Line 50: def handler(request):
-------------------------------------------------------------------------------
need doc strings on all the functions

Line 58: html_page = MakeHtmlPage()
-------------------------------------------------------------------------------
Lets make this function called MakeHtmlHeader

Line 78: MakeChangelist(records_dict, add_dict, remove_dict,
post_get_dict,
-------------------------------------------------------------------------------
You need to return something from this function call, or how would
error_to_show get populated, or anything from that matter. This is not in a
class so you need program this functionally, also once you do that, I think
it will take fewer arguments

Line 262: AddError(ip_address, "FQDN of %s needs to be updated." %
ip_address,
-------------------------------------------------------------------------------
I still don't think this is very clear as far as errors, talk to me about
it and lets come up with a useful error message on this.

Line 337: def PushChanges(add_dict, remove_dict, highlight_ips, html_page,
core_instance,
-------------------------------------------------------------------------------
most of this function should just be a call to BatchProcessRecords, if we
don't and one fails in the middle then we will be in an indeterminate
state. Also if one of them fails, it need to take you back to the page with
error boxes

Line 384: html_page.append('<table><tr><td>Existing Record</td><td
bgcolor="#FF6666">Error Record</td><td bgcolor="#66FF66">Add Record</td><td
bgcolor="#6666FF">Remove Record</td</tr></table>')
-------------------------------------------------------------------------------
this line is loooooooooooooooooong

Line 499: def UpdateInputBoxes(changed_records, record_html_data,
highlight_ips,
-------------------------------------------------------------------------------
This needs to return something, not a class

Respond to these comments at
http://code.google.com/p/roster-dns-management/source/detail?r=344
--
You received this message because you starred this review, or because
your project has directed all notifications to a mailing list that you
subscribe to.
You may adjust your review notification preferences at:
http://code.google.com/hosting/settings


--
Subscription settings: http://groups.google.com/group/roster-code-reviews/subscribe?hl=en