3
\$\begingroup\$

I have a factory method that should accept a list of coordinate lists, which are pairs of numbers, for example [[(2,4),(3,3),(2,4)],[(6,6),(7,6),(7,7),(6,7),(6,6)]]. I'd like to make sure the input is indeed in this format, but without specifying the types of sequences (might be lists, tuples, numpy arrays, and mixtures of those) or format of the numbers (int, float, etc., but no strings). So I wrote this checking method:

def _check_args(self,coords_features):
    """Check whether the input is a list of lists of pairs of numbers."""
    err = "Invalid input (not a list of coordinate lists): {}".format(coords_features)
    try:
        for coords in coords_features:
            for coord in coords:
                assert(len(coord) == 2)
                for f in coord:
                    1+f
    except (TypeError,AssertionError):
        raise ValueError(err)

Does this look "pythonic" and non-restrictive enough, or should I do anything differently?

\$\endgroup\$
1
\$\begingroup\$

My main problem with this method is that you don't make a useful docstring and consequently it's unclear what should or shouldn't pass.

You say that you're checking if the input is a lost of lists of pairs of numbers, but not explaining how. If I had a class with __len__ defined and could be iterated over, that would pass your test. That might sound unlikely, but consider someone passing a set of generators:

>>> coords = [xrange(2)] * 10
>>> _check_args(coords)

...no errors. But xrange most certainly does not return lists. If you only need the pairs to iterate over you'll be fine, but if you want to index them to get the two values then a generator will throw errors. This method should be testing what aspects of the data you need, such as indexing, not just using methods that indicate they're collections or collection like objects.

Once you know what does need to be tested this way, it would be beter to document it clearly in the docstring, so that if unusual data passes, the user can be sure that's because it's acceptable for your purposes and not just something you didn't account for.


Some other quick notes, assert is a statement not a function. You don't need to use brackets for it:

assert len(coord) == 2

You build the error string up front, which seems unnecessary and slightly wasteful as you may never need it. If you want to create it on its own line then you should still put it in the except block.

Your names could be clearer. I don't know what a coords_features is. arguments or args could be better names, since this function isn't really co-ordinate specific so much as it is format specific. I'd recommend either coordinates or pairs for the longer lists and coordinate or pair for the 2 item collections. The choice depends on how broad or narrow you intend this to be, but the docstring currently sounds more generic.

Last nitpick, in your error message you refer to lists only. If you want it to be broad you may want to use Collection as that refers to the range of list and dict like objects. Though if your intent is to encourage the user to use lists then it's a good error message.

| improve this answer | |
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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