current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 6 down vote favorite

For my application, I need to use two slightly different algorithms for persisting an entity: 

def persistRandomAlias(self, alias):
    self.em.persist(alias)
    try:
        self.em.flush()
    except DBALException as e:
        if e.previous.code != 23000:
            raise e
        self.reloadEM()

        self.persistRandomAlias(alias)



def persistCustomAlias(self, alias):
    self.em.persist(alias)
    try:
        self.em.flush()
    except DBALException as e:
        if e.previous.code != 23000:
            raise e
        self.reloadEM()

        existing = self.findByNaturalKey(alias.name)
        if existing != alias:
            raise DuplicateNameException('The requested name is already taken', e)

The difference between them is in part of the code responsible for exception handling. I thought about putting the common code in one method, with remaining operations passed in a function that would accept exception object, like this: 

def persistAlias(self, alias, exHandler):
    self.em.persist(alias)
    try:
        self.em.flush()
    except DBALException as e:
        if e.previous.code != 23000:
            raise e
        self.reloadEM()

        exHandler(e)

def persistRandomAlias(self, alias):
    self.persistAlias(alias, lambda e: self.persistRandomAlias(alias))            

def persistCustomAlias(self, alias):
    def exHandler(e):
        existing = self.findByNaturalKey(alias.name)
        if existing != alias:
            raise DuplicateNameException('The requested name is already taken', e)

    self.persistAlias(alias, exHandler)

However, I'm not sure if it is correct to pass an unused argument to a function.

I have also thought about refactoring common code into an abstract method object class, like this:

def execute(self):
    self.em.persist(alias)
    try:
        self.em.flush()
    except DBALException as e:
        if e.previous.code != 23000:
            raise e
        self.reloadEM()
        self.e = e

        self.handleE()

handleE() would be implemented in subclasses of the class implementing execute() method, and it could access exception object, or not, depending on implementation.

Should I choose one of these ideas for dealing with code duplication? Which one is better? Do you suggest a better solution?

share|improve this question

1 Answer 1

up vote 1 down vote

I was originally going to add this as a comment, but it works better as an answer.

Why don't you just have a variable among one of the method's arguments which determines whether to run the code for a custom or random alias by utilizing a single if statement?

For example:

def persistAlias(self, alias, custom_alias):
"""Please place relevant description here,
    The "custom_alias" argument determines whether to
    persist a custom or random alias."""
    try:
        self.em.persist(alias)
        self.em.flush()
    except DBALException as e:
        if e.previous.code != 23000:
            raise e
        self.reloadEM()

        if custom_alias:
            existing = self.findByNaturalKey(alias.name)
            if existing != alias:
                raise DuplicateNameException('The requested name is already taken', e)
        else:
            self.persistAlias(alias, 0)
share|improve this answer
1  
I had a similar answer on stackoverflow, so I'll just copy my response: It's not visible in the code I provided, but there are reasons for which I wouldn't pick your suggestion. First, the actual type of alias depends on user providing custom name value, or leaving it blank. The test for is_custom inside persistAlias method would be redundant, since it would be performed by the code responsible for both creating alias object and calling persist method. –  vikingr Aug 22 at 14:51
1  
Second thing is that the requested alias type (custom vs. random) will not change when running persistAlias() again as a response to the exception, which would also make the test redundant. –  vikingr Aug 22 at 14:51
    
Ok, thanks for the clarification. –  Zenohm Aug 22 at 14:54

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.