Simple interface for collecting issues after a list of validations

Question

class RadarChecker:
 def __init__(self) -> None:
 self._issues = set()
 @property
 def issues(self) -> Set[str]:
 return self._issues
 @property
 def _checkers(self) -> Iterable[Callable]:
 return (
 self._check_1,
 self._check_2,
 self._check_3,
 )
 def check(self) -> None:
 for checker in self._checkers:
 ok, issue = checker()
 if not ok:
 self._issues.add(issue)

The current interface of the RadarChecker class consists of two methods: check and issues. The whole purpose of the check method is that it triggers computation of the issues that are returned by the issues method. So, all users of RadarChecker call issues eventually:

checker = RadarChecker()
checker.check()
for issue in checker.issues:
 # process issue

Is it possible to provide the same functionality with a simpler interface?

I'm thinking about defining the RadarIssues class that implements the Iterable[str] interface. We provide the same info to RadarIssues that we do to RadarChecker but instead of calling issues to get the issues, the users would simply iterate over the instance of RadarIssues. This would look roughly as follows:

radar_issues = RadarIssues()
for issue in radar_issues:
 # process issue

Is it better? I will appreciate criticism and other ideas.

Answer 1

What you have is neither crazy nor terrible.

You've started some type hinting (good!) but it's incomplete. For instance

self._issues: Set[str] = set()

and

@property
def checkers(self) -> Tuple[
 Callable[
 [], 
 Tuple[bool, str],
 ], ...
]:

Note that it's good practice to add hints accepting the most generic possible type and return the most specific possible type, so Tuple instead of Iterable.

There's not a whole lot of win in having _issues as a private with a public accessor, because private is only a suggestion in Python and users can still get to that member. You might as well just have a public issues set. However, the better thing to do is make this class stateless - keep your checkers (which can be made public - again, private is kind of a lost cause in Python); do not store issues on the class; and yield issues as a generator from check().

• Ivan Petrushenko: Thanks! Could you please provide an implementation for check with generators? Not really sure how it will look like.
• Ivan Petrushenko: def check(self) -> Iterable[str]: for checker in self._checkers: ok, issue = checker() if not ok: yield issue

Answer 2

In this case, I would really keep it simple. I would drop the OOP and just write a simple function that validates the radar and returns a list of issues. To be clear:

def check_radar(radar: Radar) -> Set[str]:
 return {
 *_check_1(radar),
 *_check_2(radar),
 ...
 }
def _check_1(radar: Radar) -> Set[str]:
 # perform check and return set() or {"issue"}

The example above is just an idea for the implementation, you can then implement the internals as you wish, for instance by defining a list of checker functions and iterating and appending to the list as you're doing in your sample code. The issues could be str or some more complicated object, depending on your needs.

This not only avoids having to write and maintain a class that in the end has just a single method kind of an unusual interface, but also writing a class that behaves like an Iterable[str] and which make the API harder to understand.

Another note: instead of ok, issue = checker() you could just return None when there's no issue, which would simplify early returns from the checker functions.

I know it's not really OOP, but there's no need to use OOP everywhere if another pattern solves the problem more intuitively.