Hot answers tagged

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 ...


15

As @Jeff pointed out, this code is ...beautiful. Well done! I like the fluent interface a lot, but the code itself is at first glance... wow. Naming All identifiers follow conventional casing (camelCase for locals and parameters, PascalCase for types and their members). I like that you're using an _underscore prefix for field names, but that's just my ...


12

throw ex; This is a fatal mistake. It'll create a new stack-trace and you won't be able to tell where the actual exception occured. If you want to rethrow it then use just throw; without ex.


9

This is pretty good. I only have a few small nitpicks Return directly You do this a few times: if ship['passengers'] != "unknown" and int(ship['passengers']) >= num: return True return False The above can be rewritten as: return ship['passengers'] != "unknown" and int(ship['passengers']) >= num Superfluous int call int(20), here the int ...


7

public class DateFormatter { static String date1 = ""; static String date2 = ""; static String date3 = ""; static String date4 = ""; static String date5 = ""; static String date6 = ""; static String date7 = ""; public static void dateFormatter(NumberAxis xAxis){ loadDates(); xAxis.setLowerBound(0.0); ...


7

I'm really sorry, but after starting this review I have to tell you: Go back, read some more tutorials and try again This code has so many issues, that I'm not able to point them out reasonably without saying something stupid and / or offensive, so I'll give you a few tips that should help you, if you rewrite this from scratch: Avoid static Like The ...


7

Should be changed: Request mappings are not that RESTful due to the verbs: POST /api/cars/add-car => POST /api/cars GET /api/cars/cars => GET /api/cars GET /api/cars/get-car/{id} => GET /api/cars/{id} PUT /api/cars/update-car/{id} => PUT /api/cars/{id} DELETE /api/cars/delete-car/{id} => DELETE /api/cars/{id} POST can return HTTP 201 ...


7

A lot of times people post PHP here and it's often very ugly, this is not one of those times. Your classes and design are beautiful, and it's very obvious what your intentions are. You've built a good structure, and you've commented clearly. In fact, I saw this comment as was immediately impressed: //we need to get the data the following way because we ...


7

I'll try to cover the response part. SingleProduct and BundleProduct should be polymorphic. ConcretePrice, DiscountedPriceByAmount, DiscountedPriceByPercentage should be polymorphic. I think they are talking about the following: you need to have a table of products and a table of prices this is not exactly a requirement, but I guess this would be a ...


7

Inversion-of-control I wouldn't use reflection using error-prone member-by-string lookup, when a simple Func provides type-safe behavior. This signature could be used to have the error callback, client factory and operation provided using inversion-of-control. public async Task<ValidationResult<TRes>> ExecuteWithTokenRefresh<TApiClient, TRes&...


6

Even if in 1 line public static void blockHost(String hostName) { blockedHosts.get().put(hostName, hostName); } effectively consists of 2 operations, so if you want your methods to be atomic you still need synchronization. You could have an interleaving like: T1 invokes blockHost("123") and executes get() T2 invokes isHostBlocked("123"), does ...


6

I managed to get RESTEasy to work with generics (both lists and single objects) and parse the response to JAXB annotated classes by using the following code that uses reflection: <T> List<T> getList(String path, List<String[]> params, final Class<T> clazz) { GenericType<List<T>> type = getListType(clazz); Response ...


6

The fact that you must be able to swap in a new set of maps at any time--even if only once every three months--requires a design change. Here are the requirements as I understand them: Reads block until all three maps have been set the first time. Reads receive a consistent set of maps. In other words, you cannot return the old secondary map after returning ...


6

Disclaimer: I don't know sqat about Grails createUser I have a few concerns in this method. Using User.countByEmail seems inefficient. I don't know Grails, but the name of this function suggests that it will scan all the users in the database, when it should stop after the first match, effectively doing an EXISTS query. The string parameter in all the ...


6

There's a lot of code here, so I'm just going to review bitex.utils. You'll see that there's plenty here for one review. Maybe some of the other reviewers here at Code Review will review some of your other code. There is no module docstring. What is the purpose of this module? What does it contain? import requests is in the "Built-Ins" section but as far as ...


6

Dispose Strategy It seems your class doesn't even need to implement IDisposable. Should dispose HttpClient? Single Responsibility I would extract configuration management from the implementation of Client. public Client(string baseUrl) { _baseUrl = baseUrl ?? throw new ArgumentNullException(nameof(baseUrl)); // .. } Add a factory for generating ...


5

Welcome to the wonderful world of F#! You're doing fine so far! You'll find that the code stops being quite so awkward as you get more experienced. Meanwhile, here are my comments: Type annotations You'll find that in many cases, F# can infer the types for you. So you can write your first two functions like this: let loadEnvironmentConfig path = ...


5

The actions I'm wondering about are members, add_student, and remove_student. They work just fine, but they aren't RESTful [...] True, they're not part the usual CRUD actions that can be expressed with HTTP verbs. But then neither are the default new or edit actions, so it's not like going beyond CRUD is automatically illegal. Even so, associations do ...


5

Is that error, or status, or both, or neither? This API is a bit confusing. You have ErrorCode and StatusCode enums. But ErrorCode contains things like: OK(200, "NONE", "Response is success."), NO_CONTENT(204, "No Content", "Response is success."), CLIENT_TIMEOUT(1007, "Timeout", "Timeout has occured on the Client."); So two of the "error codes" indicate ...


5

You're right, you can cut down on the repetition of code. Although bundling all of the functions into one like you mention would work, you lose a bit of the readability that keeping them as class methods gives. Instead, you could just have an internal _command method that collects all of the repeated code: def _command(self, command_name, required, **kwargs)...


5

I would personally prefer to use HttpClient directly as it provides full control and perfect testability - see here. We can define our REST API as: // see https://jsonplaceholder.typicode.com/ // this site exposes some REST API example to automate public interface ITypicode { Task<BlogPost[]> GetPostsAsync(); Task<BlogPost> GetPostAsync(...


5

Creating a session As a library, I find it barely usable. If I want to use it with my own credentials, I have to: either modify the source of your library to put them in __init__; or tamper with the attributes after building an ArcsightLogger object: o = ArcsightLogger() o.login = 'spam' o.password = 'eggs' o.main() Moreover, having to manually store and ...


5

Your code looks hard to read and not like Python code. The main reason for this is that every second line is a comment. Most of these comments just repeat what the surrounding code does. These comments don't have any positive value and should therefore be removed. The other reason is that you didn't structure the code into paragraphs. After each chunk of ...


5

I know you are generating data for the sake of completion but it does complicate things on what aspect of the code you'd like to focus on. But here are a couple thoughts. With Express, I often like to treat the routes as controllers in an MVC pattern. So any fetching of data and mutating data, I would create at least a model per resource to encapsulate that ...


5

Typically in an ASP.NET app (and in fact in any app), you should only catch and handle the base System.Exception in a single place as close to the application entry point (or in this case request entry point) as possible. This means you don't have to duplicate exception handling code in every action method, and helps ensure you always return a consistent ...


5

Great job. Here are some minor notes though. In newer versions of C# you can leverage async Main in order not to wait for your test method. public static async Task Main(string[] args) { await Test(); } You can speed things up a bit with executing your tests in parallel public static async Task Test() { var successExecute = Execute("https://...


5

Disclaimer: there will be a lot of links to my own site because I am helping people with PHP for 20+ years and got an obsession with writing articles about most common issues. I don't know much about restful stuff or whether it does matter, but there are many other areas of improvement. Let's review some of them The selectData() function First of all, ...


4

For adding and removing single host names, I would use a simple ConcurrentHashMap without the AtomicReference. Initialize it with an empty map and drop the additional latch. Update: I really doubt you need to replace the list of hosts all at once, but here's the combined form anyway: private static final AtomicReference<ConcurrentHashMap<String, ...


4

import requests, json class RestConsumer(object): def __init__(self,base_url,append_json=False,**kwargs): self.base_url = base_url self.uriparts = [] self.append_json = append_json self.kwargs = kwargs Since you treat attributes "special", I recommend putting _ in front of these attributes so that are clearly offset. ...


4

AtomicReferences are a great construct when you have just one item that needs to be kept in a sane state ina multithreaded environment. Your code is trying to juggle three, and ensure they all have a sane state at the right times, and, for that, you need something bigger than the AtomicReference. You have elected to use the CountDownLatch. This, frankly, ...


Only top voted, non community-wiki answers of a minimum length are eligible