mail@pastecode.io avatar
19 days ago
6.3 kB
This code snippet has few problems:
Fat model: Model is ~200 lines of code.
Separation of concern: Model has schema, business logic, cache operations, database operations
Cyclomatic complexity: Function create_or_update_experiment is fat and has multiple if-else statements
Low Testability: Almost all the functions has side-effects

Goal is to refactor the code, making it modular such that it solves all the above 4 problems.

Write the Unit Tests (only signature and not implementation) for the completed code.
How will you design an API to create an experiment? Focus is on the request data validation, authorisation, response format, request orchestration (different service/backed layer it will go to).

class DBConStore:
   redis = RedisUtils()
   redis_shard_utils = RedisShardingUtils(shard_range=EXP_SHARD_RANGE)
   cache_ops_utils = CustomRedisCache()

class Experiment(models.Model):
   experiment_id = models.CharField(primary_key=True, max_length=60)
   title = models.CharField(null=False, max_length=100)
   description = models.CharField(max_length=100, default=None)
   props = JSONField()
   type = models.CharField(max_length=20)
   is_disabled = models.IntegerField(default=0)
   create_time = models.DateTimeField(auto_now_add=True, editable=True)
   start_time = models.DateTimeField(auto_now_add=True)
   end_time = models.DateTimeField(default=None)
   is_existing_alc_disabled = models.IntegerField(default=0)
   app_name = EnumField(choices=['pocket_fm', 'pocket_fm_lite'], default="pocket_fm")

   class Meta:
       managed = False
       db_table = "experiments"


   def __init__(self, *args, **kwargs):
       super().__init__(*args, **kwargs)
       self.output_fields = get_model_fields(Experiment)
   def get_shard_idx_for_details():
       Get shard for enabling local store for sharded experiment detail keys
       cached_data = get_request_info().get('cached_data')
       store_key = 'exp_shard'
       if cached_data.get(store_key):
           result = cached_data[store_key]
           return result

       return set_shard_num_to_local_store(shard_range=EXP_SHARD_RANGE, store_key=store_key)

   def create_or_update_experiment(self, payload):
       Create or Update an experiment
       :param payload: Data to insert/update
       :type payload: dict
        - Experiment_ID : (optional) Experiment id in case of update
        - Title : (required) Title of experiment
        - Props : (optional) Properties of experiment
        - is_disabled : (optional, default 0) If the experiment should be disabled or not
        - end_time : (optional) End time of experiment
        - update_props : (optional, default 1) If the props should be updated or not
       :return: Created/Updated Data, If created or updated
       :rtype: dict, int
       """ Break this class into three different layers: 
           1. Controller
           2. Domain logic 
           3. Repository classes 
           In repo class also make 2 different classes : 
               1. For Entity 
               2. Repository 
               The repository class would be using entity and saving it into the DB """
        """Rename update to append and use either true or false as values as it would be easier to read"""
       update = int(payload.get("update_props", 1)) # Move this to get from the already extracted payload in the next line. Perform the check for empty payload before getting this 
       payload = filter_dict_keys(payload, self.output_fields)

        """There are two checks that return a bad request: 
            1. if payload is empty 
            2. if both experiment id and titale is empty
            We can combine both of this into one separate method called validatePayload and return the reason from it"""
       if not len(payload):
           return {"status_code": 400}, None

       title = payload.get("title") 
       experiment_id = payload.get("experiment_id")
       if title is None and experiment_id is None:
           return {"status_code": 400}, None
       elif experiment_id is None:
           """Put the below lines in a separate method as create new Experiment """
           key = title + str(time.time())
           experiment_id = get_hash_key(key)
           payload["experiment_id"] = experiment_id
           props = {}
           """Use conditional instead of try-catch to populate props. This is not a valid use-case of using try-catch"""
           """Put the entire below logic in a method called as modify existing experiment """
               props = Experiment.objects.get(experiment_id=experiment_id).props
               props = {}
               if title is None:
                   return {"status_code": 400}, None
           if isinstance(props, str):
               props = json.loads(props)

        """Move the below logic to a separate method called updateProps"""
       if "props" in payload:
           if update == 1:
               props = payload["props"]
       payload["props"] = props

        """ Add logic to validate that if experiment id is coming in the call is bogus. If so throw and error rather than creating it""""
       experiment_obj, is_created = Experiment.objects.update_or_create(
           experiment_id=experiment_id, defaults=payload

       if not is_created:
           # Invalidate all shards
           """Make a sepratae private method to invalidate all shards a it is getting used at multiple places"""
           key, _ = RedisKeySchema.get_experiment_detail_key(experiment_id)

        """Mske this as a separate method called serialize ouput. Return the final output from it"""
       experiment_json = model_to_dict(experiment_obj)
       experiment_json["create_time"] = experiment_obj.create_time
       if type(experiment_json["props"]) is str:
           experiment_json["props"] = json.loads(experiment_json["props"])

       return experiment_json, is_created

Leave a Comment