New answers tagged object-oriented
3
A couple of thoughts:
You are conceiving the band as being something derived from the frequency. While this may be technically correct, from a user perspective, you generally choose a band and then choose a frequency within that band. So, I might have a model that reflects this:
typealias RadioFrequency = Decimal
enum RadioBand {
case am
case fm
}
...
6
Even Windows uses '/' as a path separator nowadays.
const string Paths::BASE_PATH = "C:\\ProgramData\\BankAcount";
The last time you needed to use '\' as a path separator was 15 years ago.
const string Paths::BASE_PATH = "C:/ProgramData/BankAcount";
If you really want to us the '\' then use the RAW character strings.
const string Paths::BASE_PATH = R"(...
4
1) The strings (BASE_PATH, USER_PATH, etc) shouldn't be hardcoded. It is fine to have program defaults but they should be configurable via some configuration file and/or commandline.
2) There is a lot of repetition in cade, say, when you ensure that all directories exist. This is because all string are seperate variables. It'd be better if they were ...
4
You already have the answer to your "bug" given in the comments, but I will try to give you a more in-depth review. Nevertheless, I will repeat the aforementioned solution: to avoid the program auto-running on import, but your main call inside of a if __name__ == "__main__" (read more here if interested).
For your __init__ method, having your method ...
5
In the sort() function its currently hardcoded to merge sort which I assume is not intentional. However, if you do allow the user to give an input you should do some error checking. You'll currently get unexpected behaviour if the user enters anything other than 1 or 2.
You should use std::swap rather than implementing your own swap function. Some types can ...
-1
READABILITY
Function: int count(T* array)
I don't prefer this way of acessing a (T) while(*(array+i)), because of readability reasons. Better way: while(array[i])
PERFORMANCE
Function: void swap(T& t1, T& t2)
Faster way is to use XOR (^) swap algorithm. it is faster and small amout of memory is used.
if (t1 != t2)
{
*t1 ^= *t2;
*t2 ^= ...
3
There is NO heal() method in class IO...
10
Each one of the filterBasedOnConsist* methods look like they would be individual predicates themselves. So convert each method into a Predicate class and use the default and method to chain them together into a composite predicate:
Predicate<String> employeePredicate =
new FilterBasedOnConsistAge()
.and(new FilterBasedOnConsistGender())
....
10
What especially jumps at me is, that you take your real data object (the Employee), convert it to a string representation and do your checks on the string.
Why? Can't you check your data object?
Apart from that, I don't see a problem with 5 and-conditions. This is clearer to read than some clever stream-through-predicates-and-reduce code. Clear. Simple. ...
3
One option would be to do it with Streams. This is assuming that all different filters are in the same class as the filter method:
private boolean filter(Employee employee) {
Stream<Predicate<String>> filters = Stream.of(
this::filterBasedOnConsistAge,
this::filterBasedOnConsistGender,
this::filterBasedOnConsistNationality,
...
5
You also have an operational problem as much as a code problem. Any tokens you use for a public demonstration should -
Be as least-privileged as possible (eg grant READ access instead of CRUD or full admin)
Be tied to demo/test/dev environments where possible (I pray you are not doing code streaming in PROD)
Expired/rotated/destroyed frequently and ...
2
Your Insert is wrong, you have to do it the same way as the Remove with shifting Elements. ArrayIndex is not increased to decreased on Remove/Insert.
Better don't call any parameter a parameter or add the DataType to it. Name as you speak. You insert an 'item' or an 'element' to your list. Not a parameter.
Always Doubling the array on overflow can lead to ...
2
There are no classes in C. However, there are functions.
Regarding:
CFLAGS = -g -O2 -Wall $(MACHDEP) $(INCLUDE)
When compiling, always enable the warnings, then fix those warnings. ( for gcc, at a minimum use: -Wall -Wextra -Wconversion -pedantic -std=gnu11 )
the = will cause this macro to be re-evaluated every time it is referenced. Suggest:
...
1
long double
long double is a niche type with an inconsistent interpretation: sometimes it's a plain old 64bit double, sometimes 80bits extended-precision, sometimes (rarely) 128bit aka "quad". In the 80bit case, the actual storage requirement may be 10, 12 or 16 bytes. What you get depends on the targeted processor as well as the compiler and perhaps any ...
2
Interface
The first thing I notice is that there is a lot of duplicated functions, because you provide a named function for each overloaded operator: coefficient, add, plus, subtract, minus, multiply, times, and print. Some of them even have non-member versions. This is not common practice in C++ as far as I can tell, so just eliminate them.
Returning a ...
1
Thank you to everyone that helped me out on this. This is what it has refactored into based on everyone's suggestions. Looks more concise and easy to read. Appreciate the advice so I can continue to learn C#.
using System;
namespace ExerciseTwo
{
internal class Post
{
public string Title { get; set; }
public string Description { ...
4
Const methods
These:
int posX() { return foodX; };
int posY() { return foodY; };
// ...
int posX() { return headX; };
int posY() { return headY; };
int len() { return length; };
don't modify anything in this, so make them const:
int posX() const { return foodX; };
int posY() const { return foodY; };
Setters
This:
void ...
6
Statics
You have a one-file program, so more of your methods and globals, including these:
int playerLossesCount[NUM_PLAYERS];
Wall walls[NUM_PLAYERS][NUM_WALLS];
GXTexObj texObj;
should be made static.
Declarations for variables
int main( int argc, char **argv ){
u32 fb; // initial framebuffer index
u32 first_frame;
f32 yscale;
u32 ...
14
Your main is a classic arrow anti-pattern. You can use guard clauses to make the code flat and easier to understand.
Bare excepts are normally not a good idea. Why would you want to print the usage if the code works fine but the user passes a keyboard interrupt? You may want to use except Exception:.
I would prefer if mask and unmask were wrapped up in a ...
10
def int_to_bytes(integer) -> bytes:
# wish we didn't have to implement ourselves...
result = []
while integer:
result.append(integer & 255)
integer >>= 8
return bytes(reversed(result))
Your wish has been granted; you don't have to implement it yourself:
def int_to_bytes(integer) -> bytes:
return integer....
2
You have a couple of good answers. Since you are an experienced developer who is new to C#, I will address some other things.
Things you do quite well
Braces and indentation
Naming (most of the time)
For the last item, most of your naming is good. As @iSR5 mentions, TimeDateCreated could have a better name. I have been programming since the 1980's, and ...
3
There is nothing to add more than @potato's answer. However, I just want to re-enforced the answer.
The naming convention for TimeDateCreated can be changed to CreatedOn or CreatedDate or any related naming for creation date. The keynote here is that you don't need to specify the datatype name in the properties as the property is public and I clearly can ...
0
You're going the right path, but to master OOP think about the logical division of things. Hands and Decks can indeed be seen as objects, but you've kept functions like display_all_cards_and_values, restart_game, and continue_playing separate---these sound like aspects of a Game to me. Some of your classes also contain methods and data that don't seem to ...
6
I don't know which version of C# you use, but as of C# 6 it's possible to set initial values on property definition, which has 2 advantages:
Can eyeball the initial values quickly by looking at the property definitions.
Doesn't require you to copy the same initial value assignment code into additional constructors you may create.
Example initial value ...
1
Taking the two answers in this post in consideration, as well as in the linked post, and the discussions, I came up with this solution. I believe it now finally respects all the SOLID principles!
public abstract class Animal : IEater
{
public AnimalKind Kind { get; set; }
public AnimalSize Size { get; set; }
public bool ...
1
This is my refactor based on all the suggestions made here. Works quite nice. Thank you all for your help and teaching me a few things.
Stopwatch.cs
using System;
namespace ExerciseOne
{
public class Stopwatch
{
private DateTime timeStart;
private DateTime timeStop;
private bool isRunning;
private void StartTimer(...
2
I agree with @Henrik_Hansen's answer. My answer builds upon his.
I absolutely don't see a reason for anything to be static.
Your Stopwatch class should only be for a stop watch. Right now you have commingled a UI into the stopwatch. In the name of Separation of Concerns, I see no reason for any Console.Write or Console.WriteLine in the Stopwatch class....
6
public static void RunProgram()
{
....
}
Why do you run the application as a method on the Stopwatch class it self?
You have made all the methods (StartTimer(), StopTimer()) private. This means that they only can be run from your RunProgram method, which occupy the main thread of the program and measures nothing. This set up makes the entire effort ...
15
PEP-8
Class names should be CapWords, so instead of rectangle you should have Rectangle.
Commas should be followed by 1 space. You've mostly followed this, except in s1 = rectangle({'p1': (1,1), 'p2': (2,2)})
Bugs
The formula for "area" is not twice the sum of width & height.
I don't know what "surrounding" is, but the formula for perimeter is not ...
7
Following the advice from K.H. we get:
class Rectangle:
def __init__(self, dct):
items = dict.items()
self.start = items[0]
self.end = items[1]
self.width = self.end[1][0] - self.start[1][0]
self.height = self.end[1][1] - self.start[1][1]
self.midpoint = [self.width / 2, self.height / 2]
self.area ...
4
You could potentially use incremental computation (think Excel cells that cache values).
Some details (in F#) here:
https://fsprojects.github.io/FSharp.Data.Adaptive/
The above is ported from the Incremental portion of the Aardvark project:
https://github.com/aardvark-platform/aardvark.base
The documentation is all F# since this model works well in a ...
5
I don't know what you mean by "more elegant", but I would recommend using pathlib. This class provides for caching to be enabled/disabled for all files. If the class is not initialized with a cache_dir, then caching is disabled. If a cache_dir is provided, then __getitem__() loads data from cached files, if they exist. Otherwise the data is calculated ...
8
Class should begin with capital, so Rectangle
You are creating local variables just to set them to object on next line using self, assign calculation to self.xyz variable directly. For example:
self.area = (self.width + self.height) * 2
self.surr = self.width * self.height
Creating list of keys, then list of values to then map it to ...
2
You should separate concerns whenever possible. Since BLE & HTTP requests are quite different, you could write the following protocols:
protocol Request {
associatedtype Provider
var provider: Provider { get }
func execute()
}
protocol BLERequest: Request {
typealias Provider = BLEProvider
var code: UInt { get }
var ...
3
that's me who did the review of your previous version.
I'd still stick to what I proposed in the other answer. So here I will just add a few additions and clarifications. Then I will mostly try to focus on where you break SOLID and maybe other OOP principles in your current version. I will also include a few references to iSR5's answer.
I realized the ...
2
Use type hints for parameters and members
def __init__(self, row, column):
self.value = 0
self.row = row
self.column = column
self.block = (row // 3) * 3 + column // 3
self.permitted_values = set([])
can be
def __init__(self, row: int, column: int):
self.value: int = 0
self.row = row
self.column = column
self.block: int ...
3
I think you're overdoing it. I know that you want SOLID pattern, but enforcing it like this way add more code complicity.
What happens on your code, is defining a AnimalKind and define multiple animals names (such as Lion, Hyena), which would be fine for a testable project, but it will make things much complicated in real or big projects. A simple example ...
2
It depends on what is exactly possible in your class - can you override class, can some fields be null at any point? For example retrofit creates classes using reflection and some classes can be very unexpectedly null unless set in constructor without parameters.
Many IDEs have generators, they do this really well.
If you override equals, you really should ...
1
After reading the answer given by slepic, I decided to take a different approach. I came up with the following solution, which I think doesn't violate neither the Open-Closed Principle nor the remaining SOLID premises. The main change I have made was to pass the "eating preferences" logic to a centralized component.
public abstract class Animal
{
public ...
1
The comment in the ISBN class should rather be outside the class declaration, and it should contain a few words for the human reader, such as the expanded abbreviation and the Wikipedia page, to make it easy to get additional information about the topic of ISBNs.
Storing an ISBN as 5 numbers is wrong. If the length of these numbers doesn't sum up to 13, you ...
2
As others have mentioned, your tabbing has quite the negative impact on readability.
As dusty stated, you can safely declare $collection as private.
There is no reason to check if the $collected class object is declared or if the designated $key exists before pushing an element into it. Just push that sucker in there!
In captureShutdown() method there is ...
5
every time I would need to add a new animal, I would not be able to do it just by extending the existing classes or by adding new ones: I would have to modify the existing classes.
The problem exists in both your approaches. It is because the eating preferences of individual species is mutualy dependent on all other existing animals. If you add new animal, ...
1
This is a mere supplement to the other answers.
First, there are several places in the code where you stack two if-statements on top of each other, because you want to check two different conditions before proceeding with an operation:
if "Citizens of " in line:
if "vaccination" not in line:
...
1
You could save yourself a lot of repetition by defining a generic class that takes elements of the tree and saves them. This way you can separate the class from the data needed to build it.
In one file you can have all your fields defined:
XMLDOC_FIELDS = {"language": ("language",),
"service_type": ("serviceType",),
"...
6
PEP 8
Overall, your code is very clean. However, there are a few minor PEP-8 deviations:
.getname() should be .get_name(), as it is two words.
In several places, where a statement continues over multiple lines, the indentation is not PEP-8 compliant, such as:
self.board = {1: ' ', 2: ' ', 3: ' ', 4: ' ',
5: ' ', 6: ' ', 7: ' ', 8: ...
1
The pandas library has various functions for creating, manipulating and selecting date ranges. For example, date_range(start, stop) creates a range of dates. bdate_range(start, stop, ...) can take a mask to only create certain days of the week, and can skip holidays.
One way to create your example date range:
import pandas as pd
a = pd.bdate_range(start=...
10
Your code is not thread-safe. You should use ConcurrentHashMap instead of HashMap, if this code is ever run in a multithreaded environment.
In Java 8 there was a terrible performance bug in ConcurrentHashMap.computeIfAbsent, which would lock the whole map even if the key already exists. That bug has been fixed in Java 9. See also this question.
8
Two very small details:
Instead of using putIfAbsent / get, you can use the single function computeIfAbsent:
instances.computeIfAbsent(
this.getClass(),
ign -> Collections.newSetFromMap(new WeakHashMap<>())
).add(this);
Instead of a new HashSet for a not-found-result, you can simply return Collections.emptySet(). As your return value is ...
0
Specific suggestions:
Parameter defaults are not relevant for reveal_cards - it's always called with hide set.
Boolean parameters are a code smell. Usually they should be replaced by two methods which each encapsulate just their difference, usually calling a third method to do the common work.
The ranks and suits and their values should probably be an ...
3
in my opinion, the code is good, but I have some suggestions.
Instead of using java.util.Map#containsKey, you can directly use java.util.Map#get and check if the value is null.
protected static int countInstances(Class<? extends InstanceRecorder> key) {
Set<InstanceRecorder> instanceRecorders = instances.get(key);
if (instances != null)
...
Top 50 recent answers are included
