Code Review

[ShaharBand]

Code Review 1#

The first thing I noticed is that the name "lasktest.py" is not very informative. File names should be descriptive and indicate their purpose.

While this project is small and maintaining a single file might seem feasible, it is generally considered bad practice. Therefore, you should consider dividing the project into multiple files, each with its own clear responsibility.

In the provided Python file, you have hardcoded sensitive configuration variables like the secret key and database information directly within the code itself. This is a serious security risk and bad practice. To improve security and maintainability, you should implement the following changes:

1. Create a separate configuration file, config.py: This file should store all sensitive configuration variables, including the secret key and database information. This allows you to keep your code clean and separate sensitive information from the main codebase.

2. Generate a secure secret key: Instead of hardcoding the secret key as "Aa123456", use the secrets library to generate a secure and random key. This will significantly improve the security of your application.

3. Store database credentials securely: Do not directly store database credentials in the configuration file. Instead, consider using environment variables, a secrets management tool, or other secure storage mechanisms.

Here's an example of how you can implement these changes:

config.py:

SECRET_KEY = secrets.token_urlsafe(32)
SQLALCHEMY_DATABASE_URI = "sqlite:///users.sqlite3"
SQLALCHEMY_TRACK_MODIFICATIONS = False

app.py:

from config import config

app = Flask(__name__)
app.config.from_object(config)

admin = False

database = SQLAlchemy(app)

This approach will not only improve your code's security but also make it easier to manage and maintain your application's configuration.

Your code introduces unnecessary redundancy and complexity. Creating two separate Comment classes solely for adding timestamps is inefficient and difficult to maintain. Instead, I suggest adopting a single Comment class with a built-in timestamp functionality. Here's an example:

class Comment(database.Model):
    """
    Model class for comments with automatic timestamping.
    """
    _id = database.Column('id', database.Integer, primary_key=True)
    user = database.Column(database.String(32))
    content = database.Column(database.String(32))
    created_at = database.Column(
        database.DateTime, nullable=False, default=datetime.now
    )

    def __init__(self, user, content):
        self.user = user
        self.content = content

Also, I want to talk about this code part:

def handle_user_not_existing():
    flash('Credentials wrong!')
    return redirect(url_for("home"))

def handle_wrong_password():
    flash('Credentials wrong!')
    return redirect(url_for("home"))

def handle_user_already_exists():
    flash("User already exists!")
    return redirect(url_for('home'))

def handle_password_confirmation_failure():
    flash("Passwords do not match!")
    return redirect(url_for('home'))

There is significant repetition in the code. Each function performs the same actions: flashing a message and redirecting to the home page. This redundancy can be easily addressed by using a single function with different message arguments.

The flash messages are generic and provide limited information to the user. They do not specify which aspect of the credentials was incorrect or why the user creation failed.

The url_for function calls use the same argument in each case ("home"). This argument can be hardcoded into the redirect call to improve readability.

Here's the improved code with suggestions:

def handle_authentication_error(error_type):
    message = {
        "user_not_existing": "No user found with these credentials.",
        "wrong_password": "Incorrect password. Please try again.",
        "user_already_exists": "This username is already taken.",
        "password_confirmation_failure": "Passwords do not match.",
    }

    flash(message[error_type])
    return redirect(url_for("home"))

# Usage examples
handle_authentication_error("user_not_existing")
handle_authentication_error("wrong_password")
# ...

App routes that don't end with '/' like here: @app.route('/forum', methods=['POST','GET']) will not work if someone types "/forum/" which is a small issue but to avoid such bad practice add '/' to the end of a route.

Reasons why embedding CSS in