Sample review
<sartak> this the way you wrote it: | |
sub _build_message { | |
my $self = shift; | |
my $line1 = "Attribute (".$self->attribute_name.") does not pass the type constraint because: "; | |
if( $self->new_member ) { | |
$line1 = "A new member value for foo does not pass its type constraint because: " | |
} | |
$line1.$self->type_constraint_message; | |
} | |
<sartak> this the way I'd write it, which I think is clearer: | |
sub _build_message { | |
my $self = shift; | |
my $line1; | |
if ($self->new_member) { | |
$line1 = "A new member value for foo does not pass its type constraint because: " | |
} | |
else { | |
$line1 = "Attribute (".$self->attribute_name.") does not pass the type constraint because: "; | |
} | |
return $line1 . $self->type_constraint_message; | |
} | |
<sartak> assigning a value to a variable only once makes things more understandable | |
<sweet_kid> yeah | |
<sartak> if you reassign a variable it's a little bit harder to understand the flow | |
<sartak> and, I like explicitly including "return" when the method body is anything but very trivial | |
<sweet_kid> I skip return statements | |
<sweet_kid> sorry | |
<sweet_kid> will include it from now onwards | |
<sartak> you don't have to include explicit "return" in all exceptions | |
<sweet_kid> okay | |
<sartak> maybe a good metric is: is there anything more complicated than variable assignment and string concatenation | |
<sartak> this had an "if" statement so it is more complicated | |
<sweet_kid> okay | |
<sweet_kid> you're right |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment