Hot answers tagged

22

Well done. This is not a complete review, but instead a (short) list of possible improvements I found when I skimmed your code. Documentation First of all: thank you! It's great to have documentation. Note that there is some debate whether to put the documentation into the header or the source. I'd like to remark that I would put only a @brief ...


14

As an addendum to @retailcoder epic answer I offer... Object Oriented Dice Lots of your existing code will simply melt away when we take a more object oriented approach. The key is making classes for the basic things in dice game world and have each class responsible for doing what it is supposed to. Die Class A single Dice, that is. public class Die { ...


12

There are a bunch of style issues you should be aware of, a validation problem, an optimization or two, and also an alternate approach: Style: Use braces for 1-liners... these are 'no-no' statements: if (i % 3 == 0) sb.append("Fizz"); if (i % 5 == 0) sb.append("Buzz"); if (sb.toString().isEmpty()) System.out.println(i); ...


11

This will not be a full review of the code at all since I don't really review how it works, but here are some little things that may be of interest to make it better anyway :) Qt5 new connection syntax Qt5 introduces a new syntax for signals and slots which allows you to directly pass function pointers instead of relying on the Qt preprocessor to do the ...


10

Don't commit an empty event handler without a comment like // todo: implement "New" toolbar button click - a comment like this would explain why you have such a handler, and why it's empty. Otherwise you're registering an event without doing anything about it and that's just plain YAGNI. Your private fields are not following established naming conventions. ...


10

Okay, lets begin at the very beginning (since that's a very good place to start. Allegedly). Swing is the Java GUI framework (now EOL, so probably not worth learning really). As with most GUI frameworks, it's single threaded. It replaces AWT, which was an attempt as a thread safe framework. It was decided, for a number of reasons, that thread safety and ...


9

Documentation As this is a library I'm missing documentation for the header file. At the very least it should say that it is a modal dialogue (is it in all cases?) and what error scenarios there are. Export signatures You're missing the __declspec(dllexport)/__declspec(dllimport) and equivalents for Linux/MacOS if you're going to pass this of as a ...


9

Make members final final member variables are easy to work with. As they can never be reassigned, they are less error prone, as accidental reuses are not possible. For example, in Dot, you can make all members final. In Controller, you can make blobs, dots, b, mf members final. Make members private It's a good policy to make member variables private by ...


8

I don't know what the standards are for Layout Naming on Android, but I can tell you that these names are not very good. textView2 btn_distance_walked textView3 none of these accurately describes what they do. you should probably look for naming schemes on Google. btn_distance_walked doesn't tell us what the button is going to do. What is it going to do ...


8

One big point is separation of concerns. Mixing GUI-Code with IO-Code is not recommended. Some pseudo Code. Make classes for Workout where just the information of your Workout is Stored. class Workout { List<WorkoutPart> parts; boolean abs; boolean squat; } class WorkoutPart { String description; int sets; int reps; } Make a class ...


8

Don't use String labels of GUI elements in flow control It's not good to use the String labels of GUI elements to make decisions. It would be slightly better if you made "Sum", "Difference", ... global constants, but only slightly. An enum would be better for this purpose: enum Operation { Sum("Sum", (x, y) -> x + y), Difference("Difference", (...


8

One of the things I'm specifically wondering, is it ok/proper to have static helper functions in the .cpp file if they are not member functions? (should they be member functions and should they even be static?) Yes, it is perfectly good. In fact, if something can be written using only the public interface, it should be written as a non-member. Yes, ...


7

A great comment from @tb-'s answer: Do not extend JPanel, instead have a private JPanel attribute and deal with it (Encapsulation). There is no need to give access to all JPanel methods for code dealing with a UserPanel instance. If you extend, you are forced to stay with this forever, if you encapsulate, you can change whenever you want ...


7

It depends on how much /* Code */ there is. If it's just a few lines, that's one thing. If it gets too large though, the reader will lose sight of the fact that you're creating an anonymous listener. I would say that if the anonymous listener is more than 5, or maybe 10 lines then you should use an inner class instead. There comes a point where even an ...


7

As someone who's been working with your project and your Console Gui here's my 2 cents: protected final TextArea textArea = new TextArea(); protected final TextField textField = new TextField(); these could use a rename. The names do not contain any information and in all the unit tests you wrote, this was confusing me very much: Which is in, which is out? ...


7

import javax.swing.JOptionPane; public class FizzBuzzCounter { Why is this way indented over here? I'm sure its a matter of copy/paste issues but the fact that you left it there wouldn't give me a good starting impression. public static void main(String[] args) { int goalNumber = Integer.parseInt(JOptionPane.showInputDialog("...


7

Your JavaFXGame class can use javadoc /** * This class just loads the FXML document which initializes its DocumentController */ public class JavaFXGame extends Application { The start method can be shortened and cleaned up: @Override public void start(Stage stage) throws Exception { Parent root = FXMLLoader.load(getClass().getResource("...


7

Don't put everything in your main method. It means that you can't: Reuse the code without editing it. Test the code without running the entire application. The UI should not be coupled with the business logic. The IO should not coupled with the UI or the business logic. Extract magic numbers from your code and replace them with named constants. The length ...


7

EditorMain import javax.swing.ImageIcon; import javax.swing.JButton; import javax.swing.JComponent; // ...You get the idea. import javax.swing.UIManager; import javax.swing.SwingUtilities; import javax.swing.WindowConstants; At this point, you may as well just do the one * import statement: import javax.swing.*; Note that the TableModel import is not on ...


7

Avoid magic numbers Number 3 appears in many places, for example: JPanel[] bulbPans = new JPanel[3]; JLabel[] switches = new JLabel[3]; JLabel[] bulbs = new JLabel[3]; But what's even worse is that it's critical that it's the same number in all these places: if you change only one of them and forget the others (including many others I didn't include in ...


7

2 male and 2 female superheros... Very politically correct ;-) I suggest to try to avoid repeated code as much as possible. For example, Desktop.getDesktop().browse(...) is used in most of the case statements, only the URL parameter is different. I would refactor this in a way that the case statements set a URL variable, so that you can write the Desktop....


7

Some suggestions: In calcator, I would use a ternary expression: return 1. if n is None else n/100. calcator can be made even more efficient by having iterate_mandelbrot return 100. if the loop finished. Then you just divide the result of that by 100. This will result in 1. if the loop exits, avoiding the if test entirely. I would only do run self....


7

Calculators seem really popular... I'll start with what stands out. new JButton("÷"); This likely doesn't work for you and definitely won't work everywhere, to use the Obelus you want to refer to its unicode, so rather instantiate using new JButton("\u00F7"); Instead of having all those variables for your number buttons simple create an array. JButton[] ...


7

I have only some small suggestions. The - (NSArray *)tc_getAllSubviewsRecursively method creates a (temporary) mutable array for each subview in the hierarchy. It might be more effective to create only a single array and passing this array around to append subviews recursively: - (NSArray *)tc_getAllSubviewsRecursively { NSMutableArray *subviews = [...


7

Do not include types in variable names carCostP ^ Remove that P (probably stands for a number type?) the compiler takes care of types for you. Avoid so many variables You can avoid those four variables altogether, you may just convert to double as soon as you read the vars, like this: double carCost = Double.parseDouble( JOptionPane....


7

A HashMap does not guarantee any order whatsoever, so that is a bad idea to use. Your Image and Label belongs together, so make a class where they actually do belong together. public class StarView { private final Label label; private final Image colorImage; // constructor and stuff... } private void createContents() { this.contents = new ...


7

Your outset is wrong. The articles you've linked to present a nice story, but what you're running into here is the issue with that ideology: When your data structures AND your views are rigid, you can't change the system to do anything else. You're now stuck! Say I had to show a listing of Employees. I can't! There is no way to use the data stored in ...


7

Using an AsyncTask instead would be much better. You've got many callbacks to the UI thread, all of this you could do in the post execute method instead: private class LoginTask extends AsyncTask<Void, Void, String>{ private String username; private String password; private Exception exception; private boolean isSaveSuccess; ...


7

I like how your form is no longer concerned with anything other than being a form. One More Abstraction Level I'd go one step further, and encapsulate the controls, so that the presenter class doesn't need to know about the implementation details of the form. For example the ChangeLabelAndCaption method knows that there's a Label control on the form, named ...


6

Lambda Usage I'm not a great fan of the lambda usage to create what are effectively partially applied functions in the self.player_button and self.computer_button. Instead I'd rather do something like: import functools self.player_button = Button(self, text="Me", command=functools.partial(self.do_start, TRUE), ...


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