3
\$\begingroup\$

I created a system that allows individuals to ask questions about code, and chat. I wanted to make this system smaller and better because this is easy to bypass for some reason.

The modules/packages I used for this chatroom project

import os
import time

Now the actual script that contains about 56 lines of code!

print("Codehelp || Question through a terminal.")
#### Codehelp Variables
chat = open('publiclogs.txt','r')
name = input("Enter Username: ")
crown = "👑"
owner = False
#### Codehelp Scripts
if name == "":
  print("\nINVALID! Name must not be Empty..\n")
  time.sleep(2)
  exit()
while True: 
  with open('publiclogs.txt','a') as chat_txt:
    chat_txt.close()
  chat = open('chat.txt','r')
  print(chat.read())
  message = input("Enter Your message: ")
  os.system("clear")
  if message.lower() == "":
    os.system("clear")
    print("You must type something in order to send a message\n")
    input("Press enter to continue ")
    os.system("clear")
  elif message.lower() == ".ask":
    title = input("Enter title of Question. Also be specific: ")
    description = input("Enter description: ")
    tags = input("Enter TAGS. Seperate by ',': ")
    os.system("clear")
    with open('question.txt','a') as ask_txt:
      ask_txt.write(f'TITLE: {title}\n')
      ask_txt.write(f'DESCRIPTION: {description}\n')
      ask_txt.write(f'TAGS: {tags}\n')
      ask_txt.write(f'Written by {name}\n')
      ask_txt.write(f'-------------------\n')
      ask_txt.close()
    with open('publiclogs.txt','a') as chat_txt:
      chat_txt.write(f'New Question has been asked! View question.txt!\n')
      chat_txt.close()
  elif message.lower() == ".login":
    print("Welcome to the Admin log-in!")
    time.sleep(2)
    password = input("What's the Password: ")
    print("Removed from Codehelp Currently..")
  else:
      if owner == True:
        with open('chat.txt','a') as chat_txt:
          chat_txt.write(f'{crown} {name}:{message}\n')
          chat_txt.close()
      else: 
        with open('chat.txt','a') as chat_txt:
          chat_txt.write(f'{name}: {message}\n')
          chat_txt.close()

What could I do to improve this code?

\$\endgroup\$
1
  • \$\begingroup\$ How is this a chat room? Is the file mapped to an NFS mount or something? \$\endgroup\$
    – Reinderien
    Jun 1 at 0:49
0
\$\begingroup\$

I have carefully read all of your code and I would like to propose some changes, in line with the PEP8 good practice convention:

  • ideally, the indentation is a multiple of 4;
  • it is not necessary to use multiple '#' to comment a line;
  • boolean expressions like if owner == True can be simplified to if owner;
  • On line 27, I replaced Seperate with Separate;
  • You can replace ------------------- with {"-" * 20};
  • It is also possible to avoid over-repeating ask_txt.write by inserting newline after \n;
  • 1 blank space after each comma;
  • 1 blank paragraph at the end of everything.

Here is the entire code including the modifications I suggested:

print("Codehelp || Question through a terminal.")
# Codehelp Variables
chat = open('publiclogs.txt', 'r')
name = input("Enter Username: ")
crown = "👑"
owner = False
# Codehelp Scripts
if name == "":
    print("\nINVALID! Name must not be Empty..\n")
    time.sleep(2)
    exit()
while True:
    with open('publiclogs.txt', 'a') as chat_txt:
        chat_txt.close()
    chat = open('chat.txt', 'r')
    print(chat.read())
    message = input("Enter Your message: ")
    os.system("clear")
    if message.lower() == "":
        os.system("clear")
        input("You must type something in order to send a message\n"
              "Press enter to continue ")
        os.system("clear")
    elif message.lower() == ".ask":
        title = input("Enter title of Question. Also be specific: ")
        description = input("Enter description: ")
        tags = input("Enter TAGS. Separate by ',': ")
        os.system("clear")
        with open('question.txt', 'a') as ask_txt:
            ask_txt.write(f'TITLE: {title}\n'
                          f'DESCRIPTION: {description}\n'
                          f'TAGS: {tags}\n'
                          f'Written by {name}\n'
                          f'{"-" * 20}')
            ask_txt.close()
        with open('publiclogs.txt', 'a') as chat_txt:
            chat_txt.write(f'New Question has been asked! View question.txt!\n')
            chat_txt.close()
    elif message.lower() == ".login":
        print("Welcome to the Admin log-in!")
        time.sleep(2)
        password = input("What's the Password: ")
        print("Removed from Codehelp Currently..")
    else:
        if owner:
            with open('chat.txt', 'a') as chat_txt:
                chat_txt.write(f'{crown} {name}:{message}\n')
                chat_txt.close()
        else:
            with open('chat.txt', 'a') as chat_txt:
                chat_txt.write(f'{name}: {message}\n')
                chat_txt.close()

Here is the complete documentation. PEP 8 -- Style Guide for Python Code.

\$\endgroup\$
2
  • \$\begingroup\$ I have rejected two of your previously approved suggested edits. You are not allowed to edit code in questions - 1 2. Additionally, please refrain from suggesting the same edit twice in the future. However, please feel free to suggest the text changes in this edit again if you don't edit the code. (so you get +2 rep) If you edit the code your suggestion will be rejected. \$\endgroup\$
    – Peilonrayz
    Jun 25 at 19:18
  • \$\begingroup\$ Thanks, I didn't know that and I really appreciate. \$\endgroup\$
    – Arnon
    Jun 26 at 0:27
0
\$\begingroup\$
  • Your initial open() is redundant and can be deleted
  • crown is unused because owner is hard-coded to False
  • Consider using pathlib which makes several things in your code more convenient
  • Your sleep calls hinder rather than enhance usability and can be deleted
  • os.system('clear') is not cross-platform, and anyway it offers no usability benefit, so that can be deleted, too
  • Your with / close pattern implies a double-close, which is incorrect. The point of with is that it already guarantees a close.
  • Use getpass instead of input for the administrative password, to prevent over-the-shoulder
  • Instead of killing the entire process on invalid username input, consider adding a validation loop

Suggested

from getpass import getpass
from pathlib import Path

CHAT_PATH = Path('chat.txt')
QUESTION_PATH = Path('question.txt')
LOG_PATH = Path('publiclogs.txt')

print("Codehelp || Question through a terminal.")

while True:
    name = input("Username: ")
    if name != "":
        break
    print("Invalid: name must not be empty.")


while True:
    if CHAT_PATH.exists():
        print(CHAT_PATH.read_text())

    message = input("Message: ")
    command = message.lower()

    if command == ".ask":
        title = input("Question title: ")
        description = input("Description: ")
        tags = input("Tags, separated by ',': ")

        with QUESTION_PATH.open('a') as ask_txt:
            ask_txt.write(
                f'TITLE: {title}\n'
                f'DESCRIPTION: {description}\n'
                f'TAGS: {tags}\n'
                f'Written by {name}\n'
                f'-------------------\n'
            )

        with LOG_PATH.open('a') as log_txt:
            log_txt.write(f'New question has been asked! View {QUESTION_PATH}\n')

    elif command == ".login":
        print("Welcome to the Admin log-in!")
        password = getpass()
        print("Removed from Codehelp currently..")

    else:
        with CHAT_PATH.open('a') as chat_txt:
            chat_txt.write(f'{name}: {message}\n')
\$\endgroup\$
0
\$\begingroup\$
  • The original value of the chat variable appears to be unused - you just open the publiclogs.txt file but never use or close it
  • Every pass through the loop you open and then immediately close the "publiclogs.txt" file again. That doesn't seem very useful
  • Using open with with statements is good and is usually the way to go, but sometimes you use open on its own which just increases the risk of opening a file and forgetting to close it - you rarely want to do that
  • On a related note, you don't need to manually close files if you opened them in the heading of a with statement. The ask_txt.close() and chat_txt.close() lines in the .ask part are unnecessary
  • You keep re-opening "chat.txt" each pass through the loop. If you want to go back to the start of the file you usually want to use chat.seek(0) rather than re-opening the file. There are situations where closing and re-opening a file is warranted, but I'm not sure whether this is one of them
  • A minor issue, but I find it annoying that both the variable chat and the variable chat_txt can sometimes refer to the file "chat.txt" and sometimes to the file "publiclogs.txt", with no obvious pattern as for which one is used when. It makes the logic harder to follow
\$\endgroup\$
1
  • \$\begingroup\$ chat.seek(0), not re-open the file - usually I would agree, but in this specific case if the file is expected to be shared between multiple processes it's actually safer to close and re-open on each loop. \$\endgroup\$
    – Reinderien
    Jun 1 at 1:30

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.