Code review comment for lp:~jcsackett/launchpad/deprecate-official_codehosting

Brad,

I actually deliberately expanded some clauses into the if constructs because I found them easier to parse.

I'm not opposed to a helper function, since the inside of that would be fairly clear, but assigning a boolean
expression to a variable is sort of difficult to scan in a large block of code.

Thoughts?

On Aug 27, 2010, at 4:49 PM, Brad Crittenden wrote:

> Review: Approve code
> This branch looks nice Jon. What would you think of creating some helper functions like:
>
> uses_Launchpad(thing)
>
> You could then collapse this:
>
> 175 + if self.codehosting_usage == ServiceUsage.LAUNCHPAD:
> 176 + configured = True
> 177 + else:
> 178 + configured = False
> 179 return [dict(link=set_branch,
> 181 + configured=configured)]
>
> to:
>
> 179 return [dict(link=set_branch,
> 181 + configured=uses_launchpad(self.codehosting_usage)
>
> I guess you could do the same with:
>
> configured = (self.codehosting_usage == ServiceUsage.LAUNCHPAD)
>
> with less overhead.
> --
> https://code.launchpad.net/~jcsackett/launchpad/deprecate-official_codehosting/+merge/33953
> Your team Launchpad code reviewers from Canonical is subscribed to branch lp:launchpad/devel.
>
> --
> launchpad-reviews mailing list
> <email address hidden>
> https://lists.ubuntu.com/mailman/listinfo/launchpad-reviews