[PATCH] Factor out duplicate code

[Andrew Bennetts]

On Tue, Oct 11, 2005 at 06:45:06PM -0700, Dan Loda wrote:
> ... by adding a factory method to InventoryEntry.
> 
> I'm new to Python; constructive criticism is most welcome ;)
> 

Ok, here's some :)

[...]
>  
> +    versionable_kinds = {'file': 'InventoryFile',
> +                         'directory': 'InventoryDirectory',
> +                         'symlink': 'InventoryLink' }

You can reference the classes directly here:

    versionable_kinds = {'file': InventoryFile,
                         'directory': InventoryDirectory,
                         'symlink': InventoryLink}

This means you don't have to use eval.  eval is almost always a bad idea,
unless you really do want to evaluate arbitrary python expressions.  If
you're using eval as a means to an end other than evaluating expressions for
the sake of it, there's probably a better way.

Well, if InventoryEntry is defined before the other inventory types, it's a
bit messier than that -- you'd have to delay setting that attribute until
after those classes are defined.  Still better than eval though!

>      @staticmethod
>      def versionable_kind(kind):
> -        return kind in ('file', 'directory', 'symlink')
> +        return InventoryEntry.versionable_kinds.has_key(kind)

You can still use 'in' here:

        return kind in InventoryEntry.versionable_kinds

> +    @staticmethod
> +    def create(kind, file_id, name, parent_id):
> +        """Return an inventory entry of specified kind"""
> +        if not InventoryEntry.versionable_kind(kind):
> +            raise BadFileKindError("unknown kind %r" % kind)
> +        return eval(InventoryEntry.versionable_kinds[kind] +
> +                    '(file_id, name, parent_id)')

As explained above, the eval isn't necessary, so this would become:

        return InventoryEntry.versionable_kinds[kind](file_id, name, parent_id)

>      def check(self, checker, rev_id, inv, tree):
>          """Check this inventory entry is intact.
> @@ -887,16 +898,8 @@
>          if parent_id == None:
>              raise NotVersionedError(parent_path)
>  
> -        if kind == 'directory':
> -            ie = InventoryDirectory(file_id, parts[-1], parent_id)
> -        elif kind == 'file':
> -            ie = InventoryFile(file_id, parts[-1], parent_id)
> -        elif kind == 'symlink':
> -            ie = InventoryLink(file_id, parts[-1], parent_id)
> -        else:
> -            raise BzrError("unknown kind %r" % kind)
> -        return self.add(ie)
> -
> +        return self.add(InventoryEntry.create(kind, file_id, parts[-1],
> +                                              parent_id))

A simpler and less fancy approach would be to add the 'create' method as you
suggest, but just leave the if statement hard-coded in there, rather than
mucking about with the versionable_kinds dictionary -- or do you have a
use-case for a plugin or something that needs this sort of extensibility?

-Andrew.