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


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

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

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

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

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


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

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


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


6

You require the interface to be IDisposable which is a bit of a shame. You're only doing this so that you can dispose of the HttpClient, but as it says in the Microsoft docs: HttpClient is intended to be instantiated once and re-used throughout the life of an application. Instantiating an HttpClient class for every request will exhaust the number of sockets ...


6

fizzBuzzOutputsRepo is an in-memory map and there is no data persistence. So if the web server crashes, then there will be data loss. Even if data persistence is out of scope for the interview question, by default a web server works in a multi-user environment. However, there is no synchronization method to protect the data integrity of fizzBuzzOutputsRepo. ...


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

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

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


5

From API design perspective let me pitch in Have a base path and then have the resource name in the corresponding request mapping. Base path /api version /v1 request mapping /fizzBuzz Add HTTP status code in response. As simple as 200 for OK and 400 Bad request should be fine for a sample API. Further reading: HTTP status LIST Add Swagger Docs it's ...


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