3 ms·

This kind of comments are kind of useless it should be self explanatory " # returns the path and current count that was just made. return {"path": path, "coun

This kind of comments are kind of useless it should be self explanatory " # returns the path and current count that was just made. return {"path": path, "count": int(data[0])}" also maybe extract variables and name them better like: "pipeline_execution_result = await pipeline.execute() count = int(pipeline_execution_result[0])" same here: return [{"path": path.decode('utf-8'), "count": count} for path, count in response] what's in settings.py should probably be in an env file overall: variables and methods name could be clearer, use good names instead of bad comments, extract variables, don't define methods inside of methods it's definitely not BAD code, but I'd expect a guy with 10YoE to do better

wruza 3y ago
That’s pure nagging and notmystyleism, except for the env bit.
formulathree 3y ago
>what's in settings.py should probably be in an env file Have you used Django? Django follows the pattern of a settings file. No environment variables. A global env file imo is definitively worse. It forces you to write code outside of the python ecosystem to extract these variables. Additionally python code references an environment variable that's not explicitly set by the env code could hit some logic errors or unexpected state. What I would do if I had more time is have the settings file reference an env. The main benefit is that settings can be reused across apps and env errors are localized to a single point of failure instead of being littered throughout the app as references to env vars. >This kind of comments are kind of useless it should be self explanatory They are useless. I agree. I put the comments there in case someone disagrees. Doesn't hurt in my opinion. >also maybe extract variables and name them better like You mean with patten matching. No that's actually syntactically less appropriate here. The return value was not a structured product type. It was not a tuple. The return value was a list which implies variable length. Should the list change in size that would change the pattern match. This is most likely just bad typing on the library. But my handling of said type is appropriate. Also it's super minor. >variables and methods name could be clearer, use good names instead of bad comments, extract variables, don't define methods inside of methods I'm actually with you on this one. I prefer clear names over elegant names. But this is not overall sentiment among the majority. Overall people prefer an elegant name over a very descriptive one simply put of some universal intrinsic ocd instinct they all have... even though short elegant names could provide zero informational value. I'm just catering to the majority here. Make myself a comment Nazi as most people don't disparage that and use elegant names as most people prefer that over longer descriptive names. >it's definitely not BAD code, but I'd expect a guy with 10YoE to do better Have you seen code written by people with 10yoe? What you will find is overall 10 yoe doesn't converge on your personal view perfect code. It converges on their view which likely is wildly different from your view. But this thread has been extremely informative on that fact seeing how literally everyone's view is completely different and how everyone thinks they're own personal view of the universe is the enlightened path. The common theme: is lack of unit tests. But to further illustrate the diversity of opinions... You didn't even touch on testing. It took a back seat to "bad comments".
- mejutoco 3y ago
  It is common to load the variables inside the django setting files from an env files. This way in different environments you can have different values, and the secrets are not committed to the repository, but managed externally.
  - formulathree 3y ago
    Yes I mentioned this.

wruza 3y ago

That’s pure nagging and notmystyleism, except for the env bit.

formulathree 3y ago

>what's in settings.py should probably be in an env file Have you used Django? Django follows the pattern of a settings file. No environment variables. A global env file imo is definitively worse. It forces you to write code outside of the python ecosystem to extract these variables. Additionally python code references an environment variable that's not explicitly set by the env code could hit some logic errors or unexpected state. What I would do if I had more time is have the settings file reference an env. The main benefit is that settings can be reused across apps and env errors are localized to a single point of failure instead of being littered throughout the app as references to env vars. >This kind of comments are kind of useless it should be self explanatory They are useless. I agree. I put the comments there in case someone disagrees. Doesn't hurt in my opinion. >also maybe extract variables and name them better like You mean with patten matching. No that's actually syntactically less appropriate here. The return value was not a structured product type. It was not a tuple. The return value was a list which implies variable length. Should the list change in size that would change the pattern match. This is most likely just bad typing on the library. But my handling of said type is appropriate. Also it's super minor. >variables and methods name could be clearer, use good names instead of bad comments, extract variables, don't define methods inside of methods I'm actually with you on this one. I prefer clear names over elegant names. But this is not overall sentiment among the majority. Overall people prefer an elegant name over a very descriptive one simply put of some universal intrinsic ocd instinct they all have... even though short elegant names could provide zero informational value. I'm just catering to the majority here. Make myself a comment Nazi as most people don't disparage that and use elegant names as most people prefer that over longer descriptive names. >it's definitely not BAD code, but I'd expect a guy with 10YoE to do better Have you seen code written by people with 10yoe? What you will find is overall 10 yoe doesn't converge on your personal view perfect code. It converges on their view which likely is wildly different from your view. But this thread has been extremely informative on that fact seeing how literally everyone's view is completely different and how everyone thinks they're own personal view of the universe is the enlightened path. The common theme: is lack of unit tests. But to further illustrate the diversity of opinions... You didn't even touch on testing. It took a back seat to "bad comments".

mejutoco 3y ago
It is common to load the variables inside the django setting files from an env files. This way in different environments you can have different values, and the secrets are not committed to the repository, but managed externally.
- formulathree 3y ago
  Yes I mentioned this.