7
\$\begingroup\$

I am adding a feature to a program that will let the user add new dictionaries as a file to the file folder to be used as a note taking repository. I just got the code finished for creating the file name from user input, however it feels as if there should be a smarter way to perform the task than what I have done.

The following is what I have come up with that does work but I don't know if I should be doing this in a shorter method with either a built in function or a smarter one-liner.

Can you explain any ways that you think this could be improved? I have searched for other solutions but they were not well explained enough that I could understand them very clearly.

EDIT: Added my imports to code. Note: the print statement at the bottom was just to test the result of the code and see it in console. It will be changed to return when I finish up the rest of the program.

from tkinter import *
import sys
import time
import tkinter.messagebox
import tkinter.simpledialog
import json

finalFilename =''
def new_lib_prompt(event=None):
    finalFilename =''
    name_of_new_library = tkinter.simpledialog.askstring('Create New Note Library', 'Alphanumeric lowercase and "_" only', initialvalue = "Name_Here")
    from string import ascii_letters,digits
    validFilenameChars = "-_.() %s%s" %(ascii_letters, digits)
    stringofnewname = str(name_of_new_library)
    stringofvalid = str(validFilenameChars)
    for i in stringofnewname:
        if i in stringofvalid:
            if i == ' ':
                new_i = '_'
                finalFilename += ''.join(new_i)
            else:
                lower_i = i.lower()
                finalFilename += ''.join(lower_i)
        else:
            pass
    print (finalFilename)

EDIT: 04/21/2017 2:30 PM

Thanks for all the great feedback everyone and thanks to holroy for his very useful and thought out response. If anyone is interested here is the solution I was looking for after taking advice from a few answers.

valid_filename = '' # will be using this in another function later
def new_lib_prompt(event=None):
    global valid_filename
    a_name = tkinter.simpledialog.askstring('Create New Note Library',
                                            'Alphanumeric lowercase and "_" only',
                                            initialvalue = "Name_Here")
    VALID_CHARS = '-_.() {}{}'.format(ascii_letters, digits)
    valid_filename = (''.join(c for c in a_name if c in VALID_CHARS)).replace(' ', '_').lower()
\$\endgroup\$
  • 1
    \$\begingroup\$ Welcome to Code Review! I hope you get some great answers. \$\endgroup\$ – Phrancis Apr 21 '17 at 16:12
  • \$\begingroup\$ Thanks for the welcome! Came over here form stackoverflow per a suggestion for this type of question. I 2 hope I get a good response. \$\endgroup\$ – Mike - SMT Apr 21 '17 at 16:26
  • \$\begingroup\$ I'm sure you will, we have a good number of Python experts here :_) \$\endgroup\$ – Phrancis Apr 21 '17 at 16:32
  • \$\begingroup\$ Be very careful adding code to your question after answers start to trickle in. It could invalidate the answer and labour of the reviewers, and change the entire concept. With that being said, to add the imports at the start is not a vital change (but moving the string import would invalidate answers, and would most likely be rolled back) \$\endgroup\$ – holroy Apr 21 '17 at 18:49
  • \$\begingroup\$ I did not move the string import for that reason. \$\endgroup\$ – Mike - SMT Apr 21 '17 at 18:57
6
\$\begingroup\$

First of all let's address some of the more prominent style issues of your code, before diving into an alternate approach to solving it.

  • Have imports at top of code – At first I didn't see the from string import ... line as it was hidden in the code, but this is not good coding practice. Put this at the top of the file, so it is easy to see.
  • Have more vertical space – Please add an extra line here and there, to increase readability in your code. Typically I would add spaces in front of for loops or if block to accentuate these blocks.
  • Be consistent in variable naming – Naming variables is hard but choose one style for variable naming, and don't mix it like name_of_new_library, validFilenameChars and stringofnewname. This make the code a whole lot harder to read, which in turns makes it harder to maintain in the long run.
  • Functions should return, not print their result – To have a function print its output is usually a code smell, and it would be better to have it return the result. If by some reasoning it's required to print it out, I would maybe indicate that in the function name.
  • Using the % operator is depreceated in favor of str.format – See format examples for the newer and better method of adding arguments into strings.
  • Why the event=None – I haven't used tkInter a lot, so you might need it for some reason, but it kind of looks superfluous to me as you don't use the event in the function at all.
  • Why the str(some_variable)? – Why do you do this? Is this related to tkInter somehow as well? Do you really need to "stringify" the return from tkInter?
  • Only join when needed! – The join command is mainly used to join lists, so joining a list of one character is extra work, and not needed. To append a single character you could do txt += char. See below for a better use of join.

Algorithm comments

When summarized your code does the following things:

  • Reads a suggestion using tkInter
  • Builds a constant of validFilenameChars
  • Converts any spaces into underscores, and
  • lowercases all other characters

As covered before the import should be done at top, and so should also declaration of constants. When you start validifying the name you split the string into single characters, and use a somewhat strange += ''.join(new_i) to add onto the new filename.

There does however exist functions which does both of your required operations. See str.lower() and str.replace(). Do however note that, the latter replace substrings. In your case str.replace(' ', '_') would work, but if replacing multiple chars see how to replace multiple characters in a string?

The only part not covered then is the valid characters part, and this is the slightly ugly part of the new alternative. My proposed solution is to do:

''.join(c for c in suggested_name if c in VALID_CHARS)

And this possibly requires a little explanation:

  • ''.join(_some list_) – This expects a list of something, which it will join together by the separator in front, that is the empty string.
  • c ... – If the for loops is valid, then return the c
  • for c in suggested_name ... – Split the suggested_name into single characters
  • if c in VALID_CHARS – Only do the action (that is return c) from the for loop if the if expression is true. And the expression verifies that the character c is within the string VALID_CHARS.

In other words, the previous statements joins all characters from suggested_filename using the empty string separator, for those characters who are present in VALID_CHARS. That is quite a mouthful, but it is a very useful expression!

A longer version to write the same is something like (with a lot of edge cases removed):

my_list = []
for c in suggested_name:
  if c in VALID_CHARS:
    my_list.append(c)

my_separator = ''
my_txt = my_list[0]
for c in my_list[1:]:
  my_txt += my_separator
  my_txt += c

I'm also accustomed to separating how you get stuff (aka the tkInter dialog) from validation/transformation of the result, so I would make a function to handle this.

Code refactored

So what would the code look like if applying all of this? Maybe something like this (untested as I don't have tkinter available):

from string import ascii_letters, digits
VALID_CHARS = '-_.() {}{}'.format(ascii_letters, digits)

def get_valid_library_name(suggestion):
  valid_filename = ''.join(c for c in suggestion if c in VALID_CHARS)
  valid_filename = valid_filename.replace(' ', '_').lower()
  return valid_filename

a_name = tkinter.simpledialog.askstring(
                 'Create New Note Library', 
                 'Alphanumeric lowercase and "_" only',
                 initialvalue = "Name_Here")

print('{} into {}'.format(a_name, get_valid_library_name(a_name))

filename = get_valid_library_name("MY someWHAT ugly suggestion#$!&&")
print(filename)    # Would output "my_somewhat_ugly_suggestion"

Note that one could join the valid_filename transformation into fewer lines, but keeping it readable is also a good thing. For real code I would most likely have the valid_filename = ''.join(..) on one line, and then do a return valid_filename.replace().lower() instead of the two next lines.

As a final note, also see Turn a string into a valid filename which I found when researching for this answer. It uses some alternate stuff from unicode, and has some other related ideas on how to handle validation of filenames.

\$\endgroup\$
  • \$\begingroup\$ Thank you for your well thought out explanation. With that I did take some advice from you and others who posted answers and the biggest change would have to be this bit: valid_filename = (''.join(c for c in a_name if c in VALID_CHARS)).replace(' ', '_').lower() This cut back on a big chunk of the code I was working with :). Also to answer you question about the (even=None) I ran into some issues with Tkinter early on when calling a function that did not take any variables. the (even=None) was a solution I found on a form post. \$\endgroup\$ – Mike - SMT Apr 21 '17 at 19:19
  • \$\begingroup\$ @BaconTech, I was contemplating on joining it all together like that, but I felt that would impose just a little too much on the readability. There is quite a lot of stuff happening in that one code line. :-) \$\endgroup\$ – holroy Apr 21 '17 at 19:22
  • \$\begingroup\$ True the line is somewhat hard to read if you didnt know what all was going on however seeing that this is my first program ever. and I am already up to 300 lines of code I was interested in learning how to shorten my existing code to improve on file size (not important now but good practice) and to learn a bit more on how I could perform the same task in different ways. Been learning python for about 2 1/2 months now so I am still learning quite a bit. \$\endgroup\$ – Mike - SMT Apr 21 '17 at 19:26
  • \$\begingroup\$ Do remember that file size is not important, however readability and maintainability is much more important. Imagine yourself coming back to the code after a while, will you still understand it in the most condensed form, or not? Do improve your coding, and use better coding practices (which often reduce file size also), but if/when you start to confuse yourself due to condensed code you're entering a dangerous place... \$\endgroup\$ – holroy Apr 21 '17 at 19:29
  • \$\begingroup\$ I understand completely. I already ran into parts of my own code that I was unsure of. I had to read through everything just to figure out what I was doing in that instance. I have been improving my code as I go and sometimes scraping entire section and remaking them so that they can be read easier. And again Thanks for your input it all helps as I am always looking to improve where I can. \$\endgroup\$ – Mike - SMT Apr 21 '17 at 19:32
6
\$\begingroup\$

Building strings the naive way (with +=) is very inefficient, use join instead.


If you want to look things up efficiently, then the target of the lookup should be a set (or a dict), rather than a list(in your case a str). The reason for this is that sets (and dicts) have (on average) constant lookup time, while lists have linear.

However since the character-set is really small, this won't matter too much.


Other than these, ignoring a few formatting issues, unneeded calls to str, strangely placed imports; your code looks fine.


''.join(c for c in a_name if c in VALID_CHARS)).replace(' ', '_').lower() is already linear in the length of a_name, but if you need a method, which is approximately 3.81 times faster than that, then you can use the following, which uses string.translate.

from string import ascii_letters,digits
from collections import defaultdict

charList = ''.join(["-_.() ",ascii_letters, digits])
charDict = dict(zip(map(ord, charList)
                   ,map(lambda c: '_' if c == ' ' else c.lower()
                       ,charList)))
charDict = defaultdict(lambda: '', charDict)

def createValidFileName(fileName):
    return fileName.translate(charDict)

def newLibPrompt(event=None):
    libName = tkinter.simpledialog.askstring(
                'Create New Note Library',
                'Alphanumeric lowercase and "_" only',
                initialvalue = "Name_Here")
    return createValidFileName(libName)
\$\endgroup\$
  • \$\begingroup\$ @Graipher Thanks, should have looked it up instead of assuming. I'll correct it right away. Funny thing is, I made the opposite of this mistake a few days earlier about std::set: I assumed it was unordered... turns out it is ordered. I won't assume anything any more, I promise. \$\endgroup\$ – Andrew Apr 22 '17 at 11:33
4
\$\begingroup\$

There are a couple of things you can do to improve your actual code:

  • import the modules that you need at the top of your program
  • don't cast strings to strings (stringofnewname = str(name_of_new_library)); here, name_of_new_library is already a string
  • use more descriptive variable names
  • you don't have to generate all lowercase and uppercase letters if you only need the lower ones. Use ascii_lowercase instead.
  • you say you only allow alphanumeric lowercases and "_", but you actually let the user insert other characters. That's ambiguous.

The code that I thought of would look like this:

import tkinter
from tkinter import simpledialog
from string import ascii_lowercase, digits


def process_user_input(event):
    name_of_new_library = simpledialog.askstring('Create New Note Library', 'Alphanumeric lowercase and "_" only',
                                                 initialvalue="Name_Here").lower().replace(' ', '_')

    valid_filename_chars = "{}{}_".format(ascii_lowercase, digits)
    final_name = ''
    for character in name_of_new_library:
        if character in valid_filename_chars:
            final_name += character

    return final_name if final_name else 'custom_name'


if __name__ == '__main__':
    root = tkinter.Tk()
    print(process_user_input(root))

Other changes that I've done:

  • directly lowered and replaced the user input so that we can get rid of some conditions
  • modified the valid_filename_chars so that it match the description in askstring
  • returned a custom_name if the user inputs only illegal characters
\$\endgroup\$
  • \$\begingroup\$ Actually all things are already imported. This is just a snip-it of my overall code. Also the reason I am allowing my user to input upper and lower case is for a display text. I want every filename to be lowercase so it becomes easy for me to weed out redundant name creation. But the user can specify case so when the program displays the name of the file I have a separate list that contains the original case used and it returns in the format the user typed. As for the string of sting part I did realize this after I posted my question but thank you for the input. \$\endgroup\$ – Mike - SMT Apr 21 '17 at 18:37

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.