HiveBrain v1.2.0
Get Started
← Back to all entries
patternpythonMinor

Search every char from a string for every char in another

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
searchchareveryforanotherfromstring

Problem

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.

```
va

Solution

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 tra

Code Snippets

''.join(c for c in suggested_name if c in VALID_CHARS)
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
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"

Context

StackExchange Code Review Q#161421, answer score: 6

Revisions (0)

No revisions yet.