ju5tAmir / Uncle-Sam-Rolls

0 stars 0 forks source link

Feedback #3

Open uldahlalex opened 1 month ago

uldahlalex commented 1 month ago

Hi guys, your application is hereby approved

Advice going forward: 1) Your CI/CD pipeline does not seem to actually find any tests.

It appears you do have tests, however, although not very many.

2) Your tests seem to be using a concrete controller instance with a service mock, so they test your API but not using an actual API client. I recommend using WebApplicationFactory instead of this approach. Currently your tests mostly just check if your controllers can produce the correct statuscode under very fixed circumstances.

We do have additional time after the holiday to look more into testing, so we'll spend additional time there.

3) You need a .gitignore for your C# app. There's one for your react app, but you have pushed bin and obj folder to the remote repository.

The scope of your application is good - there's a lot of content, and some of it is also very neat (the onion architecture for instance).

The general code-style can be slightly perfumey (a lot of comments, some of them totatally redundant since the code is sometimes self-explanatory):

    if (!order) {
        // Show NotFound if no order is found after loading
        return <NotFound />;
    }

You have a lot of try-catch blocks with toasts and logs which can create very repetitive and indented code. image

Here you could refactor to using an interceptor for you http client to always toast/log the error returned to the client:

import {Api, ApiConfig} from "../Api";
import  {AxiosError} from "axios";
import toast from "react-hot-toast";

const baseUrl = "http://localhost:1337";

const apiConfig: ApiConfig = {
    baseURL: baseUrl,
};

export const http = new Api(apiConfig);

http.instance.interceptors.response.use(
    (response) => response, // Return the response unchanged for successful requests
    (error: AxiosError) => {
toast.error(error.message);
console.log(error)
        return Promise.reject(error);
    }
);

Other than this it's great work (and cool it's serving on port 1337)

ju5tAmir commented 1 month ago

Hey Alex,

Thanks for such a detailed feedback, we'll try to address the issues you mentioned. However, I was wondering if there is any github repo with a completed version of this program?

uldahlalex commented 1 month ago

Hi Amir,

You don't have to spend additional time correcting anything - it's just a way of giving feedback on your solution. There's no "solution guide" to the assignment you should follow, but I want the feedback to be applicable to the exam project so you can keep that in mind. You're doing great, so don't owrry about it.

ju5tAmir commented 1 month ago

Okay I got it, thanks a lot and have a nice day :v

larqsine commented 1 month ago

Thank you for the feedback!