5
\$\begingroup\$

First of all, I am playing around with python for about a week tops. Absolute beginner here.

My story: I work as an IT administrator so I am familiar with eg. PS on a very basic level. I actually got into python due to the fact that I am plotting/farming chia. Now a rather long holiday is coming up, so I thought to myself I might as well write a discord bot with a couple of commands that would allow me to monitor pc resourcer, perhaps initiate PC restarts and etc....

Now I got a couple of functions done including the one I want to ask about, this script is giving back the number of files on the specified locations with the extension .plot.

First of all, this script is doing what I want it to do. However! Its very static, meaning I actually set locations in variables, which is good for now, but in the perfect scenario it would certinaly not list files with .plot extension should they be in any other location.

My second concern is that even though I have used functions to connect to my samba server (which is running on a PI4) I have a feeling this part of the code is not being used. And as long as I previously autechnicated against the IP shares on windows my script would work with the "cached" credentials.

All in all here is my code, it is probably a HUGE mess, but! could you please provide some insight on specific parts of the code to make it better, "more professional" so to say.

#!/usr/local/bin/python3
import os

from smb.SMBConnection import SMBConnection
host="xxx.xxx.xxx.xxx"  #ip or domain name to SMB server
username="***"          #SAMBA server username
password="xxxxx"        #SAMBA server password
sharename="\\\\xxx.xxx.xxx.xxx\\chia1"
sharename2="\\\\xxx.xxx.xxx.xxx\\chia2"
sharename3="I:"
conn=SMBConnection(username,password,"","",use_ntlm_v2 = True)
result = conn.connect(host, 445)

for root, dirs, files in os.walk(sharename):
    for Files in files:
        if Files.endswith(".plot"):
            totalfiles += 1

for root, dirs, files in os.walk(sharename2):
    for Files in files:
        if Files.endswith(".plot"):
            totalfiles += 1

for root, dirs, files in os.walk(sharename3):
    for Files in files:
        if Files.endswith(".plot"):
            totalfiles += 1

result = ":page_facing_up: Total number of plots: "

printoutput = ('{}{}'.format(result, totalfiles))

print('{}{}'.format(result, totalfiles))
New contributor
as role is a new contributor to this site. Take care in asking for clarification, commenting, and answering. Check out our Code of Conduct.
\$\endgroup\$
5
\$\begingroup\$

It's generally a very bad idea to put credentials in a program. That's a bigger topic, and the best approach will vary depending on your situation. But you should address this problem.

When naming things, let ordinary English usage guide you: plural for collections (lists, tuples, dicts, etc) and singular for individual values. For example, use files and file (or even just f if the context is clear), not files and Files.

Let your code breathe by putting spaces around operators and related items (equal signs, commas, etc). It helps with readability and often editability (latter can depend on the editor). Search for PEP8 if you want more details on recommended naming and layout practices (PEP8 is not gospel, but it is a good starting point).

It's often a good idea to group similar data into collections: for example, a tuple/list/dict of shares rather than sharename, sharename2, sharename3.

When you see repetition in your code, get rid of it. It's as simple as that. Usually, you can do this by writing a function to generalize the behavior: for example, a function taking a share and returning a count. In your specific case, the grouping of the shares in a collection provides a more direct route: kill the repetition via a loop over that collection. And since the logic inside the loop is just counting, you can stick the whole thing in a single sum() expression (this works because bool is a subclass of int).

shares = (
    "\\\\xxx.xxx.xxx.xxx\\chia1",
    "\\\\xxx.xxx.xxx.xxx\\chia2",
    "I:",
)

ext = '.plot'

totalfiles = sum(
    f.endswith(ext)
    for share in shares
    for _, _, files in os.walk(share)
    for f in files
)

You asked about making the script less hardcoded. The most natural way to do that is via command-line arguments and the argparse module. That's a bigger topic, but the basic idea is that the share paths and the extension (.plot) should be variable parameters with default values. Run the script without arguments, and it will do the default thing. Run it with arguments and/or options, and it will use different parameters. That's the time-tested way to go with scripts like this, and the internet is full of examples illustrating how to set up argparse.

Finally, if you intend to write more scripts as you build your Chia Empire, just adopt the practice of putting all code inside of functions. It might seem like an unnecessary hassle at first, but this is another one of those time-tested ideas. The benefits are numerous. Here's the basic structure:

import sys

def main(args):
    # Program starts here.
    print(args)

if __name__ == '__main__':
    main(sys.argv[1:])
\$\endgroup\$
1
  • \$\begingroup\$ Thank you! I managed to edit my code according to your inputs! Looks much better and feels more smooth. \$\endgroup\$ – as role 2 days ago
0
\$\begingroup\$

Raw Strings

Your code currently suffers from Leaning Toothpick Syndrome. The directory separator backslash \ in Windows paths needs to be escaped in strings (\\) to avoid being mistaken for an escape sequence like \n, \r, \t, or \x. A similar problem happens with regular expressions, which use the backslash to introduce terms like whitespace (\s).

Python provides us with Raw Strings, allowing us to avoid doubling all backslash. Simply proceeding a string with r enables the raw mode, where backslashes are not escapes -- they are just backslashes.

sharename="\\\\xxx.xxx.xxx.xxx\\chia1"
sharename2="\\\\xxx.xxx.xxx.xxx\\chia2"

becomes

sharename = r"\\xxx.xxx.xxx.xxx\chia1"
sharename2 = r"\\xxx.xxx.xxx.xxx\chia2"

Glob

Instead of using os.walk, you might consider using glob.glob("**/*.plot"), or glob.iglob("**/*.plot") to find and return just your desired .plot files.

Perhaps better, use the newer pathlib library, and use .rglob("*.plot") to recursively find the *.plot files:

from pathlib import Path

shares = (
    Path(r"\\xxx.xxx.xxx.xxx\chia1"),
    Path(r"\\xxx.xxx.xxx.xxx\chia2"),
    Path("I:"),
    )

pattern = '*.plot'

total_files = sum(1
                 for share in shares 
                 for _ in share.rglob(pattern))

Unused

printoutput = ('{}{}'.format(result, totalfiles))

is never used, and can be removed.

F-strings

The formatting code ...

print('{}{}'.format(result, totalfiles))

is hard to read. The reader has to match the first {} with the first argument in .format(...), the second with second and so on.

Python 3.6 introduces f-strings (f"..."), which allow you to embed variables (or expressions) directly in the formatting string:

print(f'{result}{total_files}')

Of course, the result variable is not necessary:

print(f':page_facing_up: Total number of plots: {total_files}')

Alternately, if you wanted the print_output variable for other purposes:

print_output = f':page_facing_up: Total number of plots: {total_files}'
print(print_output)
\$\endgroup\$
1
  • \$\begingroup\$ Thank you for this! I highly appreciate your input, very easy to understand and provided huge amounts of info, I will review this and try and apply it to my own code from scratch. Dont just simply like to copy and paste, that defeates the purpose of learning! \$\endgroup\$ – as role 2 days ago

Your Answer

as role is a new contributor. Be nice, and check out our Code of Conduct.

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.