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

Feedback #1

Open
rafaismyname opened this issue Dec 5, 2016 · 0 comments
Open

Feedback #1

rafaismyname opened this issue Dec 5, 2016 · 0 comments

Comments

@rafaismyname
Copy link

Feedback for this


README.md

Create your bot and put it's key in BOT_KEY in
/src/back/config.js

You can't use env vars attached to code. You should provide a .env.sample file with the required env vars for your project.
Env vars are secret and should be user-based.
Try: dotenv

####Database

  1. Start MongoDB
  2. Create telebot database (use telebot)
  3. Create chats collection (db.createCollection(chats))

No need to init MongoDB, since its schema-less this will be (should be) inited automatically.

MongoDB connection settings: /back/config.js

Again... env file...

Lack of build instructions

There is no instructions on how to build the project. If i run the defaults: npm install && npm start an error will occour:
Error: Cannot find module '/Users/rafa/Dev/Tests/telebot/bin/www'

Same for tests...


package.json

very incomplete and weak package.json. There is no engine reference (is node 4 or 6?). There is no author. There are dev dependencies declared in regular dependencies and is lacking declarations (eg.: istanbul)


tests

Tests crash (SyntaxError: Invalid regular expression: /^function)
when npm test...

All specs should be stored apart from the source code. You must provide a separated folder in the root of your project containing yours tests, not mixed with the actual source code. This is very messy! (Eg.: ./specs/controllers/.. ./tests/models/...)


back

  • How to build this??? Had to assume: node back/index.js

  • Where are all the code comments? Am I supposed to read the whole code to understand what's going on?

  • Out of lint and without a proper style as requested: Airbnb's Styleguide

  • At: ./back/websocket
    Why no ES6 usage? Why so many code chunks?

Your code:

function initSocket(socket) {
    prepareConnection(socket);

    return {
        sendMessage: sendNewMessage(socket),
        sendChat: sendNewChat(socket)
    }
}

module.exports = initSocket;

Expectation:

export default socket => {
	prepareConnection(socket); #actually, this should NOT exist at all!
	return {
		sendMessage: sendNewMessage(socket), # why this?
		sendChat: sendNewChat(socket) # why so this?
	}
}

This websocket module should not be that modularized, so confusing!
This websocket would easily be converted into a single Class, no need at all to separate the code into chunks.

  • At: ./back/config.js no usage of .env file... What if in production there is a secret key that only the CTO and Lead Dev can have access to it? All the other programmers would be able to see this secret, not being a secret anymore...

  • At: ./back/models

100% wrong usage of mongo. And you SHOULD be using mongoose.
Models should be a declarative representation of an entity and its relationships/functions. There is no usage for separated files by folders corresponding to its actions.

Your code:

./back/models/find/...
./back/models/insert/...
./back/models/model/...
...

Expectations:

./back/models/message.js: contains message schema and functions (or a mongoose model)
./back/models/user.js: contains user schema and functions (or a mongoose model)
...
  • At ./back/bot/

these should be your controllers, and should not be that "chunked"...


front

  • why server side render for react? twice the trouble, twice the cost.
  • why server routes? react has an already built routing system.
  • are you sure that you know how React and plain frontend apps works?
  • not even able to build this, there is no instructions at all

tl;dr;

I see that you are making progress, but this still far to be acceptable...

I know you had just a few days to do this, and you did great taking in consideration the time you had to learn and build.
Keep up with the hard work!

You should try:

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

No branches or pull requests

1 participant