22
I personally think it's fine, but it helps to turn your thinking around from a "selector" to a "filter". Then, the default is to return all books, and you selectively filter down the set you're going to eventually respond with.
(Note I haven't written Java in a long time, so grain of salt here,) I might specify a BookFilter object which can incrementally ...
22
Classes
Why do you need a separate class for each game whose data you request? This is very inflexible, should you need to request the data for more or fewer games. Just use one class to represent a game. It looks like all the attributes in your game classes are identical except for the key or index used to access GAMEID. Therefore, just use a range based ...
17
It feels like you are combining lots of hashing and string transformation in hopes that complexity will make your scheme more secure. That's not engineering or computer science, though — it's Cargo Cult Programming.
There are two ways that an API key scheme could work:
To generate the API key, concatenate the username with a salt and site-wide secret, ...
16
1. Review
It's not recommended to use using namespace std; — the problem is that this imports all of the identifiers from std, and some of these may shadow names from other modules that you need to use. See this question on Stack Overflow.
The code doesn't check for success/failure of many of the functions it calls. These can all fail:
new long double[SIZE]...
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) {
...
13
The Design
A LogReader reads a log, filters the unwanted stuff and converts it into LogEntries. According to your design.
That's a bit much. Split it up.
Make one LogReader that just reads the file and presents you with endless strings. Oh wait, that's a BufferedReader! I'd see the LogReader basically as an iterator wrapper for BufferedReader.
Make a ...
13
Currently your code is not re-useable. To make it re-useable you should move the logic inside a function.
To do this the function we will have two functions here: _get_data_from_api and get_current_iss_location. Former gets the data from API and latter one passes that data to LatLon class and returns its instance.
The advantage of having two functions here ...
13
find_game_ids is far more complicated than it needs to be. You have essentially two "counters", Idgame and i. One is being used to be placed in a string, and the other is to limit how many loops happen, but they're the same value if you think about it; just opposites. You don't need Idgame since you can just check if i < 20. You also don't need ...
12
Having worked on a similar system, I came up with a very similar design. So either we're both doing something right, or we're both doing it wrong :)
Here are some things I would change, but obviously different requirements call for different decisions and may not all be applicable here.
Remove CodeInspection class
I don't think you gain much from having a ...
12
Abstracting classes
In your question you asked how to abstract classes so I thought I would show a concrete example of how this is done. Here is a minimal example of how you could abstract your game function.
class GAME():
def __init__(self, ID):
url = ('https://na1.api.riotgames.com/lol/match/v4/matches/'+str(ID)+'?api_key='+(key))
...
11
Design
I think you need to break that class in two. Having instance members on a static class is pretty confusing and bug-prone.
I'd suggest Enumerable to be the static class, with these members (notice source being preferred over collectionObject, and explicit ByRef modifiers and Variant types):
Function Contains(ByRef source As Variant, ByVal value As ...
11
I'll just go through this from top to bottom
/**
* Returns the next log entry.
*
* The NotReadableException has more information available.
* You can recover the lines that could not be read by calling NotReadableException#getLines.
* You can see which exceptions were thrown internally by calling NotReadableException#...
11
You have a lot of null checks (well, two at least), which I think leads to less readable code. Also, it shouldn't really be necessary to have them.
if (authToken == null) this isn't really something this method should concern itself with. In my opinion, getUserFromDatabase should handle that case (via IllegalArgumentException, which is a RuntimeException). ...
10
Looking at it from a general perspective...
Put your comments that describe functions in javadoc-style outside your functions. Comment blocks before functions are a common format when it comes to describing a function. Because it's at the same indentation level as the function, it can't be mistaken as a comment about a specific section of your function.
...
10
The code is clean and understandable. There are only minor improvements that we can apply.
use more descriptive variable names: latitude_direction and longitude_direction instead of lat_dir and lon_dir
use ternary conditional operator:
latitude = data['iss_position']['latitude']
latitude_direction = 'N' if float(latitude) > 0 else 'S'
longitude = data['...
10
int NextFlag(string category)
I'd expect this to throw when it runs out of flags.
I really really don't like that the first flag just happens to be 0: that depends on the order in which they are defined, and isn't written down anywhere.
Option FromValue(int value, params Option[] options)
I don't understand what this method is really meant to achieve... ...
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,
...
9
I am not convinced that your general structure is useful as it should be. I have been trying to untangle how your code us used, and what sequence of events happen in order for the code to run the way it should.... I have run in to a number of issues in that process, and they all add up to a suggestion that you are using the wrong approach.
Even with your ...
9
Is this as extendable as I think it is?
Does it work for multi-bit flags? For instance
[Flags]
enum Modifiers : uint {
None = 0,
Private = 1,
Protected = 2,
Public = 4,
NonPublic = Private | Protected, // <- multi-bit
All = ~None
}
Are there any APIs missing that I didn't think of or would be
convinient?
BitVector32 has ...
8
First thing I would do is standardize indents, from this:
void luaW_printstack(lua_State* L)
{
int stack = lua_gettop(L);
for (int i = 1; i <= stack; i++)
{
std::cout << std::dec << i << ": " << lua_typename(L, lua_type(L, i));
switch(lua_type(L, i))
{
case LUA_TBOOLEAN: std::cout <&...
8
There's something odd about your constructors - normally chained constructors call the constructor with the most parameters, not the other way around:
public ProsperApi(string username, string password, string baseUrl)
: this(username, password)
public ProsperApi(string username, string password)
Should be:
public ProsperApi(string username, string ...
8
A few notes:
I feel like some of your properties should have the readonly attribute applied to them. Do we really want the API users to have the ability to change the username, password, or server variables after initialization?
@property (nonatomic, strong, readonly) NSString *server;
If you do really want them to have that ability, I would add a method ...
8
Good
Meaningful names are used for classes, methods, parameter and properties
using interfaces over implementation
using enums to signal a state
Not so good
Code isn't tested
Scope of property setters is public where they should be private
Using of properties which are never used
Single responsibility principle is violated
The good things ...
8
Your configuration is just a bunch of variables:
var opts = opts || {},
el = el,
minvalue = opts.minvalue || 0,
maxvalue = opts.maxvalue || 10,
step = opts.step || 1,
change = opts.change || null;
This makes it far more difficult to identify accesses to the options in the rest of the code. I recommend you use an object for the options, ...
8
Design
if (response.IsSuccessStatusCode)
{
string content = await response.Content.ReadAsStringAsync();
result = JsonConvert.DeserializeObject<IEnumerable<WorkItemDto>>(content);
}
if (!response.IsSuccessStatusCode)
{
throw new Exception((int)response....
8
I don't have much to say, but this stuff does look useful. The types are designed to be exposed to the world, so it's transparent to the consumer and could make the intentions clear.
There is one huge problem:
public string Text { get; set; }
Anyone can change this value and by-pass all the rules: I assume this is a typo and you intended for this to be ...
8
The other reviewers have already explained how you should re-organize your classes, but I want to expand a bit on how you create them and how it can save you a lot of typing.
So we assume that you use the classes created by akozi.
Then we can put the creation of these classes inside a list comprehension.
nr_games=10
games=[GAME(GAMEID[i]) for i in range(...
7
Dates and Date handling in Java have been a problem since the beginning (check out all the deprecated methods in version 1.1). You appear to be running in to the same problems that may Java developers have encountered, and then there are a whole lot of issues that you have not yet encountered.
Unlike other questions in CodeReview, I'm going to deviate from ...
7
Yep not bad certainly seen worse
Try to keep your spacing consistant for example you do for() but
foreach () one or the other
Doing this } else $image_ret = 0; is generally bad for readability / good code try to either use full if () { } else { } with the results of the if on separate lines or use ternary's where possible.
Try to segment off your process ...
7
Interesting code,
the first thing that comes to mind is that forEachand map are slower than for loops, so you might want to consider converting those statements to for loops.
the second thing is that I assume you query several times these data structures, if that is the case then the node should know who is/are the parents and who is/are the children, it ...
Only top voted, non community-wiki answers of a minimum length are eligible
