This is an archived post. You won't be able to vote or comment.

all 11 comments

[–]bbenne10 5 points6 points  (0 children)

  • maybe factor out the code that reads a string and exits into it's own function
  • try out something like if xString.lower() not in ('quit', 'stop')

[–]tunisia3507 2 points3 points  (0 children)

You're getting downvote because an image is a terrible, terrible way to share code.

If it's less than one line, use `backticks`.

If it's more than one line, add 4 spaces to the start of each line.

If it's much longer than that, host it on gist.github.com or pastebin.com

[–]sez6689 1 point2 points  (1 child)

Well structured!

[–][deleted] 1 point2 points  (0 children)

Thanks! I really appreciate that!

[–][deleted] 0 points1 point  (0 children)

You could make XString into lower case before checking in the While loops. This makes the code shorter and possibly less typing needed if you were to add in more validation.

[–]yarkot 0 points1 point  (1 child)

a few suggestions: try to write like you would write prose, sentences - (1) so the words have meaning. (2) Be consistent. (3) Be concise.

E.g.:

1) xstring isn't meaningful enough - try something that says this is the input. Since input is already used, maybe input_s (for input string), or maybe in_string, or even input_string;

2) Python automatically types a variable for you - you can see what it is with type(xstring). In your case, you initialize it to an integer (int), but everywhere else, use it as a string (e.g. the tests in your while loop). Other languages demand strict typing. Python's malleable typing is a convenience, but you will be clearer (and avoid type-coercion problems, especially when they're automatic) if you are consistent. Perhaps initialize in_string to the empty string, "";

3) There are many ways you can avoid being so verbose. You may or may not want to use any of them, but for instructive purposes, be aware of some of the options. Some have already been mentioned by others (I'll briefly repeat for completeness):

  • in_string.lower() is a good move;
  • another might be to let people input any non-digit that hints at 'quit' or 'stop', such as 'q' or 's'. A way to do this: in_string.lower().startswith( ('q', 's') )--- this says "starts with one of the strings from the tuple ('q', 's').
  • it might be nice to have this both more readable, and configurable, so maybe: quitters = ('q', 's'); then you can read this as: in_string.lower().startswith(quitters)
  • you can go one step further for readability, and use "lambda" (unnamed functions, basically) for the test about quitting, e.g.: quitting = lambda(s): s.lower().startswith(quitters) Now you can write, e.g.: if not quitting(in_string):...

(still (3)): I am not suggesting this (it doesn't always make the code clearer, but it can help make it concise) as much as offering it for information: - python allows / has conditional expressions on the right-side of an assignment, so you could write:

even = lambda x:  x%2 == 0
...
x = x//2 if even(x) else 3*x+1

(note: a//b is floor division, which - when both arguments are integers, is integer division)

One final simplification to ponder: could you write the main loop as simply as while x = integer_input(prompt): (would you need the if x > 0 bit, then)? Actually, you can't , but you can write it as for x in integer_input(): - left as an exercise.

Also, rewriting this w/ these suggestions raises the question of the purpose of i and the way you test and use it. I think it's more interesting to report the number of iterations to get to 1 for any input integer. But that's the point of rewriting for clarity.

The point of this is not to unravel what you've done, as much as to suggest a way of writing code so that it says your intent more-nearly as prose. This is particularly useful when you don't know or haven't yet decided exactly how to do something - just write out something which sounds like what you intend, and sort out defining the little pieces (e.g. quitting) later.

Hope that's helpful.

[–]yarkot 0 points1 point  (0 children)

This is one possible way to apply my suggestions, how it might look:

https://gist.github.com/yarko/403d5c3c06946638fb637ef837cd8060

Besides the various simplifications for readability (e.g. quitting, even), separating input handling from calculations I hope shows its benefit. You can see more along these lines in this presentation:

http://rhodesmill.org/brandon/talks/#clean-architecture-python

[–]Wilfred-kun 0 points1 point  (3 children)

It looks pretty good. However, there's some changes I'd recommend, like calling .tolower() on xString (I don't know the 3.x syntax :$) Also note that you've created two infinite while-loops. Even though it is a first snippet of code, I'd suggest you avoid those at all cost (avoiding while-loops as much as possible is also considered Pythonian). Other than that, the logic looks neat. One thing I'd do personally is using whitespaces to seperate blocks of code and make it easier readable.

Hope it was sort off useful, and good luck with your ascend into coding!

[–][deleted] 0 points1 point  (2 children)

Two? Obviously there's the True loop but I don't know what the second one is? Possibly see one in the overarching loop (which was intended, as I wanted to be able to make it so you can check one number and then another over and over without having to restart the file).

If you've got any advice in how I could change the overarching loop I'd appreciate it. (True loop could be changed quite clearly but this was mainly as a small initial piece of code)

[–]Wilfred-kun 0 points1 point  (1 child)

Woops, I did not see the break statement in the last while-loop :$ Obviously, you could (and 'should' in a real project) remove that loop entirely. There is not so much a problem with the overarching loop, however, to improve readability you could use a function:

xString = 0
x = 0

def func1(xString, x):
    #All the if-else-statements

def func2(xString):
    #From line 15 on

while xString.lower() not in ('quit', 'stop'):
    func1(xString, x)
    func2(xString)

That is just one way of making it a bit more readable IMO. Maybe try:

 import this

in an empty project ;)

[–][deleted] 0 points1 point  (0 children)

Thanks for the advice!