Iterating over an Array and an Iterable (List) I have a byte array and a List of a custom object ByteRule and I'm needing to loop around and perform an operation on both but only as long as one of both is available, the original method is as follows:

private final List<ByteRule> rules;

@Override
public boolean match(final InetAddress addr)
{
    final byte[] bytes = addr.getAddress();
    for(int i = 0; i < this.rules.size() && i < bytes.length; i++)
    {
        final int value = bytes[i] < 0 ? (int) bytes[i] + 256 : bytes[i];
        if(!this.rules.get(i).match(value))
        {
            return false;
        }
    }
    return true;
}

Obviously the this.rules.get(i) on every iteration is inefficient, however as I need to maintain a counter, an enhanced for loop means keeping a separate counter and performing a check on it every time like so (list declaration skipped for space):

@Override
public boolean match(final InetAddress addr)
{
    final byte[] bytes = addr.getAddress();
    int counter = 0;
    for(final ByteRule rule : this.rules)
    {
        if(counter + 1 == bytes.length) break; //In case there aren't enough bytes
        final int value = bytes[counter] < 0 ? (int) bytes[counter] + 256 : bytes[counter];
        if(!rule.match(value))
        {
            return false;
        }
        counter++;
    }
    return true;
}

Is there a cleaner/more efficient solution to this problem that I'm missing?

I would be happy with:

@Override
public boolean match(final InetAddress inetAddress)
{
    final byte[] bytes = inetAddress.getAddress();
    int end = Math.min(this.rules.size(), bytes.length);
    for(int i = 0; i < end; i++)
    {
        final int adressPart = bytes[i] & 0xFF; //byte range from -128 to 127, &0xFF makes it unsigned, therefore positive
        if(!this.rules.get(i).match(adressPart))
            return false;
    }
    return true;
}

If you think, the this.rules.get(i), then test it with a profiler. I would be surprised if this line is the reason for any slowdown, because the length of a InetAddress is probably quite limited and most likely implemented as an ArrayList annyway. If it really matters, you can change it to:

@Override
public boolean match(final InetAddress inetAddress)
{
    final byte[] bytes = inetAddress.getAddress();
    ByteRule[] byteRulesArray = rules.toArray(new String[rules.size()]);
    int end = Math.min(this.rules.size(), bytes.length);
    for(int i = 0; i < end; i++)
    {
        final int adressPart = bytes[i] & 0xFF; //byte range from -128 to 127, &0xFF makes it unsigned, therefore positive
        if(!byteRulesArray[i].match(adressPart))
            return false;
    }
    return true;
}

or if you even fear the array creation, you can use an iterator:

@Override
public boolean match(final InetAddress inetAddress)
{
    final byte[] bytes = inetAddress.getAddress();
    Iterator<ByteRule> rulesIterator = rules.iterator();
    int end = Math.min(this.rules.size(), bytes.length);
    for(int i = 0; i < end; i++)
    {
        final int adressPart = bytes[i] & 0xFF; //byte range from -128 to 127, &0xFF makes it unsigned, therefore positive
        if(!rulesIterator.next.match(adressPart))
            return false;
    }
    return true;
}

Hint: In theory, the iterator solution would be faster than the array creation. However in the real world, because of hardware reasons, the array creation will, most likely, be faster.


How can I prettify blocks with more than 2 of each block? Actually, I just have no ideas.

How can I make breaklines to make it more readable?

Can anyone suggest ways on improving this?

module TestingDataLoad
  def get_files_from(search_target)

    Dir[search_target].each do | d |
      testing_files={}
      if File.directory? d
        Dir.glob("#{d}/*json") do |file|
          if File.basename(file,".json") =~ /(\w+)(_\d+-\d+)/
            event_type = $1.to_sym
            if testing_files.has_key? event_type
              testing_files[event_type] << file
            else
              testing_files[event_type] = [file]
            end
          elsif File.basename(file,".json") =~ /(\w+)/
            event_type = $1.to_sym
            testing_files[event_type] = [file]
          end
        end
      end

      return testing_files
    end
  end

A couple of things I noticed:

Dir[...] and Dir.glob(...) are synonymous, but you're using both forms?
Speaking of, glob accepts fairly complex patterns, which could help narrow the search
Skip the testing_files.has_key? check; just use testing_files[event_type] ||= []
The explicit return makes little sense. You're looping through the globbed dir paths, but your explicit return means that only the first will ever be checked. Maybe there's only supposed to be one path, but that depends on what search_target is, which you don't know.
Your method will return nil, an empty hash or a populated hash. Limit the number of possible return values.

Mostly though, it seems like the filename-branching is unnecessary. In both cases you add grab the \w+ part of the name, and use it as a hash key for an array. The only difference is that
if the filename has numbers in it, it'll add the file to the array, otherwise it'll replace the array with a new 1-element array.

I'd say treat all files the same, and append them to the array. Don't ever replace the array. As far as I can figure, your method is entirely dependent on the platform/filesystem you're running it on, since glob doesn't dictate the order in which files are returned.

So given 3 files in the following order:

foobar.json
foobar_1-1.json
foobar_1-2.json

your method will return all three. But if the order's reversed, it'll only return "foobar.json". Or, if it's somehow mixed, it might return "foobar.json" and "foobar_1-1.json", but not the 3rd file.

So instead of risking inconsistent behavior, I'd suggest sticking to 1 way of treating the files, regardless of whether they're numbered or not.

def get_files_from(search_target)
  files = {}

  Dir[search_target].each do | d |
    next unless File.directory?(d) # skip if d isn't a dir

    Dir["#{d}/*.json"] do |file|
      event = File.basename(file)[/^\w+/]
      testing_files[event] ||= []
      testing_files[event] << file
    end
  end

  files
end

If you want to be more exacting, you can do this instead

Dir["#{d}/*.json"].select { |file| File.basename(file, '.json') =~ /^\w+(_\d+-\d+)?$/ }.each do |file|
  ...
end


A Fraction class in C++ I am trying to learn clear and concise C++. Currently I'm learning classes in school, and to practice I have made a Fraction class.

I'd appreciate any help regarding style, cleanliness or anything else that would be good to know.

(I know using namespace std; is frowned upon, but it's required in our course.)

#include <iostream>
using namespace std;

class Fraction {
    int numerator, denominator;
private:
    int gcd(int a, int b) {
        while (a != b) {
            if (a > b)
                a -= b;
            else
                b -= a;
        }
        return a;
    }
public:
    Fraction (int, int);
    double decimal();
    string fraction();
    Fraction add(Fraction);
    Fraction subtract(Fraction);
    Fraction multiply(Fraction);
    Fraction divide(Fraction);
};

int main() {
    Fraction f = {2,3};
    cout << f.decimal() << endl;
    Fraction k = {1,4};
    Fraction d = {1,3};
    Fraction result = k.add(d).add(f);
    cout << result.fraction() << endl;
    return 0;
}

Fraction::Fraction(int n, int d) {
    numerator = n / gcd(n, d);
    denominator = d / gcd(n, d);

}
string Fraction::fraction() {
    return to_string(numerator) + "/" + to_string(denominator);
}
double Fraction::decimal() {
    return (double)numerator / (double)denominator;
}

Fraction Fraction::add(Fraction f) {
    return Fraction(numerator * f.denominator + f.numerator * denominator, denominator * f.denominator);
}
Fraction Fraction::subtract(Fraction f) {
    return Fraction(numerator * f.denominator - f.numerator * denominator, denominator * f.denominator);
}
Fraction Fraction::multiply(Fraction f) {
    return Fraction(numerator * f.numerator, denominator * f.denominator);
}
Fraction Fraction::divide(Fraction f) {
    return Fraction(numerator * f.denominator, denominator * f.numerator);
}

You wished for some style and cleanliness advice, so here are a few things you may improve in that area:

Don't include gcd in you class because it is actually completely unrelated. Instead, write a separate non-class function for it.
Name your parameters. Things such as Fraction add(Fraction); usually imply that the parameter is ignored, and even if the user understands that this is not the case here, it remains unclear what your parameter actually represents.
Include all the headers that you use. Your code is currently missing a #include <string>.
Do some error checking and assure that parameters are valid. For example, what happens when somebody passes 0 for d to the constructor? You will end up in an infinite loop because your gcd function does not verify that its arguments are not 0.
Avoid C-style casts. C++ offers a variety of casts with clearly defined purposes, whereas the C cast covers a lot of different conversions, some of which you may not want. In your particular case, don't write (double)numerator; write static_cast<double>(numerator) instead (same for denominator, of course).
Instead of having add, subtract, multiply and divide, you should overload the corresponding operators (e.g. operator+ for add). You should also consider adding more operators, such as operator+=, operator-= and so on and so forth.
Don't use std::endl where a simple '\n' would suffice (which is almost always). The reason for this is that std::endl also flushes the underlying buffer which can seriously harm performance when doing lots of IO and is seldom required.
fraction is a bad name for a function that returns a string representation of your class. You could call it to_string for example, or even choose to implement explicit operator std::string().
You can safely omit return 0; at the end of main, the compiler is nice enough to add it automatically.
Currently, your constructor calculates gcd(n, d) twice. You should calculate the value once and then divide set numerator = n / gcd_value and denominator = d / gcd_value instead.
You should consider adding adding a default value of 1 to the second parameter to the constructor to enable construction of fractions from whole numbers easily, i.e.

Fraction::Fraction(int n, int d = 1);
...
Fraction f {3}; //f represents the fraction 3/1

Don't treat single characters as strings when you don't need to. In your case, replace "/" by '/' in fraction. There is an operator+ in std::string that handles single chars.
Fix the unnecessary whitespace in Fraction (int, int). Judging by how you defined your other methods, it should instead read Fraction(int, int) (I suppose this is just a typo).

Edit: As JPhi1618 pointed out in the comments, it is better to not get into the habit of relying on implicit construction when assigning values to objects. Instead, one should make use of explicit construction and uniform initialization. Accordingly I changed last line of the code sample of point 11 from Fraction f = 3; to its current representation.


To use or not to use public variables in javascript? Question:

I'm attemping to modify a Javascript snippet to more efficiently get the user's current Zipcode. One problem that I am facing however is the lack of understanding around public functions. I am wondering if they are requried, and if they are not, then what would make the code cleaner and more efficient in terms of using or not using public.?

Code:

var geoLocateAddress = function () {
        var public = {};
        public.init = function() {
            public.initEventHandlers();
        };
        public.initEventHandlers = function() {
            console.log("Using your browser's GeoLocation API");
            navigator.geolocation.getCurrentPosition(
            public.loadAddressByGeolocation);
        };
        public.loadAddressByGeolocation = function(position) {
            console.dir(position);
            console.log("Your browser only returns Long/Lat");
            public.loadAddressByLngLat(position);
        };
        public.loadAddressByLngLat = function(position){
            var data = {
                lat : position.coords.latitude,
                lng : position.coords.longitude
            };
            var url = "http://ws.geonames.org/findNearestAddressJSON";
            $.ajax({
                url: url,
                dataType: 'jsonp',
                data: data,
                success: function(data, textStatus, xhr) {
                    public.updateAddressFields({
                        zipCode: data.address.postalcode,
                    });
                },
                error: function(xhr, textStatus, errorThrown) {
                    console.log(textStatus);
                }
            });
            return false;
        };
        public.updateAddressFields = function(address) {
            console.log('Zip: ' + address.zipCode);
            $("#zip").text(address.zipCode);
            console.log('Zip Logged');

        };
        return public;
    } ();
geoLocateAddress.init();

None of this code needs to create any global variables. The pattern you have here is known as namespace creation.

It often follows the pattern:

var myNamespace = (function () {
    //private stuff
    var public = {}; //or function () ...
    //build up public object
    return public;
}());

But sometimes the public object can be inlined like it can be in your case:

var geoLocateAddress = (function () {
    function init() {
        //...
    }
    return { init: init };//unnecessary to expose the other functions as public
}());

In this case, all you are doing with this namespaced global object is calling the init function. So instead of creating a global (generally considered bad taste in most languages) you can simply initialize the object:

(function () {
    function init() {
        //...
    }

    init();//unnecessary to expose the other functions as public
}());

(this pattern of having the function surrounding your code like this is commonly referred to as an IIFE)

Aside: for declaring a namespace I prefer the following form:

(function ($, window) {
    'use strict';

    ...
    window['geoLocateAddress'] = window['geoLocateAddress'] || {};
    window['geoLocateAddress'] = $.extend({}, window['geoLocateAddress'], {
        'init': init
    };
}(jQuery, window));

Back on topic:

In my opinion, it is a bad practice to force the Javascript engine to go looking for your globals when you are not in global scope due to leaving off the window qualifier. Thus everywhere you reference console without calling it window.console (and the same goes for Navigator) or caching it in a local variable is wrong. In the event where you run this script outside of a browser that implements console you will get an exception. Navigator is slightly less of an issue because it is a reasonably safe assumption that you will not run this from a browser that doesn't supply a Navigator global namespace. For this reason I always pass the globals that I am going to use into my IIFE:

(function ($, geolocation, console) {
    ...
}(jQuery, Navigator.geolocation, window.console));

I would modify the rest of the code as follows; please comment if you want me to explain any of it in more detail and I'll try to get to it. Comments dictionary:

pref: personal preference (the opinions of others may vary)
rule: avoids possible issues with the code
possibility: potential source for further enhancement, outside of the scope of this question.
consideration: something I haven't decided one way or the other on yet

//pref: don't create global variables that are unnecessary
(function ($, geolocation, console) {//pref: pass in globals for munging purposes
    'use strict'; //pref: 100% on JsLint

    //pref: instead of defining one object variable and methods on it
    //we could define the methods and export an object at the end
    //this saves us from needing to do some sort of static object call syntax
    var updateAddressFields = function (data) { //pref: remove unused parameters
            console.log('Zip: ' + data.address.postalCode);
            //pref: renaming variable provides very little benefit;
            //outweighed by ability to simplify syntax by inlining to success
            //function and then replacing the success function here
            console.dir(data); //pref: make above a constant string and use this instead of just logging zipcode
            $(function () {  //rule: don't update DOM until after the ready event
                $('#zip').text(data.address.postalCode);
            });
            //pref: further logging beyond start of function adds nothing but cruft to your log
        },
        loadAddressByLocation = function (position) { //pref: renamed function for readability
            console.log('loadAddressByLocation:');
            console.dir(position); //pref: inlined function to simplify code
            var data = { //possibility: abstract this as a jQuery helper if it has uses outside of setting zipcode (outside of this question scope)
                    lat : position.coords.latitude,
                    lng : position.coords.longitude
                },
                url = "http://ws.geonames.org/findNearestAddressJSON"; //consideration: variable unnecessary?
            $.ajax({ //consideration: ajax vs getJSON?
                url: url,
                dataType: 'jsonp', //consideration: specify datatype or put callback in the url?
                data: data, //consideration: inline data definition?
                success: updateAddressFields, //pref: extra function wrapping unnecessary
                error: function (xhr, textStatus) {//perf: remove unused parameters
                    console.log(textStatus);//consideration: necessity of error function?
                }
            });
            //pref: return false did nothing here; removed
        };

    console = console || { //rule: safe to remove shim if you know you're browser compat
        log: function () {}, //otherwise console may not exist under certain browsers
        dir: function () {}
    };
    //perf: removed and inlined "return { init: init };" to not create globals
    console.log("Using your browser's GeoLocation API");//pref: inlined init and initEventHandlers for readability
    if (geolocation && geolocation.getCurrentPosition) { //rule: more behavior detection, also safe to remove if browser compat
        //assume correct api if this passes
        geolocation.getCurrentPosition(loadAddressByLocation);
    }
}(jQuery, navigator.geolocation, window.console));
//pref: passing in navigator.geolocation will allow better minification (though I wouldn't minify this version anyway)
//rule: more behavior detection, referring to console as window.console to not trigger possible error referencing global that doesn't exist

Simplified for minification, I don't really see anything "wrong" with this version:

(function ($, geolocation) { //console removed
    if (geolocation) { //possibly unsafe if future browser implements geolocation but not getCurrentPosition, perhaps leave this as it was above
        geolocation.getCurrentPosition(function (position) {
            $.getJSON( //using this helper instead of ajax lets us not define an object and property names
                "http://ws.geonames.org/findNearestAddressJSON?callback=?",
                //dataType: 'jsonp', unnecessary with callback in url
                { //inlined data to remove var, IMO readability is not affected too much
                    lat : position.coords.latitude,
                    lng : position.coords.longitude
                },
                function (data) { //unused params dropped from success callback
                    $(function () {
                        //don't update until after the ready event
                        $('#zip').text(data.address.postalcode);
                    });
                }
            );
        });
    }
}(jQuery, navigator.geolocation));

which shrinks down to 266 chars (via uglifyjs):

(function(e,t){t&&t.getCurrentPosition(function(t){e.getJSON(
"http://ws.geonames.org/findNearestAddressJSON?callback=?",{lat:
t.coords.latitude,lng:t.coords.longitude},function(t){e(function(){
e("#zip").text(t.address.postalcode)})})})})(jQuery,navigator.geolocation)


Improving my invoice generator In this function, the copy (Write XML) is inside, but I want it to be separated from the function.  How can I improve this function?

Sub ExporttoFolder(ByVal POSPath As String, ByVal dt As DataSet, ByVal FolderCopyto As String,ByVal SelectedItem As DataTable)

If MsgBox("Do you want to Export Invoice to " & _WhouseToComboBox.textBox.Text & " ? ", MsgBoxStyle.YesNo) = MsgBoxResult.Yes Then

Dim Whouse As String
For Each dr As DataRow In SelectedItem.Rows
Whouse = dr.Item("whouseDesc")
ProcessCopy(POSPath, dt, FolderCopyto, Whouse)
Next dr
End If

End Sub

Sub ProcessCopy(ByVal targetDirectory As String, ByVal File As DataSet, ByVal FolderTarget As String, ByVal DirectoryFather As String)

Dim subdirectoryEntries As String() = Directory.GetDirectories(targetDirectory)
Dim Filepath As String = Nothing
Dim subdirectory As String

For Each subdirectory In subdirectoryEntries
Try

If Path.GetFileName(subdirectory) = DirectoryFather Then

If IO.Directory.Exists(subdirectory & "\" & FolderTarget) Then

File.WriteXml(subdirectory & "\" & FolderTarget & "\" & File.Tables(0).Rows(0).Item("InvoiceID") & ".xml")

End If

End If

ProcessCopy(subdirectory, File, FolderTarget, DirectoryFather)
Catch ex As Exception
End Try

Next subdirectory

End Sub

In this function, the copy (Write XML) is inside, but I want it to be separated from the function.

Shallow Observations

In no particular order:

You need to indent your code, it makes it much easier to read. Give that Tab button some lovin'!
Exceptions are like urine samples: there's a lot of information in there to analyze if and when you need to, and the last thing you want to do is to swallow them.

Separation of Concerns

I think both procedures do more than they claim. Let's break down what needs to happen:

Prompt the user for confirmation, do nothing without user's consent.
Get the names of all target folders.
Find the destination folders.
Determine the name of the file.
Write the file.

Implementation

Prompt the user for confirmation, do nothing without user's consent

Asking for user confirmation before exporting stuff is probably a good idea. This doesn't mean the actual prompting needs to happen in the method that performs the actual export. Consider this:

Public Function GetUserConfirmation(ByVal prompt As String) As Boolean
    Return (MsgBox(prompt, MsgBoxStyle.YesNo) = MsgBoxResult.Yes)
End If

Yes, it's a one-liner. That doesn't make it less of a useful function. We don't really care about MsgBoxResult, we just want to know if the user answered Yes to a question. This function abstracts away the MsgBox call and the need for knowing about MsgBoxResult, which makes it much more If-friendly - whether you're reading:

If GetUserConfirmation(confirmationMessage) Then

Or

If Not GetUserConfirmation(confirmationMessage) Then

You know exactly what's going on and you don't need to worry about how it's implemented.

Now in your code, this would translate to:

Sub ExportToFolder() 'parameters omitted
    If GetUserConfirmation(confirmationMessage) Then
    ' do all that stuff
    End If
End Sub

I think this is a bad construct, it smells like the code in the If statement is begging you to be surrounding a call to that method, so you can do this:

If GetUserConfirmation(confirmationMessage) Then ExportToFolder 'parameters omitted

And have ExportToFolder look like that:

Sub ExportToFolder() 'parameters omitted
    ' do all that stuff
End Sub

This means the logic that grabs an instance variable to come up with the prompt message is also moved outside of the method. Actually by doing this we have just separated the concerns of coming up with a prompt message and prompting the user with some message.

So we've got user confirmation. One down.

Get the names of all target folders

We have some DataTable that represents a selected item which contains a column called "whouseDesc" which is the name of the "parent" folder we want to use. Your code knows about a lot of things it doesn't need to be bothered with: in fact all this DataTable boilerplate just doesn't belong there. The method doesn't want a DataTable, it needs what's it in.

I would replace SelectedItem As DataTable with TargetSubfolderNames As IEnumerable(Of String), it makes the intent much clearer: we need an enumerable bunch of folder names, and we're going to expect they're the names of subfolders of a target folder.

Reading the DataTable that contains the folder names is out of scope here.

The procedure now looks like this, notice how much more focused it's starting to be:

Sub ExportToFolder(ByVal POSPath As String, _
                   ByVal dt As DataSet, _
                   ByVal FolderCopyto As String, _
                   ByVal TargetSubfolderNames As IEnumerable(Of String))

    Dim subfolderName As String
    For Each subfolderName In TargetSubfolderNames
        ProcessCopy(POSPath, dt, FolderCopyTo, subfolderName)
    Next

End Sub

Find the destination folder

ExportToFolder takes the POSPath parameter only to pass it down to ProcessCopy at each iteration. It's the same folder every time you call ProcessCopy, and yet every time the method runs, you're fetching the subdirectories and looping through all of them every time, verifying if the target folder exists under that subdirectory. I'm exhausted, aren't you?

If each call to ExportToFolder() exports stuff into 1 file in 1 folder, then it's not exactly clear why you're looping directories here, why the method is recursive, and how you're coming up with the filename.

My guess is that you want to be sure you're writing to a directory that exists. That's fine, but if there's only 1 file to write to, you're over-complicating it.

Determine the name of the file

If I understand correctly, you know the full file name from the start, and given InvoiceId which is dt.Tables(0).Rows(0).Item("InvoiceID") this would be your file name:

Path.Combine(Whouse, FolderCopyTo) + InvoiceId + ".xml"

So we can have a GetExportXmlFileName function that does it, and that can return an empty string if the folder doesn't exist:

Function GetExportXmlFileName(ByVal ParentFolder As String, ByVal TargetFolder As String, ByVal InvoiceId As String) As String

    Dim result = String.Format("{0}.xml", _
                   Path.Combine(ParentFolder, TargetFolder), _
                   InvoiceId)

    If Not Directory.Exists(Path.GetDirectoryName(result)) Then result = String.Empty

End Function

Your method is starting to look like this:

Sub ExportToFolder(ByVal TargetSubfolderNames As IEnumerable(Of String), _
                   ByVal TargetFolder As String, _
                   ByVal Content As DataSet)

    Dim SubfolderName As String, FileName As String
    Dim InvoiceId As String = Content.Tables(0).Rows(0).Item("InvoiceID")

    For Each SubfolderName In TargetSubfolderNames
        FileName = GetExportXmlFileName(SubfolderName, TargetFolder, InvoiceId)
        If Not String.IsNullOrEmpty(FileName) Then ProcessCopy FileName, Content
    Next

End Sub

Write the file

At this point I'm questioning the need for ProcessCopy altogether:

Sub ExportToFolder(ByVal TargetSubfolderNames As IEnumerable(Of String), _
                   ByVal TargetFolder As String, _
                   ByVal Content As DataSet)

    Dim SubfolderName As String, FileName As String
    Dim InvoiceId As String = Content.Tables(0).Rows(0).Item("InvoiceID")

    For Each SubfolderName In TargetSubfolderNames
        FileName = GetExportFileName(SubfolderName, TargetFolder, InvoiceId)
        If Not String.IsNullOrEmpty(FileName) Then Content.WriteXml(FileName)
    Next

End Sub

Let's see, if we have 4 files to export, and the 2nd one blows up, we can't try the 3rd and 4th like this - we'll have an exception to deal with (read: not swallow).

Sub ExportToFolder(ByVal TargetSubfolderNames As IEnumerable(Of String), _
                   ByVal TargetFolder As String, _
                   ByVal Content As DataSet)

    Dim SubfolderName As String, FileName As String
    Dim InvoiceId As String = Content.Tables(0).Rows(0).Item("InvoiceID")
    Dim Errors As IList(Of String) = New List(Of String)

    For Each SubfolderName In TargetSubfolderNames
        FileName = GetExportFileName(SubfolderName, TargetFolder, InvoiceId)
        If Not String.IsNullOrEmpty(FileName) Then
            Try

                Content.WriteXml(FileName)

                Catch ex As Exception
                    Errors.Add(ex) 'fine, don't do anything with it.
                                   'at least you can break and inspect it.
            End Try
        End If
    Next

End Sub

This is where we can extract a method to wrap the Try...Catch block and, incidentally, the Content.WriteXml call - I'll pretend you want to log errors with an instance-level NLog logger:

Sub ExportToFile(FileName as String, Content As DataSet)
    Try

        Content.WriteXml(FileName)

    Catch ex As Exception
        _logger.ErrorException(FileName, ex)

    End Try
End Sub

This last refactoring has given its final shape to the ExportToFolder method:

Sub ExportToFolder(ByVal TargetSubfolderNames As IEnumerable(Of String), _
                   ByVal TargetFolder As String, _
                   ByVal Content As DataSet)

    Dim SubfolderName As String, FileName As String
    Dim InvoiceId As String = Content.Tables(0).Rows(0).Item("InvoiceID")

    For Each SubfolderName In TargetSubfolderNames
        FileName = GetExportFileName(SubfolderName, TargetFolder, InvoiceId)
        If Not String.IsNullOrEmpty(FileName) Then ExportToFile(FileName, Content)
    Next

End Sub


Splitting text into individual characters for animation I have code which makes use of jQuery and some additional libraries. But the jQuery part is too overwhelming (and thus it does its job slowly). So I tried to optimize it, but had no luck. Now I'm seeking help from the community.

Here is the code, from the news page in my WP theme:

/*
 CSS Reset
*/
   html,body,div,span,applet,object,iframe,audio,video,h1,h2,h3,h4,h5,h6,h7,p,blockquote,pre,a,abbr,acronym,address,big,cite,code,del,dfn,em,img,ins,kbd,q,s,samp,small,strike,strong,sub,sup,tt,var,b,u,i,center,dl,dt,dd,ol,ul,li,fieldset,form,label,legend,table,caption,tbody,tfoot,thead,tr,th,td,article,aside,canvas,details,embed,figure,figcaption,footer,header,hgroup,menu,nav,output,ruby,section,summary,time,mark {
 vertical-align: baseline;
}
#gsap-title-, #gsap-meta- {
    position: relative;
}
#gsap-title- span, #gsap-meta- span  {
 position: relative;
 display:inline-block;
 -webkit-font-smoothing:antialiased;
}
<!DOCTYPE html>

<meta charset="UTF-8">
<meta http-equiv="X-UA-Compatible" content="IE=edge,chrome=1">
<meta name="viewport" content="width=device-width, initial-scale=1">

This is title

To start animation, click Run.

Then click here, or resize the height of this window (simulate 'inview' event).

As you can see, this whole code is too overwhelming. Thus I tried to simplify the jQuery code.

I tried to process  $title and $postmeta with .html() simultaneously, and only then pass them to .on(). Thus :

$([$title, $postmeta]).each(function(){ // ...did work, but incorrectly - all $title and $postmeta were processed, but at the output - in each line containing  some id=gsap-title-, all matching elements gsap-title-1, gsap-title-2... gsap-title-n are displayed (the same as gsap-meta-), ...all of them, in each line ( e.g. merged in an unwanted manner);

Also I had a look at .add(), .concat() and .push() but according to examples, output is similar to .merge() method.

To sum up: current state - lots of code, lots of similar jQuery code, desired state - less jQuery code, with the same functionality.

There are a number of ways you can improve the javascript/jquery on this page.

If possible you should move all of your


Extracting an adjaceny matrix containing haversine distance from points on map I am extracting 10 lat/long points from Google Maps and placing these into a text file. The program should be able to read in the text file, calculate the haversine distance between each point, and store in an adjacency matrix. The adjacency matrix will eventually be fed to a 2-opt algorithm, which is outside the scope of the code I am about to present.

The following code is functional, but extremely inefficient. If I had 1000 points instead of 10, the adjacency matrix would need 1000 x 1000 iterations to be filled. Can this be optimized?

import csv
from haversine import haversine
import matplotlib.pyplot as plt
import numpy as np

def read_two_column_file(file_name):
    with open(file_name, 'r') as f_input:
        csv_input = csv.reader(f_input, delimiter=' ', skipinitialspace=True, )
        long = []
        lat = []
        for col in csv_input:
            x = float(col[0])  # converting to float
            y = float(col[1])
            long.append(x)
            lat.append(y)

    return long, lat

def display_points(long, lat):
    plt.figure()
    plt.ylabel('longitude')
    plt.xlabel('latitude')
    plt.title('longitude vs latitude')
    plt.scatter(long, lat)
    plt.show()

def main():
    long, lat = read_two_column_file('latlong.txt')

    points = []
    for i in range(len(lat)):
            coords = tuple([lat[i], long[i]])  # converting to tuple to be able to perform haverine calc.
            points.append(coords)

    hav = []
    for i in range(len(lat)):
        for j in range(len(long)):
            hav.append(haversine(points[i], points[j]))

    np.asarray(hav)
    adj_matrix = np.reshape(hav, (10, 10))  # reshaping to 10 x 10 matrix
    print(adj_matrix)

    display_points(long, lat)

main()

Sample Input:

35.905333, 14.471970
35.896389, 14.477780
35.901281, 14.518173
35.860491, 14.572245
35.807607, 14.535320
35.832267, 14.455894
35.882414, 14.373217
35.983794, 14.336096
35.974463, 14.351006
35.930951, 14.401137

Sample Output:

[[ 0.          1.15959635  5.15603243 12.15003864 12.66090817  8.06760374
  11.25481465 17.31108648 15.37358741  8.34541481]
 [ 1.15959635  0.          4.52227294 11.19223786 11.50131214  7.32033758
  11.72388583 18.35259685 16.41378987  9.29953014]
 [ 5.15603243  4.52227294  0.          7.44480948 10.26177912 10.15688933
  16.24592213 22.1101544  20.18967731 13.40020548]
 [12.15003864 11.19223786  7.44480948  0.          7.01813758 13.28961044
  22.25645098 29.42422794 27.49154954 20.48281039]
 [12.66090817 11.50131214 10.26177912  7.01813758  0.          9.22215871
  19.74293886 29.16680205 27.25540014 19.97465594]
 [ 8.06760374  7.32033758 10.15688933 13.28961044  9.22215871  0.
  10.66219491 21.06632671 19.24994647 12.24773666]
 [11.25481465 11.72388583 16.24592213 22.25645098 19.74293886 10.66219491
   0.         11.67502344 10.21846781  6.08016463]
 [17.31108648 18.35259685 22.1101544  29.42422794 29.16680205 21.06632671
  11.67502344  0.          1.93885474  9.20353461]
 [15.37358741 16.41378987 20.18967731 27.49154954 27.25540014 19.24994647
  10.21846781  1.93885474  0.          7.28280909]
 [ 8.34541481  9.29953014 13.40020548 20.48281039 19.97465594 12.24773666
   6.08016463  9.20353461  7.28280909  0.        ]]

Plot:

More than half of your code is being used to convert from one data format to another (from two lat and long list to tuples and then from a list of lists to an array).

The easiest to understand version would be to use numpy.loadtxt:

def read_two_column_file(file_name):
    return np.loadtxt(file_name, delimiter=", ")

This is then a 2D numpy.array. However, this is actually a lot slower than it could be, so you could also use pandas.read_csv instead:

import pandas as pd

def read_two_column_file(file_name):
    return pd.read_csv(file_name, header=None).values

Which one is faster depends on the size of your file.

Now we need to modify the display_points function to work with this new data format:

def display_points(points):
    plt.figure()
    plt.ylabel('longitude')
    plt.xlabel('latitude')
    plt.title('longitude vs latitude')
    plt.scatter(points[:, 0], points[:, 1])
    plt.show()

Now for the actual calculation. First, you can use itertools.combinations_with_replacement to get all pairs of points. Then you can insert them directly into the correct row of an array:

from itertools import combinations_with_replacement

def main():

    points = read_two_column_file(file_name)

    adj_matrix = np.empty(len(points)**2)
    for i, (point1, point2) in enumerate(combinations_with_replacement(points, 2)):
        adj_matrix[i] = haversine(point1, point2)
    adj_matrix.reshape((len(points), len(points))

    print(adj_matrix)
    display_points(points)

This can probably be further improved by using numpy.meshgrid to get the combinations of points and using a vectorized version of the haversine function.


Finding all divisors of an integer The problem is to find all divisors of a given integer n.

(It may be better to implement a true prime generator, rather than a fixed set, as discussed with examples here, but that was not my immediate concern. Also, I am aware that there are much faster algorithms to generate fixed number of prime numbers—I am avoiding them until I fully understand the algorithm.)

import functools
import itertools
import operator

def prime_generator(n):
    """
    Sieve of Eratosthenes
    Create a candidate list within which non-primes will be
    marked as None.
    """
    cand = [i for i in range(3, n + 1, 2)]
    end = int(n ** 0.5) // 2

    # Loop over candidates (cand), marking out each multiple.
    for i in range(end):
        if cand[i]:
            cand[cand[i] + i::cand[i]] = [None] * (
                (n // cand[i]) - (n // (2 * cand[i])) - 1)

    # Filter out non-primes and return the list.
    return [2] + [i for i in cand if i]

primes_list = prime_generator(100000)

def factorize(n):
    prime_multiples = []
    for item in primes_list:
        if item > n:
            break
        else:
            while n > 1:
                if n % item == 0:
                    n //= item
                    prime_multiples.append(item)
                else:
                    break
    return prime_multiples

def calculate_divisors(n):
    prime_multiples_list = factorize(n)

    """
    construct unique combinations
    A, B, B, C --> A, B, C, AB, AC, BB, BC, ABC, ABB, BBC
    """
    unique_combinations = set()
    for i in range(1, len(prime_multiples_list)):
        unique_combinations.update(
            set(itertools.combinations(prime_multiples_list, i)))

    # multiply elements of each unique combination
    combination_product = list(functools.reduce(operator.mul, i)
                               for i in unique_combinations)
    combination_product.sort()

    return combination_product

print(calculate_divisors(12500))
>>> [2, 4, 5, 10, 20, 25, 50, 100, 125, 250, 500, 625, 1250, 2500, 3125, 6250]

In the context of the above algorithm:

Can I shorten factorize function? (For example, I couldn’t find a way to turn into a list comprehension. I suspect there should be a shorthand via functoolsor itertools.)
Is there a more Pythonic implementation of calculate_divisors function?

Lazy generators

Most often in Python you do not want to build an actual list and return it.

If I want to sum the divisors of a number, and there are many, I will waste a lot of space if I put them in a list first.

Also, not creating a list is even shorter, you just give out the elements one by one from the function:

def factorize(n):
    for item in primes_list:
        if item > n:
            break
        else:
            while n > 1:
                if n % item == 0:
                    n //= item
                    yield item
                else:
                    break

Reduce nesting

You got some serious nesting going on, each level of nesting is a level of complexity so getting it a bit down is positive. Luckily if we break we do not need an else: the code after will not be executed anyway:

def factorize(n):
    for item in primes_list:
        if item > n:
            break

        while n > 1:
            if n % item != 0:
                break
            n //= item
            yield item

Still two nested loops, but the situation is becoming more manageable.

Now let me re-factor the while loop away:

def how_many_times_divides(n, div):
    """
    >>> list(how_many_times_divides(40, 2))
    [2, 2, 2]
    """
    while n > 1:
        if n % div != 0:
            break
        n //= div
        yield div

And now factorize is starting to look nice and small:

def factorize(n):
    """
    >>> list(factorize(480))
    [2, 2, 2, 2, 2, 3, 5]
    """
    for item in primes_list:
        if item > n:
            break
        yield from how_many_times_divides(n, item)

Doctests

You may have noted that I added some examples of usage in the doc-strings of this functions. They are actually automatically runnable with doctests and I highly recommend them when writing numerical code.

Avoid unnecessary assignments & mutation

It is just simpler to return the expression:

combination_product = list(functools.reduce(operator.mul, i)
                           for i in unique_combinations)
combination_product.sort()

return combination_product

Becomes:

return sorted(functools.reduce(operator.mul, i)
                 for i in unique_combinations)

Note that I omitted list as it was not really needed.


Displacing an enumerable by a count What I mean by displacing an enumerable is skipping the first n items, then returning the next n items, then returning the first n items, then returning the rest of the enumerable.

What do you think?  Does this idea have any value?

using System.Collections.Generic;

public class EnumerableDisplacer
{
    public virtual IEnumerable<T> Displace<T>(IEnumerable<T> source, int displaceCount)
    {
        var enumerator = source.GetEnumerator();
        var counter = 0;
        var displacedItems = new LinkedList<T>();
        while (counter < displaceCount)
        {
            enumerator.MoveNext();
            ++counter;

            displacedItems.AddLast(enumerator.Current);
        }

        counter = 0;
        while (counter < displaceCount)
        {
            enumerator.MoveNext();
            ++counter;

            yield return enumerator.Current;
        }

        foreach (var di in displacedItems)
        {
            yield return di;
        }

        while (enumerator.MoveNext())
        {
            yield return enumerator.Current;
        }
    }
}

Your implementation is missing some basic validation. A public method should always validate its passed in arguments. Assume an IEnumerable<int> will be passed containing 10 elements

with displaceCount == 10. You won't see an exception if it is a "true"  IEnumerables<int> but you will receive wrong results.

A test with

IEnumerable<int> source = new int[] { 1, 2, 3, 4, 5, 6, 7, 8, 9, 10 };
source = source.Where(i => i > 0);
EnumerableDisplacer dispatcher = new EnumerableDisplacer();
var result = dispatcher.Displace(source, 10).ToList();

will result in a List<int> containing the 20 elements { 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10 }

passing

IEnumerable<int> source = new int[] { 1, 2, 3, 4, 5, 6, 7, 8, 9, 10 };
EnumerableDisplacer dispatcher = new EnumerableDisplacer();
var result = dispatcher.Displace(source, 10).ToList();

will result in an InvalidOperationException in the second while loop at yield return enumerator.Current; because the enumeration has ended already.

You have two choices to do the input validation:

checking the count of items contained in source against displacedCount which will result for a "true" IEnumerable<T> in iterating over the elements twice

or

convert the source to an collection type like e.g an array and check its Length property against displacedCount which will result in iterating over the elements twice  as well.

I would go with the second choice because it would make the whole method easier (IMO) because you wouldn't need any other datastructure like the LinkedList.

This would result in

public class EnumerableDisplacer
{

    public IEnumerable<T> Displace<T>(IEnumerable<T> source, int displaceCount)
    {
        if (source == null)
        {
            throw new ArgumentNullException("source");
        }

        var sourceArray = source.ToArray();
        var doubledDisplacedCount = displaceCount * 2;
        if (sourceArray.Length < doubledDisplacedCount)
        {
            throw new ArgumentOutOfRangeException("source", "The source needs to contain twice as many elements than displacedCount");
        }

        return DisplaceEx<T>(sourceArray, displaceCount);
    }
    private IEnumerable<T> DisplaceEx<T>(IEnumerable<T> source, int displaceCount)
    {

        var doubledDisplacedCount = displaceCount * 2;

        for (var i = displaceCount; i < doubledDisplacedCount; i++)
        {
            yield return sourceArray[i];
        }
        for (var i = 0; i < displaceCount; i++)
        {
            yield return sourceArray[i];
        }
        for (var i = doubledDisplacedCount; i < sourceArray.Length; i++)
        {
            yield return sourceArray[i];
        }

    }
}

What advantage should this method get from beeing virtual ? I don't see any. Overriding this method in an inherited class doesn't make sense IMO.


Can this date-validation function can be simplified further or is it the correct approach? I am doing a date validation. I have no. of pages, which have the date field. User can input the date like "220875" or "22AUG75" - I need to test both and check the length as well.

function isValidDate(newDate) {

    newDate[1] = newDate[1]-1;
    newDate[2] = (parseInt(newDate[2]) < 50) ? 2000 + parseInt(newDate[2]) : 1900 + parseInt(newDate[2]);

    var testDate = new Date(newDate[2], newDate[1], newDate[0]);

    if (testDate.getDate()!=newDate[0] || testDate.getMonth()!=newDate[1] || testDate.getFullYear()!=newDate[2]) {
        return false;
    } else {
        return {valid : true, date : testDate};
    }

}

var sandBox = {
    init:function(params){
        this.element = $(params.element),
        this.value = params.value,
        this.mode = params.num;
    },
    isNotEmpty:function(){
        if(!this.value.length) {
           this.errorHandler(this.emptyMsg);
        } else {
            this.errorHandler(true);
            return true;
        }
    },
    errorHandler: function (msg) {
        var errorHolder = this.element.siblings('span');
        if(msg !== true)
            errorHolder.html(msg).show();
        else errorHolder.html("").hide();
    },
    emptyMsg : "Date field should not be empty",
    digitLengthMsg : "The Length of the value should be ",
    onlyNumber : {
        num : /\D/,
        str : /^\d{2}[a-zA-Z]{3}\d{2}$/,
        mon : /[a-zA-Z]{3}/,
        msg : "Only Digits allowed"
    },
    getMonthFromString : function(mon){
        var month =  new Date(Date.parse(mon +" 01, 12")).getMonth()+1;
        month = month < 10 ? '0'+ month : month;
        return month;
    },
    isValidData : function () {

        var result =  this.onlyNumber.str.test(this.value);

        if(this.mode && result) {
            this.errorHandler(this.onlyNumber.msg)
            return false;
        }
        else if (!this.mode && result) {
            var currentMonth = this.getMonthFromString(this.value.match(this.onlyNumber.mon)[0]);
            this.value = this.value.replace(/([a-zA-Z]){3}/g, currentMonth);
            return true;
        }
        else if (this.mode && this.value.length !== 6) {
            this.errorHandler(this.digitLengthMsg + '6');
            return false;
        }
        else if (!this.mode && this.value.length !== 7) {
            this.errorHandler(this.digitLengthMsg + '7');
            return false;
        }
        else {
            this.errorHandler(true);
            return true;
        }
    },

    DateIsOk:function() {
        if(this.isNotEmpty() && this.isValidData()) {
            var verifyDate = this.value.match(/.{1,2}/g);
            return isValidDate(verifyDate);
        }
    }

};

var dateValidator = function (e) {
    var value = e.target.value;
    sandBox.init({
        value : value,
        element : e.target,
        num : true //set month status true means validation by digits, false means digits with string
    });

   var fieldValue = sandBox.DateIsOk();

   if(fieldValue) {
    console.log(fieldValue);
   }

};

var dateField = $('input');
dateField.on('keyup focusout', dateValidator);

Live Demo

Can this be simplified ? Most certainly.

You can take advantage of the fact that new Date( 'APR 04 1977' ) and new Date( '04 04 1977' ) both work, so you don't need getMonthFromString
You can take more out of regexes with capturing groups
You can get rid of the whole Sandbox construct which does not make a lot of sense to me
You can write it so that the validator can accept but num:true and num:false, it can only make the user happier ;)
Not too excited by YY, we spent billions fixing Y2K, and here you come creating more trouble ;)
You don't need value if you have element ( simply call val() )
isValidDate seems overkill, if you ask for 30 feb 2014, then testDate.getTime() will be NaN

My counter proposal code is about 1/3rd of yours:

function DateHandler(elementId, _mandatory) {
    this.$dateField = $('#' + elementId);
    this.mandatory = _mandatory || false;
    this.$dateField.on('keyup focusout', this.validate.bind(this));
}

DateHandler.prototype.validate = function () {
    this.error('');
    var text = this.$dateField.val().toUpperCase(), matches;
    if (text.length === 0) {
        if (mandatory) {
            this.error('Field should not be empty');
        }
        return;
    } else if (text.length == 6) {
        matches = /([0-3][0-9])([0-1][0-9])(\d\d)/.exec(text);
    } else if (text.length == 7) {
        matches = /([0-3][0-9])(\w\w\w)(\d\d)/.exec(text);
    }
    if (!matches) {
        return this.error('Field should be of format DDMMYY or DDMMMYY');
    }
    value = new Date(matches[2] + ' ' + matches[1] + " " + (matches[3] < 50 ? 20 : 19) + matches[3]);
    if (isNaN(value.getTime())) {
        return this.error('Not a valid date');
    }
    console.log(value);
};

DateHandler.prototype.error = function (message) {
    var label = this.$dateField.siblings('span');
    message ? label.html(message).show() : label.hide();
};

var dateHandler1 = new DateHandler('d1');
var dateHandler2 = new DateHandler('d2');

I tested this fairly well on JsFiddle: http://jsfiddle.net/konijn_gmail_com/985ut/


Partitioning a list to a list of smaller lists I created a utility method ListUtils.partition(iterable, size) to partition a potentially large list to a list of smaller lists:

public class ListUtils {
    public static <T> List<List<T>> partition(Iterable<T> iterable, int size) {
        if (iterable == null) {
            return Collections.emptyList();
        }
        if (size < 1) {
            List<T> list = new ArrayList<>();
            for (T item : iterable) {
                list.add(item);
            }
            return Arrays.asList(list);
        }
        List<List<T>> result = new ArrayList<>();
        List<T> segment = new ArrayList<>(size);
        int i = 0;
        for (T item : iterable) {
            segment.add(item);
            if (++i == size) {
                result.add(segment);
                segment = new ArrayList<>(size);
                i = 0;
            }
        }
        if (i > 0) {
            result.add(segment);
        }
        return result;
    }
}

Unit tests:

public class ListUtilsTest {
    private <T> List<List<T>> partition(Iterable<T> iterable, int size) {
        return ListUtils.partition(iterable, size);
    }

    @Test
    public void testPartitionEmpty() {
        assertEquals(Arrays.<List<Object>>asList(), partition(Arrays.asList(), 3));
    }

    @Test
    public void testPartitionNull() {
        assertEquals(Arrays.<List<Object>>asList(), partition(null, 3));
    }

    @Test
    public void testPartitionZeroSize() {
        List<Integer> orig = Arrays.asList(1, 2, 3);
        assertEquals(Arrays.asList(orig), partition(orig, 0));
    }

    @Test
    public void testPartitionNegativeSize() {
        List<Integer> orig = Arrays.asList(1, 2, 3);
        assertEquals(Arrays.asList(orig), partition(orig, -3));
    }

    @Test
    public void testPartitionInts() {
        assertEquals(
                Arrays.asList(
                        Arrays.asList(1, 2),
                        Arrays.asList(3, 4),
                        Arrays.asList(5)),
                partition(Arrays.asList(1, 2, 3, 4, 5), 2));
    }

    @Test
    public void testPartitionStrings() {
        assertEquals(
                Arrays.asList(
                        Arrays.asList("1", "2"),
                        Arrays.asList("3", "4"),
                        Arrays.asList("5")),
                partition(Arrays.asList("1", "2", "3", "4", "5"), 2));
    }

    @Test
    public void testPartition_MultipleOfSize() {
        assertEquals(
                Arrays.asList(
                        Arrays.asList(1, 2),
                        Arrays.asList(3, 4)),
                partition(Arrays.asList(1, 2, 3, 4), 2));
    }

    @Test
    public void testPartition_Size1() {
        assertEquals(
                Arrays.asList(
                        Arrays.asList(1),
                        Arrays.asList(2),
                        Arrays.asList(3),
                        Arrays.asList(4)),
                partition(Arrays.asList(1, 2, 3, 4), 1));
    }

    @Test
    public void testPartition_BiggerSize() {
        List<Integer> orig = Arrays.asList(1, 2, 3, 4);
        assertEquals(
                Arrays.asList(orig),
                partition(orig, orig.size() + 11));
    }
}

Can you improve this? Are there interesting corner cases I might have missed in the tests?

I don't agree with your method of handling 0 or negative partition sizes.

A partition is always meant to be a sub-section of a whole. A partition with size 0 is impossible if the whole is non-empty, as there is no way to cut up a whole into sub-sections of size 0. (Note, an empty whole will of course be divisible into partitions of size 0). The same logic applies for negative values. I would think raising a RuntimeException such as an IllegalArgumentException would make more sense in that context. null objects shouldn't be partition-able either, as they are not even empty "wholes".

Now as for handling sub-section sizes larger than the size of your list, you already define that by having incomplete sub-sections if your sub-section size doesn't divide into your container size. You are missing a test for this, however.

A final remark: since Iterable isn't confined to Collection's measly 2 billion item limit for indexing, you might consider using a long for your partition size if you plan to
use this for large data sets.


Parsing command line arguments in Go I've written a reimplementation of the redis-benchmark in Go and part of that is parsing options from the command line. I evaluated several libraries but nothing I found seem to do what I had in mind.

Here is the code:

// ParseArguments parses a string array and returns a populated Options struct
func ParseArguments(arguments []string) Options {
    options := defaultOptions

    args := arguments[1:]
    var errOptions Options
    for i := 0; i < len(args); i++ {
        if args[i] == "--help" || args[i] == "-h" {
            return Options{ShowHelp: true, HelpText: helpText}
        } else if args[i] == "--host" || args[i] == "-H" {
            i++
            if i >= len(args) {
                return buildHelp("Error: Incorrect parameters specified")
            }
            options.Host = args[i]
        } else if args[i] == "--requests" || args[i] == "-n" {
            options.Requests, errOptions = parseNumber(args, &i)
            if errOptions.ShowHelp {
                return errOptions
            }
        } else if args[i] == "--clients" || args[i] == "-c" {
            options.Connections, errOptions = parseNumber(args, &i)
            if errOptions.ShowHelp {
                return errOptions
            }
        } else if args[i] == "--tests" || args[i] == "-t" {
            i++
            if i >= len(args) {
                return buildHelp("Error: Incorrect parameters specified")
            }
            options.Tests = strings.Split(args[i], ",")
            for i := range options.Tests {
                options.Tests[i] = strings.ToUpper(options.Tests[i])
            }
        } else if args[i] == "--port" || args[i] == "-p" {
            options.Port, errOptions = parseNumber(args, &i)
            if errOptions.ShowHelp {
                return errOptions
            }
        } else {
            return buildHelp(fmt.Sprintf("Error: Invalid parameter: %v", args[i]))
        }
    }

    return options
}

(For more context, the rest of the file is on github).

When gocyclo is run over the code the function gets a complexity of 20. I'm interested in suggestions that could reduce this to below the threshold of 10.

You should definitely rely on a library to achieve this. The flag package from standard library is nice, but go-flags is even better

With this library, the equivalent code is:

package main

import (
    "fmt"
    "os"

    "github.com/jessevdk/go-flags"
)

type options struct {
    Help     bool     `short:"h" long:"help" description:"show help message"`
    Requests int      `short:"n" long:"requests" default:"1"`
    Clients  int      `short:"c" long:"clients" default:"1"`
    Tests    []string `short:"t" long:"tests" env-delim:","`
    Port     int      `short:"p" long:"port" default:"9000"`
}

func main() {
    var opts options
    p := flags.NewParser(&opts, flags.Default&^flags.HelpFlag)
    _, err := p.Parse()
    if err != nil {
        fmt.Printf("fail to parse args: %v", err)
        os.Exit(1)
    }
    if opts.Help {
        p.WriteHelp(os.Stdout)
        os.Exit(0)
    }
    fmt.Printf("tests: %v\n", opts)
}

All errors (type error, required flag missing ect...) are handle directly by the library

Now the complexity of the function is 3


Android SQLite Database management I'd like some opinions about my approach at using SQLite databases in android. Everything seems to work very well, but there may be something wrong that I hadn't noticed.

I don't like working with create/alter table statements in code, so I prefer creating my database with some SQLite client out there and putting it in assets folder.  I use some logic to decide when the database must be replaced and which records need to be kept, then use this code in a class called DbManager:

private static SQLiteDatabase _db = null;

public static Object lock = new Object();

private static SQLiteDatabase getDB() {
    if (_db == null) {
        _db = SQLiteDatabase.openDatabase(DB_PATH + DB_NAME, null, SQLiteDatabase.OPEN_READWRITE);
    }

    return _db;
}

//this is the approach used for insert operations, but is the same for update, delete etc
public static boolean insert(String tableName, ContentValues values) {
    try {
        return getDB().insert(tableName, null, values) >= 0;
    } catch (Exception e) {
        LogHelper.WriteLogError("error in DB_manager.insert function", e);
        return false;
    }
}

public static synchronized void beginTransaction(String who) {
    LogHelper.WriteLogDebug("starting transaction by " + (who == null ? "null" : who));
    try {
        //in this way if a transaction is still executing it waits untill the previus ends.
        //obviously every time i call DbManager.beginTransaction the call at DbManager.endTransaction is in the finally statment
            if (_db.inTransaction()) {
                LogHelper.WriteLogInfo("DB IS IN TRANSACTION");
                synchronized (lock) {
                    lock.wait();
                }
            }
            getDB().beginTransactionWithListener(new SQLiteTransactionListener() {
                @Override
                public void onRollback() {
                    synchronized (lock) {
                         LogHelper.WriteLogInfo("onRollback listener invoked");
                         lock.notifyAll();
                    }
                }

                @Override
                public void onCommit() {
                    synchronized (lock) {
                        LogHelper.WriteLogInfo("onCommit listener invoked");
                        lock.notifyAll();
                    }
                }

                @Override
                    public void onBegin() {
                        // TODO Auto-generated method stub
                    }
                });
            } catch (Exception e) {
                LogHelper.WriteLogError("error calling begin transaction by " + (who == null ? "null" : who), e);
            }
        }

        public static void endTransaction(boolean commitChanges, String who) {
            LogHelper.WriteLogDebug("ending transaction by " + (who == null ? "null" : who));
            try {
                if (commitChanges)
                    getDB().setTransactionSuccessful();
                getDB().endTransaction();
            } catch (Exception e) {
                LogHelper.WriteLogError("error calling end transaction by " + (who == null ? "null" : who), e);
            }
        }

The code looks fine to me but it is a bit odd because the official docs recommend creating a SQLiteOpenHelper subclass to handle database interaction. And if you use one SQLiteOpenHelper instance across your application, it will be thread-safe so there's no need to add your own synchronization logic:

public class DatabaseHelper extends SQLiteOpenHelper {

  private static final String DATABASE_NAME = "location_scout.db";
  private static final int SCHEMA_VERSION = 1;

  // Singleton pattern
  private static DatabaseHelper instance;
  public synchronized static DatabaseHelper getInstance(Context context) {
      if(instance == null) {
          instance = new DatabaseHelper(context.getApplicationContext());
      }
      return instance;
  }

  private DatabaseHelper(Context context) {
      super(context, DATABASE_NAME, null, SCHEMA_VERSION);
  }

}


Accessing MySQL to make queries and scalars I've developed my own class in ASP.NET project to access MySQL and make queries and scalars, and read the results from it.

I want you to review my class and tell me where I've made a mistake.

Here are some questions that are important for me:

Do I use a reference correctly for the connector class of MySQL in methods?
Is it correct to make it NON-static class as it is? For the GC, memory reasons?
Do I need to add some events, delegate for the autoclose for the MySQL connection, or is there no need in it, because out of scope it will be auto freed from memory?

public class MySQLGear
{
    private const string connStr = "Server = localhost; Database = self; Uid = someuser; Pwd = 1234;";

    public MySqlConnection CreateConnection()
    {
        MySqlConnection connMysql = new MySqlConnection(connStr);
        connMysql.Open();

        if (connMysql.State == ConnectionState.Open)
        {
            return connMysql;
        }
        else return null;
    }

    public MySqlDataReader GetReader(ref MySqlConnection connMysql, string queryMysql)
    {
        MySqlCommand cmdMysql = new MySqlCommand(queryMysql, connMysql);
        MySqlDataReader readerMysql = cmdMysql.ExecuteReader();
        return readerMysql;
    }

    public int MakeQuery(ref MySqlConnection connMysql, string queryMysql)
    {
        MySqlCommand cmdMysql = new MySqlCommand(queryMysql, connMysql);
        int result = cmdMysql.ExecuteNonQuery();
        return result;
    }

    public object MakeScalar(ref MySqlConnection connMysql, string queryMysql)
    {
        MySqlCommand cmdMysql = new MySqlCommand(queryMysql, connMysql);
        object result = cmdMysql.ExecuteScalar();
        return result;
    }
}

Question #1

No. You do not need to ref the connection object in your methods' parameters - think object oriented way not class oriented.

Question #2

The only way is to make this as a non static class. Avoid always statics when the current problem contains some kind of state management like a MySQL connection.

Question #3

No you don't but you should implement the IDisposable interface and apply the correct disposable pattern.

public class MySQLGear : IDisposable
{
    private readonly MySqlConnection _connection;

    public MySQLGear(string connectionString)
    {
        _connection = new MySqlConnection(connectionString);
    }

    protected MySqlConnection Connection()
    {
        if (_connection == null)
        {
            throw new ObjectDisposedException("The underlying connection has been closed");
        }

        if (_connection.State != ConnectionState.Open)
        {
            _connection.Open();
        }

        return _connection;
    }

    public MySqlDataReader GetReader(string queryMysql)
    {
        using (var cmdMysql = new MySqlCommand(queryMysql, Connection()))
        {
            return cmdMysql.ExecuteReader();
        }
    }

    public int MakeQuery(string queryMysql)
    {
        using (var cmdMysql = new MySqlCommand(queryMysql, Connection()))
        {
            return cmdMysql.ExecuteNonQuery();
        }
    }

    public object MakeScalar(string queryMysql)
    {
        using (var cmdMysql = new MySqlCommand(queryMysql, Connection()))
        {
            return cmdMysql.ExecuteScalar();
        }
    }

    ~MySQLGear()
    {
        Dispose(false);
    }

    public void Dispose()
    {
        Dispose(true);
        GC.SuppressFinalize(this);
    }

    protected virtual void Dispose(bool disposing)
    {
        if (_connection != null)
        {
            if (_connection.State != ConnectionState.Closed)
            {
                _connection.Dispose();
            }
            _connection = null;
        }
    }
}


Tic Tac Toe getWinner() method logic for a grid of any size nxn I have a college assignment in which I have to make a Tic Tac Toe game work. Some of the code has been provided by my instructor like method signatures etc. Now the rules for this particular project state that a player wins if they get n consecutive spaces on an nxn grid. So 3 for 3x3, 5 for 5x5, etc. Now I got it to work for a 3x3 grid by kind of hard coding it.

Please have a look at how it is done in my code. Now I check if 3 cells contain the same player (X or O). But I don't know how to generalize my method as n increases.

package tictactoe;

/**
 * An implementation of the TicTacToeGame interface.
 *
 * ATTENTION STUDENTS: You MUST use exactly this class specification. Do not
 * rename the class, and do not remove the "implements TicTacToeGame", or
 * you will receive no credit for your submission.
 */
public class TicTacToe implements TicTacToeGame {
    int size;
    String whoseTurn = "X";//X,O,""
    String[] board;
    int count;
    /**
     * Constructs a new instance, implementing the TicTacToeGame interface.
     *
     * ATTENTION STUDENTS: You MUST use exactly this signature for your constructor.
     * Do not rename the class and do not change the argument, or you will receive
     * no credit for your submission.
     *
     * @param n the length and width of the board; n >= 3
     */
    public TicTacToe(int n) {
        size = n*n;
        board = new String[size];
        resetBoard();
        count = 0;
    }

    @Override
    public int getN() {
        return size;
    }

    @Override
    public String toString() {
        String result = "";
        int ctr = 1;
        for(int i = 0; i < size; i++){
            result += board[i];
            if(ctr % Math.sqrt(size) == 0)
                result += '\n';
            ctr++;
        }
        return result;
    }

    @Override
    public String getWinner() {
        int len = (int)(Math.sqrt(size));
        int diag = len - 1;
        int antiDiag = len + 1;
        for(int i = 0; i < (size - len); i++){
            if((board[i].equals("X")) && (board[i+1].equals("X")) && (board[i+2].equals("X")))
                return "X";
            if((board[i].equals("O")) && (board[i+1].equals("O")) && (board[i+2].equals("O")))
                return "O";

            if((board[i].equals("X")) && (board[i+len].equals("X")) && (board[i+2*len].equals("X")))
                return "X";
            if((board[i].equals("O")) && (board[i+len].equals("O")) && (board[i+2*len].equals("O")))
                return "O";

            if((board[diag].equals("X")) && (board[2*diag].equals("X")) && (board[3*diag].equals("X")))
                return "X";
            if((board[diag].equals("O")) && (board[2*diag].equals("O")) && (board[3*diag].equals("O")))
                return "O";

            if((board[antiDiag].equals("X")) && (board[2*antiDiag].equals("X")) && (board[3*antiDiag].equals("X")))
                return "X";
            if((board[antiDiag].equals("O")) && (board[2*antiDiag].equals("O")) && (board[3*antiDiag].equals("O")))
                return "O";
        }
        return "";
    }

    @Override
    public String getCurrentPlayer() {
        while(count < size)
            return whoseTurn;
        return "";
    }

    @Override
    public boolean isValidMove(int space) {
        if((count<(size)) && (board[space].equals(" ")))
            return true;

        whoseTurn = "";
        return false;

    }

    @Override
    public void move(int space) throws IllegalArgumentException {
        board[space] = this.getCurrentPlayer();
        if(whoseTurn.equals("X"))
            whoseTurn = "O";
        else
            whoseTurn = "X";

        count++;

    }
    private void resetBoard(){
        for(int i = 0; i < size; i++)
            board[i] = " ";
    }

}

You should declare the variables by using the smallest scope that is needed. By omitting the scope specifier (private,public etc) the variable can be accessed by the classes which are in the same package.

So for your class you should declare them private.

If the name of a method implies something like getN() one expect to get exactly that what is implied by the name. You are returning size which is n * n. You should store the n which is passed inside the constructor and return it in this method.

Storing n will help you a lot more than only for this method. Every time you are using Math.sqrt(size) you could just use n instead.

Using braces {} for single if statement or single line loop statements will make your code less error prone.

You don't use the isValidMove() method. In addition one wouldn't expect for a invalid move that the next player would become "".

You need to implement this method and it should be called first at the move() method and if it returns false you should throw an IllegalArgumentException.

The getCurrentPlayer() method doesn't need a while loop. A simple if statement would be enough.

For the getWinner() method you should consider to store the last player in a variable. In this way you wouldn't need to check for .Equals("X") nor .Equals("O").

If you also store the last move, you don't need the checks for diagonal lines if the last move isn't in the middle or in the corners.

Assuming a 5x5 grid and player X will make his move

X _ _ _ _    _ _ _ _ _    _ _ _ _ _
_ _ _ _ _    _ _ _ _ _    _ _ _ _ _
_ _ _ _ _    _ X _ _ _    _ _ X _ _
_ _ _ _ _    _ _ _ _ _    _ _ _ _ _
_ _ _ _ _    _ _ _ _ _    _ _ _ _ _
    1            2            3

For the first example you need to check horizontal and vertical based on the moves position and the diagonal from the upper left corner to the lower right corner.
For the second example you need to check only horizontal and vertical.
For the third example you need to check horizontal, vertical and both diagonals.

The horizontal check

For the horizontal check we need the current row where the move had been made. This row can be computed by dividing the current move by n. To get the first position of this row we multiply the row by n to get the start position of the checking loop.

int startPosition = (lastMove / n) * n;

You need to keep in your mind that by dividing 2 int you will get an int back.

Let us check this for examples

Example 1:

startPosition = (0 / 5) * 5 == 0

Example 2:

startPosition = (11 / 5) * 5 == 10

Example 3:

startPosition = (12 / 5) * 5 == 10

So now we have a starting position for the horizontal checking loop we can add a method to check the horizontal line. In the loops body we will check if the current position of the board does not equal the last player so we can return early.

Private boolean isHorizontalWin(int lastMove, String lastPlayer) {
    int startPosition = (lastMove / n) * n == 0
    for (int i = startPosition; i < startPosition + n; i++) {
        if (!board[i].equals(lastPlayer)) {
            return false;
        }
    }
    return true;
}

The vertical check

For the vertical check we need the current column. This column we can compute by subtracting n from the current move until the result is < n. Let us add a method for this.

private int getCurrentColumn(int lastMove) {
    while (lastMove >= n) {
        lastMove -= n;
    }
    return lastMove;
}

This is now the starting position for the vertical checking loop. The iterating variable needs to be incremented by n.

But what is the ending condition?

Example 1:

we have a startPosition == 0  and the last check should be performed at board[20]

Example 2:

we have a startPosition == 1  and the last check should be performed at board[21]

Example 3:

we have a startPosition == 2  and the last check should be performed at board[22]

So for all three examples:

for (int i = startPosition; i < size ; i += n) {

}

The diagonal check(s)

Here the starting positions are clear. It is the upper left corner so 0 and the upper right corner so n - 1.

If we loop from the upper left to the lower right, the last position to check is size -1.

If we loop from the upper right to the lower left, the last position to check is size - n.

Let us use new samples for getting the value of which we need to increment the iterating variable.

n == 3

0 1 2
3 4 5
6 7 8

Here if we check upper left to lower right, we need to increment by 4. From upper right to lower left we need to increment by 2.

n == 5

 0  1  2  3  4
 5  6  7  8  9
10 11 12 13 14
15 16 17 18 19
20 21 22 23 24

Here if we check upper left to lower right, we need to increment by 6. From upper right to lower left we need to increment by 4.

Again we see a pattern:

upper left to lower right: n + 1
upper right to lower left: n - 1

You need, for implementing the missing points, to add methods for

checking if the move is a special move, in other words

does the last move needs a winning check for the upper left to lower right diagonal
does the last move needs a winning check for the upper right to lower left diagonal

checking if the current player has

vertically won
left to right diagonal won
right to left diagonal won

This would lead to the refactored getWinner() method which could look like

public String getWinner() {
   if(isHorizontalWin(lastMove, lastPlayer)) {
       return lastPlayer;
   }
   if(isVerticalWin(lastMove, lastPlayer)) {
       return lastPlayer;
   }
   if(isLeftToRightCheckNeeded(lastMove) && isLeftToRightWin(lastPlayer)) {
       return lastPlayer;
   }
   if(isRightToLeftCheckNeeded(lastMove) && isRightToLeftWin(lastPlayer)) {
       return lastPlayer;
   }
   return "";
}


Python compress and send The following two functions are used to compress arbitrary Python objects and send them safely via socket or email, using only printable chars. In my specific message protocol, '=' signs are also not allowed; I could have replaced them or truncate them - I chose to truncate them.

I think they work quite well speed-wise and have no unpredicted errors. I post it here for your judgement.

## Framework
import cPickle as pickle
import zlib as zl
import base64 as b64

def compress(o):
    ## First pickle
    p = pickle.dumps(o, pickle.HIGHEST_PROTOCOL)

    ## Then zip
    z = zl.compress(p, zl.Z_BEST_COMPRESSION)

    ## Then encode
    e = b64.b64encode(z)

    ## Then truncate
    t = e.rstrip("=")

    ## Then return
    return t

def decompress(s):
    ## First pad
    e = s + "=" * (-len(s) % 4)

    ## Then decode
    z = b64.b64decode(e)

    ## Then unzip
    p = zl.decompress(z)

    ## Then unpickle
    o = pickle.loads(p)

    ## Then return
    return o

Avoid single letter variable names. Spell out the words they stand for to make your code easier to read.

Your comments are useless. You are just restating the code in less detail. The only comment you should have is explaining about the '=' like you did in your post.

Don't double-space your code.

Pickle isn't safe. You can construct a pickle that does arbitrary things like delete files or what-not. So you should pretty much never be sending them over a socket.


Separate areacode from phone number string with MySQL Database I have a database with 5000 entries of local area codes to separate the area code from a phone number string. I'm not sure if the code
I use is the fastest way to solve these job.

I shrink the number of db entries with the first three digits from phone number string. After this I put the entries found in db in an array and finally search these array for a match.

What do you think guys? Would you do it the same?

<?php
$phonenumber = '03522522492';
$country = 'DE';
$link = mysqli_connect('Host', 'DB', 'Pass','User') or die(mysqli_error());
$db_selected = mysqli_select_db( $link, 'DB');
$fristthree = substr($phonenumber,0,3);
$array = array();
$query = mysqli_query($link, "SELECT * FROM `Areacodes` WHERE `Country` LIKE '".$country."%' AND `Areacode` like '".$fristthree."%' ORDER BY CHAR_LENGTH(Areacode) ASC");
while($row = mysqli_fetch_assoc($query)){
  $array[] = $row;
}
foreach ($array as $areacode) {
    $subString = substr($phonenumber, 0, strlen($areacode["Areacode"]));
    if ($subString == $areacode["Areacode"]) {
        $phone = $subString." ".substr($phonenumber, strlen($areacode["Areacode"]));
        $location = $areacode["Location"];
    }
}
if (!empty($phone)) {
    echo $phone;
    echo '

';
    echo $subString;
    echo '

';
    echo $location;
}

else {
    echo "No Areacode found.";
}
?>

Assuming my understanding of the question and the data is correct:

  Did I get it right that you want to map your phone number to the area matching the most initial digits? Having 035, 0352, 03522 and 035225 you would expected the location of 035225 for 03522522492?

I would try a SQL-only approach:

SELECT *
FROM Areacodes
WHERE Areacode IN ('032','0352','03522','035225')
ORDER BY CHAR_LENGTH(deliveryAddress_zip) DESC
LIMIT 0,1

So in PHP you now only need to build the IN-clause. Split the phone number in substrings of suitable length (I guess 3-6 digits for Germany).

Without any validation the snipped could look like:

$phone = "03522522492";

$parts = [];
for ($len = 3; $len <= 6; $len++) $parts[] = substr($phone, 0, $len);
$in = "'" . implode("','", $parts) . "'";

echo $in;
//'035','0352','03522','035225'


React.js hook to move a car on a 5×5 board I've got this custom hook that is suffering from a bad case of violation of SRP
This is a simple 5x5 board where a car can move around. Together with a server side check if the move is valid or not.

I'm thinking of moving directions to a useReducer hook. But problem is that I'm calling that using its index when I do things like rotate and move. I'm also putting the x, y and degrees in the same data object.

What's a good way to split this up following React Hooks best practices?

import { useState } from 'react'
import { createContainer } from "unstated-next"
import axios from 'axios'

const useController = () => {
  const directions = [
    {x: 0, y: -1, degrees: 0}, // up
    {x: 1, y: 0, degrees: 90}, // right
    {x: 0, y: 1, degrees: 180}, // down
    {x: -1, y: 0, degrees: 270}, // left
  ]
  const [initiatedGame, setInitiatedGame] = useState(false)
  const [degrees, setDegrees] = useState(0)
  const [direction, setDirection] = useState(0)
  const [position, setPosition] = useState({'x': 0, 'y': 0})
  const [carPlaced, setCarPlaced] = useState(false)

  const chooseDirection = direction => {
    setDirection(direction)
    setDegrees(directions[direction].degrees)
    setInitiatedGame(true)
  }

  const changeDirection = direction => {
    if(direction === -1) {
        setDirection(3)
    } else if(direction === 4) {
        setDirection(0)
    } else {
        setDirection(direction)
    }
  }

  const rotateLeft = () => {
    setDegrees(degrees - 90)
    changeDirection(direction - 1)
  }

  const rotateRight = () => {
    setDegrees(degrees + 90)
    changeDirection(direction + 1)
  }

  const move = () => {
    const root = process.env.REACT_APP_API_ROOT
    const {x, y} = directions[direction]
    axios.post(`${root}/move`, {
        x: position.x + x,
        y: position.y + y
      })
      .then((response) => {
        if(response.data.valid) {
          setPosition({x: position.x + x, y: position.y + y})
        }
      })
  }

  const placeCar = (x, y) => {
    if(!carPlaced) {
      setCarPlaced(true)
      setPosition({x, y})
    }
  }

  const onKeyDown = e => {
    switch (e.which) {
      case 37: // Left
        rotateLeft()
        break
      case 38: // Up
        move()
        break
      case 39: // Right
        rotateRight()
        break
      default:
        break
    }
  }

  return {
    rotateLeft,
    rotateRight,
    move,
    degrees,
    position,
    placeCar,
    carPlaced,
    chooseDirection,
    initiatedGame,
    onKeyDown
  }
}

export const Controller = createContainer(useController)
export default useController

I'm trying to address the same issue.
What I'm currently doing is splitting my big hooks into smaller hooks, each with a single responsability.

For example, in your example, you could split your onKeyDown, rotateRight and rotateLeft functions into a hook which takes a state with values and setters as only parameter:

const useCarRotation = state => {
  const changeDirection = direction => {
    if(direction === -1) {
        state.setDirection(3)
    } else if(direction === 4) {
        state.setDirection(0)
    } else {
        state.setDirection(direction)
    }
  }

  const rotateLeft = () => {
    state.setDegrees(state.degrees - 90)
    changeDirection(state.direction - 1)
  }

  const rotateRight = () => {
    state.setDegrees(state.degrees + 90)
    changeDirection(state.direction + 1)
  }

  const onKeyDown = e => {
    switch (e.which) {
      case 37: // Left
        rotateLeft()
        break
      case 38: // Up
        move()
        break
      case 39: // Right
        rotateRight()
        break
      default:
        break
    }
  }

  return {onKeyDown, rotateLeft, rotateRight};
}

You can then use it in your main hook like this:

const useController = () => {
  const directions = [
    {x: 0, y: -1, degrees: 0}, // up
    {x: 1, y: 0, degrees: 90}, // right
    {x: 0, y: 1, degrees: 180}, // down
    {x: -1, y: 0, degrees: 270}, // left
  ]
  const [initiatedGame, setInitiatedGame] = useState(false)
  const [degrees, setDegrees] = useState(0)
  const [direction, setDirection] = useState(0)
  const [position, setPosition] = useState({'x': 0, 'y': 0})
  const [carPlaced, setCarPlaced] = useState(false)

  const chooseDirection = direction => {
    setDirection(direction)
    setDegrees(directions[direction].degrees)
    setInitiatedGame(true)
  }

  const move = () => {
    const root = process.env.REACT_APP_API_ROOT
    const {x, y} = directions[direction]
    axios.post(`${root}/move`, {
        x: position.x + x,
        y: position.y + y
      })
      .then((response) => {
        if(response.data.valid) {
          setPosition({x: position.x + x, y: position.y + y})
        }
      })
  }

  const placeCar = (x, y) => {
    if(!carPlaced) {
      setCarPlaced(true)
      setPosition({x, y})
    }
  }

  const {onKeyDown, rotateLeft, rotateRight } = useCarRotation({
    degrees,
    direction,
    setDegrees,
    setDirection,
  })

  return {
    rotateLeft,
    rotateRight,
    move,
    degrees,
    position,
    placeCar,
    carPlaced,
    chooseDirection,
    initiatedGame,
    onKeyDown
  }
}

To take your example further, I believe the chooseDirection and placeCar function could be placed in a hook responsible for initializing your values.

I currently keep all useState and useSelector calls into my main hook but I guess they could be put into a separate hook too if you have a lot of them.


Recurring dates within a range I'm using this code to find out the recurring dates, within a given range. Is this the correct way to find out the recurring dates within a range of dates?

between[] contains all the dates calculated within the range provided by the user. I only pasted the code that is relevant to the recurrence part only.

switch (true) {
                case (interval == 7):
                    //weekly
                    //Push in the selected dates in the selected array.
                    for (var i = 0; i < between.length; i += 7) {
                        selected.push(between[i]);

                    }
                break;
                case (interval == 30):
                    //Monthly
                    //Push in the selected dates in the selected array.
                    for (var i = 0; i < between.length; i += 30) {
                        selected.push(between[i]);

                    }
                    break;
                case (interval == 15):
                    //Bi-Monthly
                    //Push in the selected dates in the selected array.
                    for (var i = 1; i < between.length; i += 15) {
                        selected.push(between[i]);

                    }
                    break;
                case (interval == 0):
                    //One time event
                    //Push in the selected dates in the selected array.
                    for (var i = 0; i < 1; i++) {
                        selected.push(between[i]);

                    }
                    break;
                default:
                    return undefined;
            }

TL;DR; skip to if/else if/else statements

I can't say much because there is so little context, it looks like your code functions properly and efficiently,
Your indentation on the other hand is a little weird to me, I would do it like this with out so many tabs and new lines
switch (true) {
    case (interval == 7):
        //weekly
        //Push in the selected dates in the selected array.
        for (var i = 0; i < between.length; i += 7) {
            selected.push(between[i]);
        }
        break;
    case (interval == 30):
        //Monthly
        //Push in the selected dates in the selected array.
        for (var i = 0; i < between.length; i += 30) {
            selected.push(between[i]);
        }
        break;
    case (interval == 15):
        //Bi-Monthly
        //Push in the selected dates in the selected array.
        for (var i = 1; i < between.length; i += 15) {
            selected.push(between[i]);
        }
        break;
    case (interval == 0):
        //One time event
        //Push in the selected dates in the selected array.
        for (var i = 0; i < 1; i++) {
            selected.push(between[i]);
        }
        break;
    default:
        return undefined;
}

EDIT:
Mateon1's answer points out a break; that isn't indented correctly, I have fixed this in my code snippet.

after removing indentation I noticed that your one time event is smelly, you should just select the first date
    case (interval == 0):
        //One time event
        //Push in the selected dates in the selected array.
        selected.push(between[0]);
        break;

you use more overhead for a for loop than is necessary, don't get into the habit of using bigger things than you need.
After all that I also removed some unneeded comments they were just saying the same thing that the code was telling me outright.
Here is the Code that is left
switch (true) {
    case (interval == 7):
        //weekly
        for (var i = 0; i < between.length; i += 7) {
            selected.push(between[i]);
        }
        break;
    case (interval == 30):
        //Monthly
        for (var i = 0; i < between.length; i += 30) {
            selected.push(between[i]);
        }
        break;
    case (interval == 15):
        //Bi-Monthly
        for (var i = 1; i < between.length; i += 15) {
            selected.push(between[i]);
        }
        break;
    case (interval == 0):
        //One time event
        selected.push(between[0]);
        break;
    default:
        return undefined;
}

if/else if/else statements
After viewing the other answer, I think that this would be much better written as an if/else if/else statement like the following
if (interval == 7) {
    for (var i = 0; i < between.length; i += 7) {
        selected.push(between[i]);
    }
} else if (interval == 15) {
    for (var i = 0; i < between.length; i += 15) {
        selected.push(between[i]);
    }
} else if (interval == 30) {
    for (var i = 0; i < between.length; i += 30) {
        selected.push(between[i]);
    }
} else if (interval == 0) {
    selected.push(between[0]);
} else {
    return undefined;
}

and since I have gone this far I think that we could actually create a nice little function that does all of this when given an interval, I included the tests to make sure that the interval stays within the constraints but you could alter that to make it much more extendable.
function GetRecurringDates(interval) {
    if (interval != 0 || interval != 7 || interval != 15 || interval != 30) {
        return undefined;
    }
    for (var i = 0; i < between.length; i += interval) {
        selected.push(between[i]);
    }
}

From @Rolfl's answer I noticed that I too missed the 0-base array bug, so here is the function with the fix
function GetRecurringDates(interval) {
    if (interval != 0 || interval != 7 || interval != 15 || interval != 30) {
        return undefined;
    }
    for (var i = 0; i < between.length - 1; i += interval) {
        selected.push(between[i]);
    }
}

I wrote it like that so that the user doesn't have to know that the code is based on a 0-based array.

You might want to look into using JavaScript's Built in Date functionality.


Left rotation arrays algorithm This is my solution to the common 'Array Rotation' algorithm. As I am currently practicing for coding interviews, I want to make sure that my code in fact valid for coding interview scenarios. Would I lose points for using JavaScript's built-in shift and push array methods?

function main() {
var n_temp = readLine().split(' ');
var arrLength = parseInt(n_temp[0]);
var rotations = parseInt(n_temp[1]);
var frontNum;
a = readLine().split(' ');
a = a.map(Number);

for (var i = 0; i < rotations; i++) {
    frontNum = a.shift();
    a.push(frontNum);
}
 console.log(a.join(' '));
}

As Phrancis noted, there's no way for us to tell what an interviewer might want to learn from this. So this is just based on my impressions.

I don't know why this is wrapped in a main function. The code appears written to run once, and take its input from the command line. But in that case, there's no reason to wrap it at all, really.
Conversely, if you do wrap it in a function, something has to actually call that function, but I don't see that anywhere.

So the code looks like it's supposed to be a standalone example that you just run - but it won't do anything unless you call main() yourself. However, if the point is to make a function that can be used with other code, then:

Don't call your function main, because that makes no sense
Don't take over input and output. If I call a function to rotate an array, I'd expect to pass in the array to rotate, and get an array back. I don't expect to have to enter stuff manually, and just have the results printed back at me.

It'd make more sense if your code only handled the rotation itself, returned the rotated array, and left everything else to other code. Rotating the array is the core task here; not input handling or printing. That's just scaffolding to prove it works - you can still add all that, and make a self-contained little test script, but again, that's not the point of implementing array rotation.

So I'd focus on the rotation itself, put that in a function. Something like rotateArray(array, steps).

Now, for your actual implementation:

There's a straight-up bug: If the array is empty to begin with, shift() will return undefined which then that gets pushed back to the array n times. So for n rotations of an empty array, you'll get an array back with n values in it (which are all undefined).
I wonder what'll happen if I try to rotate an array, say, 9,007,199,254,740,990 times. I'm betting it'd take a while (code's \$O(n)\$). And in the end, the rotated array might be back where it started, making all those trillions of shifts and pushes pointless. So that seems like there are optimizations that can be made.
Using shift and push is a valid solution, just not a very efficient one. Rotating an array is the same as chopping it into two pieces and putting them together "backwards". That is what shift and push does too, they just can't do more than one element at a time.
Why can't I pass a negative rotation-count to rotate right instead?

Here's what I'd do:

function rotateArray(array, steps) {
  // check if there's even something to rotate
  if(array.length < 2) {
    return array.slice(0); // always return a copy
  }

  // get the number of actual rotations to perform
  var n = steps % array.length;

  // check if there's any need to rotate
  if(n === 0) {
    return array.slice(0); // always return a copy
  }

  // slice and concat
  if(n < 0) {
    return array.slice(n).concat(array.slice(0, array.length+n));
  } else {
    return array.slice(n).concat(array.slice(0, n));
  }
}

This code does a few simple checks to see if there's any reason to rotate the array at all, and, if there is, it does by concatenating two slices (code's \$O(1)\$), rather than going steps number of iterations with shift and push.
And in case there's no reason to rotate anything, it just returns a copy of the input array (so that the output is always a new array, even if nothing needed to be rotated).


Change Background Color of ASP:GridView Based On Text I am using the OnRowDataBound() event to change the Row.BackColor based on the text.  Now my issue is, this is extremely slow.  I have roughly 100 rows in the ASP:Gridview and only need to evaluate the first row or index[0].  What optimization techniques do you guru's have on how to speed this process up?

protected void GridView1_OnRowDataBound(object sender, GridViewRowEventArgs e)
{
    if (e.Row.RowType == DataControlRowType.DataRow)
    {
        TableCell cell = e.Row.Cells[1];
        string question = cell.Text;

        if (question == "View below to get data:" | question == "Check the state information below:" | question == "Check the state information below:")
        {
            e.Row.BackColor = System.Drawing.Color.Gray;
        }
    }
}

Generally, styling should be handled using client side methods such as using css/javascript so I would recommend you do some brushing up on that side. Never the less I was like you when I first started and just jumped straight into c# so here is a purely c# based solution :).

If it's only ever going to be the first row that needs to be shaded grey you can just reference that row when the binding in complete. The event you're currently subscribing to OnRowDataBound will fire for every row. There is another event that can be accessed once all rows are completed called OnDataBound.

First thing to do would be to register this event in the html markup of your gridview, using the same approach you used to register the OnRowDataBound event.

<asp:gridView ID="GridView1" runat="server" AutoGenerateColumns="false" OnDataBound="GridView1_OnDataBound"></asp:gridView>

Then you can code up the event in your c#.

protected void GridView1_OnDataBound(object sender, EventArgs e)
{
    if(GridView1.Rows.Count > 0) //Check there are rows bound
    {
        var Row = GridView1.Rows[0];// Get The First Row - you may have a header row. If this is the case change 0's to 1's!

        Row.BackColor = System.Drawing.Color.Gray;// Assign the back color
    }
}