Skip to main content
added 3184 characters in body
Source Link
J_H
  • 43.9k
  • 3
  • 38
  • 162

EDIT

I read somewhere that for private methods commenting instead of docstring was the standard.

I suspect someone had tooling that would accidentally add _my_helper to published ReadTheDocs web pages if it had a docstring.

Most folks use sphinx, which by default won't do that. I've never seen that it's a concern.

Every method, whether it's part of your Public API or not, has a specification, a contract. It can be poorly specified and implicit. Or you can use a clear English sentence to spell it out in a docstring. We want to know its single responsibility. We want to know what it promises will be true upon return. A docstring is an excellent way to communicate that to your colleagues. For public() and for _private() methods.

The DTM is a digital terrain model. I will change.

No biggie. I mean, I knew that it was some Term of Art, and I knew that I wasn't familiar enough with the literature but probably a maintenance engineer a few months from now would be. So I wasn't worried. I was just offering that a # comment which expands the TLA would have been welcome, is all. And sure, getting elevation would be a helpful phrase in the identifier.

Use "tiff", or "TIFF",

It was just a spelling nit I was quibbling over. We say *.tif in a comment when talking about file extensions. But we say "tiff", or more properly "TIFF", when we mean Tagged Image File Format. (There's no such thing as TIF.)

I didn't understand the dataclass part neither. You meant making other classes?

Yes.

In OO design we look for nouns, "objects", that exist in the Business Domain and that we can model within the machine. Now, a noun could be "big", like a Kitchen and all it contains. Or our modeling could be more granular, and we might model a Stove, a Microwave, and so on.

Your Region has eleven attributes, which isn't necessarily Too Many, but it's a code smell, it's starting to look a little on the big side. It makes you wonder whether there could be some grouping of things, which collectively are their own thing.

So when I got down to that atomic assignment of five attributes I said "Aha!", I had been looking for such an opportunity, and maybe I just found it. Now, it's up to you. I don't know what makes the most sense in your environment. But I offered it as something to consider -- maybe you'd like to group them together. And similarly for that masking 2-tuple.

assignment vs side-effect

If a ctor is creating a House object, I would much rather see

        self.kitchen = self._get_kitchen( ... )

than

        self._build_kitchen( ... )

Now, maybe the "side effect" aspect of that helper is apparent from the name. But I worry that it might update self.plumbing and some other things. Plus, it's hard to unit test. If I can reveal the side effecting via an assignment like that, it will be my preference.


EDIT

I read somewhere that for private methods commenting instead of docstring was the standard.

I suspect someone had tooling that would accidentally add _my_helper to published ReadTheDocs web pages if it had a docstring.

Most folks use sphinx, which by default won't do that. I've never seen that it's a concern.

Every method, whether it's part of your Public API or not, has a specification, a contract. It can be poorly specified and implicit. Or you can use a clear English sentence to spell it out in a docstring. We want to know its single responsibility. We want to know what it promises will be true upon return. A docstring is an excellent way to communicate that to your colleagues. For public() and for _private() methods.

The DTM is a digital terrain model. I will change.

No biggie. I mean, I knew that it was some Term of Art, and I knew that I wasn't familiar enough with the literature but probably a maintenance engineer a few months from now would be. So I wasn't worried. I was just offering that a # comment which expands the TLA would have been welcome, is all. And sure, getting elevation would be a helpful phrase in the identifier.

Use "tiff", or "TIFF",

It was just a spelling nit I was quibbling over. We say *.tif in a comment when talking about file extensions. But we say "tiff", or more properly "TIFF", when we mean Tagged Image File Format. (There's no such thing as TIF.)

I didn't understand the dataclass part neither. You meant making other classes?

Yes.

In OO design we look for nouns, "objects", that exist in the Business Domain and that we can model within the machine. Now, a noun could be "big", like a Kitchen and all it contains. Or our modeling could be more granular, and we might model a Stove, a Microwave, and so on.

Your Region has eleven attributes, which isn't necessarily Too Many, but it's a code smell, it's starting to look a little on the big side. It makes you wonder whether there could be some grouping of things, which collectively are their own thing.

So when I got down to that atomic assignment of five attributes I said "Aha!", I had been looking for such an opportunity, and maybe I just found it. Now, it's up to you. I don't know what makes the most sense in your environment. But I offered it as something to consider -- maybe you'd like to group them together. And similarly for that masking 2-tuple.

assignment vs side-effect

If a ctor is creating a House object, I would much rather see

        self.kitchen = self._get_kitchen( ... )

than

        self._build_kitchen( ... )

Now, maybe the "side effect" aspect of that helper is apparent from the name. But I worry that it might update self.plumbing and some other things. Plus, it's hard to unit test. If I can reveal the side effecting via an assignment like that, it will be my preference.

namedtuples are Good
Source Link
J_H
  • 43.9k
  • 3
  • 38
  • 162

Even nicer would be to define a Bbox named tuple. Then instead of cryptic [3] subscripts we could use (abbreviated) names for the elements.

Even nicer would be to define a Bbox named tuple. Then instead of cryptic [3] subscripts we could use (abbreviated) names for the elements.

added 210 characters in body
Source Link
J_H
  • 43.9k
  • 3
  • 38
  • 162

Your random imports are distracting;Pep-8 pleaseasks that you organize themyour imports, perhaps with $ isort *.py. That delayed Polygon import is certainly a surprise.

Dunno what those empty () brackets are doing there. Makes it look like we have a python2 "new style" class that inherits from object. Just elide them.

Dunno what those empty () brackets are doing there. Makes it look like we have a python2 "new style" class that inherits from object. Just elide them.

We can end the signature annotation with -> None:. But I usually elide that on __init__ ctors, since it's obvious to both man and machine. (It's certainly helpful on other methods and on def main() -> None:.) No need for "None" in the docstring.

It makes the side effect obvious, rather than hoping the reader will infer a side effect from the spelling of the helper's name.

"main" guard

Your random imports are distracting; please organize them with $ isort *.py. That delayed Polygon import is certainly a surprise.

Dunno what those empty () brackets are doing there. Makes it look like we have a python2 "new style" class that inherits from object. Just elide them.

We can end the signature annotation with -> None:. But I usually elide that on __init__ ctors, since it's obvious to both man and machine. (It's certainly helpful on other methods and on def main() -> None:.)

main guard

Pep-8 asks that you organize your imports, perhaps with $ isort *.py. That delayed Polygon import is certainly a surprise.

Dunno what those empty () brackets are doing there. Makes it look like we have a python2 "new style" class that inherits from object. Just elide them.

We can end the signature annotation with -> None:. But I usually elide that on __init__ ctors, since it's obvious to both man and machine. It's certainly helpful on other methods and on def main() -> None:. No need for "None" in the docstring.

It makes the side effect obvious, rather than hoping the reader will infer a side effect from the spelling of the helper's name.

"main" guard

Source Link
J_H
  • 43.9k
  • 3
  • 38
  • 162
Loading