Loop through column cells and compare value with a Collection I have a Collection of 166 elements:

Dim MyCollection As New Collection
MyCollection.Add ("%")
MyCollection.Add ("%O")
MyCollection.Add ("D")
MyCollection.Add ("CMS")
...

I want to compare them to the value of the cells in a column. If the value of a cell is not in MyCollection, I change the background color to red.

For i = 2 To lastRow
    IsValid = False
    For Each Unit In MyCollection
        If Range("A" & i).value = Unit Then
            IsValid = True
        End If
    Next Unit
    If Not IsValid Then
        Range("A" & i).Interior.Color = 192
    End If
Next i

However, it takes a long time to finish executing when there is a large number of row and I would like to improve this. Maybe by using something else than a Collection and some VBA functions.

For a lot of data dictionaries are faster than collections, and much more convenient as @RubberDuck mentioned. Lookup times are almost instantaneous, IF you do a lot of lookups, but both collections and dictionaries are slow to initialize

In your case though, the lookups are insignificant. The main issue is caused by updating the cell format in every iteration. I converted the range to an array to minimize interaction with the sheet. Here are the results using the same data set:

1. (Rng) - Total rows: 100,001 in  8.851 sec
2. (Arr) - Total rows: 100,001 in  0.398 sec

1. (Rng) - Total rows: 500,001 in 43.578 sec
2. (Arr) - Total rows: 500,001 in  2.023 sec

   Invalid cells: 77,780 (out of 100,000)

.

the code

Option Explicit

Public Sub CompareValues()

    Const MAX_SZ    As Byte = 240
    Const COL_ID    As Byte = 1
    Const COL_LTR   As String = "A"
    Const FIRST_ROW As Byte = 2

    Const SRCH_LST  As String = "%,%O,D,CMS"
    Const LST_START As Byte = 0
    Const LST_END   As Byte = 3 '165

    Dim ws As Worksheet
    Dim thisRow As Long
    Dim thisItm As Long
    Dim invalidSet As Long
    Dim tmp As String
    Dim lastRow As Long
    Dim colArr As Variant
    Dim searchItem As Variant
    Dim isValid As Boolean
    Dim invalidArr() As String

    Set ws = ActiveWorkbook.Worksheets("Sheet1")
    With ws
        lastRow = .Cells(.Rows.Count, COL_ID).End(xlUp).Row
        colArr = .Range(.Cells(FIRST_ROW, COL_ID), .Cells(lastRow, COL_ID)).Value2
    End With
    searchItem = Split(SRCH_LST, ",")
    ReDim Preserve invalidArr(lastRow)
    invalidSet = 0

    For thisRow = 1 To lastRow - FIRST_ROW + 1
        isValid = False
        For thisItm = LST_START To LST_END  'compare
            If colArr(thisRow, COL_ID) = searchItem(thisItm) Then
                isValid = True
                Exit For
            End If
        Next
        If Not isValid Then     'if not valid build string of addresses (A1,A3,A7,...)
            tmp = tmp & COL_LTR & thisRow + FIRST_ROW - 1 & ","
            If Len(tmp) > MAX_SZ Then               'if string length > 240, split it
                invalidArr(invalidSet) = Left(tmp, Len(tmp) - 1)   'remove last comma
                invalidSet = invalidSet + 1
                tmp = vbNullString
            End If
        End If
    Next

    If Len(tmp) > 0 Or invalidSet > 0 Then      'if there are invalid values
        Dim totalSets As Long
        totalSets = invalidSet

        If Len(tmp) > 0 Then                    'store the last set, if tmp not empty
            invalidArr(invalidSet) = Left(tmp, Len(tmp) - 1)
        Else
            totalSets = totalSets - 1
        End If
        ReDim Preserve invalidArr(totalSets)    'cleanup (not needed)
        With ws
            For invalidSet = 0 To totalSets     'change cell colors in sets of ranges
                .Range(invalidArr(invalidSet)).Interior.Color = 192
            Next
        End With
    End If

End Sub


Find the sum of prime numbers below 2 million This code is trying to find the sum of prime numbers below 2 million. The code does work as intended, but it takes a very long time ( 6 minutes) for the code to execute completely. What is the problem and how can I make it more efficient?

#include <iostream>
using namespace std;

unsigned long summationOfPrime();
bool isPrime(int n);
int main() {

cout <<summationOfPrime() << endl;

}

unsigned long summationOfPrime()
{
const int num = 2000000;
unsigned long sum = 0;
for(int i = 2; i < num; i++)
{
    if(isPrime(i) == true)
    {
        sum += i;

    }
}

return sum;
}

bool isPrime(int n)
 {
// Corner case
if (n <= 1)
    return false;

// Check from 2 to n-1
for (int i = 2; i < n; i++)
    if (n % i == 0)
        return false;

return true;
}

There are several issues, but here goes. Firstly, your indentation is all wonky. Some of this may be due to trying to input stuff here, but either way, indentation helps legibility.

Next, the biggest thing you can change to speed thing up is to replace for (int i = 2; i < n; i++) with for (int i = 2; i*i <= n; i++). This works because if \$a \cdot b = n\$, one of them is at most the square root of \$n\$. This change will probably make your code fast enough on its own.

That said, the much better solution to this type of problem is to use a prime sieve such as the Sieve of Eratosthenes which will generate all of the primes less than 1,000,000 faster by using addition and multiplication instead of modulo operations which are slower.


Tic Tac Toe game tree generator minimax I have coded a working Tic Tac Toe game tree generator. It doesn't only generate the game tree, it also applies minimax values to each node. The code works fine on a board that already has 2 or more played moves but slows down significantly for an empty board. I'm thinking of implementing alpha-beta pruning.

I am, however, having a hard time figuring out how I could modify my existing code to accommodate that. Can anyone point me in the right direction or suggest other improvements that may help with performance?

Notes: I am representing game states as a string in this format "0,0,0,1,0,0,-1,0,0" where 1 represents player X, -1 represent player O and 0 represents and empty square.

// Node constructor
var Node = function(state, parent, toPlay) {
  this.toPlay = toPlay;
  this.bestValue = null;
  this.state = state;
  this.parent = parent;
  this.child = [];
};

// Tree constructor
var Tree = function(state, toPlay) {
  this._head = new Node(state, null, toPlay);
};

// Possible winning combinations
var winStates = [
  [0, 1, 2],
  [3, 4, 5],
  [6, 7, 8],
  [0, 3, 6],
  [1, 4, 7],
  [2, 5, 8],
  [0, 4, 8],
  [6, 4, 2]
];

// Check if game has been won
var checkIfWon = function(gameState, mark) {
  // Convert gameState string to array
  gameState = gameState.split(",").map(function(c) {
    return parseInt(c, 10)
  });
  // Iterates thru the winning combos to see if one is active
  var winner = winStates.some(function(combination) {
    var winning = true;
    for (var y = 0; y < combination.length; y++) {
      if (gameState[combination[y]] !== mark) {
        winning = false;
      }
    }
    return winning;
  });
  return winner;
};

// Check if game is a draw
var checkIfDraw = function(gameState) {
  return possibleMoves(gameState).length === 0;
}

// Check possible moves
var possibleMoves = function(gameState) {
  // Convert gameState string to array
  gameState = gameState.split(",").map(function(c) {
    return parseInt(c, 10)
  });
  return gameState.reduce(function(p, c, i) {
    if (c === 0) {
      p.push(i);
    }
    return p;
  }, []);
}

// Populate game tree
var populateTree = function(currentNode, toPlay) {
  if (checkIfWon(currentNode.state, 1)) {
    currentNode.bestValue = 1;
    return;
  }
  if (checkIfWon(currentNode.state, -1)) {
    currentNode.bestValue = -1;
    return;
  }
  if (checkIfDraw(currentNode.state)) {
    currentNode.bestValue = 0;
    return;
  }
  // Generate possible next moves
  var possible = possibleMoves(currentNode.state);
  for (var i = currentNode.state - 1; i >= 0; i--) {
    possible.push(i);
  }

  while (possible.length) {
    // Selects the next move randomly
    var move = possible.splice(Math.floor(Math.random() * possible.length),
      1)[0];

    // Updates the new game state
    var val = currentNode.state.split(",").map(function(c, i) {
      if (i === move) {
        return toPlay;
      }
      return parseInt(c, 10)
    }).join();

    // Create a new child Node
    currentNode.child.push(new Node(val, currentNode, toPlay * -1));
    nodeCount++;
  }

  // Recursive call
  for (var j = 0; j < currentNode.child.length; j++) {
    populateTree(currentNode.child[j], toPlay * -1);

  }

  // Assign bestValue according to the value of child Nodes
  if (currentNode.toPlay === 1) {
    currentNode.bestValue = currentNode.child.reduce(function(p, c) {
      return p > c.bestValue ? p : c.bestValue;
    }, -1);

  } else {
    currentNode.bestValue = currentNode.child.reduce(function(p, c) {
      return p < c.bestValue ? p : c.bestValue;
    }, 1);
  }

};

var nodeCount = 0;
tree = new Tree("0,0,0,1,0,0,-1,0,0", 1);
populateTree(tree._head, tree._head.toPlay);
//console.log(tree);
console.log(nodeCount);

Optimization ideas

Prevent wasteful data conversions

Don't convert from a string to an array. You could do better by simply storing your state as an array instead.

I suppose you chose to use a string since they are easily copied, but arrays are also easily copied with:

copy = old.slice(0);

You could do even better by keeping the state as a string; albeit with some modifications to the model. My only concern is the -1 since it's two characters. Using X, O, and - is a reasonable option, and it keeps the indices of the characters in the correct place.

"O-OXXX---" // a win for X

You'll also have some trouble with the high order functions since strings don't have them. Speaking of which...

High order functions come with a cost

While forEach, map, and some are awesome functions (and one of my favorite parts of JavaScript), they come with quite a bit of overhead compared to their for loop equivalents. The issue comes with the need to execute a callback function over and over as opposed to just running inline code.

Since your program is computation heavy,  you probably want speed over elegance. Converting all the high order functions to for loops should help considerably.

Certain states are duplicates of others

You could use a JavaScript object to keep track of the past states, so you don't have to build redundant tree branches.

Something like:

stateMap = {};
// ...
state = makeMove(possibleMove);

if (stateMap[state] !== undefined) {
  // check state ...
  stateMap[state] = node;
  // recurse next possible moves ...
}
else {
  node.child = stateMap[state];
}

This way if a state ever comes up again, it will be defined in stateMap, and you can reuse the node. However, the parent pointers may become a bit muddled, so you might want to keep a list of parent nodes, or preform a deep copy on the child's branch.

Other optimization ideas

Don't check every way to win each time. Each move only involves a subset of the possible ways.
Certain states are merely rotations of others, but that might be hard to utilize.

Review

Modularize

Your code is global and scattered. You could store it in a function to make it more portable and scalable:

var ticTacToeTree = (function () {
    function Node(state, toPlay) {
        this.toPlay = toPlay;
        this.bestValue = null;
        this.state = state;
        this.parents = [];
        this.children = [];
    };

    var head = new Node("---------", 'X');

    // The rest of your code

    return {
        head: head
    };
})();

Reusable code

checkIfWon could be used to check both players:

function checkWinner(state) {
  for (var i = 0; i < winStates.length; i++) {
    if ("-" !==
         state[winStates[i][0]] ===
         state[winStates[i][1]] ===
         state[winStates[i][2]]) {
      return state[winStates[i][[0]];
    }
  }
  return "-"; // no one has won
}

Which prevents redundant calls:

function populateTree(currentNode, toPlay) {
  var winner = checkWinner(currentNode.state);

  if (winner === "-" && checkDraw(currentNode.state)) {
    return currentNode.bestValue = 0;
  }
  else if(winner === "X") {
    return currentNode.bestValue = 1;
  }
  else if(winner === "O") {
    return currentNode.bestValue = 1;
  }

  // ...
}

Don't worry about getting a random move; you end up computing them all anyways:

for (var i = 0; i < moves.length; i++) {
  move = moves[i];

  nextState = state.slice(0, move).concat(state.slice(move));

  if (stateMap[nextState] !== undefined) {
    var node = new Node(state, nextTurn(toPlay);
    node.parrents.push(currentNode);
    currentNode.children.push(node);
    populateTree(node, node.toPlay);
  }
  else {
    currentNode.children.push(stateMap[nextState]);
  }
}


Removing duplicates from an array I recently wrote this JavaScript algorithm for removing duplicates from an array as part of a job interview process, but was turned down for the position after submitting the code. I didn't receive any detailed feedback from the company, so I was wondering if a more experienced developer might be able to explain what I've done wrong here.

dedup = function(set){
    //loop over function, removing second instance of matching values
    for (var i = 0; i < set.length; i++){
        for (var j = i+1; j < set.length; j++){
            if (set[i] == set[j]){
                set.splice(j,1);
                i--;
            }
            //special case for objects
            if (typeof set[j] == 'object' && typeof set[i] == 'object')
                {
                //super special case for arrays
                if (Array.isArray(set[j]) && (Array.isArray(set[i])))
                {
                    if ( set[j].toString() == set[i].toString() )
                        { set.splice(j,1);
                        i-- }
                }
                //loop over key-value pairs in sets of objects
                else {
                    var firstObj = set[i];
                    var secondObj = set[j];
                    for (var key in firstObj){
                        //break loop if differences found
                        if (firstObj.hasOwnProperty(key) == false) {
                            break
                        }
                        else if (firstObj[key] != secondObj[key]){
                            break
                            }
                        else {
                            set.splice(j,1);
                            i--
                            break
                        }
                    }
                }
            }
        }
    }
    return set
}

console.log(dedup([1, 2, 2, 5, 2, 7, 3, 9, 1, 9]))
// -> [1, 2, 5, 7, 3, 9]

console.log(dedup([1, '1', 2, 0, false, true]))
// -> [1, '1', 2, 0, false, true]

console.log(dedup([8, 3, [1, 2], [1, 2, 3], [1, 2], [2, 1], true]))
// -> [8, 3, [1, 2], [1, 2, 3], [2, 1], true]

console.log(dedup([1, {a: 'b', c: 'd'}, {c: 'd', a: 'b'}, {e: 'f'}, 'b']))
// -> [1, {a: 'b', c: 'd'}, {e: 'f'}, 'b']

I've found the following possible problems:

Running time is \$\mathcal{O}(n^2)\$ - there may be a recursive algorithm which removes duplicates more effectively than nested for loops.
Code should be refactored rather than all in on big function.

I would greatly appreciate any thoughts on the problems that I've identified, or other issues with my code.

I'll start by trying to address your concerns.

Unless in special cases you can't make this function run in less than quadratic time.  Typical linear time solutions in other languages involve generating hashes from values in the filtered array and then using a hash-table to filter out characters whose hash value appears to be the same.  There is no standard way to obtain a hash of an object in JavaScript, however, some objects may be uniquely written to string, which can be used as their hash.  So, there is an O(n) solution for an array of numbers, or an array of strings, but no such solution for an array of functions.  JavaScript makes no guarantees with regard to transforming tail calls into iterations, so, if possible, an iterative solution would be better than recursive one.
Yes, it's certainly better to let one function do one thing.

Now, to the interviewer reaction: Passing an interview and writing good code aren't necessary (and, most unfortunately, aren't in most cases) the same thing.  Interviewers typically want to see the code written in a particular way they believe it should be written.  If your goal is to pass the interview, it is in your best interest to find out the exact requirements.  Sad but true.

Now, assuming I would have to assess your code, here are the things that I'd find to be bad:

The code is too lengthy.  It shouldn't take that much space to write it. (I later give my version of the answer).
Inconsistent spacing and brace positioning.  This is certainly not a crime, but remember that I'm trying to assess your code from small cues.  I wouldn't mind this as much, if, for example, I knew that the code was a result of several people working on the same code base, using different editors etc.
Changing the iterator inside the loop.  Call me superstitions, but I'd make every possible effort to avoid it.
Comparison with Boolean constants.  There is no need to do that (unless for diagnostic purposes).  The language was designed in such a way that you never need to compare anything to true or false.  But if you will continue writing JavaScript code, you will find that a lot of people don't understand this trivial concept.
You've decided to make the algorithm a destructive one.  I.e. the function modifies the argument passed to it.  There are reasons for having both versions.  I, myself, expect the default to be non-destructive.
slice isn't really a way to go here.  Not that it matters a lot on JavaScript, since arrays are typically implemented as hash-tables with integer keys, but, in general, the properties of array data structure make repeated slicing a lot more expensive than maintaining the index of last insertion (example follows).
Using typeof and other kinds of ad hoc typechecking to me always smell bad.  There is a place for it, but in the context of this task it isn't required.

Non-destructive version:

function removeDuplicates(array, comparator) {
    var result = [], i;
    function insert() {
        var match, j;
        if (comparator) {
            for (j = 0; j < i; j++) {
                if (comparator(array[j], array[i])) {
                    match = false;
                    break;
                }
            }
        } else match = result.indexOf(array[i]) == -1;
        return match;
    }

    for (i = 0; i < array.length; i++)
        if (insert()) result.push(array[i]);
    return result;
}

Destructive version:

function deleteDuplicates(array, comparator) {
    var i, k = 1;
    function insert() {
        var match, j;
        if (comparator) {
            for (j = 0; j < k; j++) {
                if (comparator(array[j], array[i])) {
                    match = false;
                    break;
                }
            }
        } else match = array.lastIndexOf(array[i], k) == -1;
        return match;
    }

    for (i = 1; i < array.length; i++)
        if (insert()) array[k++] = array[i];
    array.length = k;
    return array;
}

Note that it would be possible to make it even shorter, if I knew that user-provided comparison function isn't needed (isn't part of the task).  I still chose to include the possibility, even though I wasn't sure about it because it seems like a good thing to include one in the function's interface.  If I had to answer this interview question, I'd probably not bother writing a custom comparator just to save time and spare myself the possibility of making a silly error working on something which wasn't absolutely required.


Calling a MySQL procedure with an arbitrary parameter on a GoDaddy website I'm deploying a PHP project, and I always try to use prepared statements, but unfortunately, GoDaddy does not have the mod mysqlnd which I use it for the function mysqli_stmt_get_result($stmt), so I had to start using mysqli_query in order to use the classic.

Example:

while($row = mysqli_fetch_array($query,MYSQLI_ASSOC)){
   echo $row['Name'];
}

I've been using mysqli_query, but I'm not expert in security, so could you please evaluate or give me some tips?

<?php
   include '../session.php';
   include '../connection.php';
   $link = mysqli_real_escape_string($connection,$_GET['file']);
   $query = mysqli_query($connection,'CALL mostrarConvocatoriaLink("'.$link.'")');
   if(!$query){
     echo mysqli_error($connection);
   }
   else{
     $row = mysqli_fetch_array($query,MYSQLI_BOTH);
     extract($row);
   }
   mysqli_close($connection);
?>

This is used to get an entry on my database using a procedure (which is only a Select statement) but, we're using a random generated string to create the links, instead of using the ID of the page. This is for using a kind of a WordPress that we're creating for the project. In my project all the inserts are with prepared statements and using Post, but in this case, I create a <a href="template.php?file=[the random string]">. I wanna know if I'm screwing things up.

As a rule of thumb in code security if you don't know what attacks you've secured your code from then it is not secure.  At least you're considering that your code might have issues and looking to remedy them when many don't.

Basically consider the ID 7254872364;drop table users;// or some variant - what SQL statement is that going to produce as a string and is it what you thought would get?  I should get that will get through real_escape_string quite nicely.

One thing that will help is to rewrite your code to use prepared statements.  They are not a magic bullet (nothing is) but they will give you code that is an order of magnitude more secure than you have today.

These questions on Security StackExchange should make for some interesting further reading:-

SQL injection — but why isn't escape quotes safe anymore?
Are prepared statements 100% safe against SQL injection?

[Edit: Sorry, missed that your host doesn't provide support for prepared statements.  GoDaddy do support it but you have to enable it in the control panel.  See Enable MYSQL-PDO in godaddy windows server and if you can't get it solved it might be best to tell GoDaddy where to go]

It's a good practice to assume that all user input is hostile until it you've established otherwise, which doesn't seem to be happening in this code.  Sanitize anything that comes from outside your system at every point of entry interaction including those pesky Url parameters, fields, cookies - at some point the odds are that it will be hostile.  If the ID's will take a certain form (x digits for example) then validate them and stop processing with an error if that validation fails.


Selecting multiple checkboxes to show similar but different boxes I have 3 checkbox's with 8 different combinations and outcomes.

Selecting no checkboxes shows no divs.
Selecting one checkbox shows the colored div.
For example; clicking the red checkbox.
Selecting two checkboxes shows the color1+color2 div. But doesn't show either of the single colored divs.
For example; clicking the red and green checkboxes shows the redgreen div. But ensures both the red and green div boxes are hidden so there's only ever one div box showing.
Selecting all the checkboxes shows the rgb div. And doesn't show any other divs.

It does work I was wondering if this can be wrote more efficient?

$('input[type="checkbox"]').click(function () {
    $('.box').hide();
if ($('#red').is(':checked')) $('.red.box').show();
if ($('#green').is(':checked')) $('.green.box').show();
if ($('#blue').is(':checked')) $('.blue.box').show();

if ($('#red').is(':checked') && $('#green').is(':checked')) $('.redgreen.box').show() && $('.red.box, .green.box').hide();

if ($('#red').is(':checked') && $('#blue').is(':checked')) $('.redblue.box').show() && $('.red.box, .blue.box').hide();

if ($('#green').is(':checked') && $('#blue').is(':checked')) $('.greenblue.box').show() && $('.green.box, .blue.box').hide();

if ($('#red').is(':checked') && $('#green').is(':checked') && $('#blue').is(':checked'))  $('.rgb.box').show() && $('.red.box, .green.box, .greenblue, .redblue, .redgreen.box, .blue.box').hide();

});
.box {
    padding: 20px;
    display: none;
    margin-top: 20px;
    border: 1px solid #000;
}

.redblue .redgreen .greenblue {
    border: 1px solid black;
}
.rgb {
    border: 1px dashed black;
}
.red {
    background: #ff0000;
}
.green {
    background: #00ff00;
}
.blue {
    background: #0000ff;
}

        <input type="checkbox" id="red" name="colorCheckbox" value="red">red

        <input type="checkbox" id="green" name="colorCheckbox" value="green">green

        <input type="checkbox" id="blue" name="colorCheckbox" value="blue">blue

red

green

blue

red green

red blue

green blue

rgb

A few notes on your code:

If you don't need supporting of very old or non-standard browsers, and given that your project does not relies heavily on jQuery, vanilla JavaScript can do the same work, without jQuery's overhead.
On each checkbox click, all coloured divs are queried again and again (jQuery('.box')), which is unnecessary as there are no changes in the DOM.
All checkboxes are queried multiple times, instead of selecting them once.
The if-then structure is quite cumbersome: if #red is checked, show .red.box. Then, if #green is also checked, hide .red.box and show .redgreen.box. Then, if #blue is checked too, hide .redgreen.box and show .rbg.box. One way for improving this part would be to first determine exactly which box should be checked and show only that box.
Personally, I would have used combinations of the .red/.green/.blue classes, instead of putting a different class for each combination (e.g. .box.red.green.blue instead of .box.rgb). That way each checkbox corresponds with a particular div class.

Here is my take on the problem/homework/exercise, that addresses most of the above remarks (no CSS/HTML changes; still using jQuery):
        const RED = 1;
        const GREEN = 2;
        const BLUE = 4;

        const DIV_CLASSES_MAP = [null, '.red', '.green', '.redgreen', '.blue', '.redblue', '.greenblue', '.rgb'];
        const DIVS = $('.box');
        const RED_TICK = $('#red');
        const GREEN_TICK = $('#green');
        const BLUE_TICK = $('#blue');

        let activeDiv = DIVS.filter(':visible');
        $('input[name="colorCheckbox"]').on('click', onCheckboxChange);

        function onCheckboxChange() {
            const redBit = RED_TICK.is(':checked') ? RED : 0;
            const greenBit = GREEN_TICK.is(':checked') ? GREEN : 0;
            const blueBit = BLUE_TICK.is(':checked') ? BLUE : 0;
            const mask = redBit + greenBit + blueBit;
            const activeClass = DIV_CLASSES_MAP[mask];
            activeDiv.hide();
            activeDiv = DIVS.filter(activeClass).show();
        }


A method that shows maps users are a part of A little background:

My app has two models called mapgroups and usergroups. A map belongs to a group and a user belongs to group. You can go in a group and add maps which is how maps are "assigned" to a user. These will show up on their dashboard.

class User < ApplicationRecord
  def fetch_maps
    @groups = self.groups
    @maps = Array.new

    @groups.each do |g|
      g.maps.each do |m|
        @maps << m
      end
    end

    return @maps
  end
end

In order to display the maps the user should have shown in their dashboard I created a fetch_maps method to call on the current user.

Im wondering if there is any way to clean this up or really just any feedback. This is the first method I have written with current Ruby knowledge.

Edit:

Mapgroup Model

class Mapgroup < ApplicationRecord
  belongs_to :map, optional: true
  belongs_to :group, optional: true
end

Usergroup Model

class Usergroup < ApplicationRecord
  belongs_to :user, optional: true
  belongs_to :group, optional: true
end

This would potentially be a very expensive way to do this as you have to query the dtabase once for every group the user is in, i.e. an n+1 query. You should be able to do it with a single query by creating a relation:

class User < ApplicationRecord
  belongs_to :usergroup
  has_many   :groups,     through: :usergroup
  has_many   :maps,       through: :groups

then you can just query the map relation.  An alternative would be to use a join. There is more than one way to do this but you could do:

def fetch_maps
  Map.joins(:mapgroups => {:groups => :users} ).where(user: self)
end

Rails doesn't always run the queries the way you expect so I suggest you run your code in the console or watch your log files to make sure that you are only running one query.


Fetch and print all properties of an object graph as string Below I have setup an extension method that takes any object, cycles through its properties, and prints each out to a Console window in the format Name: Value.

  Specification / Scope:

  Handle only single objects (no collections)

code

public static string PropertiesToString<T>(this T obj, int tabs = 0) where T : class
{
    int initTabs = tabs;
    string result = string.Empty;
    PropertyInfo[] propertyInfo = obj.GetType().GetProperties();
    foreach (PropertyInfo property in propertyInfo)
    {
        string name = property.Name;
        object value = property.GetValue(obj, null);
        Type valueType = value.GetType();
        if (valueType.IsValueType || valueType.Name.Equals("String"))
        {
            for (int i = 0; i < tabs; i++)
            {
               result += "    ";
            }
            result += string.Format("{0}: {1}\n", name, value == null ? string.Empty : value.ToString());
        }
        else
        {
            result += string.Format("{0}:\n", name);
            result += value.PropertiesToString(++tabs);
        }
        tabs = initTabs;
    }
   return result;
}

Here is a class I'm using to test this code, along with the lines responsible for creating an instance of the class and writing its properties to the Console:

Class:

public class Circle : IShape
{
    public Circle(double x, double y, double radius)
    {
        Center = new Point
        {
            X = x,
            Y = y
        };
        Radius = radius;
    }

    public Point Center { get; set; }
    public double Radius { get; set; }

    public double Area(int precision = 2)
    {
        return Math.Round(Radius * Radius * Math.PI, precision);
    }
}

Main:

public static void Main(string[] args)
{
    IShape circle = new Circle(5, 5, 10);
    Console.WriteLine(circle.PropertiesToString());
    Console.ReadLine();
}

The above method will also cycle through nested objects and print those to the Console as well, adding in tabs for readability's sake.

I'm kind of unfamiliar with System.Reflection and was wondering if there was a more efficient way I could approach doing something like this.

You should have a guard clause against someone passing in null

PropertyInfo has a PropertyType and you should use that instead of

 Type valueType = value.GetType();

as if value is null you will get a null reference error where PropertyType will give you the type of the property.

You will need to fix this line as well if value is null.  Again you will get a null reference error

 result += value.PropertiesToString(++tabs);

Better to compare types then their names.  Instead of

 valueType.Name.Equals("String")

use

 valueType == typeof(string)

You should separate out gathering the properties and displaying them.  I would have an extension method that returned IEnumerable> and then you could use linq to convert that into the strings and how you want it displayed.

Other things to consider is if you have two object that reference each other you will get a stack overflow.

I would recommend looking at https://docs.microsoft.com/en-us/dotnet/api/system.runtime.serialization.objectmanager?view=netframework-4.8 to see an example of an object walker.  I think making the class IEnumerable is a bit confusion when learning and they would have been better making a method that returns IEnumerable.  still a good place to start when learning about walking an object graph.


Converting string elements of an array to upper and lower case, maintaining names of keys I'm just going through some array exercises in PHP on w3resource. I have just completed:

Sample array :
$Color = array('A' => 'Blue', 'B' => 'Green', 'c' => 'Red');
Expected Output :
Values are in lower case:
Array ( [A] => blue [B] => green [c] => red )
Values are in upper case:
Array ( [A] => BLUE [B] => GREEN [c] => RED )

Here is my answer:
<?php
    echo "
";

    $Color = array('A' => 'Blue', 'B' => 'Green', 'c' => 'Red');

    echo "Sample array:
";
    print_r($Color);

    function toLower($value)
    {
        return strtolower($value);
    }

    function toUpper($value)
    {
        return strtoupper($value);
    }

    echo "
Values are in lower case:
";
    print_r(array_map("toLower", $Color));

    echo "
Values are in upper case:
";
    print_r(array_map("toUpper", $Color));

    echo "
";
?>

And its output:
Sample array:
Array
(
    [A] => Blue
    [B] => Green
    [c] => Red
)

Values are in lower case:
Array
(
    [A] => blue
    [B] => green
    [c] => red
)

Values are in upper case:
Array
(
    [A] => BLUE
    [B] => GREEN
    [c] => RED
)

Some questions:

Originally I thought about using implode to convert the elements to a string and then set the case of the entire string before explodeing back to an array; but of course this loses the names of the keys (A, B, c). Is there any possible way to somehow use implode and explode to get around this (convert to string while somehow keeping key names in memory)?

As the questions call explicitly for a function rather than a program, should this sort of operation be done with one function rather than the two which I have implemented here?

Is this an appropriate usage of array_map?

w3resource's sample answer makes use of statements involving is_array. Is it good practise to implement such guard conditions in my own code at this point?

Is there anything about my code which looks strange to you, or could easily be improved?

Is there any possible way to somehow use implode and explode to get around this (convert to string while somehow keeping key names in memory)?

You can get all the keys using array_keys then combine them with array_combine.

$upperCase = array_combine(array_keys($Color), explode(',', strtoupper(implode(',', $Color))));
print_r($upperCase);

But why do all these efforts? Your solution is perfect, you could not do better.

  should this sort of operation be done with one function rather than the two

If you have no need to do anything else but convert the text in lowercase/uppercase, then just call

print_r(array_map('strtoupper', $Color));

  Is it good practise to implement such guard conditions in my own code at this point?

It is advisable when you do not know the data you're working. It is not in this case.


Drawing scanline of an image This draws scanline of the image to System.Drawing.Graphics. It's quite simple and it's optimized in order to merge neigbour samples with the same color into single rectangle (instead of drawing sample-by-sample in 1x1 rectangles).

I don't like the duplication of this code inside and outside the loop:

updateAlpha(brush, previousColor);
graphics.FillRectangle(brush, previousIndex + positionX, y, i - previousIndex, 1);

Do you know the good way how to follow DRY principle here and to keep code easy to understand? Or this is a kind of perfectionism and all is already fine?

private static void drawGlyphRowToGraphics(int rowIndex, FT_Bitmap glyphBitmap,
    Graphics graphics, float positionX, float positionY, SolidBrush brush)
{
    float y = rowIndex + positionY;

    byte previousColor = 0;
    int previousIndex = 0;
    for (int i = 0; i < glyphBitmap.Width; ++i)
    {
        byte color = (byte)(255 - glyphBitmap.Buffer[i + rowIndex * glyphBitmap.Width]);
        if (i == 0)
            previousColor = color;

        if (color != previousColor)
        {
            updateAlpha(brush, previousColor);
            graphics.FillRectangle(brush, previousIndex + positionX, y, i - previousIndex, 1);

            previousColor = color;
            previousIndex = i;
        }
    }

    if (glyphBitmap.Width > 0)
    {
        updateAlpha(brush, previousColor);
        graphics.FillRectangle(brush, previousIndex + positionX, y, glyphBitmap.Width - previousIndex, 1);
    }
}

private static void updateAlpha(SolidBrush brush, byte gray)
{
    brush.Color = Color.FromArgb(255 - gray, brush.Color.R, brush.Color.G, brush.Color.B);
}

First off I have to say I found this code very hard to understand. It took me quite some time to finally understand what this code actually does.

First off the function takes 6 arguments. That's way too many. Clearly the positionX and positionY should be combined into one Point object.

But the larger problem is, that you seem to be working at the wrong level of abstraction. The function is supposed to deal with one single row of a FT_Bitmap, but because there is no abstraction of such a single row, it ends up dealing with the whole FT_Bitmap, doing confusing offset calculations all over the place.

What I would do first, is to create a separate GlyphRow class:

public class GlyphRow
{
    private FT_Bitmap bitmap;
    private int offset;

    public int Width;

    public GlyphRow(FT_Bitmap bitmap, int rowIndex)
    {
        this.bitmap = bitmap;
        this.offset = rowIndex * bitmap.Width;
        this.Width = bitmap.Width;
    }

    // Returns the transparency value of a point in position x of our row
    public byte Alpha(x)
    {
        return bitmap.Buffer[offset + x];
    }

}

This will allow us to work with a single row at a time.

Now our function can take just the following arguments:

private static void drawGlyphRowToGraphics(
    GlyphRow row, Graphics graphics, Point position, SolidBrush brush)

And when calling it, we initialize Point and GlyphRow objects:

GlyphRow row = new GlyphRow(glyphBitmap, rowIndex);
Point position = new Point(positionX, rowIndex + positionY);
drawGlyphRowToGraphics(row, graphics, position, brush);

Note that the position is now exactly the position where this GlyphRow should be painted, already offset with rowIndex.

So here's the new simplified drawGlyphRowToGraphics:

private static void drawGlyphRowToGraphics(
    GlyphRow row, Graphics graphics, Point position, SolidBrush brush)
{
    int i = 0;
    while (i < row.Width)
    {
        int length = sameAlphaLength(row, i);
        brush.Color = cloneColorWithAlpha(brush.Color, row.Alpha(i));
        graphics.FillRectangle(brush, position.X + i, position.Y, length, 1);
        i += length;
    }
}

I found the algorithm in original implementation very hard to grasp, so using divide-and-conquer strategy, I created a separate function to calculate the length of a section that has the same transparency value:

private static int sameAlphaLength(GlyphRow row, int start)
{
    byte alpha = row.Alpha(start);
    int length = 1;
    while (start + length < row.Width)
    {
        if (alpha != row.Alpha(start + length))
        {
            return length;
        }
        length++;
    }
    return length;
}

In the original code, at first the gray value was calculated with (255 - alpha), and then passed to updateAlpha, where it was again transformed with (255 - gray). Pretty redundant IMHO. So I'm skipping this whole double-conversion. I also find it better to work with functions without side-effects, so I replaced updateAlpha with a function that creates a new color value from an existing one, which I can then assign to brush:

private static Color cloneColorWithAlpha(Color color, byte alpha)
{
    return Color.FromArgb(alpha, color.R, color.G, color.B);
}

Disclaimer: I've never written a single line of C# before. It probably doesn't compile, not to mention other possible bugs. But hoping to get the general point across.


Rating tennis players in a database, taking days to run I have this project in data analysis for creating a ranking of tennis players. Currently, it takes more than 6 days to run on my computer.

Can you review the code and see where's the problem?

Project steps:

I have a database of 600,000 tennis matches called matchdatabase.The database fileds are a) winner name, b) loser name, c) tournament, d) other fields for the winner and loser.
From that database, I create a playerdatabase with every player in the matchdatabase.
For each match in the matchdatabase it goes into the playerdatabase, retrieves the ranking/elo and computes the expected result.
It updates the ranking after the match into the playerdatabase

This for loop ends up running 1 match/second, so the whole database takes several days to run!

import pandas as pd
import glob
import numpy as np
import math

all_data = pd.read_csv('tennisdatabase.csv')
all_data = all_data.sort(['date'], ascending=[0])
all_data = all_data.reindex(index = np.arange(1, len(all_data) + 1))

#it checks every player in the matchdatabase and creates a database of players

playerdatabase = pd.DataFrame()
list_winners = pd.pivot_table(all_data,index=["winner_name"],values=["tourney_id"],aggfunc=np.count_nonzero)
list_losers =  pd.pivot_table(all_data,index=["loser_name"],values=["tourney_id"],aggfunc=np.count_nonzero)
firstloss =  pd.pivot_table(all_data,index=["loser_name"],values=["date"],aggfunc=np.min)
firstwin =  pd.pivot_table(all_data,index=["winner_name"],values=["date"],aggfunc=np.min)
playerdatabase = pd.concat([list_winners, list_losers, firstloss, firstwin], axis=1)
playerdatabase['NumberOfGames'] = 0

#defines a elo calculator for expectations and modified ratings

def getExpectation(rating_1, rating_2):
    "calculator for the expected result to player 1 based on the rating of both players"
    calc = (1.0 / (1.0 + pow(10, ((rating_2 - rating_1) / 400.0))))
    return calc

def modifyRating(rating, expected, actual, kfactor):
    "gives the new rating given the current rating, expected results, actual results and k factor"
    calc = (rating + kfactor * (actual - expected));
    return calc

#Elo calculation for the database
#sets initial rating for everyone at 2100

playerdatabase['Rating'] = 2100

loser_k_factor = 30
winner_k_factor = 30

#loop  for the calculations

for i in xrange(0, 616242):

    #gets the rating for both players from the playerdatabase
    winner_rating = playerdatabase.loc[all_data.iloc[i]['winner_name'], 'Rating']
    loser_rating = playerdatabase.loc[all_data.iloc[i]['loser_name'], 'Rating']
    all_data['winner_elo'][i+1] = winner_rating
    all_data['loser_elo'][i+1] = loser_rating

    #gets the expected result for both players
    winner_expectation = getExpectation(winner_rating, loser_rating)
    loser_expectation = getExpectation(loser_rating, winner_rating)

    #gets the updated result for both players
    winner_new_rating = modifyRating(winner_rating, winner_expectation, 1, winner_k_factor)
    loser_new_rating = modifyRating(loser_rating, loser_expectation, 0, loser_k_factor)
    #updates the results for both players in the playerdatabase
    playerdatabase.loc[all_data.iloc[i]['winner_name'], 'Rating'] = winner_new_rating
    playerdatabase.loc[all_data.iloc[i]['loser_name'], 'Rating'] = loser_new_rating
    #updates the number of games for both players in the playerdatabase
    playerdatabase.loc[all_data.iloc[i]['winner_name'], 'NumberOfGames'] = playerdatabase.loc[all_data.iloc[i]['winner_name'], 'NumberOfGames'] + 1
    playerdatabase.loc[all_data.iloc[i]['loser_name'], 'NumberOfGames'] = playerdatabase.loc[all_data.iloc[i]['loser_name'], 'NumberOfGames'] + 1

    #records the rating list every 500 calculations

    if i%500 == 0:
        playerdatabase[i] = playerdatabase['Rating']
        print i
    print i

playerdatabase = playerdatabase.sort(['Rating'], ascending=[1])

Without knowing what your csv file is structured, it is hard to give too much concrete.  I do have some suggestions, however.

You can most likely drastically increase performance by converting strings like the player names to categorical data.  Strings are slow in pandas, especially string lookup in a large column (as you have here many times).  Using categeorical data converts it to integers seamlessly behind-the scenes, so you can benefit from using strings while still have fast lookups.
You should loop over the rows rather than re-indexing so much.  In fact all you really need is the winner name and loser name from each match, which you can get at the beginning of each loop.
You may not be able to calculate the Rating all at once, but you can calculate Number of Games all at once by just counting how many times a player is a loser and adding that to how many times the same player is a winner.
Your other functions are one-liners.  This is probably a small part, but it would be better to not have them as functions at all.


Lucky Triples in Haskell In answering this question on Stack Overflow, I decided to stretch my legs in Haskell a bit to see if I could implement a solution to find the count of lucky triples in a list of ints.

A lucky triple is any triple (j, k, l) where j <= k <= l and l `mod` k == 0 && k `mod` j == 0. A correct implementation counts how many unique combinations of elements of an [Int] are valid lucky triples.

import Data.List (sort)

answer :: [Int] -> Int
answer xs = foldr ((+) . foldr ((+) . length) 0) 0 $ twoStepFactors
  where
    twoStepFactors = map mapFactors $ mapFactors $ sort xs

mapFactors :: [Int] -> [[Int]]
mapFactors xs = mapFactors' xs []
mapFactors' [] acc = acc
mapFactors' (x:xs) acc = mapFactors' xs newAcc
  where
    factors = filter ((==0) . (`mod` x)) xs
    newAcc | null factors = acc
           | otherwise = acc ++ [factors]

If mapFactors' is put in a where clause of mapFactors, it isn't globally accessible and you can call it go or something because the scoping already points out it belongs to mapFactors.

Swapping gos argument order lets you say mapFactors = go [].

++ [_] is a smell. Instead of passing down an accumulator and growing it to the right, you can pass it out as the return value and grow it to the left.

mapFactors :: [Int] -> [[Int]]
mapFactors = go where
  go [] = []
  go (x:xs) = newAcc $ go xs where
    factors = filter ((==0) . (`mod` x)) xs
    newAcc | null factors = id
           | otherwise = (:) factors

The filtering can happen after go is done.

mapFactors :: [Int] -> [[Int]]
mapFactors = filter (not . null) . go where
  go [] = []
  go (x:xs) = filter ((==0) . (`mod` x)) xs : go xs

tails helps express go in terms of map:

mapFactors :: [Int] -> [[Int]]
mapFactors = filter (not . null) . map go . tails where
  go [] = []
  go (x:xs) = filter ((==0) . (`mod` x)) xs

Let's use sum from that comment and inline the once-used twoStepFactors (imo if you're only giving a name to explain what something does, use comments)

answer :: [Int] -> Int
answer xs = sum $ map (sum . map length . mapFactors) $ mapFactors $ sort xs

There doesn't seem to be a reason to filter out the []s.

List comprehensions neatly let us skip the empty tail, and get rid of go.

mapFactors :: [Int] -> [[Int]]
mapFactors xss = [filter ((==0) . (`mod` x)) xs | x:xs <- tails xss]


Memory cache implementation I have created an open source memory cache called cachew and can be found here:
cachew. I would like your help to make it better.

Regarding the class Cache or its dependencies:

I am thinking of removing timeoutStyle and timeout from the Cache constructor and providing default values that can be changed via public properties. How do you like that?
How do you like my abstraction LockManager for ReaderWriterLockSlim?
Do you think my timer in Cache should be configurable? Through a property in cache, change internal to public or by another method?
Do you think it is a bad practice to start a timer in a constructor? If so, what would you do?
Would you consider the Cache threadsafe? Could it be better implemented?

public enum TimeoutStyle
{
    FixedTimeout,
    RenewTimoutOnQuery
}

public class Cache : ICache
{
    private readonly ITimer expirationTimer;
    private readonly IInternalCache internalCache;

    private readonly LockManager lockManager = new LockManager();

    public Cache(TimeoutStyle timeoutStyle, TimeSpan timeout) :
        this(new InternalCache(timeoutStyle, timeout), new SystemTimer(5000))
    {

    }

    internal Cache(IInternalCache iternalCache, ITimer expirationTimer)
    {
        if (iternalCache == null) throw new ArgumentNullException("iternalCache");
        if (expirationTimer == null) throw new ArgumentNullException("expirationTimer");
        this.internalCache = iternalCache;
        this.expirationTimer = expirationTimer;

        this.expirationTimer.Elapsed += ExpirationTimerElapsed;
        this.expirationTimer.Start();
    }

    public object Get<T>(CacheKey key, Func<T> func)
    {
        using (lockManager.EnterRead())
        {
            object existingValue;
            if (internalCache.TryGetValue(key, out existingValue))
                return existingValue;
        }

        using (lockManager.EnterWrite())
        {
            object existingValue;
            if (internalCache.TryGetValue(key, out existingValue))
                return existingValue;

            var newValue = func();
            internalCache.Add(key, newValue);
            return newValue;
        }
    }

    private void ExpirationTimerElapsed(object sender, EventArgs e)
    {
        using (lockManager.EnterWrite())
        {
            internalCache.RemoveExpiredItems();
        }
    }
}

I am thinking of removing timeoutStyle and timeout from the Cache constructor and providing default values that can be changed via public properties. How do you like that?

Does it make sense to change these properties in the middle of using the cache? Is it a desirable feature? If yes, go ahead. If not, and you just want to provide default values to make it easier to construct the class, then add a default constructor that calls the existing one.

I don't know enough C# to comment on the rest, I hope you'll get good reviews!


Simple Tree C++ implementation I've done this simple c++ assignment. The homework was "Design a Tree class that allows insertion of nodes and visit of the graph".

What do you think of the style/design I used? I chose to store in the STL container the pointers of sub trees.

Is it memory efficient?

Is it sufficiently readable?

Did I chose the right STL container?

Thanks a lot for any tips about problems or bad practices!

What do you think of the style/design I used? I chose to store in the STL container the pointers of sub trees.

Is it memory efficient?

Is it sufficiently readable?

Did I chose the right STL container?

Thanks a lot for any tips about problems or bad practices!

#include <set>
#include <deque>
#include <iostream>
  using namespace std;

template < typename T >
class Tree {

struct compare {
  bool operator()(const Tree * t1,
    const Tree * t2) const {
    return t1 -> GetContent() < t2 -> GetContent();
  }
};

typedef typename std::multiset < Tree * , typename Tree::compare > NodeSet;

private:
  NodeSet children;
T content;

public:

  Tree& AppendNode(const T& node) {
    Tree *t = new Tree(node);
    AttachTree(t);
    return *t;
  }
void Clear() {
  typename NodeSet::iterator it = children.begin();
  while (children.end() != it) {
    children.erase( *it);
    delete *it;
    it++;
  }

}
const T& GetContent() const {
  return content;
}
Tree(const T& root) {
  content = root;
}
void AttachTree(Tree* t) {
  children.insert(t);
}
void Visit(std::deque <T>& exp) const {
  exp.push_back(content);
  typename NodeSet::iterator it = children.begin();
  while (it != children.end()) {
    (*it) -> Visit(exp);
    it++;
  }
}
Tree() {}
Tree(Tree & c) {
  c.DeepCopyTo(this);
}
T & operator = (const Tree & b) {
  b.DeepCopyTo(this);
}
~Tree() {
  Clear();
}
void DeepCopyTo(Tree* dest) const {
  dest -> content = content;
  typename NodeSet::iterator it = children.begin();
  while (it != children.end()) {
    Tree* t = new Tree();
    (*it)->DeepCopyTo(t);
    dest->AttachTree(t);
    it++;
  }
}

 };

https://ideone.com/62Ggwu

Use using keyword to define your type instead of typedef

C++11 introduced type alias which can be used to define your own type and it's compatible with templates, so instead of:

typedef typename std::multiset < Tree * , typename Tree::compare > NodeSet;

you'll get this:

using NodeSet = std::multiset < Tree * , Tree::compare >;

Consider using range-based for loop instead of while loop with iterator

I think that it will result in simplier and more readable code, for example Visit function will look like this:

void Visit(std::deque <T>& exp) const {
  exp.push_back(content);
  for (const auto& child : children)
        child->Visit(exp);
}

Unnecessary erase call in Clear function

The following call:

children.erase( *it);

is unnecessary - multiset childern will be erased automatically.

Reconsider memory management

Currently it's possible to add new child to existing Tree by calling function

void AttachTree(Tree* t)

which expects pointer to existing Tree object as argument. It means that Tree is not repsonsible for creation of children. By the other hand, in destructor the Clear function is called which deletes all the children, which means that Tree takes responsibility for deletion of children. This might lead to undefined behaviour. Consider the following code:

Tree<int>* t1 = new Tree<int>(1);
{
    Tree<int> t2(2);
    t2.AttachTree(t1);
    {
        Tree<int> t3(3);
        t3.AttachTree(t1);
        std::cout << "1. t1 content: " << t1->GetContent() << "\n";
    } // t3 desctructor is called, which means that t1 has been deleted
    std::cout << "2. t1 content: " << t1->GetContent() << "\n"; // t1 doesn't exist anymore - what's the result of t1->GetContent() ?
} // t2 desctructor is called and t1 will be deleted again
std::cout << "3. t1 content: " << t1->GetContent() << "\n"; // t1 doesn't exist anymore - what's the result of t1->GetContent() ?


Improving performance in generating an Excel file I am using this code to write more than 170,000 rows to an Excel document. All in all, the file generation process takes about 3 minutes total. (I've already shaved off about ten minutes by shaving off overhead in the form of Apache POI and an unnecessary ArrayList) I'd like to shave off some more time, but I'm currently out of ideas.

public File generateDelimitedFile(String sql, String outputFilename, String delimiter, String bodyStringDelim) throws SQLException, IOException{
    File output = new File(outputFilename);

    try (

        Connection connection = DriverManager.getConnection(connectionUrl);
        Statement statement = connection.createStatement();
        ResultSet resultSet = statement.executeQuery(sql)
    ) {

        resultSet.setFetchSize(excelFetchSize);
        ResultSetMetaData meta = resultSet.getMetaData();
        int columnCount = meta.getColumnCount();

        StringBuilder headerBuilder = new StringBuilder();
        for (int i = 1; i < columnCount + 1; i++ ) {
            String name = meta.getColumnName(i);
            headerBuilder.append(name);
            if (i != columnCount) {
                headerBuilder.append(delimiter);
            }
        }
        FileUtils.writeStringToFile(output, headerBuilder.toString(), true);

        while(resultSet.next()) {
            StringBuilder lineBuilder = new StringBuilder();

            for (int column = 1; column <= columnCount; column++) {
                String value = resultSet.getString(column);
                if (value != null) {
                    if(bodyStringDelim != null){
                        lineBuilder.append(bodyStringDelim);
                    }
                    lineBuilder.append(value);
                    if(bodyStringDelim != null){
                        lineBuilder.append(bodyStringDelim);
                    }
                }

                if (column != columnCount) {
                    lineBuilder.append(delimiter);
                }
            }

            FileUtils.writeStringToFile(output, lineBuilder.toString(), true);
        }
        logger.info("Writing data to file");

    } catch (SQLException ex) {
        logger.error("Error generating file from SQL", ex);
        throw ex;
    } catch (IOException ex) {
        logger.error("Error writing file", ex);
        throw ex;
    }

    return output;
}

I'd like to shave off some more time, but I'm currently out of ideas.

Run the code in a profiler, to find out where the bottleneck is.  At a minimum, force the jvm to dump the thread stacks every 5 seconds or so, and find out where it is (because it is probably in the slow place almost all the time).

In your main loop, there are really two different ideas.  First idea: take the current result, and parse it to a string; second idea: to write that string out to a file.  You should make the separation of those ideas explicit.

That will be a lot easier if you create objects to encapsulate those responsibilities.

class Parser {
    private final String bodyStringDelim;
    private final String delimiter;
    private final int columnCount;

    Parser(String bodyStringDelim, delimiter, columnCount) {
        ...
    }

    String parser(ResultSet resultSet) {
        StringBuilder lineBuilder = new StringBuilder();

        for (int column = 1; column <= columnCount; column++) {
            String value = resultSet.getString(column);
            if (value != null) {
                if(bodyStringDelim != null){
                    lineBuilder.append(bodyStringDelim);
                }
                lineBuilder.append(value);
                if(bodyStringDelim != null){
                    lineBuilder.append(bodyStringDelim);
                }
            }

            if (column != columnCount) {
                lineBuilder.append(delimiter);
            }
        }

    }
}

If the garbage collector is getting in your way, you might try reusing the StringBuilder to see if that helps

class Parser {
    private final StringBuilder lineBuilder = new StringBuilder();

    private final String bodyStringDelim;
    private final String delimiter;
    private final int columnCount;

    Parser(String bodyStringDelim, delimiter, columnCount) {
        ...
    }

    String parser(ResultSet resultSet) {
        lineBuilder.setLength(0);
        ...

Similarly, you have a writer class -- at the moment, this is a lot simpler.

class Writer {
    private final File output;

    Writer (File output) {...}

    void write(String line) {
        FileUtils.writeStringToFile(output, line, true);
    }
}

FileUtils.writeStringToFile is the wrong idea for this problem -- in addition to opening and closing the file every time, you have to keep seeking forward in the file (which is getting longer) and you are flushing each individual line of the file to disk.

Much better here to manage your own FileOutputStream, which is opened with the append flag set.  You can still use IOUtils.write() if you like.

If that's still not fast enough an alternative would be to move the File IO to a different thread.  In that case, you would have two "writer" implementations.  The one running here, it would actually be pushing the Strings into a concurrent data structure (ConcurrentLinkedQueue, Disruptor, etc), and then the "real" writer lives in a second thread, pulling batches of data out of the shared structure, buffering it, and flushing it to disk.

Theoretically, this would allow you to overlap the slow parts of the read with the slow parts of the write, trading away CPU cycles for better wall clock time.


Base service for two different implementations I have code that fetches different templates for HTML and CSS in the file system. The templates are stored in different folders, so I have two implementations for the code that fetches them, passing the correct path as a parameter.

   private string _templatePath;

    private readonly IConfigurationManager _configurationManager;
    private readonly IFileSystem _fileSystem;

    public BaseTemplateFileService(string templatePath,
        IConfigurationManager configurationManager,
        IFileSystem fileSystem)
    {
        _templatePath = templatePath;
        _configurationManager = configurationManager;
        _fileSystem = fileSystem;
    }

    protected abstract string GetExtensionPattern(string templateName);

    public string GetTemplate(string templateName)
    {
        var templatesPath = _configurationManager.GetAppSetting(_templatePath);
        var fullName = GetExtensionPattern(templateName);

        var templatesFolder = _fileSystem.DirectoryInfo.FromDirectoryName(templatesPath);
        var files = templatesFolder.GetFiles(fullName, SearchOption.AllDirectories);
        var totalNumberOfFiles = files.Count();
        if (totalNumberOfFiles < 1)
        {
            throw new PanelException(string.Format("No template file to choose with name: {0}", fullName));
        }
        if (totalNumberOfFiles > 1)
        {
            throw new PanelException(string.Format("Multiple template files to choose with name: {0}", fullName));
        }
        using (var streamReader = files[0].OpenText())
        {
            var content = streamReader.ReadToEnd();

            return content;
        }
    }

public class HtmlTemplateFileService : BaseTemplateFileService
{
    public HtmlTemplateFileService(IConfigurationManager configurationManager,
        IFileSystem fileSystem)
        : base("panelHtmlPath", configurationManager, fileSystem)
    {
    }

    protected override string GetExtensionPattern(string folderName)
    {
        return string.Format("{0}.htm*", folderName);
    }
}

public class CssTemplateFileService : BaseTemplateFileService
{
    public CssTemplateFileService(IConfigurationManager configurationManager,
        IFileSystem fileSystem)
        : base("panelCssPath", configurationManager, fileSystem)
    {
    }

    protected override string GetExtensionPattern(string folderName)
    {
        return string.Format("{0}.scss*", folderName);
    }
}

What I don't like about this approach is that I need to set up my IoC contaier (i.e. Unity) like this

container.RegisterType<CssPanelGenerator>(new InjectionConstructor(
    new ResolvedParameter<ITemplateService>("cssTemplateService"), typeof(CssVariableService), typeof(ProcessSass)));

and same for the HTML one.

Is there a cleaner way to avoid code repetition but without the need to use this slightly more convoluted way of doing dependency injection that I can't think of?

App settings

When dealing with something like ConfigurationManager, Session, some state bag, etc., it's common to want to just inject the whole thing. The problem with that is that not only do you jump through some hoops to wrap the thing so it can be injected, but also the provider-specific means of interacting with it comes along. So you tie yourself to always having a keyed dictionary of sorts to pass in. You might be better served injecting the one setting instead of putting AppSettings access in your method. I'm not sure how this is accomplished in Unity, but you can inject .FromAppSettings(...) in Castle Windsor and even more.

File System

Kind of the same thing. I understand why you are doing this, but there's a difference between really isolating code and turning procedural code inside-out by calling the same instructions through multiple methods. It will probably help to simplify things by fetching your files a layer up and passing them to your class method(s). If you're like "Yeah, but how do I know my file access works?" I would respond with, "Are you trying to test your code in isolation, or trying to make your code runnable in unit tests?" With the former approach, you may get less test coverage, but you can get extremely granular tests around what it is you're doing. The latter approach results in integration tests that require considerable setup and probably end up testing the .NET Framework (i.e. this file would always be able to be accessed unless there was a bug in the framework).

Not sure how flexible your options are with file types, but the level of granularity you probably want to pursue is being able to pass in mock files and overriding the OpenText() method to ensure your mocked content is returned from files that pass validation. But keep it simple--don't go overboard with trying to inject file objects or wrapping them in interfaces either just so you can write tests.

Inheritance

You probably don't need to have a base class and an interface for that base class. Just register the base class in your container (why not make it abstract?) and use that in your signatures. Is that interface used for anything but making a mock? Plus, a 1:1 interface to class ratio or close to it usually means "too many interfaces" or "I'm worrying too hard about breaking the rules of making code testable."

Don't worry about specifying type associations in IoC configuration

I can't think of a more appropriate place to define any rules around typing than the container. As long as the abstract class is necessary, this is fine. This is fairly common when you have multiple classes that implement the same interface (a good thing, it means you are using an interface for its intended use and not just as a slick means to mock something).

But...

The only thing that is different between your classes is a suffix string. You don't need an inheritance structure to accomplish this--just make the suffix a parameter in your constructor! Then you can associate each different type of template generator with a specific named instance, or you can use a factory method that is run when resolving generators, where each generator registration provides a different string.

Some Philosophy

Instead of trying to get all code under test, try to completely isolate logic starting from the inside and working outwards, redrawing the boundaries between objects as you go. Chances are this C# code is not in an ASP.NET MVC app that was designed for DI. If you have to move code up a layer or completely out of the DI paradigm to get rid of awkward dependencies, it's not the end of the world, especially if you can still test it in isolation.  After all, you could still make a static method in your page or form-level layer that can be tested by a test class directly accessing it. Otherwise you may end up causing yourself a lot of pain trying to split a (rather simple) process up in steps across multiple objects.

Could the same means of simplification by parameterizing the suffix also be applied to the generators for additional reduction in complexity and number of objects? I haven't seen the code, but I'm thinking probably.

In closing

Prioritize simplicity and elegance over a focus on separation of concerns.


Javascript check if server time is greater than or less than 9am EST I offer same day shipping if the user places an order before 9AM ET. If they place the order after 9AM, same day shipping is unavailable.
My server is using UTC time so I need to check if the time is greater than or less than 9AM ET regardless of the time where the user is located.
Not being an expert with JS, I cobbled together this code.
var d = new Date();
var e = d.getHours().toLocaleString('en-US', { timeZone: 'America/New_York' });
var f = '';

if(e >= 9) {
  f = 'shipping unavailable';
}
else {
  f = 'shipping available';
}

I am getting the correct result after 9AM ET on my computer but I am concerned that same day shipping will be offered after 9AM ET.

Your code has a bug: d.getHours() will return a plain number. Calling toLocaleString on that number calls Number.prototype.toLocaleString, which:

returns a string with a language-sensitive representation of this number.

It does not do anything with timezones; the second parameter does not recognize a timeZone property. For example, where I am now, .getHours() returns 9, and (9).toLocaleString('en-US', { timeZone: 'America/New_York' }) returns "9".
On your server, to retrieve the hours in a particular timezone, one method would be to call toLocaleString on the date object itself with the appropriate timezone, then extract the number of hours from that string:

const timeString = new Date().toLocaleString('en-US', { timeZone: 'America/New_York' });
const [, hours, ampm] = timeString.match(/ (\d+).* ([AP]M)/);
console.log('In NY, it is:', hours, ampm);

Other issues:

Since it's 2020, best to prefer at least 2015+ syntax, which should not be an issue since the code is running on the server. (Among other things, being able to use const is wonderful)
You initialize f to the empty string, but then you reassign it immediately; the empty string is not used, so there's no point to assigning it.
Or, even better: since you want to conditionally assign a value to f depending on the number of hours, it would be more appropriate (and shorter) to use the conditional operator:

const f = Number(hours) < 9 && ampm === 'AM'
  ? 'shipping available'
  : 'shipping unavailable';


Throttle implementation using pure JavaScript adding and removing event listener I am trying to implement throttle function in vanilla JavaScript. I have seen other implementations on the web, but the way I have implemented it is more understandable for myself. I was wondering if any of you experts can take a look and let me know if I am missing some thing. It seems to be working fine but I am not sure the way I handle it is the correct.

HTML:

JS:

var area;
var result;

function init() {
  area = document.querySelector('#move');
  area.addEventListener('mousemove', trottle);
  result = document.querySelector('#result');
}

function updateResult(event) {
  let xCordinate = event.clientX;
  let yCordinate = event.clientY;
  result.textContent = `${xCordinate} , ${yCordinate}`;
}

function trottle(event) {
  updateResult(event);
  let element = event.target
  element.removeEventListener('mousemove', trottle);
  setTimeout(function() {
    element.addEventListener('mousemove', trottle);
  }, 2000)

}

init();

Overall the code appears easy to read, and does what you say it does.

General comments:

The name of the throttle function is misspelled (trottle versus throttle). Not a show stopper, but does make it harder to read.
I see a number of ECMA script language features being used, but the old function() { ... } style is being used for the callback in setTimeout. Consider using the more concise "arrow function expression" (e.g. lambda expression) syntax instead:

setTimeout(() => element.addEventListener('mousemove', throttle), 2000);

Code reusability and variable scope

Two global variables are being declared, and really should not exist in the global scope. Consider using an Immediately Invoked Function Expression (IIFE) to hide them:

(() => {
    let area;
    let result;

    ...
})();

Or you can use a build tool like Grunt to automatically wrap this script in an IIFE.
Other events are just as "spammy" as the mousemove event. The "scroll" event is just as bad, and this could be useful for that as well. Consider parameterizing the event that is being throttled.
Same thing with the updateResult function. It could be parameterized as a callback function.
Parameterizing the frequency of the throttling would be easy, and a good idea as well.
The CSS selectors used to target the element whose events are being throttled is the last thing that needs to be parameterized in order to make this truly reusable.

Given this, a function signature like the following would probably allow you to fix all the issues, including leaking variables to the global scope:

function throttleEvent(element, eventName, frequency, callback) {
    ...
}

And would be used like this:

let area = document.querySelector("#move");
let result = document.querySelector('#result');
let updateResult = (event) => {
    result.textContent = `${event.clientX} , ${event.clientY}`;
};

throttleEvent(area, "mousemove", 2000, updateResult);


n choose k combination in JavaScript I was asked to solve a problem for \$\binom{n}{k}\$. I did the following implementation and am wondering if there's any feedback.

Input:

n = Integer

k = Integer

Output:

result = Array of arrays of integers

Constraints:

Time: \$O(\binom{n}{k})\$

Space: \$O(\binom{n}{k})\$

The order of the output array DOES NOT MATTER.

n and k are both positive.

Resources:

N Choose K

Combinations

function combinations(n, k) {
    let result= [];

  function recurse(start, combos) {
    if(combos.length === k) {
      return result.push(combos.slice());
    }
        // Check if you can actually create a combo of valid length
        // given current start number
        // For example: 5 choose 4 can't begin with [3] since it would never have 4 numbers
    if(combos.length + (n - start + 1) < k){
      return;
    }
    recurse(start + 1, combos);
    combos.push(start);
    recurse(start + 1, combos);
    combos.pop();
  }

  recurse(1, []);
  return result;
}

I was wondering if there's a reason why the time gets optimized using a backtracking solution.

// function combinations(n, k) {
//   let result = []
//   function combine(combo, currentNumber){
//     if(combo.length === k) {
//       result.push(combo);
//       return;
//     }
//     if(currentNumber > n) {
//       return;
//     }

//     let newCombo1 = combo.slice();
//     let newCombo2 = combo.slice();
//     newCombo2.push(currentNumber);

//     combine(newCombo2, currentNumber + 1);
//     combine(newCombo1, currentNumber + 1);

//   }
//   combine([], 1);
//   return result;
// }

// speed: 1120.756 ms

//backtracking method
function combinations(n, k) {
  let result = []
  let stack = []
  function combine(currentNumber){
    if(stack.length === k) {
      let newCombo = stack.slice()
      result.push(newCombo);
      return;
    }
    if(currentNumber > n) {
      return;
    }

    stack.push(currentNumber)
    combine(currentNumber + 1);
    stack.pop();
    combine(currentNumber + 1);
  }
  combine(1);
  return result;
}

console.time("Naive Combinations")
console.log(combinations(20, 10))
console.timeEnd("Naive Combinations")

// speed: 320.756 ms

Performance testing.

Please note this answer is about performance not complexity.

Why the strange performance result?

The reason for the different results is down to poor testing.

JavaScript unlike most languages is never fully compiled. In the background there are multi level optimization engines that look at the running code and apply optimizations as the code is running.

The optimizer however has a penalty that is most evident when the code is first run. It can also optimize some code quicker than others.

To compare functions you need to let the optimizer do its thing (each browser have very different optimizing methods). Only after the function has been run several times can you start to get accurate results.

When I tested your code I found that the "backtracking" method was on average less than 5% slower.

Testing example

The snippet is a down and dirty performance tester that will test functions over many cycles and give a mean result of the function time.

Note that the result is not of a single call but that of many calls in a row. The tester will first make sure that the tests are not below the timer resolution before starting.

As an example I have kept the total test cycles low, but the numbers are still not stable and would take some more time before the optimizer is happy.

Included are your two functions as in the question, and alternative method using no recursive calls and a modification of one of your functions.

Performance increase.

In you function

function combinations(n, k) {
    let result= [];

  function recurse(start, combos) {
    if(combos.length === k) {
      return result.push(combos.slice());
    }
    if(combos.length + (n - start + 1) < k){
      return;
    }
    recurse(start + 1, combos);
    combos.push(start);
    recurse(start + 1, combos);
    combos.pop();
  }

  recurse(1, []);
  return result;
}

The combos array is being passed as an argument. There is no need to as it is the same array. Declare it outside the recurse function.

You are returning the array in one path and the other two your return undefined. Keep the return types the same.

You are recursing and then test for combos.length === k but you can test before the call saving the need to call.

There seams to be a slight favor for an arrow function, so changed the recurse function to such.

The modified function is consistently the quickest.

function combinationsAV2(n, k) {
  const result= [];
  const combos = [];
  const recurse = start => {
    if (combos.length + (n - start + 1) < k) { return }
    recurse(start + 1);
    combos.push(start);
    if(combos.length === k) {
       result.push(combos.slice());
    }else if(combos.length + (n - start + 2) >= k){
       recurse(start + 1);
    }
    combos.pop();
  }
  recurse(1, combos);
  return result;
}

Testing example

Note that the timing is adjusted to ensure good time resolution. The results are meaningless as absolute performance figures. They only have comparative meaning within a test run.

//n!/(r!(n-r)!)
Math.factorials = [1,1,2,6,24,120,720,5040,40320,362880,3628800,39916800,479001600,6227020800,87178291200,1307674368000,20922789888000,355687428096000,6402373705728000,121645100408832000];

Math.biCof = (n,r) => Math.factorials[n] / (Math.factorials[r] * Math.factorials[n-r]);

function combinationsG(n, k) {
    const result= [];
    const combo= [];
    const last = k - 1;  // points to last item in combo
    var i = k;
    while (i--) {
        combo.push(k - i);
    }
    const starts = [...combo];
    var pos,count = Math.biCof(n,k)
    while (count --) {
        result.push([...combo]);
        combo[last] += 1;
        if (combo[last] > n) {
            pos = 0;
            while (combo[last - pos] > n-pos) {
                pos += 1;
                combo[last-pos] += 1;
                if (pos === k) { break }
            }
            while (pos > 0) {
                combo[last-pos+1] = combo[last-pos] + 1
                pos --;
            }
        }
    }
    return result;
}

function combinationsAV2(n, k) {
  const result= [];
  const combos = [];
  const recurse = start => {
    if(combos.length + (n - start + 1) < k){
      return;
    }
    recurse(start + 1);
    combos.push(start);
    if(combos.length === k) {
       result.push(combos.slice());
    }else if(combos.length + (n - start + 2) >= k){
       recurse(start + 1);
    }
    combos.pop();
  }
  recurse(1, combos);
  return result;
}

function combinationsA(n, k) {
    let result= [];

  function recurse(start, combos) {
    if(combos.length === k) {

      return result.push(combos.slice());
    }

    if(combos.length + (n - start + 1) < k){
      return;
    }
    recurse(start + 1, combos);
    combos.push(start);
    recurse(start + 1, combos);
    combos.pop();
  }
  recurse(1, []);
  return result;
}

//backtracking method
function combinationsB(n, k) {
  let result = [];
  let stack = [];
  function combine(currentNumber){
    if(stack.length === k) {
      let newCombo = stack.slice();
      result.push(newCombo);
      return;
    }
    if(currentNumber > n) {
      return;
    }

    stack.push(currentNumber);
    combine(currentNumber + 1);
    stack.pop();
    combine(currentNumber + 1);
  }
  combine(1);
  return result;

}

/*===========UTIL to display results============*/
function log(data) {
     const div = document.createElement("div");
     div.textContent = data;
     logger.appendChild(div);
}
log.clear = function(){ logger.innerHTML = "" }

/*================TESTING CODE==================*/
const tests = {
    data : [15,3],  // arguments for function calls
    functions : [ {  // functions to test and a name as reference
            name : "Backtracking method",
            func : combinationsB,
        },{
            name : "Native Combinations",
            func : combinationsA,
        },{
            name : "Modified Native",
            func : combinationsAV2,
        },{
            name : "While loops",
            func : combinationsG,
        },
    ],
    cycles : 100,
    prepCycles : 30, // Number of cycles to run before testing. This
                     // is a optimizer shake down and use to ensure test times
                     // are above the timerResolution
    testPerCycle : 20,
    timedCycleTrim : 1,  // used to increase runtime if time to low
    timedCycles : 2,  // number of calls to test function * timedCycleTrim
    timerResolution : 0.4, // this is the minimum resolution of the timer in ms
                           // Note that due to security concerns performance timers
                           // have had the time resolution reduced to stop
                           // code attempting to access CPU caches
    testInterval : 10, // Time between test cycles.
    resolutionError : false, // If the run time of the timedCycles is lower than
                             // the timer resolution then the accumulating error
                             // makes results meaningless. This flag is set true
                             // when test time is too small. In the prep phase
                             // timedCycleTrim is used to increase the time
                             // till a time above timerResolution is found

}

log.clear()
startTest(tests);

function startTest(tests){

    function doCycle(test){
        var totalTime, count, time, j, i;
        const data = tests.data;
        i = tests.testPerCycle;
        totalTime = count = 0;

        while (i--) {
            j = tests.timedCycles * tests.timedCycleTrim;

            /* ========= TEST LOOP ==========*/
            const now = performance.now();
            while (j --) { test.func(...data) }
            time =  performance.now() - now;

            if (time <= tests.timerResolution) {
                tests.resolutionError = true;
                return;
            }
            count ++;
            totalTime += time;
        }
        test.count += count;
        test.time += totalTime;
        test.mean = test.time / test.count;

    }
    function resetTest(test){
        test.count = 0;
        test.time = 0;
    }
    function showTestStatus(test) {
        if (test.count) {
            log(`Test '${test.name}' mean ${test.mean.toLocaleString(undefined,numFormat)}ms`);
           // log(`Total ${test.time.toLocaleString(undefined,numFormat)}ms over ${test.count} cycles`);
        } else {
            log(`Test '${test.name}' not yet tested`);
        }
    }
    function eachTest(call,testArr = tests.functions){
        testArr.forEach(call)
    }
    const selectors = {
        index : 0,
        random() { return tests.functions[Math.random() * tests.functions.length | 0] },
        ordered() { return tests.functions[(selectors.index++) % tests.functions.length ] },
    }
    function setupPrep() {
        selectors.index = 0;
        cycles = tests.prepCycles * tests.functions.length;
        selector = selectors.ordered;
        prepPhase = true;
        tests.resolutionError = false;
        eachTest(resetTest);
        setTimeout(doNextCycle, tests.testInterval);
    }

    function doNextCycle() {
        const test = selector();
        doCycle(test)
        cycles --;
        if (tests.resolutionError) {
            if (prepPhase) {
                if (tests.timedCycleTrim  === 100) {
                    log(`Test '${test.name}' is not responding to cycle trim. Test aborted` );
                } else {
                    tests.timedCycleTrim += 2;
                    log(`Timed cycles increased to : ${tests.timedCycles * tests.timedCycleTrim}` );
                    setupPrep();
                }
            } else {
                log(`'${test.name}' failed, function runtime too low. Test aborted.` );
            }
        } else {
            if (cycles) {
                if (!prepPhase) {
                    log.clear();
                    log(`Test status ${cycles / (tests.cycles * tests.functions.length) * 100 | 0}%`);
                    eachTest(showTestStatus);
                }
                setTimeout(doNextCycle, tests.testInterval);
            } else if (!prepPhase) {
                log.clear();
                log("Testing complete...");
                log("Calls per cycles : " + tests.timedCycles * tests.timedCycleTrim);
                log("----------------------------------------");
                eachTest(showTestStatus,[...tests.functions].sort((a, b) => a.mean - b.mean));
            } else {
                log("Prep complete...")
                prepPhase = false;
                cycles = tests.cycles * tests.functions.length;
                selector = selectors.random;
                eachTest(resetTest);
                setTimeout(doNextCycle, tests.testInterval);
            }
        }
    }
    const numFormat = {minimumFractionDigits: 3, maximumFractionDigits: 3};
    var cycles, selector, prepPhase;
    log("Starting tests. Calibrating run times... ");
    setupPrep();
}


Spellathon solver in Java What is Spellathon?

The rules of Spellathon are as follows:

Create words that have 4 to 7 letters.
Include the center letter in every word.
Form at least one 7-letter word.
Plurals are acceptable.
The dictionary used is in English.

Here is a similar implementation.

The dictionary (word list) I'm using is this.

public class Spellathon {

    private static final Set<String> permutations = new HashSet<>();

    private static void permutation(String str) {
        permutation(str.charAt(0), "", str);
    }

    private static void permutation(char idx, String prefix, String str) {
        int n = str.length();

        if (n != 0) {
            for (int i = 0; i < n; i++) {
                permutation(idx, prefix + str.charAt(i), str.substring(0, i) + str.substring(i + 1, n));
            }
        }

        if(prefix.length() > 3 && prefix.indexOf(idx) >= 0) {
            permutations.add(prefix);
        }
    }

    public static void main(String[] args) {
        Scanner sc = new Scanner(System.in);
        String letters;

        System.out.print("Input: ");
        letters = sc.nextLine();

        permutation(letters);

        try {
            for(String x: permutations) {
                Scanner scFile = new Scanner(new File("words.txt"));

                while(scFile.hasNextLine()) {
                    String cur = scFile.nextLine();
                    if(cur.equalsIgnoreCase(x)) {
                        System.out.println("Found: " + cur);
                    }
                }
            }
        } catch (FileNotFoundException ex) {
            System.out.println("Word list file Not Found.");
        }
    }
}

Input: Pass the given letters as a String with the central letter as the first letter. The other letters can be in any order.

Output:

Input: elolh
Found: Hell
Found: hell
Found: Lole
Found: hello
Found: Holle
Found: Hole
Found: hole

I don't mind the redundant occurrences. My issue is that the code takes a lot of time to execute (that is why, I used 5 letters as an example).

Any comments/answers that can help reduce the execution time will be appreciated. Also, if anyone review the overall correctness of the program, it will be helpful.

The explanation for slowness is simple: you're doing a slow operation (reading a file) many many times (due to combinatorial explosion).

How many permutations of "hello" are there? For 5 items, there are 5!, or 120 permutations. However, two of the letters are indistinguishable, so it's actually \$\frac{5!}{2!}\$, or 60. Add the 12 permutations of "hell", 24 permutations of "helo", and 12 permutations of "hllo", and we have about a hundred.

Of those, I would expect only maybe a dozen to be valid English words.

Then, for each permutation, you are opening the file and reading every line to see if the candidate is a word. File IO involves the operating system and the disk — both very expensive.

Generating all permutations of something is costly. In contrast, checking whether two given words are permutations of each other is a simple matter of counting.

A smarter approach, therefore, would be to go through the dictionary once, and check whether each word meets the acceptance criteria.


Better method for updating data table I have completed an application which gathers specific records and marks them as Collected. As of now, the application runs perfectly and does exactly what I need it to, but the problem comes in when a large data set is reviewed. If I am pulling records for one day or even maybe a week, it runs at a decent pace, but once you get a month of data or more, it takes a fairly long amount of time.

DataTable ModelData = getModelData() // Returns all records to search.
...
EnumerableRowCollection<DataRow> modelRows = (from model in ModelData.AsEnumerable()
                                              where (model.Field<object>(GeographicalKey) ?? (object)String.Empty).ToString() == GeographicCode
                                              select model);
ModelResults = modelRows.Any() ? modelRows.CopyToDataTable() : ModelData.Clone();

for (int i = 0; i < ModelResults.Rows.Count; i++)
{
    for (int j = 0; j < ModelData.Rows.Count - 1; j++)
    {
        if (ModelResults.Rows[i]["Request ID"].ToString() == ModelData.Rows[j]["Request ID"].ToString())
        {
            ModelData.Rows[j]["Collected"] = "1";
        }
    }
}

The only part that executes slowly would be the for and the nested for. Is there possibly a better way that I can code this? I can't imagine this is the optimal coding for data table update operations, but it might be.  Is this a possibility in LINQ (I'm completely new to LINQ)? I think this is enough code to make sense of what I'm doing, but if more is needed just let me know and I can post it up as well.

The only part that executes slowly would be the for and the nested
  for. Is there possibly a better way that I can code this?

I take you ask us to optimize the nested loops:

The most obvious problem, as @Antonio also noted, is the O(n^2) filtering in the nested fors. Also as he noted, you need some kind of data structure with O(1) Contains method. However since you do not use other fields of from the rows of ModelResults, you only need a HashSet of ["Request ID"]s.

  I can't imagine this is the optimal coding for data table update
  operations, maybe though? Is this a possibility in LINQ (I'm
  completely new to LINQ)?

I take you are asking for a more compact, more LINQy way of updating a collection, in this case a DataTable:

LINQ is a query language and a LINQ query does not modify underlying IEnumerable. (In most cases you would select a modified copy of the underlying IEnumerable, instead.)

However, you can use List.ForEach on the result of such a query, as a more compact replacement of for loop.

Applying two suggestions above, we get:

var collectedRequestIds = new HashSet<string>(
    modelResults.AsEnumerable().Select(row => row["Request ID"].ToString()));

modelData.AsEnumerable()
    .Where(collectedRequestIds.Contains(row => row["Request ID"].ToString())))
    .ToList().ForEach(row => row["Collected"] = "1");


Identifying indices of non-delimited characters in a string, supporting greater than 1 length of delimiters The code below takes a string that potentially contains a true match for a delimiter. There can be multiple delimiters located anywhere in the string. We need to obtain the indices that are not a match and when the delimiter starts and ends.
The following code achieves this. However, I am unsure if this is the most optimal method. What do you think? Test cases below the code.
String baseText = "%!ABCD%!EF%";
String delimiter = "%";
ArrayList<String> selectedParameters = new ArrayList<>();

int lastNonDelimited = 0;
int i = 0;
while (i < baseText.length()) {
    int j = 0;
    while (j < delimiter.length()) {

        if (j + i == baseText.length() - 1 && j != delimiter.length() - 1) {
            lastNonDelimited += 1;
            System.out.println(lastNonDelimited);
            break;
        }

        char c = baseText.charAt(i + j);
        char d = delimiter.charAt(j);

        if (c == d) {
            if (j == delimiter.length() - 1) {
                lastNonDelimited += delimiter.length();
                i += (delimiter.length() - 1);
                System.out.println("End" + (i + j));
            } else {

                System.out.println("Beginnning" + (i + j + 1));
            }
        } else {
            lastNonDelimited += 1;
            System.out.println(lastNonDelimited);
            break;
        }

        j += 1;
    }

    i += 1;
}

Test case 1:
String %!ABCD%!EF% with %! delimiter results in:
Beginnning1
End2
3
4
5
6
Beginnning7
End8
9
10
11

Test case 2:
String %!ABCD%!EF%! with %! delimiter results in:
Beginnning1
End2
3
4
5
6
Beginnning7
End8
9
10
Beginnning11
End12

Some off the cuff comments:
It would help if you provided a compilable class rather than a somewhat misleading snippet.
Your search would be better as a separate method.
I don't see why you search in this way when String.indexof(String) and String.substring(int) exist.
Have you a clear understanding of how you would handle overlapping delimiters? If your delimiter is "!!", how is "abc!!!def" to be interpreted?


Elegant approach to building unordered list from XML How can I improve on this rather ugly method that builds an HTML unordered list from an XML file?

I maintain a collection of ASP.NET webforms that all need to pull in the same site navigation as our main website. The forms have a master page that mimics the main site template, and we want any changes to the main navigation to flow through to the forms automatically.

The main site uses an XML document to generate the navigation, and the document uses inconsistent formatting for the links (some have "http://mysite.com" hardcoded, and some are relative to the main site "/subsite").

Here's what I have right now:

private void LoadNavigation()
{
    string urlPrefix = "http://mysite.com";
    string xmlFilePath = ConfigurationManager.AppSettings["NavigationUrl"].ToString();
    XmlDocument doc = new XmlDocument();
    doc.Load(xmlFilePath);
    XmlNodeList navigationItems = doc.DocumentElement.FirstChild.ChildNodes;

    foreach (XmlNode item in navigationItems)
    {
        if (item.Attributes.GetNamedItem("url").Value.Contains("http://")) // link is hard coded
        {
            navList.InnerHtml += "
 " + item.Attributes.GetNamedItem("title").Value + "|
";
        }

        else
        {
            navList.InnerHtml += "
 " + item.Attributes.GetNamedItem("title").Value; //relative link

            if (item.NextSibling == null)
            {
                navList.InnerHtml += "
"; //last item in list
            }
            else
            {
                navList.InnerHtml += "|
"; //not the last item in list
            }
        }
    }
}

Believe it or not, this is an improvement over the original, which included a hardcoded URL for the XML file and got the values out of the XML document like this:

XmlNodeList nodeList = doc.ChildNodes[1].ChildNodes[0].ChildNodes;

But it still needs a lot of work. Please have at it.

The part that looks most problematic in your code is the if statement: the code in the if part and the else part is almost the same except that you prepend urlPrefix to the URL if it is relative. (Also you're only checking whether the node is the last if the URL is relative, which does not seem right). At the very least I'd factor this out into a helper method, which takes the absolute URL as a parameter to remove the code duplication.

However there's an even better way: .net already comes with the System.Uri class, which can take care of making the URL absolute for you.

private void LoadNavigation()
{
    Uri urlPrefix = new Uri("http://mysite.com");
    string xmlFilePath = ConfigurationManager.AppSettings["NavigationUrl"].ToString();
    XmlDocument doc = new XmlDocument();
    doc.Load(xmlFilePath);
    XmlNodeList navigationItems = doc.DocumentElement.FirstChild.ChildNodes;

    foreach (XmlNode item in navigationItems)
    {
        Url fullUrl = new Uri(urlPrefix, item.Attributes.GetNamedItem("url").Value);
        String title = item.Attributes.GetNamedItem("title").Value;
        navList.InnerHtml += "
 " + title + "";

        if (item.NextSibling == null)
        {
            navList.InnerHtml += "
"; //last item in list
        }
        else
        {
            navList.InnerHtml += "|
"; //not the last item in list
        }
    }
}