New answers tagged

2

Segragated the responsilibites of call center into call center and employee from abc import ABCMeta, abstractmethod from random import random class CallCenter(metaclass=ABCMeta): """ A call center class that will be inherited from by the director Attributes: name: Name of the employee number: Mobile phone number ...


3

If I had to make this OOP, I'd begin by hedging concerns : a class for user data handling (retrieving the POST and using it) a service (that you'd dependency inject) to handle the configuration of the Google Service a GoogleUser class for encapsulating the result of the service and abstracting all those foreach Then, orchestrate those in some kind of ...


2

I have been working on this code to meet the best OOP standards for production. I think your biggest problem here is that PriceCalculator does too many things: Getting user input Calculating the price per checkout item Calculating the total price Printing the bill Let's try to split this up a bit first by introducing a BillRow for each line in your Bill ...


0

I see no benefit to any of these approaches. You're just making yourself write more code. All you need is one class that analyzes a website entered from a user. If you have a predefined list of websites that you need to scrape, then iterate over each website instead of using a class for each one. Since you say there are two ways of accomplishing this task, ...


1

Amazing work! As PeterT mentioned very little to change in your code, however with that said I did notice some code that keeps repeating itself, not sure if this is intentional. Checking if the Model is null If Not this.model Is Nothing Then, this code appears 6 times through out the UserForm, I believe you can validate if the model is null in your ...


0

Hmm, since your values are symmetric, it would be better to make the code better by using the "DRY"(Don't repeat yourself) principle. You can separate your data from the code and iterate on all species to avoid code duplication. I would suggest to store all species data in a json file and then use a for loop to iterate over them. Now in one of my ...


2

A warning on version Many of the following suggestions assume that you are using Python 3. Bare inheritance class SegmentationType(object): can be class SegmentationType: Abstract statics DISPLAY_NAME = "invalid" should not really assign a value. Instead, DISPLAY_NAME: str Unpacking imread The documentation is deeply unhelpful: it says that ...


4

Overall This code is fairly well-structured and very well-documented. Excellent job. Type hints It's good that you specified types here: L : int, float Length of the reactor/lamp [m] r_outer : int, float Outer radius [m] r_outer : int, float Better would be to codify these in type hints: def reactor_volume( L: Union[int, ...


1

I don't know about this typevar but it seems over complicated. Why are you specifying the type and using two classes when you need only one. Here's my solution. class Node: def __init__(self, val): self.val = val self.left = None self.right = None def add(self, val): if val < self.val: if self.left == ...


0

To Your Questions Is there a benefit to using Classes? Right now I don't see the benefit of having a handle, like, player1 = new Player(arg,arg,etc...), player2 = new Player(arg,arg, etc...)... Maybe there are benefits, but I can't think of any serious ones now. The current version is similar to the factory pattern, where the creation of an object is ...


1

Default Parameters There should be no spaces before or after the =. def __init__(self, chips, bet=0, split_cards=False): Hand.__repr__ You can utilize the .join function to append to strings. def __repr__(self): return "Hand()\n" + ''.join(f"({card.value}-{card.suit)" for card in self.hand) Hand.card_visual You can use list ...


1

The review will be focused on readability and testability. Naming I read (I think it was in Clean Code) that variables should not have a postfix like Data and Information and instead we should give things a name that they represent. PortfolioData is actually a Portfolio while your current Portfolio is more a PortfolioRepository or PortfolioCollection. ...


5

Deck.reset bug This function has no access to num_decks, so if you decide to call this function it will fail with a syntax error. A simple fix would be to define this as an instance variable in the constructor, i.e self.num_decks, and use that. validate_answer This function should be written like this: from typing import List def validate_answer(question: ...


5

system() replace os.system with a call to subprocess, which for all intents and purposes replaces the former. Type hints def validate_answer(question, choices): can be def validate_answer(question: str, choices: Sequence[str]) -> str: The Sequence is appropriate because you need choices to be both iterable and indexable. Global variables yes_no = ['y',...


1

Naming I'm sure you already know this, but naming matters. The few keystrokes that you save using w are paid for every time somebody reads the code. Consider always using more meaningful names withdrawalAmount etc. Be cautious about reusing names of classes from the standard library. Rather than Consumer, consider giving it a more specific name to reflect ...


0

Note- the review below only pertains to the code in the post. "Code that's not included can not be reviewed."1 Overall I would say this code is well-written. Variables are declared with const and let appropriately. It has good documentation - especially for each method- although example usage is tough to find. One possible change you could consider ...


3

Unless you have a valuable/deliberate reason to use and in your condition statements, I recommend consistently using &&. Condition logic can sometimes fall prey to unintended "precedence" complications. This whole line is "jacked" (wrong / incorrectly coded): if ($this->ValidateDate($testdate[0], false) && $this->...


2

First of all, to get that out of the way, I despise var and static imports. For me, it makes the code an unreadeable mess that looks like javascript. You would not be allowed to do that on my team. Now regarding the concrete code: It is not a CSV reader. For real csv, you'd need a quote character and a way to escape the quote. Usually this is done via ...


2

I have some suggestions for your code. Use a conventional name for the static factory method name In my opinion, the of name generally aggregate a given set of data into a container; this can cause confusion in this case. I suggest to rename the method to create or newInstance. public static <T> CSVReader<T> of(BufferedReader reader, Class<T&...


1

Return Value The method addUser returns false for all negative cases. Below it is possible to see that it does not return true for the positive case.. instead it returns nothing which leads to an undefined: else { $('#loadingDiv').show(); $.post('process/editUser.php', {uobj:uobj}, function(data) { var modal = "#addUserModal"; $('#...


1

snake_case The PEP8 standard is for these: getTargetFileNames currentWorkingDir to be get_target_filenames current_working_dir and so on for your other functions and variable names. Fail-safe directory reversion os.chdir(currentWorkingDir) should be in a finally, with the try after the first chdir. That said, you shouldn't be changing the working ...


2

I suspect the root of your problem is that you're working at the wrong abstraction level. Specifically, looking at the Telegram Bot API documentation, it seems that an Update is just a wrapper for one of several unrelated objects that describe different kinds of events that your application might want to react to. Trying to treat all of these events as if ...


1

I see you removed the calls to $('#loadingDiv').show(); from the many conditional blocks and moved that before the call to the AJAX call - I was going to suggest that. I see the ecmascript-6 features like the Class syntax are used, so other ES-6 features could be used - like the short-hand syntax - i.e. on this line: $.post('process/editUser.php', {uobj:...


4

The other solutions gave an answer which is perfect for two classes. If you get to much functions//classes beside CallBack and Message, the code below might give you a better solution. Wrapper I personally would create a wrapper with a common interface. The reason is that this is not very extendable (adding a new class requires rewriting all the common ...


10

Assuming Update#getMessage() is of type Message and Update#getCallbackQuery() is of type CallbackQuery: You can make a function getUpdateAttribute that receives higher-order functions as handlers for the situation where your update has a message or callback query as follows. private static <T> T getUpdateAttribute(Update update, ...


15

Keep it like that. It is perfectly readable, easy to understand, everything is fine. Naturally you could do some lambda trickery along the lines of: public static String extractFromMessageOrQuery( Update update, Function<Update, String> messageExtractor, Function<Update, String> queryExtractor, String defaultValue) { ...


3

The real power of OOP is not that you can use classes as namespaces to bundle functions that belong together. The main power is polymorphism, meaning that you can inherit from classes and use subclasses interchangeably with their parent classes. This means that you want to be able to do something like this: class UnitaryMatrix(Matrix): ... a = Matrix(......


2

I don't see any value in writing a private method that fetches $_SERVER data. I mean, all you have to do is nominate the key and the value is accessed and I can't really foresee the need to change the source of the data -- so avoid the unnecessary overhead. Running with YCS's advice to only rely on REMOTE_ADDR, that condition block can be reduced to a ...


10

is it well-written? Well, not really... it is very redundant. A commonly accepted principle is the Don't Repeat Yourself. principle. For example, the following block: if (getenv('HTTP_CLIENT_IP')) $mainIp = getenv('HTTP_CLIENT_IP'); else if(getenv('HTTP_X_FORWARDED_FOR')) $mainIp = getenv('HTTP_X_FORWARDED_FOR'); else if(getenv('...


15

IP Detection. You are confusing an IP address with an HTTP header, the latter being utterly unreliable. It is ridiculously simple to fake an HTTP header. There is an old cautionary tale by Anthony Ferrara about how he unintentionally hacked Stack Overflow by manipulating headers. You may want to read about it. Not to mention that even if you catch one of ...


1

I tackled the book a time ago, skipped this chapter and came back now since this topic is highly/excessive used in the Qt Toolkit - even tho its a while ago. Anyhow, you asked for: I'm especially concerned about my use of dynamic_cast, as that isn't covered yet and I suspect there's a better way. Yes, by declaring the compare in the Comparable class itself,...


5

You are right about using namespace. The purpose of a namespace is to allow types and functions to have the same name in two or more areas of code. Think about the class configuration, its quite a common name for something in code so if you and I are both writing a module for an all singing all dancing app and we both have a configuration class we are ...


2

So, you would like to know if your code is "following the Single Responsibility SOLID principle". S.O.L.I.D. stand for Single responsibility, Open-closed principle, Liskov substitution principle, Interface segregation and Dependency Inversion. Single responsibility One class have a single responsibility. But both Payroll and Employees are mixing ...


Top 50 recent answers are included