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

Martin MongoDB Api #491

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Conversation

Martin-Joensson
Copy link

Copy link

@Izzibizz Izzibizz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Grymt jobbat! Din kod är tydlig och lätt att förstå, gillar att du har flera query filteringar och såklart att du valt ett fantastiskt tema för din data 🥇

/Izabel

[
{
"id": 1,
"name": "Fågelfjädersstav",

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Underbar data!

/ Izabel

// import goldenGlobesData from "./data/golden-globes.json";
// import netflixData from "./data/netflix-titles.json";
// import topMusicData from "./data/top-music.json";
import avocadoSalesData from "./data/avocado-sales.json";

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Säljer du avokados också?

/Izabel

@@ -24,9 +45,66 @@ const app = express();
app.use(cors());
app.use(express.json());

app.use((req, res, next) => {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Intressant, detta har jag missat! Fick köra en chatGPT för förklaring. Topp!!

/Izabel

);
}
// Query to filter out all entries at a price point lower than the query.
const priceSearch = req.query.price;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gillar att du har flera query filtreringar! Dock så ser man inte det i lista med endpoints, så det vore toppen att veta vad man kan söka på 😄

/Izabel

}
});

app.get("/magic-items/:id", async (req, res) => {
Copy link

@Izzibizz Izzibizz May 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ser att du har två id endpoints, antar att denna är för _id som tilldelas? Men ser den inte i din data, så vet inte vad dessa id är och kan inte söka på dem..?
Konstigt nog ser jag bara din egna id nyckel. (Gud vad skumt att skriva kod på svenska)

/Izabel

Copy link

@AntonellaMorittu AntonellaMorittu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi Martin, good job deploying your first database! 🚀 The code meets the requirements and it's well structured. Here is what can be improved:

  • Your Express routes are well-defined and handle various scenarios such as fetching all magic items, filtering by colour and price, and retrieving a specific item by ID. However, you have defined two routes with the same path (/magic-items/:id). Go ahead and remove one of them to avoid conflicts
  • Would be nice to document every filtering criteria in your API, you could add the available search queries when listing the endpoint:
app.get("/", (req, res) => {
  let endpoints = expressListEndpoints(app);

  // Add information about search queries to each endpoint
  endpoints = endpoints.map(endpoint => {
    // Customize this object to include information about search queries for each endpoint
    if (endpoint.path === "/magic-items") {
      endpoint.searchQueries = {
        color: "Filter by color",
        price: "Filter by price",
        // Add more search queries as needed
      };
    }
    return endpoint;
  });

  res.json(endpoints);
});

also you can remove the datasets if unused :) Well done!

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 this pull request may close these issues.

3 participants