Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Code review #58

Closed
tshemsedinov opened this issue Jan 20, 2023 · 0 comments · Fixed by #59
Closed

Code review #58

tshemsedinov opened this issue Jan 20, 2023 · 0 comments · Fixed by #59
Assignees

Comments

@tshemsedinov
Copy link

  1. Long try blocks are dangerous, they can generate multiple exceptions in different places and you can't localize it with catch, only by eyes or if-statement logic:
    try {
    const headers = {
    'Content-Type': 'application/json',
    };
    const data = {
    email, password, username: email
    };
    const response = await fetch('api/auth/registration', {
    method: 'POST',
    headers,
    body: JSON.stringify(data),
    });
    if (!response.ok) {
    return rejectWithValue(await response.json());
    }
    return {
    ...await response.json(),
    };
    } catch (error) {
    if (error.response && error.response.data.message) {
    return rejectWithValue(error.response.data.message);
    } else {
    return rejectWithValue(error.message);
    }
    }
    and
    try {
    const deleteFavRecipes = this.prisma.favourite_recipes.deleteMany({
    where: {
    recipe_id: parseInt(recipeId),
    },
    });
    const deleteRecipe = this.prisma.recipes.delete({
    where: {
    id: parseInt(recipeId),
    },
    });
    await this.prisma.$transaction([deleteFavRecipes, deleteRecipe]);
    return true;
    } catch (error) {
    console.error(error);
    return false;
    }
    and other places
  2. Using JWT you must understand all problems and security issues it can bring and estimate advantages and disadvantages. Have you?
  3. Following code blocks have multiple duplications, think how to combine it with higher program abstractions, you have multiple such files: https://github.com/readme-experts/cyberchef/blob/63d42b070137b56913308143a0b6d6a308f3adec/src/client/src/app/actions/account/loginUser.js or https://github.com/readme-experts/cyberchef/blob/63d42b070137b56913308143a0b6d6a308f3adec/src/client/src/app/actions/account/deleteUserRecipe.js
  4. Hardcoded port number https://github.com/readme-experts/cyberchef/blob/63d42b070137b56913308143a0b6d6a308f3adec/src/client/src/setupProxy.js and password length:
    if (data.password.length < 8) validationErrors.push(new ValidationError(
    'password', 'Password is shorter than 8 symbols',

Overall codebase looks fine

@akaeyuhi akaeyuhi self-assigned this Jan 20, 2023
@akaeyuhi akaeyuhi mentioned this issue Jan 20, 2023
@akaeyuhi akaeyuhi linked a pull request Jan 20, 2023 that will close this issue
@akaeyuhi akaeyuhi pinned this issue Jan 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants