New answers tagged object-oriented
0
If we define SOLID as it is stated on Wikipedia:
Single-responsibility principle
Open–closed principle
Liskov substitution principle
Interface segregation principle
Dependency inversion principle
Then several improvements are possible:
Split the class into a Matcher and a Container class, to separate responsibilities
Avoid protected methods and public ...
16
#include <bits/stdc++.h>
it is a cheap hack, you're basically avoiding including the separate header file - string, vector, iostream. This doesn't even work on my compiler in visual studio ( msvc ).
why should I not #include <bits/stdc++.h>
using namespace std
This one is worse, it's making your program a lot more confusing. When there are so ...
0
Good work updating to ES6 syntax. With ES6 come block scoped variables - declared with const and let. It is advised to default to using const and then switch to let when re-assignment is deemed necessary. This helps avoid accidental re-assignment and other bugs.
If you want to optimize this to get better fps you could explore ways to reduce calculations - e....
2
It is often good practice to put the main program controller in an if __name__ == "__main__": so that in case you would want to import this program in another file you would not open the window up immediately, only if you run this file directly. In your case, that controller would be the last three lines:
...
self.aggression = aggression
...
6
A little for the styling of the GUI
Add an icon to your application
Tkinter has made it very easy to add an icon to any of our windows, it's a single line that makes your GUI look good.
All you need to do is go to a website like this one and download any .ico file of size 16x16, I haven't tested it with other sizes and formats, but usually, you'll find ...
8
To add to the answers of Aryan Parekh and pacmaninbw, which I agree with:
Avoid repeating the name of a class in its member variables
For example, in class Event, all the member variable names are prefixed with event_, but that is redundant. I would just remove that prefix.
Avoid using std::string unless something is really text
Apart from date/time ...
10
General Observations
Welcome to the Code Review Site. Nice starting question, very good for a begining C++ programmer and a new member of the Code Review Community.
The functions follow the Single Responsibility Principle (SRP) which is excellent. The classes also follow the SRP which is very good as well. You aren't making a fairly common beginner mistake ...
5
Looking at your program, I would say that you have done a pretty good job considering that you are just a beginner.
private or public?
Let's have a look at your Event class
class Event {
private:
std::string event_type;
std::string event_priority;
std::string event_date;
std::string event_time;
public:
Event(std::string eventType, std::...
3
This program is used to create a random bank card number. Card number is created using account type and client location for the first eight digits and the remaining 8 digits are completely random.
This isn't a critique of the coding per se, but of the requirements gathering. Bank cards do not allow for a fully random last eight characters. Because the ...
3
Always use the base class / interface in the left part of the variable when possible
By setting the java.util.List interface in the variable part, this could make the code more refactorable in the future, since you could easily change the list type without changing everything (inheritance).
Before
private static ArrayList<String> storedCardNumbers = ...
4
Hi Nicholas and welcome to code review.
A few things that I noticed in your code:
The indentation isn't consistent. The closing bracket of menu isn't where it should be. The whole setAccountLocation method is one tab to far right. The readability of the code would be better if you'd fix that. In Eclipse, you can press Ctrl + Shift + F to automatically fix ...
4
class Card():
You don't need parenthesis in the class declarations. Also, while most of your formatting is pretty good, there's a couple nitpicky things. When supplying a default argument, there shouldn't be spaces around the =, and there should be two blank lines between top-level definitions.
You should really never have a mutable default parameter like ...
3
Instead of using either global variables or another class to manage the collections of agents, you could use class variables that do that for you. Consider this (reduced) example of an Agent class:
class Agent:
id_ = 0
alive = set()
dead = set()
def __init__(self):
self.id = Agent.id_
Agent.id_ += 1
self.alive = True
...
4
I'm not an expert in SFML, so I can't really give any advice about that. Though, let me look at the code as is.
Let's start with Game.h:
Your Game is taking a const char * as argument, I would recommend std::string_view if you would be compiling with C++17. It has a lot of features of std::string and it behaves as const char *
I like how you encapsulate ...
5
Prefer member variables to be private if possible
There are a lot of member variables that are public that are not used outside the class itself. For example, Game::score, Bird::texture_wing_up, and many more. These should all be private, as this prevents other classes from accidentily accessing these member variabels.
Avoid premature inlining of functions
...
4
Overview
Prety good. Big points.
Move Semantics should be noexcept
Overuse of normalize() is going to make the class ineffecient.
I would only normalize for printing personally.
Don't use nr() when m_nr is just as valid.
You are already tightly bound in all other member functions.
read() should not change the state of the object if it fails.
Things You ...
2
I've read answer by @user673679 and just want address a few issues.
I strongly disagree with disabling default constructors in classes like Member/Date/BookItem. If class has no default constructor then using it with std::vector, std::map and other template containers becomes very awkward in general. So it is a bad advise.
Instead you should make default ...
4
Date:
Date.hh is missing some includes (<iostream>, <string>).
Don't supply a default constructor. It doesn't make sense to have a default date.
Don't supply one- and two-argument constructors. Specifying a month and date in 1970 is unlikely to be very common.
We should support years before 1970. There were books back then!
year should be a ...
3
constexpr
Since monthsPerYear is a compile time constant, you should declare it constexpr instead of const. See this answer for more details about constexpr
Date constructors
Your Date constructors take int whereas your data members are unsigned int.
Your constructors are calling another constructor of the same class, which in turn calls setDate method, ...
-1
unsigned int month;
unsigned int day;
unsigned int year;
may also be written in one line
unsigned int month, day, year;
You use unsigned int, that's ok, but you should nevertheless
use int, especially for small numbers.
unsigned int is mostly used for transfering data from and
to storage devices / network streams.
signed int is better portable, because ...
1
Your problems include:
naming
not following MVC
inconsistent optionality decisions
having only a single view controller
not keep separate things separate
naming things inconsistently
Naming is important. You have:
var maze: MazeGenerator - A maze is not a maze generator.
var cell: MazeElement - Choose cell or element and stick with one word for it.
var ...
3
Unnecessary classes
The snake, ladder and dice classes are not at all useful. They can simply be replaced with a namedtuple or a dataclass.
Similarly, player and playerposition should both be a single class element. A player object should be responsible for keeping track of their position.
Verbosity
s1, s2, ... s8 and similary l1, l2, ... l8 are not really ...
3
First issue is that I don't see why you make it via OOP. Why Book is an interface? It has almost zero functionality. Use interface for objects with complex functionality when you have several possible implementations. While the Book class can only serve for downcasting which in itself is best avoided unless necessary. Also it has a bug that the destructor ...
0
Streams
A couple things to keep in mind.
The new syntax for .forEach() is cool but it should be used for simple things and it should not be nested. Either refactor it to avoid nesting or extract the nested parts to methods. This way those parts will have names which should make their purpose obvious.
Don't modify collections with .forEach. If you need to ...
2
Passing larger sized data in functions by const &data is a good idea since it does not make a copy. Note that when a parameter is passed by const&, the extra cost dereferencing and fewer opportunities for compile optimizing. You should do this typically when the data is large in size
From your next() function
int next() {
int curr_next =...
2
As the comments have noted, this is such a tiny amount of code that it's somewhat difficult to comment on it.
So, take the following with a grain of salt. It may be reading entirely too much intent into entirely too little code to really mean much.
Pointers
You're using unique_ptrs, which are generally preferred over raw pointers, but the code shows no ...
4
First I would like to re-iterate the points made by @cliesens regarding naming and avoid uint.
Keep in mind the most number of elements you may have in an array is int.MaxValue. Theoretically, with your use of unit, someone could try to create an array that is 3 billion wide by 3 billion deep, which would throw exceptions. And your code doesn't handle ...
1
The only thing I think worth calling out is your init_successful pattern. This is somewhat antithetical to the way that object-oriented languages with exception handling encourage us to think about construction. The way to "make a constructor fail" is to throw an exception, which in turn effectively prevents us from retaining a constructed object ...
5
Welcome to Code Review!
I don't have much time to write a full review right now, but I'll give you a concise list of things you could do to improve your code (in my opinion).
Naming
Use camelCase instead of PascalCase for local variables, class variables & method arguments. In addition to making your code a lot more readable (I don't have to wonder, &...
4
This:
Console.WriteLine("Index invalid");
return default(T);
is dangerous. Having out-of-bounds indices is dangerous enough that you should probably just raise an exception here, rather than returning a default that will probably be wrong from the perspective of the caller. If this is a feature that you're really going to use, wrap it in a ...
2
General observations
If you notice your Parking class, you see that there are too many member variables. The first way to improve would be to try to reduce the extra variables. And that could be done by improving the code logic. But before I get into the logic part, I would like to point out.
Use enum for clarity
if (cartype == 1)
if (cartype == 2)
These ...
5
Observation
You can simplify this using arrays.
You don't need two numbers for each type (max and current). Simply track the number of open spots (per type) and count down.
if you are not using arrays consider a switch statement.
Code Review:
Prefer "\n" over std::endl.
If your main() does not return anything but 0 at the end.
Then leave out the ...
1
Here is some minor things on the class Airline
class Airline:
def __init__(self, name=None):
self._name = name
self._booked = []
def get_name(self):
return self._name
def set_name(self, name):
self._name = name
Is strange to me that you can have an airline that have no name, so I would recommend to make the ...
3
Nice implementation, it's easy to understand and well structured. Few suggestions:
Seat class
class Seat:
def __init__(self):
pass
class FirstClass(Seat):
def __init__(self):
super().__init__()
self.tier = 'First Class'
self.price = 500
class Coach(Seat):
def __init__(self):
super().__init__()
...
3
Only the Character and WeaponBehaviour classes are actually useful and neither of them needs to inherit anything. The rest can be just factory functions, because only the constructors differ. If a class constructor does anything except assign arguments to properties, it is probably wrong.
Strategy pattern is based on composition rather then inheritance.
...
3
Bug: no assignment when calling filter
There are places where Array.filter() is called but not assigned to anything.
Array.from(links).filter(link => Scraper.internalLinks.add(link.getAttribute('href')));
and this in addEmails()
Array.from(emailsInDom).filter(el => {
if(el !== null) {
el.filter(el => this.emails.add(el))
}
});
...
4
I choose to focus on the Single Responsibility Principle in this answer.
To move closer to the Single Responsibility Principle, I think some of the functionality needs to be moved around. I have decomposed the class and removed all state that is shared on the class (rhetorical question: is the class responsible for state or functionality? If it is "both&...
1
I don't think the generator should inherit from the solver. Rather, any methods that they both need could be moved into a third class that either 1) they both inherit from or 2) they both include as a member.
I would try to do #2.
2
Actually I did not reviewed your code as such (as warned by @Emma). But for an "architectural" perspective I see some improvements. However some are more personal guts than real good practices.
The first reflexion is about your the motivation between those two microservices. Why did you split reporting from anything else. Why did you group Account, ...
2
This review will focus on what object oriented programming [oop] is and how we can use two concepts of oop (encalpsulation and abstraction) in your code to make it more object oriented.
The self-drawn pictures of this review come from my Github-Repository for a presentation that is supposed to show the basics of oop.
How to write Object-Oriented Code
Object-...
Top 50 recent answers are included
