Mars Rover Python exercise I have given the Mars Rover challenge a go in Python.

(edit) Here is the challenge, for those unfamiliar:

A rover’s position and location is represented by a combination of x and y co-ordinates and a letter representing one of the four cardinal compass points. The plateau is divided up into a grid to simplify navigation. An example position might be 0, 0, N, which means the rover is in the bottom left corner and facing North.

In order to control a rover , NASA sends a simple string of letters. The possible letters are ‘L’, ‘R’ and ‘M’. ‘L’ and ‘R’ makes the rover spin 90 degrees left or right respectively, without moving from its current spot. ‘M’ means move forward one grid point, and maintain the same heading.

Test Input:

5 5

1 2 N

LMLMLMLMM

3 3 E

MMRMMRMRRM

Expected Output:

1 3 N

5 1 E

I am still relatively new to Python so know this is a bit basic - but I wondered if I could get some general feedback on my code for best coding practice?

class MarsRover():

    RIGHT_ROTATE = {
        'N':'E',
        'E':'S',
        'S':'W',
        'W':'N'
    }

    LEFT_ROTATE = {
        'N':'W',
        'W':'S',
        'S':'E',
        'E':'N'
    }

    def __init__(self, X, Y, direction):

        self.X = X
        self.Y = Y
        self.direction = direction

    def rotate_right(self):

        self.direction = RIGHT_ROTATE[self.direction]

    def rotate_left(self):

        self.direction = LEFT_ROTATE[self.direction]

    def move(self):

        if self.direction == 'N':
            self.Y += 1
        elif self.direction == 'E':
            self.X += 1
        elif self.direction == 'S':
            self.Y -= 1
        elif self.direction == 'W':
            self.X -= 1

    def __str__(self):

        return str(self.X) + " " + str(self.Y) + " " + self.direction

    @classmethod
    def get_rover_position(self):

        position = input("Position:")
        X = int(position[0])
        Y = int(position[2])
        direction = position[4]

class Plateau():

    def __init__(self, height, width):

        self.height = height
        self.width = width

    @classmethod
    def get_plateau_height(self):

        plateau_size = input("Plateau size:")
        height = int(plateau_size[0])
        width = int(plateau_size[2])

def main():

    plateau = Plateau.get_plateau_height()

    rover = MarsRover.get_rover_position()

    current_command = 0
    command = input("Please input directions for rover.")
    command_length = len(command)

    while current_command <= command_length - 1:

        if command[current_command] == 'L':
            rover.rotate_left()
            current_command += 1
        elif command[current_command] == 'R':
            rover.rotate_right()
            current_command += 1
        elif command[current_command] == 'M':
            rover.move()
            current_command += 1

    result = str(rover)
    print(result)

if __name__ == '__main__':

    main()

I am also getting the following error when I run the code:

     94         if command[current_command] == 'L':
---> 95             rover.rotate_left()
     96             current_command += 1
     97         elif command[current_command] == 'R':

AttributeError: 'NoneType' object has no attribute 'rotate_left'

Is anyone able to shed some light on why the 'rover' object is of NoneType?

Thanks in advance.

get_rover_position does not return anything, and therefore the variable is None. You need to instantiate MarsRover in that class method with the provided input and return the newly created instance. The same applies also to get_plateau_height.


Underscore template conditional can be reduced I have 11 data items and would like to print them as a group. Each group will have header and at the end of the group I need to print continue label.

For that, I use this Underscore template and multiple conditions.  I'd like to make this more compact.

If I understand your code correctly, it looks like you want to render tables with 3 records each, with the #header template for the top of each table and the #closer template for the bottom of each table, except for the last one, which should end with #footer instead of #closer.

First, the little things:

I think you have some unused variables in your code: It looks like dLength and done are never used.
limit is a magic number and seems out of place in your template. If I were you I would pass that number to the template function along with your other data.
You render
 several times, but element IDs must be unique in a document. We shouldn't re-use an ID.
For the last table, the
 is never rendered, so the above div is never closed! Fixing this is discussed below.
You're compiling templates inside of templates, which means that every time the outer template is rendered, you recompile the inner template, which is unnecessary and negates most of Underscore.js' rendering speed advantage. Fixing this is discussed toward the end.

Deal with groups of items, not index math

Now, on to the meat of the question. I think the biggest code smell here is the modulo arithmetic, i.e. <% if(index % limit == 0 ) { %> et al. This works, but it's hard to read. Instead, we can make the code match our intent: We want tables with 3 records each, so let our code deal with the data in groups of 3. This saves us from having to do arithmetic on their indexes—or worring about their indexes at all. Changing only the #recourseParameterTemp template, we have this:

Here's a fiddle of it in action.

I think further improvements are possible, however. For one thing, why is
 inside a sub-template, but
 is inside the main template? Not only is this an odd inconsistency, it actually introduced the missing
 bug mentioned above. We can fix this by moving Continue... and
 into the #closer template, and also adding
 to the #footer template (see this fiddle), but the bug points to a more general code smell: Templates with no closing tags, or no opening tags. If a template closes all of the tags it opens, it's much easier to notice problems like our missing
.

The single responsibility principle

We usually talk about the single responsibility principle as it relates to object-oriented programming, but it's very useful here, too: Our templates are meant to isolate blocks of code that are related, and a great template has a single responsibility. Our #recourseParameterTemp template has a bunch of responsibilities—it iterates through the data, it decides whether or not to render each
, and it decides what kind of footer render. That's too much! Using this principle, a number of possible improvements reveal themselves.

In my changes, I introduced an additional loop, i.e. I put a while around the _.each. This makes it a little harder to read, but also reminds us that it's often good to extract the inside of a loop into a sub-template that has a single responsibility. Extracting the inside of the while loop into a template called #groupTmpl (along with the #closer/#footer changes mentioned above) gives us this:

Our intent has become clearer. We see that #recourseParameterTmpl's single responsibility is to render a sub-template (#groupTmpl) once for each group, and that's all it does. This makes it easier to see the intent of the rest of the code, now in #groupTmpl. It also makes it easier to see that #groupTmpl still has multiple responsibilities: It renders the #header sub-template, it loops through each item in group, and it decides what kind of footer to render.

(You probably also noticed that I changed #recourseParamaterTemp to #recourseParameterTmpl; I did this just because "temporary" is frequently shortened to "temp" in programming; "tmpl" is less ambiguous.)

One footer template

We could extract the inside of the _.each loop into a sub-template, but I think we can agree that that's probably overkill. The main thing we can improve here is that #groupTmpl is deciding what kind of footer to render, but that shouldn't be its responsibility. Instead, we should just render one template, and let that template decide what the footer should look like. Looking at our existing #closer and #footer templates, we can see that they're almost identical, except for the contents of . It makes sense to consolidate these into one template. Removing #closer entirely, #footer and #groupTmpl now look like this (here's the fiddle):

Now #groupTmpl's intent is very clear, and we don't have any duplicated footer code.

Self-contained templates close their own tags

This is looking pretty good, but we still have the problem I mentioned above: #header opens tags that it doesn't close, and #footer closes tags that it doesn't open.

An alternative is to combine those templates into a single #wrapper template. Since Underscore.js templates don't have good support for "nested" templates, this gets a little hairy, unless we do one simple thing—which we ought to do anyway: Initialize the sub-template functions outside of the template itself. Instead of doing this:

<%= _.template($('#groupTmpl').html())( ... ) %>

We should do this, in our JavaScript:

var groupTmpl = _.template( $('#groupTmpl').html() );

...for each of our templates, and then pass the compiled template functions as locals when we use a template, which will allow us to do this in a template:

<%= groupTmpl( ... ) %>

In addition to looking cleaner, this avoids recompiling the sub-template every time the (outer) template is compiled, as mentioned above.

Along with this, instead of passing around a bunch of variables (recourseParameter, parameterCode, etc.) among our templates, it's probably a good idea to use the variable option to _.template. That is, when we call var tmpl = _.template(html, { variable: 'locals' }), then render it with tmpl({ foo: "bar" }), foo will be available as locals.foo inside the template, instead of just foo. While this seems less convenient, it lets us just pass locals to sub-templates instead of specifying the whole object. Instead of passing { variable: 'locals' } every time we compile a template, though, we can set it globally using _.templateSettings.variable = 'locals'.

Finally...

...we end up with templates that look like this:

And JavaScript that looks like this:

_.templateSettings.variable = "locals";

var tmpl = _.template( $('#recourseParameterTmpl').html() ),
    locals = {
        limit: 3,
        parameterCode: "foo",
        recourseParameter: [
            { description: "Foo 1" },
            // ...
        ]
    },
    renderedTmpl
;

locals.wrapperTmpl = _.template( $('#wrapperTmpl').html() );
locals.groupTmpl   = _.template( $('#groupTmpl').html() );
// You could also automate the above with jQuery, e.g.:
//
//     $("[type='text/template']").each(function() {
//       var el = $(this), id = el.attr('id');
//       locals[id] = _.template( el.html() );
//     });

renderedTmpl = tmpl(locals)

$('body').append(renderedTmpl)

Here's the final fiddle. I hope that's helpful!


Create Dummy Files for Testing I have two different folders.

In one folder, I have files that will be exported to a remote site via FTP.

In a second folder, I have .dat files that will be processed by a ftp client program that I have. Each .dat file contains basic information that my ftp program needs to process a request.

I needed to create a bunch of dummy files and dummy requests to test my ftp program. I decided to create these dummy files with Powershell.

I know almost nothing about Powershell, so even the most basic advice would be helpful.

This is all inside a Powershell script that can be run from the Powershell ISE.

$export_src_base = "D:\csv_exports"
$export_src_dir = "$export_src_base\test"
$queue_dir = "D:\FTP\Queue"

# Create the export source directory if it does not exist.
if (-not(Test-Path $export_src_dir)){
    New-Item -ItemType directory -Path $export_src_dir
}

for ($i=1; $i -le 100; $i++){
    # Create 100 dummy files if they do not exist.
    $file = "$i.txt"
    $filepath = "$export_src_dir\$file"

    if (-not(Test-Path $filepath)){
        New-Item -ItemType file -Path $filepath -Value $i
    }

    # Create 100 dummy requests if they do not exist.
    $queue_file = "$i.dat"
    $queue_filepath = "$queue_dir\$queue_file"

$value = @"
[Item]
SrcFileName=$filepath
DestFileName=$file
FTPSite=Site1
DestPath=Temp\Testing
Sent=0
ErrorCount=0
LastTriedTime=
"@

    if (-not(Test-Path $queue_filepath)){
        New-Item -ItemType file -Path $queue_filepath -Value $value
    }

}

Pretty hard to pick on this, looks pretty nice already!

You have this duplicated pattern though:

if (-not(Test-Path $filepath)){
    New-Item -ItemType file -Path $filepath -Value $i
}
# ...
if (-not(Test-Path $queue_filepath)){
    New-Item -ItemType file -Path $queue_filepath -Value $value
}

You could create a function that takes $path and $value and use it twice instead of duplicating code.

And the comments are pointless, your code already speaks for itself (nice!), so just delete them!

Other than these nitpicks, I don't see a better way to do what you want.


Merging sorted arrays in Python def merge_arrays(list1, list2):
  len_list1 = len(list1); len_list2 = len(list2)
  merge_len = len_list1 + len_list2
  merge_list = []

  l1_ptr = 0
  l2_ptr = 0
  # import pdb; pdb.set_trace()
  while(l1_ptr <= len_list1-1 and l2_ptr <= len_list2-1):

    if (list1[l1_ptr] <= list2[l2_ptr]):
      merge_list.append(list1[l1_ptr])
      l1_ptr += 1

    elif (list1[l1_ptr] > list2[l2_ptr]):
      merge_list.append(list2[l2_ptr])
      l2_ptr += 1

  if l1_ptr > len_list1-1: #list1 exhausted
    for item in list2[l2_ptr:]:
      merge_list.append(item)
  else:
    for item in list1[l1_ptr:]:
      merge_list.append(item)

  return merge_list

I am trying to merge sorted arrays in python. How can I improve this? Honestly it looks like I've written this in C and not in Python

Use a 4-space indent.
Don't repeatedly subtract 1 from the same unchanging value.
Simplify the compare conditional: just use else.
Take advantage of list.extend().
Drop the wrap-up conditionals: they aren't actually needed. Code like zs.extend(xs[xi:]) will work fine even if xi exceeds the list
bounds.
Shorten the variable names to lighten the code weight and increase readability. There's no loss of meaning here, because all of the short names are quite conventional and make sense in a generic function like this.
def merge_arrays(xs, ys):
    # Setup.
    xmax = len(xs) - 1
    ymax = len(ys) - 1
    xi = 0
    yi = 0
    zs = []

    # Compare and merge.
    while xi <= xmax and yi <= ymax:
        if xs[xi] <= ys[yi]:
            zs.append(xs[xi])
            xi += 1
        else:
            zs.append(ys[yi])
            yi += 1

    # Merge any remainders and return.
    zs.extend(ys[yi:])
    zs.extend(xs[xi:])
    return zs

Last night I wrote an iterator-base solution but somehow forgot that next()
supports a handy default argument: the code was awkward and Maarten
Fabré did a nicer
implementation. But if your willing to use
more_itertools.peekable()
you can achieve a simple, readable implementation. Thanks to
superb-rain
for an idea in the comments that helped me simplify further.
from more_itertools import peekable

def merge(xs, ys):
    xit = peekable(xs)
    yit = peekable(ys)
    while xit and yit:
        it = xit if xit.peek() <= yit.peek() else yit
        yield next(it)
    yield from (xit or yit)


A series of yes-no questions to predict yes-no outcomes I have been writing R scripts for about a week. I have been writing code for 20 years. I have a series of yes-no questions that I hope to predict yes-no outcomes so I know the R-squared will be terrible but as long as the variable is significant I am fine.

Since I am creating a number with my yes-no questions the order makes a difference and I have 14 questions which I plan to turn into 8-bit numbers (121,080,960 permutations). This is taking about 88 hours on my desktop. I am looking for anyplace to optimize my code. I have read about Multi core/thread SNOW but that is beyond me right now without some help. I also know I can compile functions but my code is short and only see one place to do so.

Please review my code, any help would be appreciated.

library(data.table)
ffName = "Perms8bit.csv"
# Read table of 1s and 0s
Bin_Matrix <-read.table("LC_FR_F17BIN.csv",header=TRUE,sep=",")

Perm_Best <- 0
holdR  <- 0
s<- Sys.time()
cnt <-0

# Read the file 121,080,960 permutations to test
# Originally read the file in blocks but benchmark testing
rawData <- fread(ffName, sep=",")

   for (cnt in 1:121080960){
        # create a table result x binary number
        Bin_num <- data.table(result = c(Bin_Matrix[,15]),numb = c(2^0* (Bin_Matrix [,as.numeric(rawData[cnt,1])]) + 2^1*(Bin_Matrix [,as.numeric(rawData[cnt,2])]) +
2^2*(Bin_Matrix [,as.numeric(rawData[cnt,3])]) + 2^3*(Bin_Matrix [,as.numeric(rawData[cnt,4])]) +
2^4*(Bin_Matrix [,as.numeric(rawData[cnt,5])]) + 2^5*(Bin_Matrix [,as.numeric(rawData[cnt,6])]) +
2^6*(Bin_Matrix [,as.numeric(rawData[cnt,7])]) + 2^7*(Bin_Matrix [,as.numeric(rawData[cnt,8])])))

    #linear regession
    linearMod <- lm(result ~ numb, data = Bin_num)
    ModSum <- summary(linearMod)
    theR <- ModSum$r.squared

    # Find the top R squared, result is 1s and 0s so R squared will not be high
    if (theR > .0175) {
        holdR = theR
        Perm_Best <- data.table(cnt = cnt,R = holdR,rawData[cnt,])

         #append a file with best fits
         write(toString(Perm_Best,nrows= 1),file = "RSQ.txt", append = TRUE)
      }

   }
print (Sys.time()-s) #benchmark
print(holdR)         # just to let me know its done
print (Perm_Best)

Bin_Matrix is a file with around 2,000 rows, header, and a bunch of 1s and 0s in csv form.  RawData is a  table with 121 million rows with the different permutations (examples:

1,2,3,4,5,6,7,8
1,2,3,4,5,6,7,9
...
14,1,2,3,4,5,6,7
14,1,2,3,4,5,6,8

Unfortunately, without the two csv files, we are not able to test your code... Maybe if you could show us the profiler output (see the example in ?summaryRprof) on a few iterations, it would help. What I suspect is that it will show most of the time is wasted either in the lm fit, or in the write call. Here are suggestions to improve on both as potential issues:

Alternative to lm

Since you are doing simple linear regression, i.e. regression with a single variable, there is a closed-form solution for the R-squared and it is fast to compute with R's built-ins:

RSQ1 <- function(x, y) {
   mx  <- mean(x)
   my  <- mean(y)
   mxy <- mean(x * y)
   mxx <- mean(x * x)
   myy <- mean(y * y)
  (mxy - mx * my)^2 / (mxx - mx^2) / (myy - my^2)
}

Compare this with your implementation

RSQ2 <- function(z) {
  linearMod <- lm(y ~ x, data = z)
  ModSum <- summary(linearMod)
  ModSum$r.squared
}

Here I test both functions assuming your Bin_Matrix has 1000 rows. Your mileage may differ depending on how many observations you really have, something your question does not yet provide:

n <- 1000
z <- data.frame(x = runif(n), y = runif(n))

RSQ1(z$x, z$y)
# [1] 0.0007529693
RSQ2(z)
# [1] 0.0007529693

library(microbenchmark)
microbenchmark(RSQ1(z$x, z$y), RSQ2(z))
# Unit: microseconds
#            expr      min        lq      mean    median        uq      max neval
#  RSQ1(z$x, z$y)  111.707  120.8345  146.3035  126.5945  143.2855  689.371   100
#         RSQ2(z) 2531.381 2554.2105 2883.7659 2626.9020 2966.8550 5103.368   100

Inefficient write

The way you are using write is inefficient in that every time it is called, it must open the file, move the filehandle all the way to the end, write a line, then close the file. If your R squared is improved upon very often, this could start adding up. You would gain by doing the following.

Before your for loop, open a filehandle: fh <- open("RSQ.txt")
Make write append to the filehandle: write(toString(Perm_Best, nrows= 1), file = fh)  (notice, there is no need for append = TRUE anymore)
After the for loop, close the file handle: close(fh)

Can you please give these suggestions a try and let me know if it helps? If it doesn't, maybe provide the output of the profiler? I hope it helps, good luck!

P.S.: Regarding coding style, you might find this approach for computation  Bin_num a little easier on the eye; it should not have a significant speed impact:

idx <- as.numeric(rawData[cnt, 1:8])
bm  <- Bin_Matrix[, idx]
Bin_num <- data.table(result = c(Bin_Matrix[, 15]),
                      numb   = colSums(2^(0:7) * t(bm)))


Improvement to Luhn-Checksum Algorithm in Java I just made a small Luhn Checksum validator
Description of Luhn Checksum
Every number in the Integer gets added individually together, but every second digit gets instead doubled and added. If the doubled number is over ten each individual digit gets added.
It works, but is there a way to make it more elegant and maybe more efficient, while maintaining readability?
The while loop seems a bit ugly to me, and I'm not completely satisfied with the name getLuhn but I don't know how else to name it.
public class Luhn {
    public static void main(String[] args) {
        System.out.println(validate(1762483));
    }

    public static boolean validate(int id){
        int totalSum = 0;
        while(id>0){
            totalSum += id%10;
            id /= 10;
            if(id>0){
                totalSum += getLuhn(id%10);
                id /= 10;
            }
        }
        return (totalSum%10 == 0);
    }

    private static int getLuhn(int id){
        id *= 2;
        return id%10 + id/10;
    }
}

Every comment is appreciated <3
From cleaner code, over more idiomatic Java, to improvements in performance.

There is not much to improve in your code, it's compact and efficient. These are few suggestions:
Input validation
When the input number is negative the result is always true. To avoid confusion you can launch an exception.
public static boolean validate(int id) {
        if (id < 0) {
            throw new IllegalArgumentException("Input cannot be negative.");
        }
        // ..
}

Clarity
The Luhn Checksum algorithm is described very well on Wikipedia and by you on your question, but your implementation is not easy to follow. For example:
totalSum += id%10;

Here the last digit of id is added to totalSum. Adding a method (with the explanation of the operation in the name) makes it more readable:
totalSum += getRightMostDigit(id);

Same for:
id /= 10;

This operation removes the last digit of id, which can be changed to:
id = dropRightMostDigit(id);

I would also change the input variable name from id to number, but this is personal taste.
Perfomance
It's hard to improve performance and keep readability for your method. The only change I would suggest is to replace the getLuhn method with a static array.
This change makes it two times faster on my machine and gets rid of the additional method.
Code refactored
public static boolean validate(int number) {
    if (number < 0) {
        throw new IllegalArgumentException("Input cannot be negative.");
    }
    // Array containing:
    // - for index in [0,4]: the double of the index value
    // - for index in [5,9]: the sum of the digits of the doubled index value. E.g. index = 6 -> 6*2 = 12 -> 1+2 = 3
    int[] luhn = new int[] { 0, 2, 4, 6, 8, 1, 3, 5, 7, 9 };

    int totalSum = 0;
    while (number > 0) {
        totalSum += getRightMostDigit(number);
        number = dropRightMostDigit(number);
        if (number > 0) {
            totalSum += luhn[getRightMostDigit(number)];
            number = dropRightMostDigit(number);
        }
    }
    return totalSum % 10 == 0;
}

private static int getRightMostDigit(int number) {
    return number % 10;
}

private static int dropRightMostDigit(int number) {
    return number / 10;
}

Personal opinion
Many implementations of the Luhn Checksum accept a String as input, so they can be used to validate credit cards or simply to operate on numbers bigger than an int. What is the use case of your algorithm?
The purpose of your implementation can also be included as a comment, it will help others to understand whether they need it or not.


Fetching a number of data tables from a stored procedure Within a method, this code is calling a SPROC that is doing a 'select *' from 9 different views. This method then goes on to access the returned DataTableCollection array by the index specified here (0-9). If the order of the stored procedure select statements ever changed, this would also need changing round here.

This seriously looks dodgy to me. Does anyone have any opinions?

if (_dataSet != null)
            {
                _applicationContext = new ApplicationContext();

                //build the object for System Mode.
                _applicationContext.SystemMode = BuildSystemMode(CheckDataTableScope(_dataSet.Tables,0));

                //build the object for data parameters.
                _dataParameters = BuildDataParameters(CheckDataTableScope(_dataSet.Tables, 1));

                //build the object for data columns.
                _dataColumns = BuildDataColumns(CheckDataTableScope(_dataSet.Tables, 2));

                //build the object for data command.
                _applicationContext.DataCommand = _dataCommand = BuildDataCommand(CheckDataTableScope(_dataSet.Tables, 3));

                //build the object for screens.
                _screenConfig = BuildScreen(CheckDataTableScope(_dataSet.Tables, 4));

                //build the object for user menus.
                _applicationContext.UserMenus = BuildUserMenu(CheckDataTableScope(_dataSet.Tables, 5));

                //build the object for application menus.
                _applicationContext.ApplicationMenus = BuildApplicationMenus(CheckDataTableScope(_dataSet.Tables, 6));

                //build the object for system messages.
                _applicationContext.SystemMessages = BuildSystemMessages(CheckDataTableScope(_dataSet.Tables, 7));

                //build the object for widget configuration.
                _applicationContext.WidgetConfigs = BuildWidgetConfig(CheckDataTableScope(_dataSet.Tables, 8));
            }

Couple of things.

I would encapsulate the "magic" numbers into an enum

public enum DataSetTableInfo
{
    System = 0,
    Parameters = 1,
    DataColumns = 2
    //etc
}

then change the method Sig of CheckDataTableScope to take the enum instead of an int.

If CheckDataTableScope method isn't public or someplace else you are using it without _dataSet I would drop passing _dataSet into it.  Just makes method sig more chatty than needs to be. If you can pass others values in for _dataSet then I would still probably lean towards two methods or have a default value for parameter.  Something like

private DataTable CheckDataTableScope(DataSetTableInfo dataSetTableInfo)
{
    return CheckDataTableScope(_dataSet, dataSetTableInfo);
}

public DataTable CheckDataTableScope(DataSet ds, DataSetTableInfo dataSetTableInfo)
{
}

Then your calls to the fill in the data change to be like

 if (_dataSet != null)
 {
     _applicationContext = new ApplicationContext();

     //build the object for System Mode.
     _applicationContext.SystemMode = BuildSystemMode(CheckDataTableScope(DataSetTableInfo.System));

     //build the object for data parameters.
     _dataParameters = BuildDataParameters(CheckDataTableScope(DataSetTableInfo.Parameters));

     //build the object for data columns.
     _dataColumns = BuildDataColumns(CheckDataTableScope(DataSetTableInfo.DataColumns));

To me this seems more readable, not in love with the method name of CheckDataTableScope but I don't know what that method actually does.  Plus if the stored procedure changes you can just update the enum to add more tables or change it to match the new table order and not touch any other piece of code.

You can cast the enum to the int of the DataTable in CheckDataTableScope

var tableIndex = (int) dataSetTableInfo;

if (tableIndex <= ds.Tables.Count)
{
   var dt = ds.Tables[tableIndex];
}


Entropy calculation function in Haskell I've written a function in Haskell for calculating the entropy of a collection.  I'd like feedback on how the function could be rewritten to be more flexible/reusable, and also how to profile the function and how it could be tuned and/or modified for better performance.

import Data.List (foldl1')

entropy :: [Int] -> Int -> Int -> Double
entropy itemFrequencies totalElements logarithmicBase =
  -(foldl1' (+) $ map (\p -> p * (logBase b p)) probabilities)
    where
      is = map fromIntegral itemFrequencies
      l  = fromIntegral totalElements
      b  = fromIntegral logarithmicBase
      probabilities = map (\i -> i / l) $ is

For some background, the Entropy calculation is a core function used in building decision trees.  For more complex datasets and decision trees, this function would get called very often.  I'm working on a sequential implementation of the ID3 algorithm, which this entropy function is a part of, that I will later make parallel/concurrent as a separate exercise, and then I will eventually also write implementations of ID3's descendants: C4.5 and C5.0.

Please respect my desire to struggle on my own with the concurrency/parallelism aspect of rewriting this code, I'm only interested in any sequential improvements I could make for performance and any refactoring that could be done to make this code easier to maintain and reuse.

General Code Review

First, your function and my proposed changes (entropy') side-by-side.

import Data.List (foldl1', foldl')

entropy :: [Int] -> Int -> Int -> Double
entropy itemFrequencies totalElements logarithmicBase =
  -(foldl1' (+) $ map (\p -> p * (logBase b p)) probabilities)
    where
      is = map fromIntegral itemFrequencies
      l  = fromIntegral totalElements
      b  = fromIntegral logarithmicBase
      probabilities = map (\i -> i / l) $ is

entropy' :: (Foldable f, Integral a, Floating b) => a -> a -> f a -> b
entropy' totalElems base =
  negate . foldl' (\ent f2 -> ent + freqEntropy f2) 0
  where
    freqEntropy f = let p = (fromIntegral f) / l
                    in  p * logBase b p
    l             = fromIntegral totalElems
    b             = fromIntegral base

My comments, in an arbitrary order:

Why are you using foldl1'? It makes sense to use foldl' from a performance perspective, but it isn't clear to me why you require a nonempty list. Perhaps use a Maybe to encapsulate this possibility of failure, or outline why you expect a nonempty list in a comment. It's a good idea to keep tabs on where your partial functions are to avoid surprises at runtime. My function just returns 0 for a null Foldable.
Your types can be generalized more than Int and Double, if you want this to be more flexible/reusable. What I did to find these types was track down what functions you were using and figure out what their types were (which were more general than Int or Double or []). Then I resolved the overall function to its most general type. Whether this is necessary or useful depends on your application. I think the most useful generalization here is to Foldable in case you wanted to calculate entropy of things that were not lists.
When I changed to generalize to Foldable, I rolled all of the maps into the foldl'. This may be more performant if the compiler doesn't combine maps, but it's also a tad bit more complicated to understand.
I moved the itemFrequencies to be the last argument so I could write the function pointfree. Pointfree is kind of cute, but if you think it's more readable you can change the order and/or put the explicit itemFrequencies back in.
I added an explicit call to negate (I didn't get what was going on at first with the -(foldl1' ...)).
I shortened the names so that things are not overwhelmingly long or verbose. This is just my personal taste. I think if you're going to have descriptively long variable names, you shouldn't skimp on the description for your temporary variables. I think b is OK since it's a common variable for base, but I would recommend using something like len instead of l and itemFreqs' instead of is.

Performance and Profiling

I can't really help you on this front. GHC does have a profiler, which might be useful if you want to do some serious profiling.


Convert MS SQL datetime to Oracle varchar(32) interval I have a query against an MS SQL database that takes the resulting dataset and stores it in an Oracle database table which then gets joined to another Oracle table using the column LABEL_YYYY_MM_DD_HH24_MI varchar(32).  I have converted the MS SQL datetime column q.InTimeStart as per below.

Is there a more efficient way of writing this line which takes datetime and finds beginning of 15min interval and stores as varchar(32)?

left(convert(varchar(32),convert(smalldatetime,floor(cast(q.InTimeStart as float)\*(24/.25))/(24/.25)),120),
len(convert(varchar(32),convert(smalldatetime,floor(cast(q.InTimeStart as float)*(24/.25))/(24/.25)),120)) -3) as LABEL_YYYY_MM_DD_HH24_MI,

As my data for LABEL_YYY_MM_DD_HH24_MI will always be the same length I can simply write:

left(convert(varchar(32),convert(smalldatetime,floor(cast(q.InTimeStart as float)*(24/.25))/(24/.25)),120),16) as LABEL_YYYY_MM_DD_HH24_MI,

If the results were of a dynamic length and we needed to remove the last three we would need to use len(x,-3):

left(convert(varchar(32),convert(smalldatetime,floor(cast(q.InTimeStart as float)*(24/.25))/(24/.25)),120),
 len(convert(varchar(32),convert(smalldatetime,floor(cast(q.InTimeStart as float)*(24/.25))/(24/.25)),120)) -3) as LABEL_YYYY_MM_DD_HH24_MI,


Do these unit tests cover my method under test? I've written an extension method to truncate strings:

/// <summary>
/// Returns the string, or a substring of the string with a length of <paramref name="maxLength"/> if the string is longer than maxLength.
/// </summary>
/// <param name="maxLength">The maximum length of the string to return.</param>
/// <exception cref="ArgumentException">If <paramref name="maxLength"/> is smaller than zero, an ArgumentException is raised.</exception>
/// <![CDATA[ Documentation:
/// If the string is null or an empty string, the input is returned.
/// If the string is shorter than or equal to maxLength, the input is returned.
/// If the string is longer than maxLength, the first N (where N=maxLength) characters of the string are returned.
/// ]]>
public static String Truncate(this String input, int maxLength)
{
    if (maxLength < 0)
    {
        throw new ArgumentException("Must be >= 0", "maxLength");
    }

    if (String.IsNullOrEmpty(input) || input.Length <= maxLength)
    {
        return input;
    }

    return input.Substring(0, maxLength);
}

I have written these test cases:

[TestFixture]
public class TruncateTests
{
    String longString = "ABC";
    String shortString = "A";
    String nullOrEmptyString = null;
    String output = "";

    [Test]
    public void LessThanOrEqual()
    {
        // If input is shorter than or equal to maxLength, the input is returned.
        output = longString.Truncate(longString.Length + 5);
        Assert.AreEqual(longString, output);

        output = longString.Truncate(longString.Length);
        Assert.AreEqual(longString, output);
    }

    [Test]
    public void GreaterThan()
    {
        // If input is longer than maxLength, the first N (where N=maxLength) characters of input are returned.
        output = longString.Truncate(1);
        Assert.AreEqual(shortString, output);
    }

    [Test]
    public void NullOrEmpty()
    {
        // If input is null or an empty string, input is returned.
        output = nullOrEmptyString.Truncate(42);
        Assert.AreEqual(output, nullOrEmptyString);

        nullOrEmptyString = "";
        output = nullOrEmptyString.Truncate(42);
        Assert.AreEqual(output, nullOrEmptyString);
    }

    [ExpectedException(typeof(ArgumentException))]
    [Test]
    public void MaxLengthException()
    {
        // If maxLength is smaller than zero, an ArgumentException is raised.
        "".Truncate(-1);
    }

    //http://www.fileformat.info/info/unicode/char/1f3bc/index.htm
    string multiByteString = "\x1F3BC";

    [Test]
    public void MultiByte()
    {
        Assert.IsTrue(multiByteString.Length == 2);

        output = multiByteString.Truncate(1);

        Assert.IsTrue(output.Length == 1);
    }
}

Now how can I confirm that:

The method Truncate() does what it is supposed to do?
I have written test code to test al promises made?
This test code follows practices and guidelines valid for unit testing?

I'm especially curious about the last one. I've written a few tests in my time, but I'm never sure whether I'm testing enough and not too much at the same time. Can anyone shed a general light on this and maybe point me towards invaluable resources about unit testing?

About the function comment:

You're over-complicating the summary. As of how I understand the function does Get the first <paramref="maxLength"> characters as a summary. Special conditions are mentioned bellow. Those should be simplified as well.
For  maxLength I'd check for min/max integer as well as for -1/0/1 (does the function behave correctly at the limits?)
Check input for null and additionally for those combinations:

input.length < maxLength
input.length = maxLength
input.length > maxLength

You're not testing your code when testing for MB-functionality.
Test one thing at a time. If NullOrEmpty fails, how to you know which of those assertions failed? Does it fail if input is empty or if it is null?\
Remove comments, make the test-names more clear instead. Tests are simple and easy to understand (at least they should be :)). Safe your time of writing comments and improve the test instead.

For most of the functions you're just repeating the behavior asserted. However this is already written down by the assertion itself. The comment just duplicates the code.
In my opinion, test names should be speaking of what they do. NullOrEmpty doesn't state anything about what actually happens. NullInputReturnsNull and EmptyInputReturnsEmptyString do however. Furthermore speaking test-names can be used as a documentation (i.e testdox).

When are you done? Never. Eventually you (or someone else) will find bugs, misbehavior, ... (e.g. during integration). It's more important to keep the tests up to date at those times as well.

Update

On 2. I'm testing for MAX_INT because your promise is this functions accepts integer values between [0, MAX_INT]. Imagine at some time there is a maxLength + 1 statement for some reason. As a rule of thumb: always test the border values of parameters. Further reading: equivalence partitioning and boundary value analysis

On 3. That'd be four tests. You should split those.

On 4. The point is your are testing the c# library, not your code. You don't have any code dedicated for MB handling. What you are actually doing is testing the Substring method and Length property for MB handling.


Speeding up Codechef FROGV This is my solution to the FROGV problem on Codechef:

N,K,P = map(int,raw_input().split(' ')) # N: No of Inputs, K: highest possible jump, P: no of test cases
L = map(int,raw_input().split(' ')) # Takes in the N space separated inputs
Li = sorted(L)
Breaks = [] #Records the starting point a of pair a,b such that the jump from a to b is larger than k
for i in xrange(0,N-1):
    if Li[i+1]-Li[i] > K: # The difference basically gives the jump size
        Breaks.append(Li[i])
for i in xrange(0,P): # Checking over each pair
    a,b = map(int,raw_input().split(' '))
    a,b = a-1,b-1 # The pairs use numbering from 1 to n while python numbering is from 0 to n-1
    for Break_Pt in Breaks:
        if (abs(L[a]-L[b])>K) and (((L[a]<=Break_Pt and Break_Pt<L[b]) or (L[b]<=Break_Pt and Break_Pt<L[a]))): # If the jump from a to be is greater than K then it checks whether there is a break pt in between a and b.
            print 'No'
            break
    else:
        print 'Yes'

Codechef tells me that this code exceeds the time limit, but I couldn't find any way to improve the speed.

You said that only checked your code against the example on Codechef. But the example only has 5 frogs and 3 pairs! Running this tiny example won't tell you how your code will perform on the worst case, where there might be 100,000 frogs and 100,000 pairs.

So the first step is to make a test for the worst case. This is easily done, for example with a function like this:

from __future__ import print_function
import random

def frogv_test(N=10**5, P=10**5, file=None):
    """Make a test case with N frogs and P pairs; write it to file."""
    M = 10**9       # Maximum coordinate of frog
    K = 4 * M / N   # Make sure there are lots of "breaks"
    print(N, K, P, file=file)
    print(*(random.randrange(M) for _ in range(N)), file=file)
    for _ in range(P):
        print(*(1 + random.randrange(N) for _ in range(2)), file=file)

Let's see how your program performs on this test case:

$ time python2.7 cr56451.py < frogv.txt > /dev/null

real    0m48.012s
user    0m40.880s
sys     0m0.400s

48 seconds! That's bad, given that the time limit at Codechef is 1 second (and of course one would like to have a decent margin in case Codechef is running on slower hardware).

So, how can you speed it up? Well, the most important thing to look at is the time complexity of the algorithm. If you look at the structure of your code, you can see that you iterate over the pairs, and then for each pair, you iterate over the breaks:

for i in xrange(0,P):
    # ...
    for Break_Pt in Breaks:
        # ...

There are \$P\$ pairs, and there are \$O(N)\$ breaks: there might be a break between every two adjacent frogs. So the inner loop might have to be executed \$O(NP)\$ times! I put a counter in the inner loop, and sure enough, in this test case the inner loop was executed 60,897,434 times.

(That should be plenty for you to go on: I won't spoil the challenge for you by giving away the solution.)


Solar System model: Moving Space Balls I have a model of the solar system that I'm working on at the moment. Now is the time to start cleaning my code up a little.

Here's what I have so far:

A main 'driver' method:

public class SolarModel {

public static void main(String[] args){
    SolarSystem model = new SolarSystem(700, 700);

    /* Create new instances of each object in the solar system */
    SunObject sun = new SunObject(0, 0, 50, "YELLOW", 0);
    PlanetObject mercury = new PlanetObject(60, 30, 15, "LIGHT_GRAY" ,5);
    PlanetObject venus = new PlanetObject(90, 30, 15, "GREEN", 3);
    PlanetObject earth = new PlanetObject(140, 30, 15, "BLUE", 2);
    PlanetObject mars = new PlanetObject(180, 30, 15, "RED", 1.2);

    MoonObject moon = new MoonObject(15, 30, 2, "WHITE", 1.5);

    while(true){
        /*Draw all the planets onto the SolarSystem model */
        model.drawSolarObject(sun.getDistance(), sun.getAngle(), sun.getDiameter(), sun.getColour());
        model.drawSolarObject(mercury.getDistance(), mercury.move(), mercury.getDiameter(), mercury.getColour());
        model.drawSolarObject(venus.getDistance(), venus.move(),venus.getDiameter(), venus.getColour());
        model.drawSolarObject(earth.getDistance(), earth.move(), earth.getDiameter(), earth.getColour());
        model.drawSolarObject(mars.getDistance(), mars.move(), mars.getDiameter(), mars.getColour());

        /* Draw the Earth's moon.
         * Use the Earth's PointInSpace as a point in which the moon orbits around
         */
        model.drawSolarObjectAbout(moon.getDistance(),moon.getAngle() + moon.move(), moon.getDiameter(), moon.getColour(),
                                    earth.getDistance(), earth.getAngle());

        /* Draw Mars's moon.
         * Use Mars's PointInSpace as a point in which the moon orbits around
         */
        model.drawSolarObjectAbout(moon.getDistance(), moon.getAngle() + moon.move(), moon.getDiameter(), moon.getColour(),
                                    mars.getDistance(), mars.getAngle());

        model.finishedDrawing();
        }
    }

}

A PointInSpace class:

public class PointInSpace {

private double distance;
private double angle;
private double diameter;
private String colour;
private double speed;

public double getDistance(){
    return distance;
}
public double getAngle(){
    return angle;
}
public double getDiameter(){
    return diameter;
}
public String getColour(){
    return colour;
}
public double getSpeed(){
    return speed;
}

public double move(){
    angle += speed;
    return angle;
}

public PointInSpace(double distance, double angle, double diameter, String colour, double speed){
    this.distance = distance;
    this.angle = angle;
    this.diameter = diameter;
    this.colour = colour;
    this.speed = speed;
    }

}

And classes representing Objects in the solar system

public SunObject(double distance, double angle, double diameter, String colour, double speed){
    super(distance, angle, diameter, colour, speed);

    }
}
    public PlanetObject(double distance, double angle, double diameter, String colour, double speed){
    super(distance, angle, diameter, colour, speed);

    }
}
    public MoonObject(double distance, double angle, double diameter, String colour, double speed){
    super(distance, angle, diameter, colour, speed);

    }
}

Any tips on how I can possibly refactor this code? As you can tell my 'driver' class is huge. How would I pass each object into an array and use the drawSolarObject() method from there? Any other possible way to cut down the size of this class?

What are the other weaknesses in my code?

First up, this is a nice project, and your implementation looks like it has the right basic concepts, but you're right, it could be done better.

There are two concepts you need to apply that will help you a lot...

Parent -> child relationships

The first is the concept of a parent/child relationship. You should consider that each object in your solar system has one or more child objects. The sun has the planets, the planets may have moons.

The logic of 'animating' your solar system is common to all objects. First, you move the object, then you move the dependent ones.

To do this, I extend the PointInSpace class to be:

public class ObjectInSpace extends PointInSpace {
    private final List<ObjectInSpace> dependents = new ArrayList<>();
    private final ObjectInSpace parent;

    public ObjectInSpace(ObjectInSpace parent, double distance, double angle, double diameter, String colour, double speed) {
        super(distance, angle, diameter, colour, speed);
        if (parent != null) {
            parent.addDependent(this);
        }
    }

    private void addDependent(ObjectInSpace kid) {
        // can only be called from constructor of kid instance
        dependents.add(kid);
    }

    public ObjectInSpace getParent() {
        return parent;
    }
    public List<ObjectInSpace> getDependents() {
        // return a copy, not the actual list.
        return new ArrayList<>(dependents);
    }
}

Right, now, with the above class, you can construct your solar system from the top down....

ObjectInSpace sun = new ObjectInSpace(null, .....);
....
ObjectInSpace earth = new ObjectInSpace(sun, ....);
....
ObjectInSpace moon = new ObjectInSpace(earth, ....);

Note that the only thing you need to remember is the sun, and all the other objects 'hang off' the sun. So, create a method:

public ObjectInSpace buildSolarSystem() {
    ObjectInSpace sun = .....
    ....
    return sun;
}

and then, in your code, you can just have:

ObjectInSpace solarsystem = buildSolarSystem();

Now, when you need to draw/move the solar system, you just need to visit every object, from the sun down, and process that object, then process in turn all of its children...

... which leads on to the second concept:

Recursion

The next concept to process is recursion. Consider the need to draw the solar system. All you need is a method:

public void draw(ObjectInSpace object) {
    // draw the object
    ....
    for (ObjectInSpace kid : object.getDependents()) {
        // recurse and draw the kid
        draw(kid);
    }
}

Something similar can be done to move the objects:

public void move(ObjectInSpace object) {
    // move the object
    if (object.getParent() != null) {
        // move the object relative to the parent.
    }
    ....
    for (ObjectInSpace kid : object.getDependents()) {
        // recurse and move the kid
        move(kid);
    }
}


De-bouncing a (Gray) Rotary Encoder Switch in C I  would like feedback on the correctness and performance of this code.

My priorities are also are readability, simplicity and self-documenting code - but I'm happy with those as they are.

I still need to do some factoring of common code between the two input lines - but I'm brushing up my C and will do that.

RA2 and RA3 are the two digital input lines. My algorithm is not dissimilar to how debouncing would be done with a low pass capacitor and resistor followed by a schmitt trigger. This function is called by the ISR every 0.5ms, and the rotary switch produces transitions as short as 5-10ms.

I am curious how to deal with situations where the rotary switch is turned a bit faster than the algorithm can process. Can further information be deduced from the Gray encoding, even when a step  is missed because of the low pass delay and limited sample rate? I suppose this could be taken as far as using neural networks?!

I'm mainly asking this question because I think the running time and the amount of code of the below can both be reduced by a factor of two at least. And I'd just prefer not to get out the logic analyzer and completely reinvent the wheel.

rotsw.h

#define ROTSW_AVG_SIZE 8
#define ROTSW_MAX 7
#define ROTSW_UPPER 5
#define ROTSW_LOWER 2
#define ROTSW_MIN 0

void rotsw_sample_avg_schmitt_count();

extern volatile int rotsw_count_steps;

rotsw.c

unsigned int rotsw_samples[ROTSW_AVG_SIZE];
unsigned char rotsw_write_idx = 0;

int rotsw_avg_ra2 = 0;
int rotsw_avg_ra3 = 0;

int ra2 = 0;
int ra3 = 0;

volatile int rotsw_count = 0;

void rotsw_sample_avg_schmitt_count()
{
    int sample, old_sample, ra2_changed, ra3_changed, count = rotsw_count;

    // Sample RA2 and RA3
    old_sample = rotsw_samples[rotsw_write_idx];
    rotsw_samples[rotsw_write_idx] = sample = PORTA & 0b00001100;
    if (++rotsw_write_idx >= ROTSW_AVG_SIZE) rotsw_write_idx = 0;

    // Moving average, turning digital sample to "analogue"
    if (old_sample & 0x04)  rotsw_avg_ra2--;
    if (old_sample & 0x08)  rotsw_avg_ra3--;
    if (sample & 0x04)      rotsw_avg_ra2++;
    if (sample & 0x08)      rotsw_avg_ra3++;

    // 3. Do limit for Schmitt trigger
    if      (rotsw_avg_ra2 < ROTSW_MIN) rotsw_avg_ra2 = ROTSW_MIN;
    else if (rotsw_avg_ra2 > ROTSW_MAX) rotsw_avg_ra2 = ROTSW_MAX;
    if      (rotsw_avg_ra3 < ROTSW_MIN) rotsw_avg_ra3 = ROTSW_MIN;
    else if (rotsw_avg_ra3 > ROTSW_MAX) rotsw_avg_ra3 = ROTSW_MAX;

    // Do Schmitt trigger
    if (rotsw_avg_ra2 > ROTSW_UPPER){
        ra2_changed = !ra2;
        ra2 = 1;
    } else if (rotsw_avg_ra2 < ROTSW_LOWER){
        ra2_changed = ra2;
        ra2 = 0;
    }
    if (rotsw_avg_ra3 > ROTSW_UPPER){
        ra3_changed = !ra3;
        ra3 = 1;
    } else if (rotsw_avg_ra3 < ROTSW_LOWER){
        ra3_changed = ra3;
        ra3 = 0;
    }

    // Do rotsw state change count.
    if (ra2_changed){
        if (ra2 == ra3) count++;
        else count--;
    }
    if (ra3_changed){
        if(ra2 == ra3) count--;
        else count++;
    }
    rotsw_count = count;
}

I don't see why bother with clipping averages. If something was less than ROTSW_MIN, it is also less than ROTSW_LOWER.
count is useless, you may operate directly on rotsw_count.

The moving averages could be computed a bit faster in assembly. The rotsw_samples array effectively contains 8 2-bit entries, and hence could be stored in a 16-bit register, say R. Now consider something along the lines of

            rlc R             ; Shift the ra2 of the "old sample" into carry
            sbc avg_ra2, $0   ; Subtract carry
            rlc R             ; Same for ra3
            sbc avg_ra3, $0

            or R, sample      ; New sample is added to the "array"

            rrc sample        ; Shift ra3 of the "new sample" into carry
            adc avg_ra3, $0   ; Add carry
            rrc sample        ; Same for ra2
            adc avg_ra2, $0

The exact syntax depends on the architecture and an assembler; it also assumes that the sample is already shifted right by 2.

Your target architecture may allow even faster variants.

Note that the similar tricks could be used to speed up the final rotsw_count computation.

There is not enough context to address your worries about the switch being too speedy. In any case, the code is already quite fast. If doesn't keep up with the peripheral, the best answer is to upgrade the CPU.


CFML implementation of Array.reduce() G'day
Just before I submit this to CFLib, it'd be great to get feedback:

<cfscript>
/**
* @hint CFML implementation of Array.reduce(), similar to Javascript's one ref https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/Reduce
* @array Array to reduce
* @callback Callback function to use to reduce. Will receive the following arguments: element (of current iteration of the all), index, array, (optional) result (of preceeding call to callback())
* @initialValue The initial value to use to start the reduction
*/

public any function arrayReduce(required array array, required any callback, any initialValue){
    var startIdx = 1;
    if (!structKeyExists(arguments, "initialValue")){
        if (arrayLen(array) > 0){
            var result = callback(array[1], 1, array);
            startIdx = 2;
        }else{
            return;
        }
    }else{
        var result = initialValue;
    }
    for (var i=startIdx; i <= arrayLen(array); i++){
        result = callback(array[i], i, array, result);
    }
    return result;
}
</cfscript>

I've created proper unit tests as a gist.

Interesting discussion!

@AdamTuttle Regarding the behavior in Underscore that you've highlighted:
I did some quick tests and found that the same behavior exists in Ruby and native JS (Chrome and FF). This confirmed my suspicions that this seemingly weird behavior is just a product of the reduce algorithm itself.

In the algorithm, "memo" represents the current state of the fold operation. If you don't pass in an initial value, you're implying that the first element of the collection represents the initial state of the fold. From that perspective, the behavior you've shown is exactly what I'd expect.

If you want to remove uppercase letters from a collection and convert that to a string in a functional way, it would make more sense to call filter() to and then reduce(). Example:

adamArr = ['A','d','a','m'];
damArr = _.filter(adamArr, function (letter) {
    var asciiCode = asc(letter);
    return asciiCode >= 97 && asciiCode <= 122;
};
damString = _.reduce(damArr, function (memo, letter) {
    return memo & letter;
};


Prolog predicate for adjacency of integers with all signatures and no spurious choice points I want to create a predicate nearby(X,Y) which represents "X and Y are integers 1 apart from each other" and fully satisfies:

%%! nearby(+X:Int, +Y:Int) is semidet.
%%! nearby(+X:Int, -Y:Int) is multi.
%%! nearby(-X,Int, +Y:int) is multi.

In other words it must work for any combination of two bound inputs or one bound and one free input, and must have no spurious choice point when both inputs are bound.

Is this the best way to do so? Note that while I understand that using a built in predicate like plus might make the code more readable, would it inevitably end up substituting down to something like this?

nearby2(X,Y) :- nonvar(X), X = Y.
nearby2(X,Y) :- nonvar(X), Y is X+1.
nearby2(X,Y) :- nonvar(X), Y is X-1.
nearby2(X,Y) :- var(X), nonvar(Y), nearby2(Y,X).

nearby3(X,Y) :- nonvar(X), nonvar(Y), nearby2(X,Y), !.
nearby3(X,Y) :- nearby2(X,Y).

The best way to reason about integers is to use your Prolog system's CLP(FD) constraints.

For example, in SICStus Prolog and SWI-Prolog:

:- use_module(library(clpfd)).

nearby(X, Y) :- abs(X - Y) #= 1.

This works in all directions out of the box.

For example, with both arguments integers:

?- nearby(1, 2).
true.

Second, with only one argument known:

?- nearby(X, 3).
X in 2\/4,
_G1047+3#=X,
_G1047 in -1\/1.

Third, with both arguments variables:

?- nearby(X, Y).
_G1085+Y#=X,
_G1085 in -1\/1.

etc.

All serious Prolog systems ship with CLP(FD) constraints, and I recommend you use them when describing relations over integers. Just state the constraints and let Prolog do the reasoning for you.

Note that the predicate is semideterministic: It either fails or succeeds precisely once. You can use labeling/2 to enumerate solutions if the domains are finite.


C++ Solution for ProjectEuler Problem #5 What is the smallest positive number that is evenly divisible by all of the numbers from 1 to 20?

Is there a more elegant solution in C++?

#include <iostream>
using namespace std;

int main(){
    long long num = 1;
    int divMin = 1;
    int divMax = 20;
    int tempDivMax = divMax;

    while(true){
        if(num % tempDivMax == 0)
        {
            tempDivMax--;
            if(tempDivMax == divMin){
                cout << num << endl;
                break;
            }
        }else{
            tempDivMax = divMax;
            num++;
        }
    }
    return 0;
}

Code review

To start with, some points about your code:

using namespace std; is something I consider a code smell. It doesn't save much time and I find it clearer to see exactly where cout etc. are coming from. Additionally, it clutters your namespace although I myself haven't run into name collisions with the STL being an issue before.
It looks like your condition to exit the while loop is tempDivMax == divMin. Use this rather than while (true) to make it clearer what behaviour is intended.
You are also performing two iterations inside a single loop: num is being incremented from 1 until it succeeds and tempDivMax is being decremented until it equals divMin. Split these into separate loops! The code will be clearer to read and will be just as fast. The outer loop I would turn completely into a for loop for (; tempDivMax != divMin; ++num) { ... } and but the inner one I would make a while loop while (tempDivMax > divMin && num % tempDivMax == 0) { ... } since the long condition expression makes for some rough reading when condensed into a for loop.
Finally, define your constants before your working variables and declare them as const. I would even do this outside of the main function.

This leaves you with:

#include <iostream>

const int DIV_MIN = 1;
const int DIV_MAX = 20;

int main() {
    long long num = 1;
    int tempDivMax = DIV_MAX;
    for (; tempDivMax > DIV_MIN; ++num) {
        tempDivMax = DIV_MAX;
        while (tempDivMax > DIV_MIN && num % tempDivMax == 0) {
            --tempDivMax;
        }
    }

    std::cout << num << std::endl;
    return 0;
}

A "more elegant" solution...

== Obligatory code below is untested ==

I'd imagine they're looking for something like this?

For something to be divisible by a number, it must have at least that number's prime factors. Furthermore, in any given number below 20, there can be a maximum of $$\lfloor{log_p(20)}\rfloor$$ copies of the prime factor $$p$$

The following snippet calculates the product of these.

#include <iostream>

const int DIV_MAX = 20;
const int[] PRIMES = {2, 3, 5, 7, 11, 13, 17};

int main() {
  int result = 1;
  for (int p : PRIMES) {
    int tmp = 1;
    while (tmp < DIV_MAX) tmp *= p;
    result *= tmp;
  }

  std::cout << result << std::endl;
  return 0;
}


Reading a file representing word frequencies in Wikipedia, for clustering analysis I am writing a system that will run Kmeans to determine the most used words in Wikipedia. I do this by building a core set with streaming data. Wikipedia is 50TB but after a year of processing I was given data of size 14 GB which I need to parse and I want it to run fast. From client I send server 10000 points which are processed.

CODE:

import gc
import time

class point:
    def __init__(self, x, y, w):
        self.x = x
        self.y = y
        self.w = w

def parse(pathToFile):
    myList = []
    with open(pathToFile) as f:
        for line in f:
            s = line.split()
            x, y, w = [int(v) for v in s]
            obj = point(x, y, w)
            gc.disable()
            myList.append(obj)
            gc.enable()
    return myList

if __name__ == "__main__":
    startTime = time.time()
    L = parse('C:/Users/user/workspace/finalSub/api/data100mb.txt')
    print("--- %s seconds ---" % (time.time() - startTime))

Parsing a 114MB file takes 130 seconds when I've been told it should take few seconds.

I tried splitting the data to multiple chunks and apply multiprocessing, but it ended up that reading from multiple files is bad and results in even longer time time to parse. Look here.

File Sample:

1 1 1
1 2 1
1 3 1
1 4 1
.
.
1 N 1
2 1 1
2 2 1
2 3 1
2 4 1
.
.
2 N 1

How should I parse the file correctly or access it in-order to make it faster?

Avoid reinventing-the-wheel, especially with common tasks that are likely to have optimized implementations. In this case, I recommend trying pandas.read_csv() to read the file. Then, the matrix can be fed to sklearn.cluster.KMeans().


Is it ok to pull out specific arguments from a Java dynamic proxy invocation? I have a Java dynamic proxy with an invoke method that looks like this:

 @Override
public Object invoke(Object proxy, Method method, Object[] args) throws Throwable {
    Object result;
    try {
        //Not sure if this line is ok?
        Token token = (Token) args[0];

        System.out.println("Token with id: "+token.getId()+ " was passed");
        result = method.invoke(realObject, args);
    } catch (InvocationTargetException e) {
        throw e.getTargetException();
    } catch (Exception e) {
        throw new RuntimeException("unexpected invocation exception: " + e.getMessage());
    } finally {
        System.out.println("after method " + method.getName());
    }
    return result;
}

Would it be considered bad practice to pull out and cast that Token object from the args array?

The problem I can see is that it means the proxy isn't generic and in fact will only work for method invocations that pass an instance of a Token object as the first parameter.

Casting is never 'pretty'.... But, in a proxy situation like this (assuming you really need the proxy), it is possibly unavoidable.

I can recommend though that you do it defensively... you should really do:

if (args.length > 0) {
    if (args[0] instanceof Token) {
        System.out.println("Token with id: "+((Token)args[0]).getId()+ " was passed");
    } else {
        System.out.println("Proxy for a method which does not have an initial token: " + args[0]);
    }
} else {
    System.out.println("Proxy for a method that is being passed no arguments.");
}

If your situation requires that these values are populated, then you should throw exceptions instead of the other println methods.

Also, you should consider checking that the number of arguments in the args matches the number of arguments expected in method.getParameterTypes(), but with var-args, that can be hard.

Additionally, this line:

throw new RuntimeException("unexpected invocation exception: " + e.getMessage());

Should incorporate the actual cause.... when you get an exception through this catch block, it will be one of those times when the cause is critical, and you are throwing the cause away! That exception should look like:

throw new RuntimeException("unexpected invocation exception: " + e.getMessage(), e);


String tokenizer via UDF I am trying to implement strings tokenizer in Excel via UDF using C# and Excel-Dna. Personally I found the mIRC's function $gettok pretty useful due to the lack of string functions in Excel.

Examples

getTok("Aquel que no espera vencer ya esta vencido",   1, 32) -> "Aquel"
getTok("Aquel que no espera vencer ya esta vencido",  -1, 32) -> "vencido"
getTok("Aquel que no espera vencer ya esta vencido", 2-4, 32) -> "que no espera"
getTok("Aquel que no espera vencer ya esta vencido",   0, 32) -> "Aquel que no espera vencer ya esta vencido"

Code

[ExcelFunction(Category="String Utilities", Name = "getTok")]
public static string getTok(string text, string token, int delimiter)
{
    int from = 0;
    int to = 0;

    string tokenPatter = @"(\d+)(-)(\d+)?";
    string[] tokens = text.Split(new char[] { (char)delimiter });

    Regex tokenRegex = new Regex(tokenPatter, RegexOptions.IgnoreCase);
    Match tokenMatch = tokenRegex.Match(token);

    if (tokenMatch.Success)
    {
        StringBuilder sb = new StringBuilder();
        from = short.Parse(tokenMatch.Groups[0].Value);

        if (tokenMatch.Groups.Count == 1) to = from;
        else if (tokenMatch.Groups.Count == 2) to = (short)tokens.Length;
        else to = short.Parse(tokenMatch.Groups[2].Value);

        for (int i = from; i <= to; i++)
        {
            sb.Append(tokens[i - 1] + ((char)delimiter));
        }

        return sb.ToString();
    }

    int index = int.Parse(token);
    if (index > 0) return tokens[index - 1];
    else return tokens[tokens.Length + index];
}

Questions

What would you improve on?
Have you tried something similar with Excel?

In this statement:

for (int i = from; i <= to; i++)
    {
        sb.Append(tokens[i - 1] + ((char)delimiter));
    }

The way that you have it written, I don't think that i will ever hit to.

Also, I would change some variable names to make it more readable. from and to got confusing, I had to keep telling myself that those are variables.  I recommend doing something like:

int intFrom;
int intTo;

at the very least, so that you automatically know that those are integer variables.

Also you have an int for a delimiter? Why not just pass a char as a delimiter?

Other than that I think the code looks good.


Copying and comparing two sheets before deleting duplicates I am running this code as a macro in Excel, which copies two columns from sheet 1 and pastes them into sheet two. It then compares the first column to a column in sheet two before deleting any duplicates. The two columns I am copying are product code and price. I need to compare the product code to the existing ones.

Private Sub CommandButton1_Click()

Dim MasterList As New Dictionary
Dim iListCount As Integer
Dim x As Variant
Dim iCtr As Integer
Dim v As Variant
Dim counter As Integer, i As Integer
counter = 0

With Sheets("Sheet2")
.Range("M:M").ClearContents
.Range("S:S").ClearContents

Sheets("Sheet1").Range("C:C").Copy
Sheets("Sheet2").Range("M1").Select
ActiveSheet.Paste

Sheets("Sheet1").Range("X:X").Copy
Sheets("Sheet2").Range("S1").Select
ActiveSheet.Paste

Application.ScreenUpdating = False
Application.Calculation = xlCalculationManual

' Get count of records in master list
iListCount = .Cells(Rows.Count, "A").End(xlUp).Row
'Load Dictionary:
For iCtr = 1 To iListCount
    v = .Cells(iCtr, "A").Value
    If Not MasterList.Exists(v) Then MasterList.Add v, ""
Next iCtr

'Get count of records in list to be deleted
iListCount = .Cells(Rows.Count, "M").End(xlUp).Row

' Loop through the "delete" list.
    For iCtr = iListCount To 1 Step -1
        If MasterList.Exists(.Cells(iCtr, "M").Value) Then
            .Cells(iCtr, "M").Delete shift:=xlUp
            .Cells(iCtr, "S").Delete shift:=xlUp
        End If
    Next iCtr
  End With

Application.ScreenUpdating = True
Application.Calculation = xlCalculationAutomatic

MsgBox "Done!"
End Sub

There is just under 30,000 rows that it has to compare so I know that it is always going to take some time, but I was wondering if there was any way to speed it up or even just make my code more streamline and efficient.

It's poor form to have this much logic in an event procedure. The first step is to move 99% of this code into another module and call it from the event procedure.
Don't activate and select. This slows down your code and makes it bug prone. Use a variable to reference the worksheets and ranges you need instead.
There's no reason to select and paste. The Copy method takes an optional Destination parameter.

Instead of this

Sheets("Sheet1").Range("C:C").Copy
Sheets("Sheet2").Range("M1").Select
ActiveSheet.Paste

Use this

Sheets("Sheet1").Range("C:C").Copy Sheets("Sheet1").Range("M1")

Anytime you turn sheet calculation or screen updating off, you need to use an error handler to make sure it always gets turned back on.
Look into using an ADODB connection to query the workbook instead of working directly with the COM object. It's typically a much, much faster approach.


NinjectWebCommon implementation/translation - C# to F# I've translated this code from c# and would like it to be more idiomatic f#:

namespace Foo.App_Start

open System
open System.Web

open Microsoft.Web.Infrastructure.DynamicModuleHelper

open Ninject
open Ninject.Web.Common

type NinjectResolver(kernel:IKernel) =
    let _kernel = kernel
    interface System.Web.Http.Dependencies.IDependencyResolver with
        member this.BeginScope():Http.Dependencies.IDependencyScope = upcast this
        member this.GetService(t) =
            _kernel.TryGet(t)
        member this.GetServices(t)=
            _kernel.GetAll(t)
        member this.Dispose() = ()

//was static
[<AbstractClass; Sealed>]
type NinjectWebCommon () =
    static let bootstrapper = new Bootstrapper()

        /// <summary>
        /// Load your modules or register your services here!
        /// </summary>
        /// <param name="kernel">The kernel.</param>
    //private static void RegisterServices(IKernel kernel)
    static let RegisterServices(kernel:IKernel) =
        System.Web.Http.GlobalConfiguration.Configuration.DependencyResolver <- new NinjectResolver(kernel)
        do()
        /// <summary>
        /// Creates the kernel that will manage your application.
        /// </summary>
        /// <returns>The created kernel.</returns>
        //private static IKernel CreateKernel()
    static let KernelCreation(kernel:IKernel) =
        System.Diagnostics.Debug.WriteLine "kernel Creation!"
        RegisterServices(kernel)
        let bs = fun () -> new Bootstrapper()
        let kernelFun= System.Func<IKernel> (fun () -> bs().Kernel)
        kernel.Bind<System.Func<IKernel>>().ToMethod(fun ac -> kernelFun) |> ignore
        kernel.Bind<IHttpModule>().To<HttpApplicationInitializationHttpModule>() |> ignore
        let fetchedKernel:IKernel =System.Web.Http.GlobalConfiguration.Configuration.DependencyResolver.GetService(typeof<IKernel>) :?> IKernel
        fetchedKernel

    static let CreateKernel():IKernel =
        System.Diagnostics.Debug.WriteLine "standard kernel creating!"
        let kernel= new StandardKernel()
//
        try         //RegisterServices(kernel) |> ignore;
            KernelCreation(kernel)
        with
            | ex ->
                System.Diagnostics.Debug.WriteLine "kernel disposing!"
                kernel.Dispose()
                raise ex

    /// <summary>
    /// Starts the application
    /// </summary>
    static member Start() =
        DynamicModuleUtility.RegisterModule typedefof<OnePerRequestHttpModule> |> ignore
        DynamicModuleUtility.RegisterModule typedefof<NinjectHttpModule> |> ignore
        bootstrapper.Initialize(System.Func<_>(CreateKernel))
        System.Diagnostics.Debug.WriteLine "app Start()"
//
//
    /// <summary>
    /// Stops the application.
    /// </summary>
    static member Stop() =
        System.Diagnostics.Debug.WriteLine "app stop"
        bootstrapper.ShutDown()

module DummyModuleToAttachAssemblyAttribute =
    [<assembly: WebActivatorEx.PreApplicationStartMethod(typeof<NinjectWebCommon>, "Start")>]
    ////[assembly: WebActivatorEx.ApplicationShutdownMethodAttribute(typeof(FLochart.App_Start.NinjectWebCommon), "Stop")]
    [<assembly: WebActivatorEx.ApplicationShutdownMethod(typeof<NinjectWebCommon>, "Stop")>]
    do()

Your code seems like it's primarily plumbing. There are no algorithms or types that could be given the functional treatment. I have only two recommendations.

Instead of NinjectWebCommon being abstract/sealed with static members you can make it a module with let-bound functions.
_kernel in NinjectResolver is unnecessary because primary constructor parameters are available throughout the body of the type. So you can just reference kernel throughout instead.


Script to store, search, and delete IPFS data with description in JSON file Here is the script that I would like reviewed for the following:

Best practices and design pattern usage
Correctness in unanticipated cases
Better dictionary access

The script is made to store (IPFS) Interplanetary File System hash objects. If you would like to know more check out https://ipfs.io

Script:

#!/usr/bin/python
# PHT - Personal Hash Tracker
#
import json
import os

hashjson_global = "/Users/troywilson/testing/pht/hash.json"

choice = raw_input("What do you want to do? \n a)Add a new IPFS hash\n s)Seach stored hashes\n d)Delete stored hash\n >>")

if choice == 'a':
    # Add a new hash.
    description = raw_input('Enter hash description: ')
    new_hash_val = raw_input('Enter IPFS hash: ')
    new_url_val = raw_input('Enter URL: ')
    entry = {new_hash_val: {'description': description, 'url': new_url_val}}

    # search existing hash listings here
    if new_hash_val not in data['hashlist']:
    # append JSON file with new entry
        data['hashlist'].update(entry) #must do update since it's a dictionary
        with open(hashjson_global, 'w') as file:
            json.dump(data, file, sort_keys = True, indent = 4, ensure_ascii = False)
        print('IPFS Hash Added.')
        pass
    else:
        print('Hash exist!')

elif choice == 's':
    # Search the current desciptions.
    searchTerm = raw_input('Enter search term: ')
    with open(hashjson_global, 'r') as file:
        data = json.load(file)
        hashlist = data['hashlist']
    # build dictionary map and search for description value
    d = {v['description']: h for h, v in hashlist.items()}
    print d.get(searchTerm, 'Not Found')

elif choice == 'd':
    # Search the current descriptions and delete entry.
    del_hash = raw_input('Hash to delete: ')
    with open(hashjson_global, 'r') as file:
        data = json.load(file)
    del data['hashlist'][del_hash]
    with open('hashjson', 'w') as file:
            json.dump(data, file, sort_keys = True, indent = 4, ensure_ascii = False)
    print ('Hash removed')

Example JSON file:

{
"hashlist": {
"QmVZATT8jWo6ncQM3kwBrGXBjuKfifvrE": {
    "description": "Test Video",
    "url": ""
},
"QmVqpEomPZU8cpNezxZHG2oc3xQi61P2n": {
    "description": "Cat Photo",
    "url": ""
},
"QmYdWb4CdFqWGYnPA7V12bX7hf2zxv64AG": {
    "description": "test.co",
    "url": ""
}
}
}%

I would make an object that acts like a dict and that automatically writes any changes to disk, similar to the one I wrote in this answer.

import os
import json

class PersonalHashTracker(dict):

    def __init__(self, filename):
        self.filename = filename
        if os.path.isfile(filename):
            with open(filename) as f:
                # use super here to avoid unnecessary write
                super(PersonalHashTracker, self).update(json.load(f) or {})

    def write(self):
        with open(self.filename, "w") as f:
            json.dump(self, f)

    def __setitem__(self, key, value):
        super(PersonalHashTracker, self).__setitem__(key, value)
        self.write()

    def __delitem__(self, key):
        super(PersonalHashTracker, self).__delitem__(key)
        self.write()

    def update(self, d, **kwargs):
        super(PersonalHashTracker, self).update(d, **kwargs)
        self.write()

You can then use it like this:

MENU = """What do you want to do?
a)Add a new IPFS hash
s)Seach stored hashes
d)Delete stored hash
q)Quit
>>"""

if __name__ == "__main__":
    hash_store = PersonalHashTracker("/Users/troywilson/testing/pht/hash.json")

    while True:
        choice = raw_input(MENU)
        if choice == 'a':
            # Add a new hash.
            description = raw_input('Enter hash description: ')
            new_hash_val = raw_input('Enter IPFS hash: ')
            new_url_val = raw_input('Enter URL: ')

            if new_hash_val not in hash_store:
                hash_store[new_hash_val] = {'description': description,
                                            'url': new_url_val}
            else:
                print 'Hash exists!'
        elif choice == 's':
            # Search the current descriptions.
            search_term = raw_input('Enter search term: ')
            descriptions = {v['description']: h for h, v in hash_store.items()}
            print descriptions.get(search_term, 'Not Found')
        elif choice == 'd':
            # Search the current descriptions and delete entry.
            del_hash = raw_input('Hash to delete: ')
            del hash_store[del_hash]
            print 'Hash removed'
        else:
            print 'Exiting'
            break


Safe, simultaneous string replacement I've written a function which takes as an input:

a string to be modified
a vector of regular expressions on which to match
a vector of replacements
option to ignore case in the match statements

The function will then, from the users perspective, simultaneously perform all replacements in a 'safe' fashion. By 'safe' I mean sub-string matches are ignored in favor of longer matches (if 'the' and 'they' are patterns to match, ignore the sub-string 'the' in 'they') and also allow users to have cross-replacement (swap 'hey' for 'ho' and 'ho' for 'hey').

Examples:

rcpp_mgsub("hey ho",{"hey","ho"},{"ho","hey"},true)
// Expect "ho hey"
rcpp_mgsub("Hey, how are you?",{"hey","how","are","you"},{"how","are","you","hey"},true)
// Expect "how, are you hey?"

I've written a function in the R programming language that accomplishes this, but R isn't a fast language. R libraries can include pre-compiled C++11 code which can result in vastly faster functions.  To this end, I've written a C++11 function but the performance is actually significantly slower than R (3-7x slower in various comparisons I've done) so I realize there must be some significant inefficiencies in my function.

So, I'm curious what inefficiencies are present in the rcpp_mgsub function that I should focus on improving.

#include <iostream>
#include <string>
#include <regex>

std::string rcpp_mgsub(std::string string, std::vector<std::string> const& match, std::vector<std::string> const& replace, bool const& ic) {
  std::string newString = "";
  std::smatch matches;
  std::string prefix = string;
  std::string detected;
  std::string suffix;

  std::regex::flag_type flags;
  flags |= std::regex_constants::optimize;
  if(ic) flags |= std::regex_constants::icase;

  std::vector<std::regex> r(match.size());
  std::transform(match.begin(), match.end(), r.begin(),
                 [&flags](std::string m) {return std::regex (m, flags);});

  int j=0;
  while(string.size() > 0){
    prefix = string;
    detected = "";
    suffix = "";
    j = 0;
    for(int i=0;i < match.size();i++){
      if (std::regex_search(string,matches, r[i])) {
        std::string pr = matches.prefix();
        std::string m = matches[0];
        if(pr.size() < prefix.size() || (pr.size() == prefix.size() && m.size() > detected.size())){
          prefix = pr;
          detected = m;
          suffix = matches.suffix();
          j=i;
        }
      }
    }
    if(prefix == string){
      newString = newString+string;
      string = "";
    } else {
      newString = newString+prefix+std::regex_replace(detected,r[j],replace[j],std::regex_constants::format_sed);
      string = suffix;
    }
  }
  return newString;
}

int main()
{
   std::cout << rcpp_mgsub("hey ho hey",{"hey","ho"},{"ho","hey"},true) << "\n";
   std::cout << rcpp_mgsub("Hey, how are you?",{"hey","how","are","you"},{"how","are","you","hey"},true) << "\n";
   std::cout << rcpp_mgsub("Dopazamine is not the same as Dopachloride and is still fake.",{"[Dd]opa(.*?mine)","fake"},{"Meta\\1","real"},false);
   return(0);
}

What compiler are you using? Some implementations of the C++ standard library are better than others for supporting Regex.

Do you really need backreferences? That usually makes regex much slower.

It's very unusual to use a variable with the same name as the variable's type.

You don't need to initialize prefix=string before the loop, it's done first thing inside the loop.

Have you done any profiling of your code to see which lines are slowing it down? It'd be very good to know if the slowness is due to the regex or due to the string concatenation.


Given a String and dictionary of words, break the string in minimum space sentence Given a String and dictionary of words, break the string in minimum space sentence.

Ex:
inputStr = "ilikefacebook"
dictionary = {"i","like","face","book","facebook"}
Possible Strings:
i like face book - 3 spaces
i like Facebook - 2 spaces - this is expected answer.

Here's a trivial solution. It's a simple solution, but the runtime performance is poor O(n2). There must be better ways of doing this.

public class MyClass {

    public static void main(String[] args) {

        String input = "IlikeFacebook";

        HashSet<String> dict = new HashSet<String>();
        dict.add("i");
        dict.add("like");
        dict.add("face");
        dict.add("book");
        dict.add("facebook");

        int left = 0;
        int right = input.length();
        int count = -1;

        boolean[] status = new boolean[input.length()];

        while (left < right) {
            for (int i = 0; i <= right; i++) {
                String word = input.substring(i, right);
                word = word.toLowerCase();
                if (dict.contains(word)) {
                    count++;
                    right = i;
                    System.out.println(" -> " + word);
                }
            }

        }
        System.out.println(" Space count " + count);
    }

}

Can anyone provide a better and efficient solution of this problem?

Advice 1

You can get a substantial speed up by setting input = input.toLowerCase() and removing word = word.toLowerCase().

Advice 2

What comes to code, I suggest you separate the actual algorithm from the standard output. That way you can reuse your solution on larger string without getting overwhelmed with all the output. Also, it allows passing the result to some other processing routines, etc:

public static String[] opGetShortestTokenizationV2(String text,
                                                   Set<String> vocabulary) {
    text = text.toLowerCase();
    int left = 0;
    int right = text.length();
    List<String> result = new ArrayList<>();

    while (left < right) {
        for (int i = 0; i <= right; i++) {
            String word = text.substring(i, right);
            if (vocabulary.contains(word)) {
                right = i;
                result.add(word);
            }
        }
    }

    Collections.reverse(result);
    return result.toArray(new String[result.size()]);
}

Advice 3

left is not used. Your while loop condition becomes right != 0.


N Queens in Python using backtracking I write this solution to the popular N Queens problem using backtracking algorithm. I am relatively new to Python. I would like to know what are the ways to refactor this code and also code style of Python in general.

def isSafe (board, row, col):
    # check left row
    for y in range(col):
        if board[row][y] == 1:
            return False
    # check diagonal left top
    for x, y in zip(range(row, -1, -1), range(col, -1, -1)):
        if board[x][y] == 1:
            return False
    # check diagonal left bottom
    for x, y in zip(range(row, N, 1), range(col, -1, -1)):
        if board[x][y] == 1:
            return False

    return True

def generateSolution(board, col):
    # terminating condition
    # all columns covered
    global N
    if col >= N:
        return True
    # loop over all the rows
    for i in range(N):
        if isSafe(board, i, col) == True:
            board[i][col] = 1
            # recursively place other queens
            if generateSolution(board, col + 1) == True:
                return True
            # unmark queen spot
            board[i][col] = 0
    # backtrack
    return False

N = int(input())
startCol = 0
board = [[0 for i in range(N)] for j in range(N)]
# print(board)

if generateSolution(board, startCol) == False:
    print("No Solution Exists")
else:
    print("Solution exists")
    print(board)

PEP 8

There is a generally accepted style for Python known as PEP 8. Most notably camelCase functions/methods should become snake_case. There are some other issues with stylistic conventions, I would give the document I linked to an overview.

Main Function

I would advise you add a main function to your program. The main reason being: If I want to use the functions from your script, when I import your script I probably don't want to actually run your program, just use generateSolution and isSafe. So:

N = int(input())
startCol = 0
board = [[0 for i in range(N)] for j in range(N)]
# print(board)

if generateSolution(board, startCol) == False:
    print("No Solution Exists")
else:
    print("Solution exists")
    print(board)

Becomes:

def main():
    N = int(input())
    startCol = 0
    board = [[0 for i in range(N)] for j in range(N)]

    if generateSolution(board, startCol) == False:
        print("No Solution Exists")
    else:
        print("Solution exists")
        print(board)

if __name__ == '__main__':
    main()

Avoid global

You should almost always not use the global keyword. In this case: just pass N into your generateSolution function, i.e. generate_solution(board, col, N).

Naming

I am not entirely sure how your code works (I am, ashamedly, unfamiliar with the N-Queen problem). I assume N refers to the number of queens? I would recommend that you call it queen_count or something of the sort. In addition, generate_solution is rather unspecific. I would call it (possibly) n_queen_solvable.

Does generate_solution actually do that?

I mentioned changing it to the name n_queen_solvable because you have return a truth value. I would expect a function like that to actually give me a configuration, not answer whether a solution exists or not. You may want to refactor this into two functions: n_queen_solvable and gen_queen_config or something of the sort.

Default parameters

You seem to want to start at the zeroith column. Makes sense. Instead of explicitly passing the startCol explicitly, I would just make col default to 0. This is done by changing:

def generateSolution(board, col):

to

def generateSolution(board, col=0):


Representing destinations in a square I've written a program following the instructions of an exam paper. There are no solutions available so I would appreciate a feedback. I'm concerned with main in particular (I've already posted a similar question here). I've tried to solve the problem as I've seen it's done in other solutions made available by our teacher. Also the constructors made me think a lot.

I was asked to make a program to represent destinations in a square [0,1]x[0,1], a salesman that has to go in some destinations in that square and another salesman inheriting from the former but with slightly different habits.

#include <iostream>
#include <vector>
#include <set>
#include <cstdio>
#include <cmath>
#include <algorithm>
#include <cstdlib>

class Location {
private:
    double x_, y_;
public:
    Location(double x, double y) : x_(x), y_(y) { }
    Location() {
        x_ = drand48();
        y_ = drand48();
    }
    Location(const Location & L) : x_(L.x_), y_(L.y_) { }
    double getx() {return x_; }
    bool operator<(const Location & L) { return x_ < L.x_; }

    double distance(const Location & L) {
        return std::sqrt(std::pow(x_ - L.x_, 2) + std::pow(y_ - L.y_, 2));
    }

};

class Salesman {
private:
    Location start_;
protected:
    std::vector<Location> tobevisited_;
public:
    Salesman(const Location & start, std::vector<Location>::iterator it1, std::vector<Location>::iterator it2) : start_(start), tobevisited_(it1, it2) { }

    int ndest() { return tobevisited_.size(); }

    virtual double visit() {
        double dist = 0;
        sort(tobevisited_.begin(), tobevisited_.end());
        dist = (*(tobevisited_.begin())).distance(start_);
        for(std::vector<Location>::iterator it = tobevisited_.begin()+1; it != tobevisited_.end(); it++) {
            dist += (*(it)).distance(*(it-1));
        }
        return dist;
    }

};

class LazySalesman : public Salesman {
private:
    double d_;
public:
    LazySalesman(double d, std::vector<Location>::iterator it1, std::vector<Location>::iterator it2) : d_(d), Salesman(Location(0,0), it1, it2) { }

    double visit() {
        double dist = 0;
        dist = Salesman::visit();
        dist += d_*ndest();
    }
};

int main() {

    srand48(time(NULL));
    Location start(0., 0.);
    Location start2(1,1);
    int cont = 0;

    for(int i=0; i<3000; i++) {
        std::vector<Location> v;

        for(int j=0; j<10; j++) {
            v.push_back(Location());
        }
        Salesman Willy(start, v.begin(), v.end());
        if(Willy.visit() < 2.) cont++;
    }

    std::cout << "cont = " << cont << std::endl;

    std::vector<Location> loc;
    Location a(0.1, 0.1);
    Location b(0.5, 0.1);
    Location c(0.5, 0.5);
    loc.push_back(a);
    loc.push_back(b);
    loc.push_back(c);

    std::vector<Salesman*> vett;

    vett.push_back(new Salesman(start2, loc.begin(), loc.end()));
    vett.push_back(new LazySalesman(0.1, loc.begin(), loc.end()));
    vett.push_back(new LazySalesman(0.5, loc.begin(), loc.end()));

    for(auto el : vett) {
        std::cout << el->visit() << std::endl;
        delete el;
    }

}

Tell your instructor to become familiar with the Standard Guidelines.  You can find presentations on youtube where Stroustrup introduces this as a keynote speech, and many others speaking on them since.

Don’t use naked new (and delete).  Make your vector of unique_ptr instead.

The srand48 is not a standard function.  And the old C rand/srand are infamous for poor quality random numbers.  See the <random> header file.

Why does one location use 0. (funny way to write 0.0, a double) and the other use 1 as an int?

You don’t need to put private: at the top of the class; that is assumed.

Does visit modify the object? You did not make it const.  Also, use the override keyword in the derived version!  That is very important in real code for maintainability as well as error checking.

for(int j=0; j<10; j++)  v.push_back(Location());

Did you know that vector elements are always initialized? And what will it use do you suppose?  The default constructor of the element, of course!

So just write:  v.resize(10+v.size())  to add 10 default locations to the end.

Location a(0.1, 0.1);

Use uniform initialization syntax.

Location a{0.1, 0.1};

Thou hast scribed an olde dialect.  You’re using the ranged for loop so your textbook must be written after 2011; perhaps it is just a slightly updated lesson plan from 1998?

Location() {
    x_ = drand48();
    y_ = drand48();
}

A random-initialized location might be handy for this specific program, but it’s sufficiently bizzare that it hinders the understanding of the code.  Make a function to return a random location instead!

double getx() {return x_; }
bool operator<(const Location & L) { return x_ < L.x_; }

You are definitly missing const on your member functions.

Writing good relational operators is a whole lesson and not the point here — this is probably fine for the class as it stands, but reviewers will wonder why you didn’t make it a non-member and supply all the relational and equality operators.

Since the (partial) ordering exists specifically for your algorithm, you might want to make it a named function instead. Consider the future maintainer that writes a test using > and then has to figure out why it didn’t compile, and of course documenting that less means East and is not a complete ordering.

double distance(const Location & L) {
    return std::sqrt(std::pow(x_ - L.x_, 2) + std::pow(y_ - L.y_, 2));

Besides being const, just call std::hypot.  It will be faster and more accurate.

Salesman(const Location & start, std::vector<Location>::iterator it1, std::vector<Location>::iterator it2)
: start_(start), tobevisited_(it1, it2)
{ }

Very interesting that you allow salesman to be initialized with a segment of a larger vector.  Is that a use case you anticipate (e.g. a huge list divided up among several salesmen)?

It’s enlightened but only part way to what we expect: it is limited to segments of another vector, and can’t take begin/end iterators to locations in general (anything that can be iterated).

Also, you should be using the const_iterator.

Here is the simple/direct way to take that information in the constructor.

      ⋮
    Location start_;
    std::vector<Location> tobevisited_;
public:
    Salesman (start, std::vector<Location> locations)
      : start_{start}, tobevisited_{std::move(locations)}
      { }

Get a post-it note and write using a fat marker, “Can it be const?”. Stick that on the edge of your monitor.


Inserting a Java object into a MySQL database I'm writing a method to insert objects into a MySQL database and while everything runs fine I feel my code is bulky and inefficient.  What steps would you take to make this code more pleasant to look at and more efficient?

public void addAuthorization(Authorization authorization) {
    try {
        // Builds the query, lastHeader is used to prevent a trailing comma
        query = "INSERT into authorizations (";
        for(int i = 0; i < authorizationsHeaders.size() - 1; i++) {
            query += authorizationsHeaders.get(i) + ", ";
            questionMarks += "?, ";
        }
        String lastHeader = authorizationsHeaders.get(authorizationsHeaders.size() - 1);
        query += lastHeader + ") values(" + questionMarks + "?)";

        openConnection();
        ps = con.prepareStatement(query);
        ps.setString(1,  authorization.getCompany());
        ps.setString(2,  authorization.getPromoType());
        ps.setString(3,  authorization.getPromoDescription());
        ps.setString(4,  authorization.getStartDate());
        ps.setString(5,  authorization.getEndDate());
        ps.setString(6,  authorization.getVlMarketingNum());
        ps.setString(7,  authorization.getMarketingNum());
        ps.setString(8,  authorization.getStatus());
        ps.setDouble(9,  authorization.getForecast());
        ps.setDouble(10, authorization.getActual());

        ps.executeUpdate();
    } catch (SQLException e) {
        e.printStackTrace();
    } finally {
        closeConnection();
    }
}

If you don't have Java 8, here are a few things you can do.

Use a string builder instead of concatenating string together. Each time you concatenate to a string, a new string is created. Using a String builder is a more efficient way of doing the same thing.

 StringBuilder sb = new StringBuilder("INSERT into authorizations (");
 sb.append("another string");
 sb.append("another string");
 String query = sb.toString();

You can use String.join to generate a comma separated string of headers

    String headers = String.join(",", authorizationHeaders);
    sb.append(headers);

I'm not sure the best way to generate the list of question marks, but this is how I normally do it.

    sb.append( ") values(");
    boolean first = true;
    for(String header: authorizationHeaders) {
        if(first){
            sb.append("?");
            first= false;
        }else{
            sb.append("?,");
        }

    }
   sb.append(")");

When setting your parameters, using i++ instead of hardcoded numbers makes code maintenance easier. If you end up needing to add one more setString(), you can add the one line of code without having adjust the rest of the indexes

    int i=1;
    ps.setString(i++,  authorization.getCompany());
    ps.setString(i++,  authorization.getPromoType());
    ps.setString(i++,  authorization.getPromoDescription());
    ps.setString(i++,  authorization.getStartDate());
    ps.setString(i++,  authorization.getEndDate());
    ps.setString(i++,  authorization.getVlMarketingNum());
    ps.setString(i++,  authorization.getMarketingNum());
    ps.setString(i++,  authorization.getStatus());
    ps.setDouble(i++,  authorization.getForecast());
    ps.setDouble(i++, authorization.getActual());