My Voting App - let me have it

Code Feedback
1

I’ve finally done it. I’ve written a fullstack app.

Sure, it ain’t the best, but it’s mine. I think I’m finally getting the hang of the file structure and the whole MVC thing. I look forward to improving on the last four projects.

Any feedback would be appreciated. I am especially interested in critiques on the how the files/apis interface. I know this is not great - I only started to get it about halfway through and still am not 100% there.

Here is the site and here is the code.

2

Good job. I played with it a bit trying to break. Only once it threw 500 error, but I couldn’t reproduce, so I assume it was you doing something on the server.

One bug I found is if you add more than 10 choices, they aren’t displayed correctly (check poll postman4)

And deleting of a poll should have at least some kind of notification. Currently after clicking X nothing happens (although poll gets deleted).

1 Like
3

Thanks for the feedback. I’ll look into those tomorrow.

With regards to 10 choices, that was one way that I deviated from the suggested app. It is only supposed to allow a max of 10 choices. That was working before - I’ll have to check it out.

Thanks again.

4

OK, I checked it and it is preventing more than 10 choices in the window. Am I to understand from the title that you were using postman to send it “incorrect” data? That is an interesting problem and one that I don’t know how to solve yet. It’s a fair observation, but perhaps a bit beyond my ken at the moment. I’ll have to look up ways to protect against that. Any suggestions?

5

Never trust the front-end. All data must be checked on the back-end. You do checks on front-end to provide users with pleasant UX, but you can’t just put everything your back-end receives straight into db.

In your case I would just write a separate function to handle checks :

// routes/polls.js

req.body.choices = parseChoices(req.body.choices, 10); // second parameter is optional

// my cool new function
// takes an array and max number (defaults to 10)
function parseChoices(choices = [], max = 10) {
  const result = [];
  
  for (let choice of choices) {
    if (result.length >= max) return result;
    result.push(choice.trim());
  }
  
  return result;
}

Also what you did with .map() + .filter() in two passes could be done in one with .reduce() (but reduce doesn’t provide option of premature return, therefore I used for);

  req.body.choices = req.body.choices
    .reduce((array, choice) => {
      return array.includes(choice) ? array : array.concat(choice.trim());
  }, []);
6

Thank you for the suggestion.

That is a deeper level of thinking. I think the assignment is to build a functioning website that stands up to normal usage. Handling people hacking in maliciously is not part of the assignment. But you are right, that that is something important to consider if your building real world apps.

But if I need to check people hacking in and forcing too many choices, then I also need to check every possible bit of data that’s sent to it - the amount of choices, that choices are unique, that they are not too long, that they are printable characters, and similar things for the title and username. Then I’d have to do the same thing for registration. Wouldn’t every put and post request need every piece of data sent to it validated?

It’s a shame there isn’t away to prevent all http methods that aren’t originating in the code. I don’t understand this well enough, but perhaps there is no way for the code to confirm it that can’t be spoofed.

There are some built in validation techniques in mongoose schema - I think I’ll look into those on the next build.

I don’t think I’m going to worry about it on this project since it is not a requirement. But thank you for making me think about it. That will be an important topic if I do this professionally.

And I finally figured out the message on the poll delete - thanks for that.

7

then I also need to check every possible bit of data that’s sent to it

Yes

Then I’d have to do the same thing for registration.

Yes

every put and post request need every piece of data sent to it validated?

Yes

It’s a shame there isn’t away to prevent all http methods that aren’t originating in the code.

What do you mean by that? Your router won’t do anything if it can’t find a matching method for the requested path.
If I send request to /polls/hack with a method TROLOLO nothing will happen (unless you’ll create that route with that method :smile:)

8

Yeah, I get that they can’t access routes that I don’t give them.

My point is that it would be good if they could only access those routes through my frontend, not being able to hack into them like you did. But that may not be possible. I guess I have to think about making the route such that they can’t do anything harmful.

My only concern is that then I would want error checking on both the frontend and the backend. I hate defining constants in two places. I can see two solutions:

  1. Have all the validation on the backend, and just have it kick back to the frontend an error message if the validation fails. The problem with that is that you couldn’t do real time (as you type) validation.

  2. Pass the validation constants to the GET page when it loads. This seems like a smarter way to do it.

In any case, I think I’ll try to do this on my future projects.