Hot answers tagged

9

Also said before, but I think it worth mentioning again: there is not need for prefix_ for all columns. As far as I know, Hungarian and Leszynski notations are not exactly recommended. Since column names are almost always used in conjunction with their table, the table alias will make the reader known the table for a column. It's a matter of taste, but ...


8

As per the Microsoft Guidelines, properties use PascalCase. Following line: public string name { get; set; } becomes: public string Name { get; set; } Not that it is prohibited, but I'd leave out the _ in a property name, I'd change sord_order to: public string SortOrder { get; set; } This is also more of a personal choice but I tend to place Is or Has ...


7

There was a lot of duplicate code, I changed it to be more clean: using System; using System.Data; using System.Data.SqlClient; using System.Collections.Generic; namespace MyApplication.Administration.Model { public class Supplier { public int Id { get; set; } public string Name { get; set; } public int SuId { get; set; } ...


7

This is a more elegant solution as we have incorporated generics. I've started with 2 constructors, with a single parameter result indicating success. One could build off more functionality as needed. Instead of object, one can now specify the type of result model to return. I believe this makes the code a little more explicit. Sample code include ...


5

I'm not sure I really buy into this as an idea. I can see it being useful where you want to do things like collect validation errors, but I'm not sure I'd want it to be responsible for swallowing exceptions and putting them into a list. Also, in your AddCountry method you're doing this: catch (Exception ex) { result.AddError(ex.Message); //add to the ...


5

I know you are generating data for the sake of completion but it does complicate things on what aspect of the code you'd like to focus on. But here are a couple thoughts. With Express, I often like to treat the routes as controllers in an MVC pattern. So any fetching of data and mutating data, I would create at least a model per resource to encapsulate that ...


5

Disclaimer: there will be a lot of links to my own site because I am helping people with PHP for 20+ years and got an obsession with writing articles about most common issues. I don't know much about restful stuff or whether it does matter, but there are many other areas of improvement. Let's review some of them The selectData() function First of all, ...


4

To answer your question: yes, I think it could be interesting as a stand-alone module. There's can be a lot of repetitive boilerplate involved in connecting a REST api to a CRUD backend, reducing that is certainly worth a module, in my opinion. Now to dive into the code... index.js if(!config.collection || !config.type){ //Raise an error here... ...


4

Use try-with-resources Use try-with-resources when working with resources like database connections, statements, file handlers. Odd throw-away objects This kind of code is quite strange: ResultSet result = statement.executeQuery(); while (result.next()) { product = new Product(); product.setEan(result.getLong("ean")); product.setHope(result....


4

You could make your life a bit easier if Contact had the same order as row, or the other way around. Then you could just use tuple unpacking to create a new Contact: cont = Contact(*row) Fortunately this is rather easy, just change your queries: cursor.execute("select name,email,mobile,cid from contact ...") cont = Contact(*cursor.fetchone()) The Contact ...


3

The first obvious problem in the code isn't really what you wanted reviewed, but public string CountryName { get; set; } //mandatory and upto 32 chars is going to run into trouble with the Former Yugoslav Republic of Macedonia. As for ResultModel, the purist OO way of doing it would be to have subclasses SuccessResultModel and ErrorResultModel. But if ...


3

On top of the other good answers I would like to point out that it can be dangorous to have a public setter for the id property. Assume you accidently change that value, each update or delete call will target the wrong data which can lead to serious problems, for the data and probably for your job. Another problem I see is in the Save () method. You are ...


3

This is by no means a comprehensive review, just some little things that I would change. You are often injecting HttpServletRequest and HttpServletResponse as method arguments. This is not necessary and bloats the method signatures. You can just define two instance fields of those two classes and annotate them with @Autowired. Spring creates proxies for ...


3

I don't think it's best-practice to put any logic that isn't strictly presentation-specific directly into a code-behind event handler like this, let alone that a UserControl knows anything about any DomainService1 object. If you're shooting for best-practices, you need to look into the Model-View-ViewModel pattern; it's not the job of any UI component to ...


3

Is there a way I can avoid repeatedly mentioning ctxMenuStripRightClick. Yes, I guess you could do something like: ctxMenuStripRightClick = new ContextMenuStrip(); ctxMenuStripRightClick.Opening += ctxMenuStrip_Opening; ctxMenuStripRightClick.Items.AddRange(new List<ToolStripMenuItem> { new ToolStripMenuItem("Add", null, AddMenuItem_Click), ...


3

The best aspect of your approach is that you keep the autogenerated class as is, which makes it more readable and clean, and keeps your extra code safe from deletion if you modify the database schema. You can extend this class with a partial class to add a constructor setting automatic properties like Date to DateTime.UtcNow. The static classes Do, Get, ...


3

Quickfire opinions: Swing is deprecated. If you have the choice: do not use swing! Empty lines are a useful thing. It's pretty conventional to use empty lines between members and around the import section of code. It helps delineate logically connected sections of code fromone another. The use of extends JPanel is something that many swing tutorials do. ...


3

Your code is good, but some improvements could be made. Here are my suggestions: 1) Use lowercase for first letter of variable Example: BrandService BrandService; rename to: BrandService brandService; 2) Use Lombock @Getter @Setter @Slf4j @*Constructors and so on. It will make code clean and readable 3) Use ControllerAdvise for global error handling: ...


2

The main security concern here I guess is protection from sql injection. Good news. You got that part right by using bound parameters. Here is what you should improve and why: Make the name of the class match what it is. In this case, it represents 1 Contact. So, name it Contact. You said "besides no use of getters and setters". However, I'll point out ...


2

A few comments on your code as follows : Your UI code should not contain a SaveChanges methods as per SRP, it should be in EF itself. Your UserInterface is actually just a utility method. Encapsulate your validation and prompt logic different places. Although Utility method is not considered as good but it is okay to use in this case although their ...


2

Well it looks incredibly clean, the amount of effort to read it is almost minimal. Everything seems very straight forward too! Nice. However, that leaves little to actually be critiqued! UserDAO It's common to see private variables prefixed with an underscore. It's up to you to decide if you'd like to follow this style. Some (including me) find that it ...


2

Naming In Product class, the constant names are too verbose. They could have been named just MIN_LENGTH_EAN, MAX_LENGTH_EAN etc. Or EAN_MIN_LENGTH, EAN_LENGTH_MIN and the like. And they don't need javadoc, because their meaning should be clear from the names. That may be a question of taste, but the 'I' prefix in interface names (IProductDatabase) is ...


2

I'd like to add a few points to your proposed implementation. Inversion of control The first thing I'd change is the constructor of your ProductManager class. I'd pass in an IProductDatabase rather than the connection string. This will make the code more testable as you'd be able to pass in test doubles at 'test time'. public class ProductManager { ...


2

In general, I recommend the John Papa's styleguide: https://github.com/johnpapa/angular-styleguide modules news/ news.module.js -> To define your module and dependencies news.constants.js -> all constants - can be accessible from .config news.mocks.js -> data to use in TDD news.directive.js news.directive.spec.js news....


2

Ideally your code should be easily understandable where you read it. The buttons says "Go to edit", then is better readable as ng-click="edit(Company). Also the Company is not a class, so general convention is not to capitalize it. But what is CRUDOperationEnum.Update? I don't see where it comes from, so I'd rather avoid it there. If it is already on your ...


2

If you're unsure whether the API is going to throw a ViewException or a COMException, or under which circumstances it's going to throw which, your exception handling has holes. And it's not very DRY. Instead of repeating this block everywhere (well, you're not repeating it everywhere, but it's not clear whether that's intended): catch (ViewException ...


2

I wrote about 'poor-mans MVC' here, but I'll write about it again for you. Basically, you want to separate your code into 'Models', 'Views' and 'Controllers'. The 'Controller' does the brunt of the PHP work, and it fills a 'Model' with some data. Then the 'Controller' passes the aforementioned 'Model' off to the 'View', which simply displays what is in the '...


2

Your code does not do what you said it does (My code performs CRUD operations in PHP using OOP and dynamic queries). At least not in the way I would expect it. It does much more. And your way of using OOP just uses the possibility to extend a class. But even your base class DB already does completely not db related stuff (issuing location headers is no db-...


2

I quickly looked at the code, and didn't see that we use the lazyness of the fields of Userpoly, so we could make them strict: data UserPoly name city age = User { userName :: !name , userCity :: !city , userAge :: !age } deriving (Eq, Show, Generic)


1

Thoughts: Consider moving classes, includes, config, and initialization out of web directory. There is no reason for them to be there. Eventually if you get to the point of putting your static assets on a CDN, you could conceivably only have your index.php file and robots, sitemaps, etc. files in the web directory. Consider configuring rewrite directly in ...


Only top voted, non community-wiki answers of a minimum length are eligible