This service will be undergoing maintenance at 00:00 UTC, 2016-08-01. It is expected to last about 1 hours
Bug 656968 - duplicate code
duplicate code
Status: CLOSED WONTFIX
Product: Red Hat Enterprise Linux 6
Classification: Red Hat
Component: subscription-manager (Show other bugs)
6.1
Unspecified Unspecified
low Severity medium
: rc
: ---
Assigned To: Bryan Kearney
Bryan Kearney
:
Depends On:
Blocks: rhsm-rhel62
  Show dependency treegraph
 
Reported: 2010-11-24 10:34 EST by Miroslav Suchý
Modified: 2011-07-28 14:05 EDT (History)
4 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2011-07-28 14:05:34 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:


Attachments (Terms of Use)

  None (edit)
Description Miroslav Suchý 2010-11-24 10:34:47 EST
R:  1: Similar lines in 3 files
==certlib:52                   
==factlib:29                   
==repolib:30                   
    def __init__(self, lock=ActionLock()):
        self.lock = lock                  

    def update(self):
        lock = self.lock
        lock.acquire()  
        try:            
            action = UpdateAction()
            return action.perform()
        finally:                   
            lock.release()



Class CertLib, FactLib, RepoLib are identical. Use only one. Or create one parent and those 3 will inherit from it.


R:  1: Similar lines in 2 files                                                                                    
==managerlib:508                                                                                                   
==managerlib_async:58                                                                                              
    def filter_pools(self, compatible, overlapping, uninstalled, text):                                            
        """                                                                                                        
        Return a list of pool hashes, filtered according to the given options.                                     

        This method does not actually hit the server, filtering is done in
        memory.                                                           
        """                                                               
        pools = self.incompatible_pools.values()                          

        if compatible:
            pools = self.compatible_pools.values()

        pool_filter = PoolFilter()

        # Filter out products that are not installed if necessary:
        if uninstalled:                                           
            pools = pool_filter.filter_out_installed(pools)       
        else:                                                     
            pools = pool_filter.filter_out_uninstalled(pools)     

        # Do nothing if set to None:
        if overlapping:             
            pools = pool_filter.filter_out_non_overlapping(pools)
        elif overlapping == False:                               
            pools = pool_filter.filter_out_overlapping(pools)    

        # Filter by product name if necessary:
        if text:                              
            pools = pool_filter.filter_product_name(pools, text)

        return pools

    def merge_pools(self, compatible=True, overlapping=None, uninstalled=False,
            text=None):                                                        
        """                                                                    
        Return a merged view of pools filtered according to the given options. 
        Pools for the same product will be merged into a MergedPool object.    

        Overlapping filter by default is None, meaning the pools will not be
        filtered at all. Use True to filter out pools which do not overlap, 
        or False to filter out pools which do.                              
        """                                                                 
        pools = self.filter_pools(compatible, overlapping, uninstalled, text)
        merged_pools = merge_pools(pools)                                    
        return merged_pools                                                  
R:  1: Similar lines in 2 files                                              
==managerlib:487                                                             
==managerlib_async:36                                                        
        self.all_pools = {}                                                  
        self.compatible_pools = {}                                           
        log.debug("Refreshing pools from server...")                         
        for pool in list_pools(self.backend.uep,                             
                self.consumer.uuid, self.facts, active_on=active_on):        
            self.compatible_pools[pool['id']] = pool                         
            self.all_pools[pool['id']] = pool                                

        # Filter the list of all pools, removing those we know are compatible.
        # Sadly this currently requires a second query to the server.         
        self.incompatible_pools = {}                                          
        for pool in list_pools(self.backend.uep,                              
                self.consumer.uuid, self.facts, all=True, active_on=active_on):
            if not pool['id'] in self.compatible_pools:                        
                self.incompatible_pools[pool['id']] = pool                     
                self.all_pools[pool['id']] = pool                              

        log.debug("found %s pools:" % len(self.all_pools))
        log.debug("   %s compatible" % len(self.compatible_pools))
        log.debug("   %s incompatible" % len(self.incompatible_pools))


There is no need to duplicate so much code of PoolStash class. Create PoolStashBase with common code and in both module change only that part which behave differently.
Comment 6 Miroslav Suchý 2011-06-24 02:43:13 EDT
pylint
Comment 7 Bryan Kearney 2011-06-24 09:57:11 EDT
Main offenders are removed. There is a couple of import dups, but I am not going to worry about that.

Fixed in commit:

7828287a80aa9896ff6cf72b27d3c9327c2584bd

in master.
Comment 10 Miroslav Suchý 2011-06-29 05:54:21 EDT
in subscription-manager git:

find src -name '*.py'|xargs pylint

4.5 % of code is still duplicate

and e.g. class factlib.FactLib and repolib.RepoLib and 100% identical. And it is 75% identical to certlib.CertLib

another example is:
./src/subscription_manager/managercli.py function CliCommand._request_validity_check should use utils.get_dbus_iface instead of its 5 first lines.

Additionally to comment #7 - are you sure you committed your work? Because such commit does not exist:
[msuchy@dri/~/rhn/subscription-manager{master}]$ git pull --rebase
Current branch master is up to date.
[msuchy@dri/~/rhn/subscription-manager{master}]$ git show 7828287a80aa9896ff6cf72b27d3c9327c2584bd
fatal: bad object 7828287a80aa9896ff6cf72b27d3c9327c2584bd
Comment 11 Bryan Kearney 2011-06-29 06:46:35 EDT
Please look at:

http://git.fedorahosted.org/git/?p=subscription-manager.git
Comment 13 Miroslav Suchý 2011-06-29 07:44:58 EDT
find src -name '*.py'|xargs pylint

gives me:
Duplication
-----------

+-------------------------+------+---------+-----------+
|                         |now   |previous |difference |
+=========================+======+=========+===========+
|nb duplicated lines      |368   |368      |=          |
+-------------------------+------+---------+-----------+
|percent duplicated lines |4.336 |4.336    |=          |
+-------------------------+------+---------+-----------+
Comment 15 RHEL Product and Program Management 2011-07-28 14:05:34 EDT
Quality Engineering Management has reviewed and declined this request.  You may
appeal this decision by reopening this request.

Note You need to log in before you can comment on or make changes to this bug.