Hot answers tagged

5

Thing that makes me more perplex is error-handling. First of all you're catching all exceptions, it may hide subtle bugs unrelated to web request itself (for example wrong parameters you supply in your own code, StackoverflowException, OutOfMemoryException and so on). You're swallowing exceptions, you just log them and return null (which is little bit ...


5

No, it's not correct. View-model shouldn't contain anything related to view. It means LoginViewModel shouldn't contain references on progress bar and label. ProgressBar and Label properties inside a view-model violate MVVM. Any changes in UI elements should be done via bindings specified in XAML. Those bindings have to be linked to properties of a view-...


5

I'm just going to comment on one line: if (dbUser == null || dbUser.Password != user.Password) As you confirmed, you're currently storing passwords in the clear. Encrypting passwords is not the right solution either. Cryptographic Right Answers (2009, Colin Percival, author of scrypt) Password handling: As soon as you receive a password, hash it ...


4

I'm assuming that the code works and fulfills all the usability requirements that you got. Also assuming that this was a home assignment with "unlimited" time where you could code at your own pace and no one was hanging over your shoulder. To summarize, I think there are a bunch of "red flags" showing poor habits and/or poor awareness of ...


3

Generally speaking I find a good implementation and like the simplicity of the approach. I would point out some problems I found though. Inconsistent MVVM style You are using two different ways to instantiate views and viewmodels and linking them together in both views, an inconsistence that could get bigger the and messier the more complex the program ...


3

I would define it as an extension to the Uri type, so it could be used this way: class Program { static void Main(string[] args) { var blogPosts = Typicode.Posts(); var blogPost = Typicode.Posts(1); } } // see https://jsonplaceholder.typicode.com/ // this site exposes some REST API example to automate public static class ...


3

General remarks The naming and the design are ok. The only thing that could be improved is the HttpClientBase It's not a base class so you shouldn't call it like this. I suggest HttpClientService. Timeout = TimeSpan.FromMilliseconds(15000); Or simply TimeSpan.FromSeconds(15); Singleton There are of course mutliple patters for creating a singleton but ...


3

If you know that ItemsSource is a dictionary then you can simply cast it into IDictionary and iterate it because you just need objects and it gives you exacly that: var items = (IDictionary)ItemsSource; foreach (var key in items.Keys) { AddToItemsAndSelectionValues(key, items[key]); } You cast it into a concrete dictionary but you never use the types ...


3

Two things: You clear ToolbarItems and CustomToolbar when page dissapearing, this causes problem: when you show next page and then go back, you won't see your items anymore, because they were cleared. Handlers OnToolbarItemPropertyChanged will not be removed in foreach cycle, because you clear list before this cycle.


3

WebClient.DownloadData is a blocking method; you should use DownloadDataTaskAsync. Network I/O is almost certainly the slowest part of the whole operation, so it's important that we're not blocking here. You might also want to consider using the newer HttpClient instead of WebClient. The third parameter of DecodeByteArrayAsync is the length of the array, ...


3

Naming Assuming you or Mr.Maintainer will have to fix a bug in this code in 3 months, will you still know what any of these variables avm stackIbro2 stackIbro articlesH should do or represent ? You should give your objects/variables meaningful names so that you or Mr.Maintainer will see at the first glance what to expect. Indentation ...


3

This is not how it should be done in WPF especially that you are not validating the text but just changing it upper case - you validate only the numeric input. The right way would be to use the Binding Validation This method would allow you to write specialized validators without ifs or switches.


3

General naming guidelines suggest that an async method should end its name with "Async", so the method name should be RevalidateAccountAsync. Some would argue that a underscore prefix should never be used for naming of any variables. For those that do allow underscore prefixes, it should only be for class-level variables and never used for variables local ...


3

This is a common problem. Sometimes it's tricky to generalise a process even when its iterative in nature. By keeping track of both: the location of the user's cursor (cursor_position), and an iterable data structure storing which characters are shown (pinNumbers), it is possible to implement this behaviour without so much repeated code. This gave the ...


3

Thanks for clarifying the requirements, let's say they are: First call to GetDraws, no cached dates, has internet, download dates into cache, return date. First call to GetDraws, no cached dates, no internet, indicate error. First call to GetDraws, cached dates, download and store new cache of dates, return dates. Let's go with that. You mention "...


3

Re-entrance I don't think that these: private static TinyIoCContainer _container; public static Double ScreenHeight { get; set; } public static Double ScreenWidth { get; set; } should be static. Particularly for the purposes of unit testing, these should simply be instance properties on your App. The feasibility of this might be ...


2

You asked for a more elegant approach... This is certainly more concise: var dict = completed .GroupBy( item => item.DateClosed.ToString("MMMM dd yyyy"), // Key selector item => new TradeCell(item)) // Value selector .ToDictionary(g => g.Key, g => g.ToList()); However, you don't actually need the dictionary (that's just ...


2

Your code is little bit long then, even if usually I try to rewrite portions to explain what I suggest, in this case I will mostly highlight issues I see without much code. In general the two biggest issues I see are: Sub-optimal exception handling, first of all you catch a generic Exception. It may be caused by an exceptional condition (like running out-...


2

I don't see any rough mistake with your code, but I would refactor some parts of it. There are a lot of magic string constants like "AddPProc", "VerifyUpload", "FedAuth". They should be defined as named constants: private const string AddPProc = "AddPProc"; This code string url = Config.BaseAPIUrl; url = url + "AddPProc"; can be replaced with var url = ...


2

Note: the following refers to native Android development, I am not familiar with xamarin. About the approach It depends on what your aim is. The downside of using AsyncTask is that it's tied to the Activity you're calling it from. I recently implemented continuous location tracking, where AsyncTasks were not an option, as the process of continuous tracking ...


2

Handling exceptions within the action is a cross-cutting concern that can be extracted and placed into one of the available extensiblity points. This will greatly reduce the repeated exception handling in all controller actions. Reference Global Error Handling in ASP.NET Web API 2 Exception handlers are the solution for customizing all possible ...


2

Properly name everything: filterFunc and (even worse) filterFunc1 aren't informative at all. Don't do this: a.IssueType.Title.ToLower().Contains(this.IssueTypeSearchQuery.ToLower()) If you need a case insensitive Contains, write a helper method: private bool Contains(string source, string filter) { return source.IndexOf(filter, ...


2

Spot the inconsistency: var fault = result.Root.Element("{" + nsSoap + "}Body").Element("{" + ns_soap + "}Fault"); ... var codeWithNs = error.Element("{" + nsSoap + "}Code").Element("{" + ns_soap + "}Value").Value; ... return userId = result.Descendants(ns + "UserId").First().Value; Ok, there are two inconsistencies. Firstly, ...


2

By using a guard condition regarding CalEvents being null you can save one level of indentation like so private List<eventAndColor> GetEventosToPopulateCalendar() { // A List with only the days that have events List<Eventos> calEvents = eventDao.Select(FromEventos.DistinctDate); if (calEvents == null || calEvents.Count == 0) { ...


2

MainPageViewModel You provide an interface with a single method: public interface IMainPageViewModel { void LoadGroups(); } Yet, the implementation provides a rich set of public members: public string FileLocation { get; set; } public ObservableCollection<string> Groups { get; set; } public List<Phrase> LoadedPhrases { get; set; } public ...


2

You could have a collection of ValueConverters which would validate your TextInputs' strings. For example one could be a PhoneNumberValidator or PhoneToBoolConverter (example names there). This way HasError would bind to HasError="{Binding PhoneNumber, Converter={StaticResource PhoneNumberValidator}}". Converters are handy and you can reuse them in multiple ...


2

It is fine to refresh the token on every app start. It will also prevent potential hijacked tokens to be usefull for a longer time, when the token is refreshed everytime the user starts the app and the old token becomes invalid.


2

In this review let me try to focus on a single thing: eliminating duplicates. What is the problem with duplicates? The size of your code base could become quite lengthy. It ruins legibility. If you need to change one thing on the functionality, which affects all duplicates, then you have to do that in all occurrences. If you need to extend your datasource ...


1

Global Why don't you use the usual serialization stuff instead of creating you Interface (which by the way has the same name than System.Runtime.Serialization.ISerializable) and doing some kind of weird tinkering to serialize and deserialize? Grid.cs 1) foreach (string squareData in msg[1].Split(',').ToList()) : you don't need to convert the result of ....


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