0
\$\begingroup\$

the introduction is this:

You are a member of a biotechnology programming team that is responsible for creating a system for lab technicians, which will assist them with drug analysis.

Your goal is to create the application that will let them input their findings into the system, provide a meaningful analysis and verify the correctness of the data that they have sent.

tasks:

Your goal in this part is to implement the app.drug_analyzer.DrugAnalyzer class. It will be responsible for analyzing data like the data presented below:

+-----------+-------------+------------------+-------------+
|   pill_id | pill_weight | active_substance | impurities  |
+-----------+-------------|------------------|-------------|
|    L01-10 | 1007.67     | 102.88           | 1.00100     |
|    L01-06 |  996.42     | 99.68            | 2.00087     |
|    G02-03 | 1111.95     | 125.04           | 3.00004     |
|    G03-06 |  989.01     | 119.00           | 4.00062     |
+-----------+-------------+-------------+-------------+-----

The initialization of the class can be done from Python's list of lists (or nothing) and stored in the instance variable called data as per example below:

>>> my_drug_data = [
...                 ['L01-10', 1007.67, 102.88, 1.00100],
...                 ['L01-06', 996.42, 99.68, 2.00087],
...                 ['G02-03', 1111.95, 125.04, 3.00100],
...                 ['G03-06', 989.01, 119.00, 4.00004]
... ]
>>> my_analyzer = DrugAnalyzer(my_drug_data)
>>> my_analyzer.data
[['L01-10', 1007.67, 102.88, 0.001], ['L01-06', 996.42, 99.68, 0.00087], > ['G02-03', 1111.95, 125.04, 0.00100], ['G03-06', 989.01, 119.00, 0.00004]]
>>> DrugAnalyzer().data
[]

The class should also have an option to add single lists into the object. Adding a list to the DrugAnalyzer object should return a new instance of this object with an additional element. Adding improper type or a list with improper length should raise a ValueError. An example of a correct and wrong addition output is shown below:

>>> my_new_analyzer = my_analyzer + ['G03-01', 789.01, 129.00, 0.00008]
>>> my_new_analyzer.data
[['L01-10', 1007.67, 102.88, 0.001], ['L01-06', 996.42, 99.68, 0.00087], > ['G02-03', 1111.95, 125.04, 0.00100], ['G03-06', 989.01, 119.00, 0.00004], ['G03-01', 789.01, 129.00, 0.00008]]
>>> my_new_analyzer = my_analyzer + ['G03-01', 129.00, 0.00008]
Traceback (the most recent call is displayed as the last one):
  File "<stdin>", line 1, in <module>
ValueError: Improper length of the added list.

Part 2

Implement the verify_series method inside the app.drug_analyzer.DrugAnalyzer class.

The goal of this method is to receive a list of parameters and use them to verify if the pills described inside the instance variable data matches the given criteria. It should return a Boolean value as a result.

The function would be called as follows:

verify_series(series_id = 'L01', act_subst_wgt = 100, act_subst_rate = 0.05, allowed_imp = 0.001)

Where:

  • the series_id is a 3 characters long string that is present at the beginning of every pill_id, before the - sign; for example, L01 is the series_id in pill_id = L01-12.
  • the act_subst_wgt is the expected weight (mg) of the active substance content in the given series in one pill.
  • the act_subst_rate is the allowed rate of difference in the active substance weight from the expected one. For example, for 100 mg, the accepted values would be between 95 and 105.
  • the allowed_imp is the allowed rate of impure substances in the pill_weight. For example, for 1000 mg pill_weight and 0.001 rate, the allowed amount of impurities is 1 mg.

The function should take all pills that are part of the L01 series, sum their weight and calculate if the amount of active_substance, as well as impurities, match the given rates. It should return True if both conditions are met and False if any of them is not met.

The False result should mean that all the passed parameters are proper, but either the active substance amount or the impurities amount is improper. In case of a series_id that is not present in the data at all or in case of any improper parameter, the function should throw a ValueError. Please think what could be the possible edge case in such a scenario.

Example:

>>> my_drug_data = [
...                 ['L01-10', 1000.02, 102.88, 1.00100],
...                 ['L01-06', 999.90, 96.00, 2.00087],
...                 ['G02-03', 1000, 96.50, 3.00100],
...                 ['G03-06', 989.01, 119.00, 4.00004]
... ]
>>> my_analyzer = DrugAnalyzer(my_drug_data)
>>> my_analyzer.verify_series(series_id = 'L01', act_subst_wgt = 100, act_subst_rate = 0.05, allowed_imp = 0.001)
False
>>> // The overall active_substances weight would be 198.88, which is within the given rate of 0.05 for 200 mg (2 * act_subst_wgt).
>>> // However, the sum of impurities would be 3.00187, which is more than 0.001*1999.92 (allowed_imp_rate * (1000.02 + 999.90).
>>> my_analyzer.verify_series(series_id = 'L01', act_subst_wgt = 100, act_subst_rate = 0.05, allowed_imp = 0.0001)
True
>>> my_analyzer.verify_series(series_id = 'B03', act_subst_wgt = 100, act_subst_rate = 0.05, allowed_imp = 0.001)
Traceback (the most recent call is displayed as the last one):
  File "<stdin>", line 1, in <module>
ValueError: B03 series is not present within the dataset.

my code passed through all of tests

After my first failure I tried to make the code as compact and light as possible, using list comprehension and eliminating unnecessary variables, even though it added just 5% more to my score. Can someone can tell me what is wrong with my code and how I can write better code?

my latest code

(score: 58%)

  class DrugAnalyzer:
    def __init__(self, data):
        self.data = data

    def __add__(self, data):
        if len(data) == 4:
            if all(isinstance(i, float) for i in data[1:]) and isinstance(data[0], str):
                self.data = self.data + [data]
                return self
            else:
                raise ValueError('Improper type on list added')
        else:
            raise ValueError('improper length on list added')

    def verify_series(
        self,
        series_id: str,
        act_subst_wgt: float,
        act_subst_rate: float,
        allowed_imp: float,
    ) -> bool:
        pills = [pill for pill in self.data if series_id in pill[0]]
        if pills:
            return act_subst_wgt*len(pills)-(act_subst_wgt * len(pills) * act_subst_rate) < sum([i[2] for i in pills]) < act_subst_wgt*len(pills)+(act_subst_wgt * len(pills) * act_subst_rate) and sum([i[3] for i in pills]) < allowed_imp*sum([i[1] for i in pills])
        else:
            raise ValueError(f'There is no {series_id} series in database')

previous version

(score: 55%)

class DrugAnalyzer():
    def __init__(self,data):
        self.data = data

    def __add__(self,data1):
        if len(data1) < 4:
            raise ValueError('improper lenght of list added')
        if type(data1[0]) == str:
            if type(data1[1]) and type(data1[2]) and type(data1[3]) == float:
                self.data = self.data + [data1]
                return self
            else:
                raise ValueError('Improper type on list added')
        else: raise ValueError('Improper type on list added')


    def verify_series(
        self,
        series_id: str,
        act_subst_wgt: float,
        act_subst_rate: float,
        allowed_imp: float,
    ) -> bool:
        serie = []
        for pill in self.data:
            if series_id in pill[0]:
                serie.append(pill)
                active = []
                imp = []
                weight = []
                print(serie)
                for pill in serie:
                    weight.append(pill[1])
                    active.append(pill[2])
                    imp.append(pill[3])

        if serie == 0:
            raise ValueError('the serie isnt in list')

        dif = (act_subst_wgt*len(serie))*act_subst_rate
        if  act_subst_wgt*len(serie)-dif < sum(active) < act_subst_wgt*len(serie)+dif and sum(imp) < allowed_imp*sum(weight):
            return True

        else:
            return False
\$\endgroup\$
3
  • 4
    \$\begingroup\$ Welcome to Code Review (again)! The current question title, which states your concerns about the code, is too general to be useful here. Simply deleting your old question and reposting it as a new question, without fixing issues like the title is not fixing the problem. Edit the title to something like "Drug Analyzer". Put your concerns in the question's body. If this is an online challenge, post a link to it. \$\endgroup\$
    – AJNeufeld
    Commented Jul 30, 2021 at 19:14
  • 1
    \$\begingroup\$ It's worth stating that this is the entry test for Brainly recruitment. \$\endgroup\$
    – Reinderien
    Commented Jul 31, 2021 at 0:02
  • \$\begingroup\$ What does the "score" mean? If it's a fraction of unit tests that pass, then clearly the code isn't finished and ready for review. If it's a performance measure (and functionally incorrect code doesn't get any score), then please say so, and add the performance tag to indicate what kind of review is required. \$\endgroup\$ Commented Jul 31, 2021 at 7:03

2 Answers 2

3
\$\begingroup\$

I don't know how the code is scored, but flat is better than nested. If you want to raise an exception, do it early. Consider

def __add__(self, data):
    if len(data) != 4:
        raise ValueError('improper length on list added')
    if not all(isinstance(i, float) for i in data[1:]) or not isinstance(data[0], str):
        raise ValueError('Improper type on list added')
    self.data = self.data + [data]
    return self

I am also not clear why do you insist on data[1:] being float. Fraction would work perfectly fine, as would many other types do, as long as + and < are defined.


series_id is a 3 characters long string that is present at the beginning of every pill_id, before the - sign

I don't see how this requirement is implemented.


        act_subst_wgt*len(pills)-(act_subst_wgt * len(pills) * act_subst_rate) < sum([i[2] for i in pills]) < act_subst_wgt*len(pills)+(act_subst_wgt * len(pills) * act_subst_rate) and sum([i[3] for i in pills]) < allowed_imp*sum([i[1] for i in pills])

is utterly unreadable.

\$\endgroup\$
1
  • \$\begingroup\$ Your second point about float could be improved by saying to use numbers.Real instead in the isinstance check. \$\endgroup\$
    – Peilonrayz
    Commented Jul 31, 2021 at 11:51
1
\$\begingroup\$

I don't know about how these scores are being figured, so I can't say what would improve your score. Your code looks like an OK "face value" implementation of the spec, with two three details "wrong".

  1. The spec doesn't say that the addition operation should modify self, it actually implies that the operation should not modify self. "Adding a list to the DrugAnalyzer object should return a new instance of this object with an additional element." This is consistent with how we usually think of +; 5+1 returns 6, but it doesn't change 5.
  2. if series_id in pill[0] will be True for 'L01' and 'G02-L01'; you want startswith. (Note that it's a method of the larger string.)
  3. You didn't account for the empty constructor call.

Other stuff:

  • You're using type hints; that's great! Make sure you're validating them with MyPy, and go ahead and use them for everything.
  • pycodestyle will help keep your formatting standardized. It's pretty strict; I don't always use it. But your line-23 is really bad!
  • There's no reason I can imagine for the validation to only happen when adding to an existing object; it should be centralized. Maybe it could be centralized in its own class? That way you could have a consistent "list of row objects" instead of a less trustworthy "list of lists". Some extra conversion machinery will be needed though... Anyway, I'd use dataclasses for this.
  • The validation code itself could be tightened up a little, in my opinion.
  • The spec says "... the function should throw a ValueError. Please think what could be the possible edge case in such a scenario." Maybe they mean negative numbers or something; IDK? Whoever's designing this API isn't doing a good job, but that's out of your hands!
  • Many functions (like sum) can take bare generators instead of a list.
  • I think for the verification inequalities, you should technically use the "or-equal-to" versions, but I would really hope it doesn't matter!

IDK how helpful this version will be to you. If you run it you'll find that the assertions at the end don't pass; they're based on the shell output examples in your question, which seem to have incorrect values in the last column of the output? This could be a really important detail you've overlooked, but I'm out of time. Sorry!

import collections.abc as c  # these are _classes_, not _types_.
from dataclasses import dataclass
from numbers import Real  # again, a class, for typing we'll use float.
from typing import Any, Iterable, Sequence, List  # these are _types_.
#    You can just use List for everything; I like to split hairs.


@dataclass(frozen=True)  # I think they should be frozen by default.
class PillRow:
    pill_id: str
    series: str
    weight: float
    active: float
    impure: float

    @staticmethod
    def from_data(data: Any) -> "PillRow":
        if isinstance(data, PillRow):
            return data
        try:
            assert isinstance(data, c.Sized) and isinstance(data, c.Iterable), ("Could not parse data.", data)
            assert 4 == len(data), f"Four columns needed; found {len(data)}."
            pill_id, pill_weight, active_substance, impurities = data
            assert isinstance(pill_id, str), ("Column One must be the pill id.", pill_id)
            series_id = pill_id.split('-')[0]
            assert 3 == len(series_id), ("Could not parse series id from the pill id.", pill_id)
            assert isinstance(pill_weight, Real), ("Invalid pill weight.", pill_weight)
            assert isinstance(active_substance, Real), ("Invalid active-substance weight.", active_substance)
            assert isinstance(impurities, Real), ("Invalid impurities weight.", impurities)
            return PillRow(pill_id=pill_id,
                           series=series_id,
                           weight=float(pill_weight),
                           active=float(active_substance),
                           impure=float(impurities))
        except AssertionError as ae:
            raise ValueError(ae)

    def to_list(self) -> list:
        return [self.pill_id, self.weight, self.active, self.impure]


class DrugAnalyzer:
    def __init__(self, *datas: Iterable[Sequence]):  # taking *args will make __add__ easier to write.
        self._data = [PillRow.from_data(pill)
                      for data in datas
                      for pill in data]
        self.data = [pill.to_list() for pill in self._data]

    def __add__(self, data):
        return DataAnalyzer(self._data, [data])  # that's what all the above work was for!

    def verify_series(self,
                      series_id: str,
                      act_subst_wgt: float,
                      act_subst_rate: float,
                      allowed_imp: float) -> bool:
        pills = [p for p in self._data if p.series == series_id]
        if not pills:
            raise ValueError(f'There is no {series_id} series in database')
        else:
            num_pills = len(pills)
            target_active = act_subst_wgt * num_pills
            margin_active = target_active * act_subst_rate
            actual_active = sum(p.active for p in pills)
            max_impure = allowed_imp * sum(p.weight for p in pills)
            actual_impure = sum(p.impure for p in pills)
            return (abs(target_active - actual_active) <= margin_active) and (actual_impure <= max_impure)


if __name__ == "__main__":
    # tests constructors
    my_drug_data = [
            ['L01-10', 1007.67, 102.88, 1.00100],
            ['L01-06', 996.42, 99.68, 2.00087],
            ['G02-03', 1111.95, 125.04, 3.00100],
            ['G03-06', 989.01, 119.00, 4.00004]
    ]
    my_analyzer = DrugAnalyzer(my_drug_data)
    assert tuple(map(tuple, my_analyzer.data)) == (
            ('L01-10', 1007.67, 102.88, 0.001),
            ('L01-06', 996.42, 99.68, 0.00087),
            ('G02-03', 1111.95, 125.04, 0.00100),
            ('G03-06', 989.01, 119.00, 0.00004)
        ), tuple(map(tuple, my_analyzer.data))
    assert tuple(DrugAnalyzer().data) == (), DrugAnalyzer().data

    # tests plus
    my_new_analyzer = my_analyzer + ['G03-01', 789.01, 129.00, 0.00008]
    assert tuple(map(tuple, my_new_analyzer.data)) == (
            ('L01-10', 1007.67, 102.88, 0.001),
            ('L01-06', 996.42, 99.68, 0.00087),
            ('G02-03', 1111.95, 125.04, 0.00100),
            ('G03-06', 989.01, 119.00, 0.00004),
            ('G03-01', 789.01, 129.00, 0.00008)
        ), my_new_analyzer.data
    thrown = False
    try:
        my_new_analyzer = my_analyzer + ['G03-01', 129.00, 0.00008]
    except ValueError:
        thrown = True
    assert thrown, my_new_analyzer

    # tests verify
    my_drug_data = [['L01-10', 1000.02, 102.88, 1.00100],
                    ['L01-06', 999.90, 96.00, 2.00087],
                    ['G02-03', 1000, 96.50, 3.00100],
                    ['G03-06', 989.01, 119.00, 4.00004]]
    my_analyzer = DrugAnalyzer(my_drug_data)
    assert not my_analyzer.verify_series(series_id='L01',
                                         act_subst_wgt=100,
                                         act_subst_rate=0.05,
                                         allowed_imp=0.001)
    assert my_analyzer.verify_series(series_id='L01',
                                     act_subst_wgt=100,
                                     act_subst_rate=0.05,
                                     allowed_imp=0.0001)
    thrown = False
    try:
        my_analyzer.verify_series(series_id='B03',
                                  act_subst_wgt=100,
                                  act_subst_rate=0.05,
                                  allowed_imp=0.001)
    except ValueError:
        thrown = True
    assert thrown
```
\$\endgroup\$
3
  • \$\begingroup\$ Asserting, catching the assertion, and re-raising as a ValueError trashing the original stack is... not great? You'd be better-off replacing the assertions with individual raises of your own error type. Even if you kept your current assert-reraise pattern, you would want to perhaps raise ValueError(str(ae)) from ae to preserve the original stack. \$\endgroup\$
    – Reinderien
    Commented Jul 31, 2021 at 0:00
  • \$\begingroup\$ It's not great. The spec asks for a specific error type, and I really think having a separate if...raise... for each cause is too verbose. Asserting/catching/re-raising smells bad, but gets the point across. Using raise...from preserves some extra information, which is good, but in this case it actually makes the error messages harder to read, so I feel conflicted. Ideally/intuitively the "error" clause of an assert... statement could be the actual exception you want raised, but python doesn't do that :( \$\endgroup\$ Commented Jul 31, 2021 at 19:00
  • \$\begingroup\$ I suppose there's always the danger of someone running this with the -O flag set... \$\endgroup\$ Commented Jul 31, 2021 at 19:04

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