3 ms·

Neat project. A couple code-reviewish notes: * why use protected_attributes in an new Rails 4 project? I'm curious what motivated this decision. * if you're d

by al2o3cr 13y ago

Neat project. A couple code-reviewish notes: * why use protected_attributes in an new Rails 4 project? I'm curious what motivated this decision. * if you're destroying records, you'll want to look into using the :dependent option to has_many to make sure you clean things up. However, you may want to carefully consider where in the app "deletion" actually makes sense; anything that winds up in an accounting system probably shouldn't be deletable at all. * get the tests running. The version on Github has a controller test for "ZusersController" which doesn't appear to exist in the application... * a minor thing, but it'll drive future contributors crazy: hard tabs vs. soft tabs. Ruby code is usually two-space soft-tabs, but even if you don't agree with that at least be consistent. There are files on GH that are a riot of mixed tabs (configurations_controller.rb is a notable example) and nobody's got time for that. I'm not the best at catching replies on HN, but ping me on Github (same username) if you want to discuss further.

mtrpcic 13y ago
Thanks for someone pointing this out. There are a lot of places in the code that could use some cleaning up. The overall frontend code structure is a little lacking: * There's a "main.js", and a bunch of empty .coffee files that were presumably generated via a generator. Choose one frontend language and use it. * Vendor libs should go in vendor/assets/javascripts. This codebase has assets/javascripts/vendor, and then random vendor libs that aren't even in that folder, they're just in "javascripts". * The frontend code is going to quickly become jquery soup. There's no structure, all code executes on all pages. * Very minor, but OS specific files should be omitted from git (like .DS_Store) * Test coverage seems pretty low * The secret token is public in the git repo (Why not use secrets.yml and keep it out of SCM to be safe?) There are a myriad of other minor issues as well. It's a good idea for a project, and would help a lot of entrepreneurs out of done properly, but there are some glaring code quality issues here that worry me about the project as it grows.
- websitescenes 13y ago
  Also really great feedback. I have been programming for only three years so I am still learning much of what you describe. I will be cleaning out the unused template files very soon. Good point on jQuery firing on each page, although I am not really sure why it matters if it has no effect. Processing time? There are no tests but I hope to add some soon. (I have never written any, so I will have to learn)
  - allyjweir 13y ago
    For the JQuery, it is more about separation of concerns and keeping a maintainable an clear codebase. If all of your JQuery is in one file and somehow a small change to one part breaks another page's code that can be very difficult to catch/fix. It's just good form in general to separate it out. You're also right about performance. Loading unnecessary stuff on a page is just going to slow everything down. It's a great idea and also being a relatively new starter myself I can relate. Hopefully you can take all the feedback everyone is giving and make your project better!
websitescenes 13y ago
This is very valuable feedback! Thank you so much. I will remove protected attributes. I have thought about deletion and dependents and so forth I just wasn't sure how to address it yet. I must admit, I have completely ignored tests, thinking that I would write them later, but perhaps this is the wrong approach. As for indentations, they show properly on my end in Sublime, I will have to see why they appear differently in the repo. Again, really great feedback, I may take you up on a further discussion.

by al2o3cr 13y ago

mtrpcic 13y ago

Thanks for someone pointing this out. There are a lot of places in the code that could use some cleaning up. The overall frontend code structure is a little lacking: * There's a "main.js", and a bunch of empty .coffee files that were presumably generated via a generator. Choose one frontend language and use it. * Vendor libs should go in vendor/assets/javascripts. This codebase has assets/javascripts/vendor, and then random vendor libs that aren't even in that folder, they're just in "javascripts". * The frontend code is going to quickly become jquery soup. There's no structure, all code executes on all pages. * Very minor, but OS specific files should be omitted from git (like .DS_Store) * Test coverage seems pretty low * The secret token is public in the git repo (Why not use secrets.yml and keep it out of SCM to be safe?) There are a myriad of other minor issues as well. It's a good idea for a project, and would help a lot of entrepreneurs out of done properly, but there are some glaring code quality issues here that worry me about the project as it grows.

websitescenes 13y ago
Also really great feedback. I have been programming for only three years so I am still learning much of what you describe. I will be cleaning out the unused template files very soon. Good point on jQuery firing on each page, although I am not really sure why it matters if it has no effect. Processing time? There are no tests but I hope to add some soon. (I have never written any, so I will have to learn)
- allyjweir 13y ago
  For the JQuery, it is more about separation of concerns and keeping a maintainable an clear codebase. If all of your JQuery is in one file and somehow a small change to one part breaks another page's code that can be very difficult to catch/fix. It's just good form in general to separate it out. You're also right about performance. Loading unnecessary stuff on a page is just going to slow everything down. It's a great idea and also being a relatively new starter myself I can relate. Hopefully you can take all the feedback everyone is giving and make your project better!

websitescenes 13y ago

This is very valuable feedback! Thank you so much. I will remove protected attributes. I have thought about deletion and dependents and so forth I just wasn't sure how to address it yet. I must admit, I have completely ignored tests, thinking that I would write them later, but perhaps this is the wrong approach. As for indentations, they show properly on my end in Sublime, I will have to see why they appear differently in the repo. Again, really great feedback, I may take you up on a further discussion.