Daan, Agreed. If it's unknown, the variable of that type can just be null which is an accepted practice for saying the business logic hasn't determined the value yet.
--Alex From: Daan Hoogland [mailto:daan.hoogl...@gmail.com] Sent: Wednesday, July 24, 2013 12:41 PM To: Alex Huang Cc: cloudstack Subject: RE: Review Request 12685: CLOUDSTACK-1532 added utility functions to scan URIs Ok, I don't like changing this enum. I'd rather throw it out and start over but you are answering the question by sharing your views on school of programming, i think. BroadcastDomainType by its name implies it could be unknown but never undecided. Do you agree? I can comply with any school. Op 24 jul. 2013 19:19 schreef "Alex Huang" <alex.hu...@citrix.com<mailto:alex.hu...@citrix.com>> het volgende: Daan, Sorry for the late reply. Now, it kinda goes into code/design philosophy. I'll tell you what mine is. There's one school of thought that code should cover all bases. To me having undecided is in that school because it asks "could it be undecided" and the answer is of course yes because in cs everything is possible. The other is to say Undecided is never accepted because the code can never do anything with that. Having assert to tell developers that this is just not used in this manner and speaks louder and more accurate than an Undecided. I'm more in this school. I think it should ask the question "should it be undecided". --Alex From: Daan Hoogland [mailto:daan.hoogl...@gmail.com<mailto:daan.hoogl...@gmail.com>] Sent: Sunday, July 21, 2013 8:37 AM To: Alex Huang Cc: Hugo Trippaers; cloudstack Subject: Re: Review Request 12685: CLOUDSTACK-1532 added utility functions to scan URIs Your remarks on the assert resembles a comment in the code that I didn't write ;) But seriously; If the string contains a scheme that is not in the enum, you want a exception to be thrown instead of UnDecided or UnDefined? regards, On Sun, Jul 21, 2013 at 5:32 PM, Alex Huang <alex.hu...@citrix.com<mailto:alex.hu...@citrix.com>> wrote: Daan, I'm not sure I understand what you're saying here. --Alex From: Daan Hoogland [mailto:daan.hoogl...@gmail.com<mailto:daan.hoogl...@gmail.com>] Sent: Sunday, July 21, 2013 4:23 AM To: Alex Huang Cc: Hugo Trippaers; cloudstack Subject: Re: Review Request 12685: CLOUDSTACK-1532 added utility functions to scan URIs Alex, I saw you put you comments in the commit as comment...??? why not as code then? On Sat, Jul 20, 2013 at 3:42 PM, Alex Huang <alex.hu...@citrix.com<mailto:alex.hu...@citrix.com>> wrote: Daan, It should assert and throw a CloudRuntimeException. https://cwiki.apache.org/confluence/display/CLOUDSTACK/Exceptions+and+logging --Alex From: Daan Hoogland [mailto:daan.hoogl...@gmail.com<mailto:daan.hoogl...@gmail.com>] Sent: Saturday, July 20, 2013 3:25 AM To: Alex Huang Cc: Hugo Trippaers; cloudstack Subject: Re: Review Request 12685: CLOUDSTACK-1532 added utility functions to scan URIs Alex, it falls through to 'UnDecided' if not known. Do you mean it should throw something? or maybe add an extra 'UnDefined'? On Fri, Jul 19, 2013 at 5:31 PM, Alex Huang <alex.hu...@citrix.com<mailto:alex.hu...@citrix.com>> wrote: This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/12685/ Ship it! master: 2d4464d One review comment is if you are checking for the colon to see if the value has the schema already, then shouldn't it also check if the schema matches what's declared? But that shouldn't block this review. The code itself is technically sound. - Alex Huang On July 17th, 2013, 3:46 p.m. UTC, daan Hoogland wrote: Review request for cloudstack and Hugo Trippaers. By daan Hoogland. Updated July 17, 2013, 3:46 p.m. Bugs: CLOUDSTACK-1532 Repository: cloudstack-git Description the review for the complete patch was open to long and no longer applicable, so I am starting with smaller patches as this is really needed to implement CLOUDSTACK-1532 Testing unit testing Diffs * api/src/com/cloud/network/Networks.java (5aede05) * api/test/com/cloud/network/NetworksTest.java (PRE-CREATION) View Diff<https://reviews.apache.org/r/12685/diff/>
