What you've done looks good to me. However, here's what I would do if I were to write the same function : def append_nones(length, list_): """ Appends Nones to list to get length of list equal to `length`. If list is too long raise AttributeError """ diff_len = length - len(...
Your code is correct - it should work nicely. The first thing that I would improve on is the // runtime-safe get calls. This is an anti-pattern in Scala and is generally discouraged. In this case you can use pattern matchings powerful extractors to get the value the Option is storing: case (Som...
Like @jesse said you can have a custom method, maybe pass a user object to that method. Something like this: class User { public int AutoLoginRequestTemplate { get; set; } public string username { get; set; } public string password { get; set; } public string Val1 { get; set; } ...
you have a lot of if statements in your SQL, this screams at me that you are doing something in your SQL that should be done in the application, and that this SQL should be separated into two distinct stored procedures. the benefits faster stored procedures use an actual coding language to d...
I have had a play with your query. I don't believe there is a way to do the query without the sub-selects in the select fields... but, you can use the having to avoid the duplication of the code (and processing effort) to limit your results to just those with a positive target. Additionally, beca...
A few improvements I notice right off the bat newtype vs. data replace single field data declarations with newtype. newtype has no runtime overhead. data GrayscalePixel = GrayscalePixel Int -- becomes newtype GrayscalePixel = GrayscalePixel Int Use StateT A common pattern in your code (ex. p...
As far as I can understand default: comment.replace(i, 0, "\\"); makes case '\\': comment.replace(i, 0, "\\"); break; useless.
These dependencies can't be fixed by naming. They are a symptom of some fundamental design problems. If you're having trouble finding methods, that's a symptom of an overly coupled design. You have a class names that really sound like methods in a completely different class. Without knowing m...
There is a lot that can be simplified. AppendLine() instead of Append() with Environment.NewLine, object initializer syntax, etc. ws = new XmlWriterSettings is never used. Also, StringReader is also an IDisposable resource and should therefore be wrapped in using. I've also put the very short ...
You are correct in suspecting that the try/catch is not necessary. Generally speaking, Caesar ciphers are implemented using modular arithmetic. We can determine if a character is alphabetical by simply checking if alphabet contains that character; anything else (whitespace, digits, punctuation, e...
Blog markup The site title should be a h1. Use an ul for the navigation links. Or, if you don’t want that, use at least a div element for each link, or a textual separator between each link. The page title (+ the content under its scope) needs be enclosed in a sectioning element (probably sect...
So first off, global variables. Since posting this code, I made a lot of changes, signals, daemonization, logging, processing data from device, and at some point I have moved variables like: int sigflag=0; char dev_status=0x00; char dev_power_status=0x00; int serial_fd; int local_socket; int cli...
Ruby allows -1 as an index that means last, so you don't have to calculate it at all. Calculating the whole left_sum every time is repeating work since it is always the previous left_sum + arr[index-1] (except for when index = 0). Similarly the right_sum is always the previous right_sum - n. Yo...
I tend to have a class with my constants in there, matching the enum in the database; for example: class Model_MyData { // because it is an ENUM in the database, I use text names rather then numbers. const TYPE_BUILDS = 'BUILDS'; const TYPE_ISO = 'ISO'; const TYPE_INSTALLER = 'INSTALLER...
Another implementation is a available at: http://csharptest.net/browse/src/Library/Collections/LurchTable.cs The benefit to the approach in the LurchTable is that it does not aggregate an existing dictionary implementation, but rather is both the dictionary and linked list rolled into one class...
Your current implementation you will loose all but the first word. std::string str("this is a sentence"); std::string t = trim(str); std::cout << t << "\n" // Output this If that is your intention then there is an easier way of doing that: std::strint trim(std::string const& str) { st...
Never use get method on option. In this case you could use case Some(a) instead. You don't need curly braces in case branches oa match { case None => G.unit(None) case Some(a) => val x: G[B] = f(a) G.map(x)(Some(_)) } I don't know if variable x makes any sense in this case. I'd j...
A version that's probably no faster but slightly less verbose. I'm sure there's a much better way but it's a start: case class A(serial: Int) { def matches(b: B): Boolean = this.serial == b.serial } case class B(serial: Int) def pairUp2(list1: List[A], list2: List[B]): List[(A,B)] = { def p...
A few things: IData is not a particularly good name for the interface as it represents a source of data rather than the data itself. Therefor IDataSource seems more appropriate. The interface is a bit ambiguous - which method am I supposed to use GetDataStream or GetDataString? Why use one or t...
In your hover method you want to select the next two divs after the img. Assuming that the divs are located (in DOM) immediately after the img, then you can select them using the jQuery .next() method, instead of selecting them by name. ALso instead of selecting each img by id, you could give th...
I am not an expert at scala, but this works for me def pairUp2( list1: List[A], list2: List[B]): List[(A, B)] = { (for{ a <- list1 b <- list2 if a.serial == b.serial } yield (a,b)) } val pairs = pairUp2(l1, l2) pairs: List[(A, B)] = List((A(0),B(0)), (A(1),B(1)), (A(2),...
Regarding the problem selecting the right image type, you have enough information in your header to select which type of image to create. case header of Nothing -> putStrLn $ source ++ ": Bad image" Just (header,_) -> case (imageEncoding header) of ASCII -> putStrLn $ source ++ "...
You consistently do String foo = new String("some string"). This is useless, as "some string" already is a string. Simplify your code to String foo = "...". You declare some variables far before you use them. Try to declare your variables as close to their point of use as possible, e.g. instead ...
Increase DRYness. Something like this: var makeAnchor = function(prefix, suffix) { return "#" + prefix + suffix; } var nflPrefix = "nfl-"; if ($(".userLocation").length > 0) { nflPrefix = "nfl"; } $('header').attr("href", makeAnchor(nflPrefix, "city"); The ternary operator may be suitabl...
Since you have a C++ tag on this question, I assume this is eventually meant as a C++ program. If that is the case, you should have class Fraction { int denominator; int numerator; Fraction(int d, int n) : denominator(d), numerator(n) {} // define operators for +, -, etc. } If you...
Only thing I can think of is that wrap the condition: var hasUserLocation = function (hasFunc, noFunc) { if ($(".userLocation").size() > 0) { hasFunc(); } else { noFunc(); } } then you can call like this: hasUserLocation(function () { alert("has"); }, function ...
Apart from the fact I can't find any use of var loadPage It doesn't need to be an expression. Instead try: function loadPage(page) { } It is still accessible only within your code here, it is cleaner and easier to debug. Keep your click events consistent $(".sub-menu .menu-item, .module-item...
As far as I can tell, this code seems pretty easy to follow, especially with the comments. Here are several things that stood out to me: Since you haven't provided your own queue implementation, I assume you're using std::queue. If so, remove using namespace std and use std:: where necessary....
I'd put the cookies into an object so they can be named. var userCookies = { height: { name: 'user-height', options: { path: '/', expires: 365 } }, weight: { name: 'user-weight', options: { path: '/', ...
Interesting question, I am assuming you know that usually, in essence, the setting of the name would be written like this: Player.setName('somename' , function( err, result ) { console.log('Name changed'); }); and in essence setName would be Player.prototype.setName = function( nam...
By default, GZipStream owns the underlying stream, so it disposes it when it's disposed itself. If you're okay with relying on that, then you could put the right Stream into a variable and then have just one using around it. Something like: var writer = new FileStream(path, FileMode.Create); if...
Apart from the fact I can't find any use of var loadPage It doesn't need to be an expression. Instead try: function loadPage(page) { } It is still accessible only within your code here, it is cleaner and easier to debug. Keep your click events consistent $(".sub-menu .menu-item, .module-item...
Many beta-sites suffer with a problem of voting. Code Review is one of them. We at Code Review have realized that it is hard to vote for answers that you're not aware of, therefore some of us (no monkeys named) posts a link to a recent answer every now and then in the chat. Instead of us posting...
Regarding the problem selecting the right image type, you have enough information in your header to select which type of image to create. case header of Nothing -> putStrLn $ source ++ ": Bad image" Just (header,_) -> case (imageEncoding header) of ASCII -> putStrLn $ source ++ "...
Building off of what Benjamin Kloster has If you pass the parameter by reference instead of value it will not choke trying to pass in an invalid index. function getDefault(&$isset, $default) { return isset($isset) ? $isset : $default; } Or if you want a dynamic amount of parameters to def...
Let me know what I can improve on or what I'm doing poorly. If you are going to do animations with those lines, you will want to keep track of them, I am assuming that is why you have an array of LINES. However, in createLine you create the SVG element and just return it, you do not keep a refer...
When you look at the color highlighting of your code, you can see that WithoutFNC1 is colored blue like other Java types. This also suggests that in the Java world, it is not recommended for non-static variable names to start with an upper case. These two blocks are very similar and you can alre...
A few things: IData is not a particularly good name for the interface as it represents a source of data rather than the data itself. Therefor IDataSource seems more appropriate. The interface is a bit ambiguous - which method am I supposed to use GetDataStream or GetDataString? Why use one or t...
For improving readability, you could perhaps replace the for-loop variable names with more descriptive names, such as 'currentColumn' and 'currentRow.' For performance, you can replace 'x++' with '++x' in every loop. 'x++' actually creates a copy of x, increments x, and returns the copy. It is u...
You ask many questions on CodeReview, which in itself is good, but you have to start helping the reviewers actually review your code. You have this habit of dumping code and expecting a review. It does not work that way (very well). For a start, let's review the ideal process for a 'real' review:...
There are a ton of good template rendering libraries out there, but I'd leave it to you to figure out if they are better suited to your needs than hand rolled code. Assuming you still want to write this yourself, I'd look for ways to make the code more completely data driven and also to make it...
md5 usage may be okay on small projects, but it's generally considered weak and shouldn't be used. As of the MD5 wikipedia page: The security of the MD5 hash function is severely compromised. and http://us1.php.net/md5 It is not recommended to use this function to secure passwords, due ...
A simple solution is to have 2 arrays, one with chars to map from and one with chars to map to. Then a simple look up, something like this, is all that is necessary char[] mapFrom = "ABC".ToCharArray(); char[] mapTo = "LMN".ToCharArray(); mapTo[Array.IndexOf(mapFrom, ch)] Advantages inclu...
You can actually change the performance to O(n) by changing one line of code here (and adding a couple of imports): import java.util.Set; import java.util.HashSet; ... private Set<String> listTempName = new HashSet<>(); Although this works, we can simplify the code a little bit. Effectively,...
The algorithm as you describe it is O(n^2) worst case. Assuming that there are n images and no two images are equal then the first one gets compared against n-1, the second one against n-2, ... etc. so you have 1/2 * n * (n + 1) comparisons. A way around this is by calculating a checksum with lo...
Right, first: short answers to your 4 questions: Is this proper OOP? No, there are several issues Or what can I do to improve this class? A few things, we'll get to that in a moment. Do I need to make it more abstract? You may want to look into that What can I do to make the _parse_weather_xml ...
My question would be: Why is it supposed to "overwrite" the left value? This seems dubious from a software engineering point of view. The product of two matrices is: Anm x Bmp = Cnp hence you can only "overwrite" A with C (re-using the same Matrix object) if m == p or in other words if B is a qua...
1) For readability, I'd go with explicitly using the single conversion constants instead of lumping them together: const int uptimeHours = uptimeMSecs / 1000 / 60 / 60; Possibly off-topic, but I would probably do away with all these structs and auto-conversions entirely. If you use well name...
What you done looks good but I am wondering if it is not a bit over-engineered. Here's what I would have done (which is probably under-engineered) : #include <iostream> enum TimeUnit { Second = 1, Minute = Second*60, Hour = Minute*60, Day = Hour*24, Week = Day*7 }; ...
What about boost? std::string untrimmed( " This is an untrimmed string! " ); std::string trimmed( boost::algorithm::trim( s ) ); It doesn't get much terser than that, and I trust the boost libs to be more efficient than what I could hack together on a whim...
You definitely want to document those interfaces. WarehouseClient should probably not be a static class. The name of the NextSession event in IWarehouse doesn't really mean anything to me. Just from reading the interface I don't really know when this event would be raised. Maybe NextSessionCreat...
Let's improve the function step by step, focusin on readability as well as on the algorithm itself. I believe the type of the function is not what it should be. We get a list of characters, and we should produce a list of lists: pack1 :: [Char] -> [[Char]] pack1 xs = foldl (\acc x -> if not(nu...
As far as I can understand default: comment.replace(i, 0, "\\"); makes case '\\': comment.replace(i, 0, "\\"); break; useless. Edit to add another comment : Please activate all warnings on your code on one hand and on the generated code on the other hand. I think that : void comment(...
Just a quick random note: Instead of Map<String, List<String>> you could use Guava's Multimap (doc, javadoc) which was designed exactly for that.
In general it really depends on the use cases of your data. If you need to perform queries using just year or just month then you reduce complexity in the code and queries by having separate fields. However, if you aren't bounding by just a year or just a month then you would want to use a date...
I'm going to answer this question: is there a better way to Add different icon to each li item? The short answer is, yes. The better way is to use the classes that FontAwesome provides out of the box, rather than duplicating it in your own css. In this example I would make the following cha...
First things first: You have a navigation but refuse to use anchor tags. Why? This is potentially harmful. How are you going to navigate if you don't have links? The only possible way would be completely relying on JavaScript. All users with disabled JavaScript have a useless list of words instea...
My question is: are there any performance issues with the conditional functions approach? In a word: no. The additional performance cost is an extra select statement, whose cost probably depends on the number of switch options in the select. I say "no" because I expect that the performance...
I tinkered a bit with the fiddle, I have the following observations: If you were to place the colons in the HTML, then you would no longer need to concatenate the colons into the time fields: <div id="stopwatch"> <span id="hours"></span>: <span id="minutes"></span>: <span id="seconds"><...
It would be much easier just to let the CSS selector engine select the elements to hide: var hideDepth = function(root, depth) { root.find('ul').show(); // Make sure all sublists are visible var selector = Array(depth + 1).join(' > li > ul'); // Repeat string root.find(selector).hide()...
Had a few thoughts on your code. Your code doesn't accept a '%' to quit. Changing the menu to accept a '%' makes the rest of it inconsistent. so making all the input match the operation and changing the enum to reflect the character values instead of arbitrary ones helps with this. You're par...
You ask many questions on CodeReview, which in itself is good, but you have to start helping the reviewers actually review your code. You have this habit of dumping code and expecting a review. It does not work that way (very well). For a start, let's review the ideal process for a 'real' review:...
I agree with what you said, i.e.: The only thing which can break these tests is a change to this specific method and surely if anybody is changing this method they are doing so with good reason, and all I've done is given them an extra job of changing the tests. I can definitely say ...
Python seems like overkill here: this is surely a job for the shell? Depending on exactly which files you want to touch, then use this: touch -- */testResults/*.xml or this: find . -path '*/testResults/*.xml' -exec touch -- {} \+ You indicated that you are using Windows. But even on Windo...
You can swap entire rows. public static void swapRows(int[][] m) { for (int i = 0, k = m.length - 1; i < k; ++i, --k) { int[] x = m[i]; m[i] = m[k]; m[k] = x; } } public static void rotateByNinetyToLeft(int[][] m) { transpose(m); swapRows(m); } public s...
I would just add a columnGroup attribute to Column and then do a groupBy-type operation on that attribute. That would reduce your Table view-model to just a collection of Rows, which would be nice. Here's what I'm thinking for Column function Column(columnName, columnGroup /* ... all your other ...
Here's my best effort: from pygame import * init() screen = display.set_mode((1000,720)) colors = {(1, 0, 0): Color('white'), (0, 0, 1): Color('black')} for e in iter(event.wait, event.Event(QUIT)): col = colors.get(mouse.get_pressed()) if col and e.type in (MOUSEBUTTONDOWN, MOUSEMOTION)...
Your problem is not with the loop construct - nor is it with GOTOs. Your problem is you are avoiding some tools that can make your code better. Here's an example. enum Validator { IsNotComment("Skipping commented line in file.") { @Override boolean isValid(String s) { ...
There are a couple of inefficient things you do, and there's some better-practice items too. First, let's take one of the methods: public static List<String> loadNodes2_00() { List<String> node = new ArrayList<>(); try { InputStream inputStream = XmlUtil.class.getClassLoader...
First, some smaller things (in terms of the size of code changed): I think you should use var more, especially when it's clear what type the object has, because you're just creating it. You shouldn't read the response as a single byte array. Instead, you should use Streams where possible (it ce...
With the answer using reduce of Nathan, each time you add a column you need to read all the old columns again. Another approach is to use a ColumnType object and save all the columns in this object. //column definition function Column(columnName, minLength, maxLength, minValue, maxValue, reg_ex...
Since you have already other answers , I will focus on different issues This : #accordion > li:first-child >div{ border-radius: 5px 5px 0 0; } #accordion > li:last-child >div{ border-radius: 0 0 5px 5px; } Is not needed. You have already border-radius on the container. And the same for ...
It's definitely a good and simple way of doing it. I wish node.js would have been implemented using the AMD Api. As for the 2 code samples you proposed, I would pick the lasted one. If you translate it in coffeescript you would get almost the same code. I would make one slightly change. I would...
I think you might run into some design ugliness with the in the future because you are now coupling the knowledge that an entity could be filtered by a certain property to each entity. Imagine a requirement comes in to filter by Course - what would that look like in your design? Are you going t...
As you found yourself, this has to work. In only have 2 minor nitpickings No console.log() in production code.. You are not using element and attrs, you might as well declare function($scope) Very nice code.
Your complexity is indeed o(n^2) because you traverse a list bounded by n for each n elements of your list. You can reduce this list to o(n*log(n)) quite easily : Sort your array -> o(n*log(n)) Compare each element with the next one listNames.get(i).equals(listNames.get(i+1)) -> o(n) Total i...
The method modifies the parameter while it is processing input. It should store the processing result locally and then assign it once it is finished. It is unclear what the return value of the method means just by reading the code. If it is supposed to indicate success (return is not 0) or failu...
You ask many questions on CodeReview, which in itself is good, but you have to start helping the reviewers actually review your code. You have this habit of dumping code and expecting a review. It does not work that way (very well). For a start, let's review the ideal process for a 'real' review:...
Unfortunately, I don't think so. There is a question about this topic on the website of Dozer (which is a similar library for Java) which mentions a trick: Should I write unit tests for data mapping logic that I use Dozer to perform? [...] Regardless of whether or not you use Dozer...
I have some general design comments. You seem to approach the problem from the OO perspective: you have the Image object with a bunch of virtual methods and the Pixel object with virtual readPixel and encodePixel. These two methods, in turn, internally dispatch based on the type of encoding. Ev...
are there any performance issues with the conditional functions approach? No. Would the checking that takes place every time the function is called have a significant impact on the performance? No Which approach is better if I want to maximize performance? Insignificant differen...
What you did well on: Your program is easy to read. You are allocating memory, even though it is unsafe in certain cases (other answers have addressed this, so I won't cover it). You prepare for some corner cases. Things you could improve: You are returning 0 when your program runs into a...
A few notes (with efficiency aside, as you said): Your defined ABS surrounded by parenthesis is not the usual way that C programmers declare macros so that they aren't problematic. #define ABS(a) do { typeof (a) _a = (a); _a > 0 ? _a : -_a; } while(0) Why are all of your single letter variabl...
Here are a few comments I would make: You can keep a permanent reference to the tail, which makes appending at the end o(1) operation rather than o(n) You can make a double-link list, by keeping reference to next and previous links. Deletion of the last link becomes o(1) instead of o(n) Your re...
For combining (1) and (2) you can reverse the sort by only trying if your are under a test directory Also this is a great use for a generator / map combo to avoid extra loops import os import re is_xml = re.compile('xml', re.I) is_test = re.compile('testResults', re.I) def find_xml_tests(roo...
I don't care for all that whitespace myself. I prefer to leave empty divs on a single line like so: <div id="footer"></div> I put the closing tag on a separate line only where there is content, and in either case, of course, indent as appropriate for ease of reading. You can reduce your CSS r...
A lot of this type of stuff is just personal style, or if you're working on a team, the layout is defined ahead of the project. The use of classes vs id's is also personal. I've seen people say that they ALWAYS use classes. I've tried both classes and id's on many pages, and I find that I get in...
next day → last day (113 days later) »