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 ...
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, ...
15
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 ...
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 ...
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 ...
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 ...
7
You have some nested if statements that I think you could un-nest a little bit
For Each item In mCollection
If IsEmpty(result) Then
result = item
Else
If item < result Then
result = item
End If
End If
Next item
so like this instead
For Each item In mCollection
...
7
A few points on top of @Heslacher's excellent review.
Another violator of the single responsibility principle is the MoveToNextQuestion method, since it does two things:
Moves to the next question
Updates quiz state
When a method does something in addition to what its name suggests,
that becomes a burden on the programmer:
since the second action is not ...
7
Instead of having a private CreateClient() method you should consider to make it protected virtual if you would ever have the need to use headers different to "application/json".
Changing to protected virtual forInternalExecuteRequestAsync()should be done too.
You have small code duplication in the TMDbApi class.
The GetTvGenres() and the ...
Only top voted, non community-wiki answers of a minimum length are eligible
