Module to support autocomplete for movie tags I've just completed a JavaScript module and I've got a strong feeling that this is borderline spaghetti code. My task was to build a tagging form which adds selected tags to hidden outputs to be sent to a server.

var tags = [];
var createdTags;
var addedTags = [];
var isNewTag, selectedTag, i;

$("#created-tags").attr("value", "[]");

$.ajax({

    type: "GET",
    cache: false,
    dataType: 'json',
    url: "/portal/index.php/movies/get_tags",
    success:
        function(response) {
            $("#tags-typealong").autocomplete({
                source: response,
                minLength: 1,
                select: function (event, ui) {
                    event.preventDefault();
                    selectedTag = ui.item;
                    //tags = current tags
                    tags = $.parseJSON($('#tags').attr('value'));
                    isNewTag = true;
                    //is tag already added?
                    for(i = 0; i < tags.length; i++) {
                        if(selectedTag.id == tags[i].id)
                            isNewTag = false;
                    }
                    //update addedTags if new
                    if(isNewTag) {
                        tags.push(selectedTag);
                        addedTags.push(selectedTag.id);
                        add_tag_render(selectedTag.name);
                    }
                    //update dom for tag_form.js
                    $("#tags").attr("value", JSON.stringify(tags));
                    $("#added-tags").attr("value", JSON.stringify(addedTags));
                    $("#tags-typealong").val('');
                }

            }).data( "autocomplete" )._renderItem = function( ul, item ) {
                return $("

")
                    .data( "item.autocomplete", item )
                    .append( '' + item.name  + '' )
                    .appendTo(ul);
            }
        }
});

$("#tags-typealong").keyup(function (event) {

    var isNew = true;

    if(event.keyCode == 32) {
        //get updated tag selections
        var tagName = $(this).val();
        tagName = tagName.slice(0, -1);
        createdTags = $.parseJSON($('#created-tags').attr('value'));
        tags = $.parseJSON($('#tags').attr('value'));
        //check tags from db
        for(i = 0; i < tags.length; i++) {
            if(tags[i].name == tagName)
                isNew = false;
        }
        //check existing created tags
        for(i = 0; i < createdTags.length; i++) {
            if(createdTags[i] == tagName)
                isNew = false;
        }
        if(isNew) {
            add_tag_render(tagName);
            createdTags.push(tagName);
            $("#created-tags").attr("value", JSON.stringify(createdTags));
        }
        $("#tags-typealong").val('');
        //TODO!
        //is tagname in db already?

    }

});

function add_tag_render(tagName) {

    $(".tag-container").append('

' + tagName + '

');
    $("#tags-typealong").appendTo(".tag-container");
    $("#tags-typealong").focus();

}

.attr('value') is bad. Always. You should use .val() instead.
var i; can go into the callback function instead of whatever scope you are in at the very beginning.


Validator function in Backbone I have inside a Backbone model validate function several if statements:

if(!attrs.firstAttr) errorMsg+="first attr mandatory";
if(!attrs.secondAttr) errorMsg+="second attr mandatory";

as you can see there is a lot of repetition (I have actually 10 fields to check with this.

I was thinking of introducing an interface Validator with a method validate. Then implement a MandatoryFieldValidator with:

validate: function(attr, errors) {
   if(!attr) errors.push(attr + " mandatory");
}

So the validate method would become like this:

mandatoryFieldValidator.validate(attr.firstAttr, errors); // errors now is an array
mandatoryFieldValidator.validate(attr.secondAttr, errors);

Would you think is a good solution? I still don't like it because I need to rewrite it a lot of times. Next step would be to have some kind of foreach but I don't have a list of parameters. The attributes passed by the Backbone validate is an object with the properties of the model.

Would it be ok to declare an array with every element I want to validate like [attr.firstAttr, attr.secondAttr]?

If yes, how would it be possible to do it in Backbone? I'm referring to the fact that _.each function from underscore.js just passes an array with a callback function and I still need to pass the error array and apply the validate method of my Validator.

I would approach that like this:

Have an array with all the fields you

var mandatoryFields = ['firstAttr','secondAttr'];

Then have a validate function that takes attrs, mandatoryFields and errors

function validate( o , mandatoryFields, errors )
{
  for( var i = 0 , i < mandatoryFields.length ; i++ )
  {
    var field = mandatoryFields[i];
    if(!o[field])
      errors.push(field + " mandatory");
  }
}

Note that I replaced attrs with fields but fare more readable, I also added a new-line  after that if statement for readability.


Jquery code is sloppy my shoppingcart is working but i used to many var and functions and i think it looks sloppy. i dont know how to set it right.

here is the Jsfiddle link:

https://jsfiddle.net/z0c1ja37/

can someone help me to put things right in this code?

look at my jsfiddle link there is the full code

$('.productprijzen').hide();
var $select = $('<select>').addClass('keuzes');
$select.append('<option>Kies je optie</option>');
$select.append('<option value="volvo">Volvo</option>');
$select.append('<option value="saab">saab</option>');
$select.append('<option value="peugeot">peugeot</option>');
$select.append('<option value="mazda">mazda</option>');

$select.on('change', function () {
    $parent = $(this).closest(".row");
    $('.productprijzen', $parent).slideDown();

    // prijs en btw laten zien
    var merk = $(this).val();
    var productInfo = oProducten[merk];
    $parent.find('.prijs').html("€" + productInfo.prijs);
    $parent.find('.btw').html(productInfo.btw + "%");

    // berekening van aantal x prijs
    var $aantalKeuze = $parent.find('input.aantal');
    var $subtotaal = $parent.find('.sub');

    $aantalKeuze.bind('keyup mouseup', function () {
        var $subtotaalUiteindelijk = productInfo.prijs * $aantalKeuze.val();
        $subtotaal.html($subtotaalUiteindelijk);

        var totaalBtw6 = 0;
        var totaalBtw21 = 0;
        var $totaal = 0;
        var $subtotaalVeld = $(".sub");
        $subtotaalVeld.each(function () {
            if ($(this).text() !== "") {
                $totaal = $totaal + Number($(this).text());
            }
        });

Use named functions

It's very common to write event handlers inline as anonymous functions, but as code gets more complex, this leads to deeply nested code that can be hard to follow. The inner event in particular could be something like $aantalKeuze.bind('keyup mouseup', handleAantalKeuzeChange); (apologies for the mix of languages).

Register events only once

At the moment, you seem to be registering the keyup mouseup handler every time the user changes the parent select. If the HTML it's checking isn't being replaced, this is going to end up with duplicate handlers. If the HTML is dynamically created (which doesn't show in this code) you could use a .on() "delegated event" to bind it in advance.

Separate UI logic from calculations

Try to separate your workflow into:

Get data from HTML
Calculate values (totals etc)
Populate new HTML

This might mean more (named!) functions, or it might just mean moving lines around and more variables. The aim is that you should be able to examine and change the "business logic" in step 2 separately from the "display logic" in steps 1 and 3.

Be consistent

You mostly use the convention that variables prefixed with $ are jQuery DOM objects and others are values, but you have var $totaal = 0; instead of var totaal = 0;. This is closely related to the previous point - the business logic part of the code should have no $ variables in it.

You mostly scope your variables with var, but have it missing on $parent. Because of how JS "hoists" variables, it may be sensible to declare all your variables close to the top of the function as possible. Again, this may encourage you to split out some sections into their own function - if a set of variables is only used within a tight section of code, that code might be asking to become a separate function, with defined inputs and outputs.

You mostly use $parent.find(...), except in $('.productprijzen', $parent). Standardise on one or the other, unless I'm mistaken and there's a significant difference.


Find the dominator in an array of integers A zero-indexed array A consisting of N integers is given. The
  dominator of array A is the value that occurs in more than half of the
  elements of A.

  For example, consider array A such that

  A[0] = 3    A[1] = 4    A[2] =  3    A[3] = 2    A[4] = 3    A[5] =
  -1    A[6] = 3    A[7] = 3

  The dominator of A is 3 because it occurs in 5 out of 8 elements of A
  (namely in those with indices 0, 2, 4, 6 and 7) and 5 is more than a
  half of 8.

  Write a function

  int solution(int A[], int N);

  that, given a zero-indexed array A consisting of N integers, returns
  index of any element of array A in which the dominator of A occurs.
  The function should return −1 if array A does not have a dominator.

  Assume that:

  N is an integer within the range [0..100,000]; each element of array A
  is an integer within the range [−2,147,483,648..2,147,483,647].

 int solution(int A[], int N)  {
        int size = 0;
        int value = 0;
        for(int i = 0 ; i < N ; i++)  {
                if(size == 0)  {
                   size++; value = A[i];
                }
                else  {
                   if(value != A[i])
                       size--;
                }
         }
         int candidate = -1;
         if(size > 0) {
            candidate = value;
         }
         int leader = -1;
         int count = 0;
         for(int j = 0 ; j < N ; j++)  {
            if(A[j] == candidate) {
                count++;
            }
         }
         if(count > (N/2)) {
            leader = candidate;
         }
         else {
            return -1;
         }
         int index[count];
         int cnt=0;
         for(int k = 0 ; k < N ; k++) {
            if(A[k] == leader) {
               index[cnt] = k;
               cnt++;
            }
         }
         return index[0];
    }

I see a number of things that may help you improve your program.

Test thoroughly

There is a flaw in your program which causes it to fail on certain kinds of input.  Here's the test code I used:

#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <stdbool.h>

// solution() goes here

struct test {
    int expected;
    int N;
    int numbers[20];
};

struct test tests[] = {
    { -1, 6, { 3, 3, 0, 6, 9, 3 }},
    { 3, 8, { 3, 4, 3, 2, 3, -1, 3, 3 }},
    { 3, 8, { 3, 3, 4, 2, 3, 3, -1, 3 }},
    { 3, 7, { 3, 3, 4, 3, 3, -1, 3}},
    { 3, 7, { 3, 3, 4, 3, 3, -1, 0}},
    { 3, 6, { 3, 3, 4, 3, 3, -1 }},
    { 6, 2, { 6, 6}},
    { 8, 1, { 8}},
};

bool doTest(struct test* t) {
    int index = solution(t->numbers, t->N);
    if (t->expected == -1) {
        return index == -1;
    }
    return t->expected == t->numbers[index];
}

int main()
{
    int testcount = sizeof(tests) / sizeof(tests[0]);
    for (int i=0; i < testcount; ++i) {
        printf("Test %d: %s\n", i+1, doTest(&tests[i]) ? "OK " : "BAD");
    }
}

With your code as posted, tests 5 and 6 (of 8) fail.

Fix your formatting

The indentation of the code is not consistent, which makes it a little harder to read and understand than if you format the code consistently.

Read the problem statement carefully

The problem statement says that the return value can be any of the index values that point to the dominator number, so there's no need to store all of them as the code does in the index[] array.

Reduce redundant variables

The value, candidate and leader could all be collapsed to a single local variable which would make the code shorter and more understandable.

Minimize loops through arrays

The code attempts to identify a candidate and then does several more loops through the code to find the count and an index value to return.  The code could be simplified to this (although the bug still remains):

int solution(int A[], int N)
{
    int size = 0;
    int candidate = 0;
    for (int i = 0; i < N; i++) {
        if (size == 0) {
            size++;
            candidate = A[i];
        } else {
            if (candidate != A[i])
                size--;
        }
    }
    if (size <= 0) {
        return -1;
    }
    int count = 0;
    int index = -1;
    for (int i = 0; i < N; i++) {
        if (A[i] == candidate) {
            count++;
            index = i;
        }
    }
    if (count > (N / 2)) {
        return index;
    }
    return -1;
}

Consider a different algorithm

There is another approach that passes all of the tests I've tried.  It first sorts the array.  Because the array is now sorted, we know that if there is a dominator, more than half the sorted array must be the dominator value.  In other words, for an array size of N, the index A[N/2] of the sorted array must contain the dominator value if one exists.  Then all that remains is to verify that it is indeed a dominator and return an index into this sorted version of the original array.  (Alternatively, this can be done with a copy of the original array if you make the assumption that the passed array can't be modified.  The problem statement doesn't say and doesn't specify const for the array parameter.)  Here's the code:

int comp(const void *a, const void *b)
{
    return *(const int *)a - *(const int *)b;
}

int solution(int A[], int N)
{
    qsort(A, N, sizeof(int), comp);
    int count = 0;
    for (int i = 0; i < N && count <= N / 2; ++i) {
        if (A[i] == A[N / 2]) {
            ++count;
        }
    }
    return (count > N / 2) ? N / 2 : -1;
}

Note that the index of N / 2 is guaranteed to point to the dominator if it exists, and the specification says that one may return any element of A in which the dominator exists.

Put another way, if one has a 10 integer array, there is no way to place 6 contiguous numbers within that array without having one of them be at index 5.

The corrected original algorithm

As @vnp and @janos have pointed out, the algorithm above, while simple and correct, is not optimal in terms of performance, so by popular demand, here's a corrected version of your original algorithm:

int solution(int A[], int N)
{
    int count = 1;
    int candidate = A[0];
    int index = 0;
    for (int i = 1; i < N; i++) {
        if (A[i] == candidate) {
            ++count;
        } else if (count > 0) {
            --count;
        } else {
            candidate = A[i];
            index = i;
            count = 1;
        }
    }
    if (count > 0) {
        count = 0;
        for (int i = 0; i < N && count <= N / 2; ++i) {
            if (A[i] == candidate) {
                count++;
            }
        }
        if (count > (N / 2)) {
            return index;
        }
    }
    return -1;
}

Thanks to @JS1 who pointed out that an earlier version of this failed on an all zeroes input, reinforcing my first point: Test thoroughly!  :)  I've added the following line to my test array:

{0, 5, {0, 0, 0, 0, 0}},

Timing tests

I modified the test routines a bit to have them copy each test and then sort rather than using a pointer.  Then I had each version run 100000 (1e6) times over the test suite of 8 tests.  Timing was then noted on each version and, as expected, the qsort algorithm is slower.  Tests were all done on a 64-bit Linux machine with a quad-core i7 running at 3.40GHz.  Compiler was gcc version 5.3.1 with -O2 optimization.

qsort version: 1.25 seconds
Moore version: 0.77 seconds

Results are fairly consistently repeatable and independent of which algorithm is tested first.  Since qsort is \${\mathcal O}(n \log n)\$, it's likely that the difference would tilt even more toward the Moore algorithm if longer strings had been used.


Generate 10000 matrix barcodes When generating a matrix barcode, it takes 1 minute per 10000 files .That's how to generating a matrix barcode for big quantities in less time.

Here is the library used to encoder data.

My code for button generating:

Class1 CLS = new Class1();
DataTable dt = CLS.ShowalldataSerial(textBox4.Text);

for (int i = 0; i <= Convert.ToInt32(textBox1.Text); i++)
{
    Serial = SRL.Rnd().ToString();
    txt = "UserID" + dt.Rows[0][0] + "FirmName" + dt.Rows[0][1] + "OrderNo" + dt.Rows[0][2] + "BtachNo" + dt.Rows[0][3] + "Quantity" + dt.Rows[0][4] + "ProductName" + dt.Rows[0][5] + "SerialNo" + Serial;

    dm.DM(txt, Color.FromName(comboBox1.SelectedItem.ToString()), Color.White).Save(root + "\\" + Serial + ".emf", System.Drawing.Imaging.ImageFormat.Emf);

}
MessageBox.Show("Records generated success ");

When creating 10000 in textbox1 it take 1 minute. If I write 200000 in textbox1 it takes 20 minutes.

The code is working without any problems and gives me results that I need, but it is slow. Can someone help me speed it up?

Strings are immutable so whenever you're concatenating them you're effectively creating new instances every time, this can slow down performance a lot, instead use StringBuilder and it's method Append() to build strings.
Avoid duplicate operations per each cycle unless it's necessary, such as Convert.ToInt32(textBox1.Text) convert the value to int before the for loop and save the value in variable, than later use the variable instead of converting the value every time. Another example is comboBox1.SelectedItem.ToString().


Numbers to English Strings in Python3 Inspired by the recent surge of questions dealing with changing numbers to their English equivalents (See here, here, and here), I decided to write my own version in Python.

zero_to_nineteen = (
    "zero",
    "one",
    "two",
    "three",
    "four",
    "five",
    "six",
    "seven",
    "eight",
    "nine",
    "ten",
    "eleven",
    "twelve",
    "thirteen",
    "fourteen" ,
    "fifteen",
    "sixteen",
    "seventeen",
    "eighteen",
    "nineteen"
)

tens = (
    "zero",
    "ten",
    "twenty",
    "thirty",
    "forty",
    "fifty",
    "sixty",
    "seventy",
    "eighty",
    "ninety"
)

suffixes = {
    3 : "thousand",
    6 : "million",
    9 : "billion",
    12 : "trillion",
    15 : "quadrillion",
    18 : "quintillion",
    21 : "sextillion",
    24 : "septillion",
    27 : "octillion",
    30 : "nonillion",
    33 : "decillion",
    36 : "undecillion"
    39 : "duodecillion",
    42 : "tredicillion",
    45 : "quattuordecillion",
    48 : "quinquadecillion",
    51 : "sedecillion",
    54 : "septendecillion",
    57 : "octodecillion",
    60 : "novendecillion",
    63 : "vigintillion",
    66 : "unvigintillion",
    69 : "duovigintillion",
    72 : "tresvigintillion",
    75 : "quattuorvigintillion",
    78 : "quinquavigintillion",
    81 : "sesvigintillion",
    84 : "septemvigintillion",
    87 : "octovigintillion",
    90 : "novemvigintillion",
    93 : "trigintillion",
    96 : "untrigintillion",
    99 : "duotrigintillion",
    102 : "trestrigintilion",
    105 : "quattuortrigintillion",
    108 : "quinquatrigintillion",
    111 : "sestrigintillion",
    114 : "septentrigintillion",
    117 : "octotrigintillion",
    120 : "noventrigintillion",
    123 : "quadragintillion"
}

def spell_out(number):
    """Returns a string representation of the number, in english"""
    if isinstance(number, float):
        raise ValueError("number must be an integer")

    if number < 0:
        return "negative " + spell_out(-1 * number)

    if number < 20:
        return zero_to_nineteen[number]

    if number < 100:
        tens_digit = number // 10
        ones_digit = number % 10
        if ones_digit == 0:
            return tens[tens_digit]
        else:
            return "{}-{}".format(tens[tens_digit], zero_to_nineteen[ones_digit])

    if number < 1000:
        hundreds_digit = zero_to_nineteen[number // 100]
        rest = number % 100
        if rest == 0:
            return "{} hundred".format(hundreds_digit)
        else:
            return "{} hundred {}".format(hundreds_digit, spell_out(rest))

    suffix_index = log(number)
    suffix_index -= suffix_index % 3

    prefix = spell_out(number // 10 ** suffix_index)
    suffix = suffixes[suffix_index]

    rest_of_number = number % (10 ** suffix_index)

    if suffix_index in suffixes:
        if number % (10 ** suffix_index) == 0:
            return "{} {}".format(prefix, suffix)
        else:
            return "{} {} {}".format(prefix, suffix, spell_out(rest_of_number))

    return "infinity"

def log(number):
    """Returns integer which is log base 10 of number"""
    answer = 0
    while number > 9:
        answer += 1
        number //= 10
    return answer

I would appreciate any comments on my code.

It looks mostly fine. I only have three comments:

The definitions of zero_to_nineteen and tens  is too long for my taste. It will look more readable like this::

tens = ("zero", "ten", "twenty", "thirty", "forty",
        "fifty", "sixty", "seventy", "eighty", "ninety")

Reject floats straight away is too much. One cannot then ask for the spelling of 1e6. I would do:

if isinstance(number, float):
     if number % 1 == 0:
         number = int(number)
     else:
         raise ValueError("number must be an integer")

elif not isinstance(number, int):
     raise ValueError("number must be a number")

Note that your function would happily accept things like 1j, "abc", None...
For Guido's beard, don't roll your own log function! use int(math.log10)


Chunking a list into separate lists of 5, and then iterating through each I'm writing a service that receives a list of Latitude/Longitude objects, prepares them for transaction, and then calls a web service to write them to the organization's Mainframe.  According to the existing application that I'm replacing, the Mainframe can only accept "chunks" of up to 5 Lat/Long values at a time, however the user is able to enter as many as they want.

After getting the list of Lat/Longs from the UI, if it is more than 5 pairs I break it into separate lists of 5, and then call another developers code to prepare them to transmit.  This preparation includes setting up the transaction with the Case File's relevant information (FileType/FileNumber, Username, password, ect), calling SetCode() or SetMisc() on each individual Latitude and Longitude string, and then calling the web service. I do this for every chunk of 5 Lat/Longs, and if the last chunk is less than 5, I only call SetCode() or SetMisc() for however many there are left. There may also be fewer than 5 records to begin with, and often times will only be 1 Latitude/Longitude.

While my code works, I feel like it could be improved.  Specifically, I don't care for all of the if checks, but I haven't been able to find another way to work it.  I also feel like this is inefficient code, however this specific code only runs once per each case, so efficiency isn't absolutely vital.

public void PostLatLong(string fileType, string fileNumber, List<LatLongDEntry>     latLongList,  DateTime? transactionDate = null, string commentText1 = null, string commentText2 = null, string username = null, string password = null)
{
    string transactionCode = "LATLONGS";
    List<List<LatLongEntry>> splitList = new List<List<LatLongEntry>>();

    try
    {
        while (latLongList.Any())
        {
            splitList.Add(latLongList.Take(5).ToList());
            latLongList = latLongList.Skip(5).ToList();
        }

        foreach (List<LatLongEntry> list in splitList)
        {
            SetupCaseFileUpdateTransaction(transactionCode, fileType, fileNumber, commentText1, commentText2, transactionDate, username, password);
            SetCode(1, list[0].Latitude);
            SetCode(2, list[0].Longitude);
            if (list.Count > 1)
            {
                SetCode(4, list[1].Latitude);
                SetCode(5, list[1].Longitude);
            }
            if (list.Count > 2)
            {
                SetCode(6, list[2].Latitude);
                SetCode(7, list[2].Longitude);
            }
            if (list.Count > 3)
            {
                SetMisc(1, list[3].Latitude);
                SetMisc(2, list[3].Longitude);
            }
            if (list.Count > 4)
            {
                SetMisc(3, list[4].Latitude);
                SetMisc(4, list[4].Longitude);
            }
            CallWebService();
        }
    }
    catch (Exception e)
    {
        LogTransactionException(transactionCode, e);
        throw;
    }
}

Separate the mechanics and the semantics of the code. Here's my very well used Batch<T> extension method on IEnumerable<T>:

public static class EnumerableExtensions
{
    public static IEnumerable<IEnumerable<T>> Batch<T>(this IEnumerable<T> enumerable, int batchSize)
    {
        if (enumerable == null) throw new ArgumentNullException(nameof(enumerable));
        if (batchSize <= 0) throw new ArgumentOutOfRangeException(nameof(batchSize));
        return enumerable.BatchCore(batchSize);
    }

    private static IEnumerable<IEnumerable<T>> BatchCore<T>(this IEnumerable<T> enumerable, int batchSize)
    {
        var c = 0;
        var batch = new List<T>();
        foreach (var item in enumerable)
        {
            batch.Add(item);
            if (++c % batchSize == 0)
            {
                yield return batch;
                batch = new List<T>();
            }
        }
        if (batch.Count != 0)
        {
            yield return batch;
        }
    }
}

Now that's the batching taken care of, we need to sort out the loop. What would we like it to look like?

foreach (var batch in latLongList.Batch(5))
{
    SetupCaseFileUpdateTransaction(transactionCode, fileType, fileNumber, commentText1, commentText2, transactionDate, username, password);
    SetWebServiceBatch(batch.ToList());
    CallWebService();
}

Great! The main method is now much shorter and easier to reason about. However, we've just moved the problem elsewhere. So let's look at the SetWebServiceBatch piece:

// Typed directly into CR so may not compile/be 100% right.
private void SetWebServiceBatch(List<LatLongEntry> latLongs)
{
    if(latLongs.Count > 5) throw new ArgumentException(nameof(latLongs));
    for (var i = 0; i < latLongs.Count; i++)
    {
        if (i < 3)
        {
            SetCode(1 + i * 2, latLongs[i].Latitude);
            SetCode(2 + i * 2, latLongs[i].Longitude);
        }
        else
        {
            SetMisc(1 + (i - 3) * 2, latLongs[i].Latitude);
            SetMisc(2 + (i - 3) * 2, latLongs[i].Longitude);
        }
    }
}

Note that I've assumed that skipping SetCode(3 was a mistake and that you want to set code 1,2,3,4,5,6 and set misc 1,2,3,4. If that's not the case you'll need to slightly modify the for loop to take into account this discontinuity.


How can I make this piece of code more secure? I am using db_dataobject and have provided two examples of an insert and a select query below.

Can you please help me rewrite them in a more secure way using PDO?  Is there any other way to improve them?

INSERT

 $user= new DataObjects_user;
                        $user->username  = mysql_real_escape_string (strip_tags($_REQUEST['username']));
                        $user->password  =encryptpass(mysql_real_escape_string(strip_tags($_REQUEST['password'])));
                        $user->email  =mysql_real_escape_string(strip_tags ($_REQUEST['email']));
                        $user->dob  = mysql_real_escape_string(strip_tags($_REQUEST['dob']));
                        $user->member_since  = date("Y-m-d H:i:s");
                        $id = $user->insert();

SELECT

$username= mysql_real_escape_string (strip_tags($_REQUEST['username']));
    $password=encryptpass($_REQUEST['password']);
    $user= new DataObjects_user;
    $password=mysql_escape_string(($password));

    $user->query("select activated,userid,email,username from {$user->__table} where (username = '$username' or email='$username') AND password = '$password' ");

    if($user->fetch())
    {

I come back to this at the end, but if this answer is tltr, basically, I end up suggesting not to write your own DB abstraction layer, It's been done before, way better than you're doing it now.
That doesn't mean the rest of this answer doesn't contain valuable tips on how to improve your code quality in general, so please do read through it:

For a start, when you're using an object to contain data, that object is responsible for that data's integrety, not the user of the object. By that I mean: if I am to use your object, you must be able to ensure that, regardless of how I use your object (I may forget the strip_tags call, for example) the input won't contain any markup.
The only way to do that is to define setter methods that take care of this:

class DataObjects_User
{
    private $name = null;
    public function getName()
    {
        return $this->name;
    }
    public function setName($name = null)
    {//setting null ~= unsetting
        if ($name !== null && !is_string($name))
        {
            throw new InvalidArgumentException('Name must be string');//don't hush-up errors
        }
        $this->name = is_null($name) ? $name : strip_tags($name);
        return $this;
    }
}

Your code appears to require me to set a creation date manualy. Why? Why not create a field in the table that has NOW() as a default value? or an ON UPDATE CURDATE clause? wouldn't that be easier?
even so, you could omit this, by simply adding a constructor to the object above, too:

class DataObjects_User
{
    private $name = null;
    private $created = null;
    public function __construct()
    {
        $this->created = new \DateTime;//I prefer to keep everything OO
    }
    public function getCreated($format = 'Y-m-j H:i:s')
    {//use class constants for the format
        if ($format === false)
        {//allow user to get the object itself?
            return $this->created;
        }
        return $this->created->format($format);
    }
    public function setCreated(DateTime $obj = null)
    {//again, null to unset
        $this->created = $obj;
        return $this;
    }
    public function getName()
    {
        return $this->name;
    }
    public function setName($name = null)
    {//setting null ~= unsetting
        if ($name !== null && !is_string($name))
        {
            throw new InvalidArgumentException('Name must be string');//don't hush-up errors
        }
        $this->name = is_null($name) ? $name : strip_tags($name);
        return $this;
    }
}

You may be wondering why I keep adding the option of setting the properties to null... well, that's perfectly simple: If these objects represent data in the DB, it's not unlikely you'll find yourself using these objects to build queries. If some of these properties have default values, or values that are irrelevant to a query, you can uset them, by passing null (or nothing, since it's the default value) to their setters.

$user = new DataObjects_User();//constructor sets created date!
$user->setName('Bobby');
    ->setCreated();//unset created

Now you can pass $user to a generic createBind() method. Besides, if you have a couple of these objects, just create an interface/abstract class if only for typehinting, and extend all dataobjects from that:

class DataObjects_User extends DataObjects_Abstract{}
//the createBind method then could look like this:
public function createBind(DataObjects_Abstract $model)
{
    $getters = get_class_methods($model);
    $bind = array();
    foreach($getters as $getter)
    {
        if (substr($getter,0,3) === 'get' && method_exists('s'.substr($getter,1), $model))
        {//getter and setter exist, this is a property
            if ($model->{$getter}() !== null)
            {//avoid null properties
                $bind[':'.substr($getter,3)] = $model->{$getter}();
            }
        }
    }
    return $bind;
}
//when applied to $user
var_dump(
    $queryObject->createBind(
         $user->setName('Bobby')
              ->setCreated()
    )
);

That should yield:

array(':name' => 'Bobby');

Which makes life a lot easier when using prepared statements, doesn't it?

since I'm on the subject: When using PDO, you don't have to bother with all those deprecated mysql_real_escape_string calls anymore... just use prepared statements. If you can't, then learn to use them. It's not that hard, and an awful lot safer at any rate than what you have now.

Basically, with the $user object, being passed through the createBind method, all you need to do is:

$bind = $queryObject->createBind(
     $user->setName('Bobby')
          ->setCreated()
);
$fields = array_keys($bind);
$query = 'SELECT * FROM tbl WHERE ';
foreach($fields as $field)
{
    $query .= substr($field,1) .' = '.$field;
}
$statement = $pdoInstance->prepare($query);
$statement->execute($bind);

This is just as secure as your code, if not more secure. The downsides:

the properties of your objects have to be the same as the field-names in your db
data and logic are too tightly coupled here...

Lastly, I'd like to point that, even though it's not something that'll leave a gaping hole in your app's security, storing passwords the way you are doing is not done. Common practice is to store password-hashes, not encrypted passwords. Encryption, as the word implies, is a two way deal: anything that is encrypted can be decrypted. A hash-function, by its very nature is a one-way algorythm: A hash can't be de-hashed (well, it can be brute-forced, but even then, collisions, salt and time-complexity make that less viable to would-be hackers). Instead of encrypting the pass, try this, inside the user model:

private $passHash = null;
const PASS_SALT = '$0m3_r@Nd0M_5tr1n9';
public function setPassHash($password)
{
    $this->passHash = hash('sha256', self::PASS_SALT.$password);
    return $this;
}

But that's just on the data model. Any abstraction layer you're writing to query the DB isn't going to cut it as your app grows. As I've stated previously, in other answers:

On the whole, I'd suggest you use Doctrine entities, though. ORM's aren't very inviting at first, but once you get the hang of how easy it is to distil tables from objects and objects from tables (yes, it works both ways), you haven't got anything to worry about.


Outputting a .txt file from a .csv file I wrote this little script in PHP a bit ago. It takes a .csv file and outputs a .txt file with certain data arranged in a "psuedo-array" looking format. I recently started messing around with Ruby so I decided to redo it. It wound up essentially as an exact translation, though.

Is there a simpler / more Ruby way to do this?

require "csv"

class ArrayGenerator

    def initialize( file )
        @file = file
        open_csv
    end

    private
    def open_csv
        File.open("#{@file}" + "_output.txt", 'w') do |file|
            CSV.foreach(@file) do |row|
                lat = row[6]
                lon = row[7]
                img = "#{row[0]}".downcase
                part = "#{row[2]}"
                loc = "#{row[5]}"
                country = "#{row[4]}"
                focus =  "#{row[3]}"
                desc = "#{row[8]}"
                link = "#{row[9]}"

                file.write("[#{lat}, #{lon}, #{img}, #{part}, #{loc}, #{focus}, #{country}, #{desc}, #{link}],
")
            end
        end
    end
end

ARGV.each do |file|
    g = ArrayGenerator.new( file )
end

img = "#{row[0]}".downcase
part = "#{row[2]}"
loc = "#{row[5]}"
country = "#{row[4]}"
focus =  "#{row[3]}"
desc = "#{row[8]}"
link = "#{row[9]}"

The only difference between writing "#{ expression }" (without anything else in the string) and just writing expression is that the former converts the result of the expression to a string. If that is your intent, you should write expression.to_s instead because that way it's clearer what's going on.

However in your case the values in row are already string, so there's no need to convert them to strings, so using "#{}" here is just redundant. You can just write img = row[0].downcase, part = row[2] etc.

Instead of taking each column out of the row array individually, you can also write this:

CSV.foreach(@file) do |img, _, part, focus, country, loc, lat, lon, desc, link|
  img = img.downcase
  file.puts("[#{lat}, #{lon}, #{img}, #{part}, #{loc}, #{focus}, #{country}, #{desc}, #{link}],")
end

Note that I've used the variable name _ to denote a variable we're not going to use because you never used row[1] in your code (_ is conventionally used in Ruby to denote variables that aren't used and is in fact the only variable name that's allowed to appear multiple times in a parameter list).

I've also replaced write with puts which automatically inserts a newline after the string, so you don't have to include the newline in the string.

A couple of more general design notes:

I think open_csv is rather misleadingly named. It does a lot more than just opening the csv file.

I also think it's a bad idea to call open_csv in your constructor. Generally the constructor should only set up the object, so that it's in a usable state. It should not do any actual work. The way you designed it, everything the object is ever going to do is done in the constructor. That's why when you use it, you create the object with ArrayGenerator.new and then never do anything with the object - because everything that can be done with the object has already been in the constructor. Another sign of this is that the only public method in your class is initialize. This is a very strange way to use objects.

On a related note there's no point in creating a g variable if you're not going to use it.

You should either redesign your class to behave more like an actual class, with a usage more like this:

g = ArrayGenerator.new( file )
g.generate

Or you could get rid of your class altogether and instead define a stand-alone method that could be used like this:

generate_array( file )


Functional, recursive FizzBuzz in JavaScript I was talking to a coworker about over engineering things, which somehow lead to me over engineering the hell out of fizzbuzz. I went for a functional recursive solution in JavaScript. What do you think?

Full disclosure: my knowledge of functional programming begins and ends at "functions are king" and "no side effects" so I would LOVE some feedback regarding functional programming. But please point out other inefficiencies as well.

fizzbuzz();

function fizzbuzz() {
    var i = 1;
    var result = [];

    (function loop() {
        var str = '';
        str += fizz(i);
        str += buzz(i);

        result.push(ifFalsy(str, i));

        if (isLt100(i++)) {
            loop();
        }
    })();

    print(result.join(', '));
}

function fizz(num) {
    return isDivBy3(num) ? 'fizz' : '';
}

function buzz(num) {
    return isDivBy5(num) ? 'buzz' : '';
}

function isDivBy3(num) {
    return num % 3 === 0;
}

function isDivBy5(num) {
    return num % 5 === 0;
}

function isLt100(num) {
    return num < 100;
}

function ifFalsy(value, fallback) {
    return !value ? fallback : value;
}

function print(str) {
    console.log(str);
}

Functional programming is not about writing as many functions as you can. I would suggest to start with a function library (e.g. Ramda) and write as few new functions as needed, taking advantage of currying.

Obviously, your loop function has side effects: it modifies result and i.

Without going to deep into FP rabbit hole, your function could look like this:

function getFizzBuzzArray() {
    function getFizzBuzz(v) {
        var ret = R.concat(
          v % 3 === 0 ? 'fizz' : '',
          v % 5 === 0 ? 'buzz' : ''
        );

        return ret || v;
    }

    return R.map(getFizzBuzz, R.range(1, 101));
}

document.write(getFizzBuzzArray());
<script src="https://cdn.jsdelivr.net/ramda/0.18.0/ramda.min.js"></script>


Quicksort in JavaScript using destructuring and spread syntax I'm learning Haskell, and there was quicksort implementation, so I thought that I could implement it in a similar way in JavaScript. The result:

const quicksort = ([x, ...xs], compareFn = (a, b) => a - b) => {
  // If x is undefined, the array is empty
  if (x === undefined) return [];
  const smallerSorted = quicksort(xs.filter(a => compareFn(a, x) <= 0), compareFn);
  const biggerSorted = quicksort(xs.filter(a => compareFn(a, x) > 0), compareFn);
  return [...smallerSorted, x, ...biggerSorted];
};

// Example usage:
console.log(quicksort(
  Array.from({ length: 10 }).map(_ => Math.floor(Math.random() * 100))
));

console.log(quicksort(
  Array.from('a quick brown fox jumped over a lazy dog'),
  (a, b) => a.charCodeAt(0) - b.charCodeAt(0)
).join(''));
.as-console-wrapper { max-height: 100% !important; }

I realize that calling filter() twice might be inefficient, so I made an alternative version, using the _.partition() function from Lodash:

const quicksort = ([x, ...xs], compareFn = (a, b) => a - b) => {
  // If x is undefined, the array is empty
  if (x === undefined) return [];
  const [smallerSorted, biggerSorted] = _.partition(
    xs,
    a => compareFn(a, x) <= 0
  ).map(arr => quicksort(arr, compareFn));
  return [...smallerSorted, x, ...biggerSorted];
};

Interesting question;

your quicksort is far slower than the regular sort, and is harder to read.
You can find details here: https://jsperf.com/codereview-js-quicksort/15

But in essence this implementation is 90% slower than a regular sort. Using CPU profiling I can tell that most of the time is spent in _.partition which makes sense. Plus you keep creating functions like arr => quicksort(arr, compareFn) in every call of quicksort which adds up. Finally, that name should really be quickSort; JavaScript functions follow the lowerCamelCase convention.

My counter proposal would simply be:

console.log(
  Array.from('the quick brown fox jumps over the lazy dog').sort(
    (a, b) => a.charCodeAt(0) - b.charCodeAt(0)
  ).join('')
);

Historically, I have never heard of folks beating the native sort.


Travelling salesman using brute-force and heuristics I have implemented both a brute-force and a heuristic algorithm to solve the travelling salesman problem.

import doctest
from itertools import permutations

def distance(point1, point2):
    """
    Returns the Euclidean distance of two points in the Cartesian Plane.

    >>> distance([3,4],[0,0])
    5.0
    >>> distance([3,6],[10,6])
    7.0
    """
    return ((point1[0] - point2[0])**2 + (point1[1] - point2[1])**2) ** 0.5

def total_distance(points):
    """
    Returns the length of the path passing throught
    all the points in the given order.

    >>> total_distance([[1,2],[4,6]])
    5.0
    >>> total_distance([[3,6],[7,6],[12,6]])
    9.0
    """
    return sum([distance(point, points[index + 1]) for index, point in enumerate(points[:-1])])

def travelling_salesman(points, start=None):
    """
    Finds the shortest route to visit all the cities by bruteforce.
    Time complexity is O(N!), so never use on long lists.

    >>> travelling_salesman([[0,0],[10,0],[6,0]])
    ([0, 0], [6, 0], [10, 0])
    >>> travelling_salesman([[0,0],[6,0],[2,3],[3,7],[0.5,9],[3,5],[9,1]])
    ([0, 0], [6, 0], [9, 1], [2, 3], [3, 5], [3, 7], [0.5, 9])
    """
    if start is None:
        start = points[0]
    return min([perm for perm in permutations(points) if perm[0] == start], key=total_distance)

def optimized_travelling_salesman(points, start=None):
    """
    As solving the problem in the brute force way is too slow,
    this function implements a simple heuristic: always
    go to the nearest city.

    Even if this algoritmh is extremely simple, it works pretty well
    giving a solution only about 25% longer than the optimal one (cit. Wikipedia),
    and runs very fast in O(N^2) time complexity.

    >>> optimized_travelling_salesman([[i,j] for i in range(5) for j in range(5)])
    [[0, 0], [0, 1], [0, 2], [0, 3], [0, 4], [1, 4], [1, 3], [1, 2], [1, 1], [1, 0], [2, 0], [2, 1], [2, 2], [2, 3], [2, 4], [3, 4], [3, 3], [3, 2], [3, 1], [3, 0], [4, 0], [4, 1], [4, 2], [4, 3], [4, 4]]
    >>> optimized_travelling_salesman([[0,0],[10,0],[6,0]])
    [[0, 0], [6, 0], [10, 0]]
    """
    if start is None:
        start = points[0]
    must_visit = points
    path = [start]
    must_visit.remove(start)
    while must_visit:
        nearest = min(must_visit, key=lambda x: distance(path[-1], x))
        path.append(nearest)
        must_visit.remove(nearest)
    return path

def main():
    doctest.testmod()
    points = [[0, 0], [1, 5.7], [2, 3], [3, 7],
              [0.5, 9], [3, 5], [9, 1], [10, 5]]
    print("""The minimum distance to visit all the following points: {}
starting at {} is {}.

The optimized algoritmh yields a path long {}.""".format(
        tuple(points),
        points[0],
        total_distance(travelling_salesman(points)),
        total_distance(optimized_travelling_salesman(points))))

if __name__ == "__main__":
    main()

I enjoyed the first look at the code as it's very clean, you have
extensive docstrings and great, expressive function names.  Now you know
the deal with PEP8, but except for the one 200 character long line I
don't think it matters much really.

There're a few typo with the wrong spelling "algoritmh".

The coordinates should be immutable 2-tuples.  The reason being the
safety of immutable data-structures.  YMMV, but that makes it really
obvious that those are coordinates as well.

optimized_travelling_salesman should make a defensive copy of
points, or you should otherwise indicate that it's destructive on that
argument.

Instead of if start is None: start = points[0] you could also use
start = start or points[0] to save some space while still being
relatively readable.

For the algorithms the only thing I'd is not to use square root if you
don't have to.  You can basically create a distance_squared and use that
instead of distance because the relationship
between a smaller and bigger distance will stay the same regardless.
That doesn't apply for the final output of course. Edit: And, as mentioned below by @JanneKarila, you can't use that for the brute-force version.


AppVersion Struct & VersionParse Exception I am working on an app that will interact with assembly (app) versions. I created a struct for that, along with an exception for parsing error.

Here's the code:

    [Serializable]
    public class VersionParseException : Exception
    {
        public VersionParseException()
        { }

        public VersionParseException(string message)
            : base(message)
        { }

        public VersionParseException(string message, Exception innerException)
            : base(message, innerException)
        { }

        protected VersionParseException(SerializationInfo info, StreamingContext context)
            : base(info, context)
        { }

    }

    public struct AppVersion
    {
        int _majorVersion;
        public int MajorVersion
        {
            get { return _majorVersion; }
            set
            {
                _majorVersion = value;
            }
        }

        int _minorVersion;
        public int MinorVersion
        {
            get { return _minorVersion; }
            set
            {
                _minorVersion = value;
            }
        }

        int _buildNumber;
        public int BuildNumber
        {
            get { return _buildNumber; }
            set
            {
                _buildNumber = value;
            }
        }

        int _revision;
        public int Revision
        {
            get { return _revision; }
            set
            {
                _revision = value;
            }
        }

        public string Version
        {
            get { return MajorVersion.ToString() + "." + MinorVersion.ToString() + "." + BuildNumber.ToString() + "." + Revision.ToString(); }
            set
            {
                try
                {
                    MajorVersion = int.Parse(value.Split('.')[0]);
                    MinorVersion = int.Parse(value.Split('.')[1]);
                    BuildNumber = int.Parse(value.Split('.')[2]);
                    Revision = int.Parse(value.Split('.')[3]);
                }
                catch (Exception exception)
                {
                    throw new VersionParseException("Invalid App Version", exception);
                }
            }
        }
    }

Is there any better way to handle AppVersions and exceptions occurring in the process of using it?

And here's an example of using the struct:

AppVersion version1;
version1.Version = "0.500.965.201";
Console.WriteLine(version1.MinorVersion); // outputs "500"
version1.BuildNumber = 753;
Console.WriteLine(version1.Version);      // outputs "0.500.753.201"

Another example on throwing the exception:

AppVersion version2;
version2.Version = "0.blabla.987.test"; //Throws VersionParseException

I am working on an app that will interact with assembly (app) versions

There's a class, System.Version, that might be suitable for your needs. System.Version:

  Represents the version number of an assembly, operating system, or the common language runtime.

It has Major, Minor, Build, and Revision properties.

If you still want to roll your own, my advice is to

Make AppVersion immutable
Be stricter about the values you allow
Implement IEquatable<AppVersion>
Consider implementing IComparable<AppVersion>

In general, you should avoid mutable structs.

From MSDN

  X DO NOT define mutable value types.

  Mutable value types have several problems. For example, when a property getter returns a value type, the caller receives a copy. Because the copy is created implicitly, developers might not be aware that they are mutating the copy, and not the original value.

See also: Why are mutable structs “evil”?

With the code as written, this works just fine:

appVersion.Version = "   +123 . -4236 . +0 . -789";

Use the overloads of int.Parse/int.TryParse that take a NumberStyles argument. If you provide constructors as suggested below, check that the values passed in are non-negative.

From MSDN

  ✓ DO implement IEquatable<T> on value types.

  The Object.Equals method on value types causes boxing, and its default implementation is not very efficient, because it uses reflection. Equals can have much better performance and can be implemented so that it will not cause boxing.

I find it hard to imagine not ever wanting to compare app versions, so you will probably want to implement IComparable<AppVersion>.

I would recommending getting rid of the Version property entirely. Instead, provide one or more constructors like

public AppVersion(int majorVersion, int minorVersion)

public AppVersion(int majorVersion, int minorVersion, int buildNumber)

...

and then provide static methods AppVersion Parse(string) and bool TryParse(string, out AppVersion).

Also, you should be aware of the existence of auto-implemented properties.


Group students by grade and having max age Please help to improve my Java 7 code. I would like a best code logic to achieve this.

package main.java;

import java.util.ArrayList;
import java.util.List;

public class Student {
    private int id;
    private String grade;
    private int age;

    public Student(int id, String grade, int age){
        this.id = id;
        this.grade = grade;
        this.age = age;
    }

    public int getId() {
        return id;
    }
    public void setId(int id) {
        this.id = id;
    }
    public String getGrade() {
        return grade;
    }
    public void setGrade(String grade) {
        this.grade = grade;
    }
    public int getAge() {
        return age;
    }
    public void setAge(int age) {
        this.age = age;
    }

    // group student by grade and having max of age
    public static List<Student> getStudentsWithMaxAgeOfEachGrade(List<Student> students){
        ArrayList<Student> result = new ArrayList<>();

        for(Student s : students){
            boolean found = false;
            for(Student rs : result){
                if(rs.getGrade().equals(s.getGrade()) && rs.getAge() < s.getAge()){
                    found = true;
                    result.remove(rs);
                    result.add(s);
                    break;
                }
            }
            if(found == false){
                result.add(s);
            }
        }

        return result;

    }

    public static void main(String[] args) {
        List<Student> students = new ArrayList<>();
        Student s1 = new Student(1, "a", 32);
        Student s2 = new Student(2, "a", 34);
        Student s3 = new Student(3, "b", 23);

        students.add(s1);
        students.add(s2);
        students.add(s3);

        List<Student> result = getStudentsWithMaxAgeOfEachGrade(students);

        for(Student s : result){
            System.out.println(s.getGrade() + ":" + s.getAge());
        }
    }
}

in your code, you have getter and setter for id. But those methods are not used. I have posted a similar question before. And multiple people told me "we shouldn't have dead code."

Since you want to return List of Student, I would construct the HashMap to map from string(grade) to Student instead of string to integer (where you will lose the student id).


How to simplify Regex with Data.Text? This function tells that elements of content either (maybe) match regexes that you like or match regexes that you don't like.

This messy code requires Data.Text.Lazy but internally shuffles around internal and strict versions. Also list Monad code looks fishy to me.

How can it be made simpler, faster, more general (especially work with any Text and String)?

{-# LANGUAGE OverloadedStrings #-}

module Search (analyze) where

import qualified Data.Text.Lazy as T
import Data.Maybe()
import Text.Regex.TDFA
import Text.Regex.Base.Context()
import Text.Regex.Base.RegexLike()
import Text.Regex.TDFA.Text
import Data.Function (on)
import Data.Maybe (maybeToList)

matches :: [T.Text] -> T.Text -> [T.Text]
matches patterns content = map (T.fromStrict) $ concat $ do
  pattern <- patterns
  return $ maybeToList $ (on (=~~) T.toStrict) content pattern

analyze :: [T.Text] -> [T.Text] -> T.Text -> Either [T.Text] [T.Text]
analyze likes dislikes text =
    case matches likes text of
      liked@(_:_) ->
        case matches dislikes text of
             [] -> Right liked
             disliked@(_:_) -> Left disliked
      _ -> Left []

To fix lazy/strict shuffling you need to import Text.Regex.TDFA.Text.Lazy module. It contains TDFA-related instances for Data.Text.Lazy

Some other things to consider.

You can skip maybeToList because (=~) returns m target for some monad m.

- return $ maybeToList $ (on (=~~) T.toStrict) content pattern
+ return $ (on (=~~) T.toStrict) content pattern

This will work without any further changes because (=~) will select list monad and match failure will be represented by empty list.

Do-notation for list monad can be replaced with single map and concat . map f -- with concatMap

- matches patterns content = map (T.fromStrict) $ concat $ do
-   pattern <- patterns
-   return $ maybeToList $ (on (=~~) T.toStrict) content pattern
+ matches patterns content = map T.fromStrict
+   $ concatMap ((on (=~~) T.toStrict) content) patterns

Now it's time to add import Text.Regex.TDFA.Text.Lazy and remove text conversions

- matches patterns content = map T.fromStrict
-   $ concatMap ((on (=~~) T.toStrict) content) patterns
+ matches patterns content = concatMap (content=~~) patterns

You can swap arguments and drop patterns varaible.

As for the analyse function, it's just a matter of taste but I think it is more readable to enumerate all variants in single case:

analyze likes dislikes text
  = case (matches text likes, matches text dislikes) of
    ([], _)       -> Left []
    (liked, [])   -> Right liked
    (_, disliked) -> Left disliked

For the last step you can import Text type unqaulified to make function types look cleaner.

Here is the result:

import Data.Text.Lazy (Text)
import qualified Data.Text.Lazy as T
import Text.Regex.TDFA.Text.Lazy
import Text.Regex.TDFA

matches :: Text -> [Text] -> [Text]
matches content = concatMap ((=~~) content)

analyze :: [Text] -> [Text] -> Text -> Either [Text] [Text]
analyze likes dislikes text
  = case map (matches text) [likes, dislikes] of
    [[], _]       -> Left []
    [liked, []]   -> Right liked
    [_, disliked] -> Left disliked


Trie for lowercase words I need to develop a dictionary program so I used a trie data structure to implement it. I have created a array of object with size of 26 - so each object of class Dict will have 26 sub objects like a child.

This program takes no input from user for now. I gave my input in main function. but it will print all the words in the trie.

This program works fine. Is this program somewhat efficient? do I have to replace the fixed sized array of object of class Dict with any other technique?

public class Dict {
        boolean isWord;
        Dict obj[];
        Dict()
        {
            obj=new Dict[26]; // as there are 26 letters in english
            isWord=false;
        }
        Dict insertWord(Dict Node,String str, int it) // used to insert a word into trie
        {
            if(it<str.length())
            {
            int Linkno=((int)(str.charAt(it)))-97; //97 is the ascii value of 'a'.
            if(Node.obj[Linkno]==null)
            {
                Node.obj[Linkno]=new Dict();
            }
            insertWord(Node.obj[Linkno],str,it+1);
            }
            else
            {
                Node.isWord=true;
            }
            return Node;
        }
        void displayAll(Dict Node,String str) // used to display all the words in the trie
        {
            for(int i=0;i<26;i++)
            {
                if(Node.obj[i]!=null)
                {
                    if(Node.obj[i].isWord==true)
                        System.out.println(str+((char)(i+97)));
                    displayAll(Node.obj[i],(str+((char)(i+97))));
                }
            }
        }
        public static void main(String ar[])
        {
            Dict root=new Dict();
            root.insertWord(root,"trasaction",0);
            root.insertWord(root,"transformation",0);
            root.insertWord(root,"transmission",0);
            root.insertWord(root,"trance",0);
            root.insertWord(root,"trap",0);
            root.displayAll(root,"");
        }
    }

Do I have to replace the fixed sized array of object of class Dict with any other technique?

I suggest making the size a constant, like this:

public static int ALPHABET_SIZE = 26:

And then you may use:

obj = new Dict[ALPHABET_SIZE];

As you can see, the comment now becomes redundant and we can remove it, and it is always a good thing, as comments may become obsolete or wrong.

The same applies to:

int Linkno=((int)(str.charAt(it)))-97; //97 is the ascii value of 'a'.

Make a constant:

LOWER_A_ASCII_VAL = 97;

Get rid of repetition

Looking at:

    root.insertWord(root,"trasaction",0);
    root.insertWord(root,"transformation",0);
    root.insertWord(root,"transmission",0);
    root.insertWord(root,"trance",0);
    root.insertWord(root,"trap",0);

What if you want to add 10 more words? The code will become very long and repetitive, instead use a foreach loop:

for (String word : {"transaction", "transformation", ...} ) {
    root.insertWord(root, word, 0);
}

Miscellanous

Wrong indent hurts readibility, fix it in:

    if(it<str.length())
    {
    int Linkno=((int)(str.charAt(it)))-97; //97 is the ascii value of 'a'.
    if(Node.obj[Linkno]==null)
    {
        Node.obj[Linkno]=new Dict();
    }
    insertWord(Node.obj[Linkno],str,it+1);
    }

Always use braces to prevent bugs in :

        if(Node.obj[i].isWord==true) {
            System.out.println(str+((char)(i+97)));
        }

Avoid general names like obj and think about a more precise name.


Checks if user is part of Active Directory group I'm working in C# using PrincipalContext to do a check if user is part of security group for access to the site. Site is asp.net MVC and part of an internal network with security groups set up via active directory.

Currently load times for pages are about 2.5-3 seconds on first load and then about 1.4-1.5 seconds on subsequent loads for each action. I'd like to decrease the time if possible and have set the query to check the domain of the user which cut some time out.

public override void OnAuthorization(AuthorizationContext filterContext)
{
    var alias = HttpContext.Current.User.Identity.Name;
    var domain = alias.Substring(0, alias.IndexOf("\\"));
    alias = alias.Substring(alias.IndexOf("\\") + 1);
    var accessLevel = this.Roles;

    if (CheckAccess(accessLevel, alias, domain))
    {
        return;
    }
}

private bool CheckAccess(string group, string user, string userDomain)
{
    PrincipalContext ctx;
    GroupPrincipal AdGroup = null;
    UserPrincipal AdUser = null;
    using (Forest forest = Forest.GetCurrentForest())
    {
        foreach(Domain domain in forest.Domains)
        {
            if (domain.Name.Contains("domain1"))
            {
                ctx = new PrincipalContext(ContextType.Domain, domain.Name, null, ContextOptions.Negotiate | ContextOptions.SecureSocketLayer, "username", "password");
                if (group == "WRITE")
                {
                    AdGroup = GroupPrincipal.FindByIdentity(ctx, "group1");
                }

                if (userDomain.ToLower().Contains("domain1"))
                {
                    AdUser = UserPrincipal.FindByIdentity(ctx, user);
                }
            }
            else if (domain.Name.Contains("domain2"))
            {
                if (userDomain.ToLower().Contains("domain2"))
                {
                    ctx = new PrincipalContext(ContextType.Domain, domain.Name, null, ContextOptions.Negotiate | ContextOptions.SecureSocketLayer, "username", "password");
                    AdUser = UserPrincipal.FindByIdentity(ctx, user);
                }
            }
        }

        if (AdGroup != null && AdUser != null)
        {
            if (AdUser.IsMemberOf(AdGroup))
            {
                return true;
            }
            else
            {
               return false;
            }
        }
        else
        {
            return false;
        }
     }
}

Also first question on CodeReview, let me know if there is anything to do to improve the question that I might have missed in the FAQ.

OnAuthorization()

If the possibility exists that HttpContext.Current.User.Identity.Name dosn't contain \ the call to alias.Substring() will fail. Instead of calling two times alias.IndexOf("\\") you could use string.Split().

This

if (CheckAccess(accessLevel, alias, domain))
{
    return;
}

is a little bit useless. You don't rellay care about the returned value of CheckAccess() hence you could remove it. But I guess thats not what you want. Instead you should e.g throw an AuthenticationException.

CheckAccess

PrincipalContext, GroupPrincipal and UserPrincipal are implementing the IDisposable interface, hence its best practice to enclose the usage in a using block.

Local variables should, based on the .NET Naming Guidelines, be named using camelCase casing and shouldn't use abbreviations. Hence AdUser should be activeDirectoryUser and AdGroup should be activeDirectoryGroup.

You should validate group and userDomain wether you really need to do the loop and you could store the result in a bool as well like so

bool userDomainIsInDomain1 = userDomain.ToLower().Contains("domain1");
bool userDomainIsInDomain2 = userDomain.ToLower().Contains("domain2");
bool groupCanWrite = group == "WRITE";

if(!userDomainIsInDomain && !userDomainIsInDomain2 && !groupCanWrite)
{
    return false;
}

We can now use the introduced bool variables in the loop like so

foreach (Domain domain in forest.Domains)
{
    if (domain.Name.Contains("domain1"))
    {
        using (var ctx = new PrincipalContext(ContextType.Domain, domain.Name, null, ContextOptions.Negotiate | ContextOptions.SecureSocketLayer, "username", "password"))
        {
            if (groupCanWrite)
            {
                activeDirectoryGroup = GroupPrincipal.FindByIdentity(ctx, "group1");
            }

            if (userDomainIsInDomain1)
            {
                activeDirectoryUser = UserPrincipal.FindByIdentity(ctx, user);
            }
        }
    }
    else if (domain.Name.Contains("domain2"))
    {
        if (userDomainIsInDomain2)
        {
            using(var ctx = new PrincipalContext(ContextType.Domain, domain.Name, null, ContextOptions.Negotiate | ContextOptions.SecureSocketLayer, "username", "password"))
            {
                activeDirectoryUser = UserPrincipal.FindByIdentity(ctx, user);
            }
        }
    }
}

but wait we can do better than that. We should extract two methods, one to get the an UserPrincipal and one to get a GroupPrincipal.

private UserPrincipal FetchUserPrincipal(string domainName, string userName, string password, string user)
{
    using (var ctx = new PrincipalContext(ContextType.Domain, domainName, null, ContextOptions.Negotiate | ContextOptions.SecureSocketLayer, userName, password))
    {
        return UserPrincipal.FindByIdentity(ctx, user);
    }
}

I am unsure about the user variable because I don't know if user is the same as "username".

private GroupPrincipal FetchGroupPrincipal(string domainName, string userName, string password, string groupName)
{
    using (var ctx = new PrincipalContext(ContextType.Domain, domainName, null, ContextOptions.Negotiate | ContextOptions.SecureSocketLayer, userName, password))
    {
        return GroupPrincipal.FindByIdentity(ctx, groupName);
    }
}

After implementing these methods the former CheckAccess() method will look like this

private bool CheckAccess(string group, string user, string userDomain)
{
    bool userDomainIsInDomain1 = userDomain.ToLower().Contains("domain1");
    bool userDomainIsInDomain2 = userDomain.ToLower().Contains("domain2");
    bool groupCanWrite = group == "WRITE";

    if(!userDomainIsInDomain && !userDomainIsInDomain2 && !groupCanWrite)
    {
        return false;
    }

    GroupPrincipal activeDirectoryGroup = null;
    UserPrincipal activeDirectoryUser = null;
    using (Forest forest = Forest.GetCurrentForest())
    {
        foreach (Domain domain in forest.Domains)
        {
            if (domain.Name.Contains("domain1"))
            {
                if (groupCanWrite)
                {
                    activeDirectoryGroup = FetchGroupPrincipal(domain.Name, "username", "password", "group1");
                }

                if (userDomainIsInDomain1)
                {
                    activeDirectoryUser = FetchUserPrincipal(domain.Name, "username", "password", user);
                }
            }
            else if (userDomainIsInDomain2 && domain.Name.Contains("domain2"))
            {
                activeDirectoryUser = FetchUserPrincipal(domain.Name, "username", "password", user);
            }
        }

        if (AdGroup != null && AdUser != null)
        {
            return AdUser.IsMemberOf(AdGroup);
        }
        return false;
     }
}

As you can see, I have simplified the if..else at the bottom as well.


Function that inserts values for an array of fields into a database table public function insert($table, $fields = array(), $values = array()) {
    $fields = '`' . implode ( '`,`', $fields ) . '`';
    $values = "'" . implode ( "','", $values ) . "'";
    $sql = "INSERT INTO {$table} ($fields) VALUES($values)";

    if ($this->_pdo->prepare ( $sql )) {
        if ($this->_pdo->exec ( $sql )) {
            return true;
        }
    }
    return false;
}

This is my first PHP OOP PDO insert function I have ever written.  My function is working well, but I believe this is not the best way I would have written.  Can somebody guide me on what I might have missed?

There's a couple of issues with it, for start your nested if blocks are unnecessary: instead of manually checking and returning true or false, just return the result of those methods, like so:

return $this->_pdo->prepare($sql) and $this->_pdo->exec($sql);

Additionally, you have made $fields and $values into optional parameters by making them auto-initialise to empty arrays, however you cannot have an INSERT INTO statement without specifying at least one field and value, therefore instead of making them optional array parameters, you should use type hinting to ensure they are arrays, and then check to make sure that the arguments are correct before you send the query off to the server, something like this:

public function insert($table, array $fields, array $values) {

    $numFields = count($fields);
    $numValues = count($values);

    if($numFields === 0 or $numValues === 0)
        throw new Exception("At least one field and value is required.");
    if($numFields !== $numValues)
        throw new Exception("Mismatched number of field and value arguments.");

    $fields = '`' . implode('`,`', $fields) . '`';
    $values = "'" . implode("','", $values) . "'";
    $sql = "INSERT INTO {$table} ($fields) VALUES($values)";

    return $this->_pdo->prepare($sql) and $this->_pdo->exec($sql);
}


Speedup for Project Euler 44 - pentagon numbers I'm trying to solve the problems of Project Euler with Haskell within less than 10s with optimized code (ghc -O2). Unfortunately, I'm struggling to find a correct algorithm for Problem 44, which proves the correctness of the solution without setting a predefined upper limit (setting a limit and omitting the proof yields a fraction of a second... unfortunately, you'll almost only find this solution while searching the web, which might be due to a slightly different wording of the question in the past).

  Pentagonal numbers are generated by the formula, \$P_n=n(3n−1)/2\$. The first ten pentagonal numbers are:

  \$1, 5, 12, 22, 35, 51, 70, 92, 117, 145, \ldots\$

  It can be seen that \$P_4 + P_7 = 22 + 70 = 92 = P_8\$. However, their difference, \$70 − 22 = 48\$, is not pentagonal.

  Find the pair of pentagonal numbers, \$P_j\$ and \$P_k\$, for which their sum and difference are pentagonal and \$D = |P_k − P_j|\$ is minimised; what is the value of \$D\$?

The way I want to go is straightforward. Check every possible difference D, till a solution is found. Unfortunately, this means, that a total of around \$1.2\cdot10^{9}\$ checks for pentagonal numbers have to be made.

My solution is the following:

-- calculate the n-th pentagonal number
pn :: Int -> Int
pn n = n*(3*n-1) `quot` 2

-- check if number is pentagonal
ispn :: Int -> Bool
ispn x = x == pn n
       where n = round $ (\x -> 1/6 * (1 + sqrt(1+24*x))) $ fromIntegral x

-- solution finder
-- l  > difference index
-- m  > prior limit of list with relevant pentagonal numbers
-- pl > list of pentagonal numbers
finder :: Int
finder = finder' 1 0 []
finder' :: Int -> Int -> [Int] -> Int
finder' l m pl = if length fpl /= 0
                   then l
                   else finder' (l+1) nm npl
                     where pnl = pn l
                       nm = (pnl - 1) `div` 3 + 1 -- +1 as backup
                       npl = (map pn [(m+1)..nm]) ++ pl
                       -- pnl     -> difference
                       -- x       -> smaller number
                       -- x+pnl   -> larger number
                       -- 2*x+pnl -> sum
                       fpl = filter (\x -> ispn (x+pnl) && ispn (2*x+pnl)) npl

main = print finder

As you can see, the code goes through every pentagonal difference \$D=P_l\$ and checks all relevant numbers by calculating the upper limit for the smaller of both numbers (index nm; above, the distance between two pentagonal numbers is larger than \$D\$) and testing the resulting larger number and sum for their pentagonality.

Profiling with

ghc -O2 -fforce-recomp -rtsopts -prof -fprof-auto main.hs
./main +RTS -sstderr -p

yields an increase of runtime by 200% (from 34s to 103s) and reports the following costs:

COST CENTRE   MODULE  %time %alloc

finder'.fpl.\ Main     30.7    0.0
ispn          Main     23.7    0.0
ispn.n        Main     22.5    0.0
ispn.n.\      Main     12.6    0.0
finder'.fpl   Main     10.0    0.0
finder'.npl   Main      0.1   99.9

I already tried to convert the check for pentagonality to a set lookup, but this didn't really help because this will grow up to 2 million elements. Also, converting the to-be-filtered list to an array or set didn't work as expected.

For a given difference \$D\$ (given by the pentagonal number \$P_l\$ which is pnl = pn l in the code), we know that starting with a given \$k\$ (nm in the code) the difference between \$P_k\$ and \$P_j\$ (\$j > k\$) will always be larger than \$D\$. This starting index can be calculated as \$k = \lfloor\frac{D-1}{3}\rfloor + 1\$.

Is there any way to squeeze some additional time out of this code, preferably while sticking to base libraries?

You're right that the problem condition that \$\left|P_k - P_j\right|\$ be minimized means that to properly solve it, it's not sufficient to find a pair of pentagonal numbers whose sum and difference are both pentagonal: you have to also establish that there is no pair of pentagonal numbers with the required property and a smaller difference.

Once you've found one pair of pentagonal numbers with the required property, this gives you a bound on \$j\$ and \$k\$; unfortunately the bound is in the millions, so that any \$\Omega(n^2)\$ algorithm (in particular, any algorithm that compares pairs of pentagonal numbers below the bound) will take far too long. It's no good just speeding up what you've got: you have to take a completely new approach.

So here's a rough sketch of an alternative algorithm.

The \$n\$th pentagonal number is $$P_n = {n(3n-1)\over 2}.$$ By completing the square, $$P_n = {(6n - 1)^2 - 1 \over 24}.$$ If \$P_k - P_j = P_x\$ and \$P_k + P_j = P_y\$ are both pentagonal numbers, then $$ \eqalign{ (6k - 1)^2 - (6j - 1)^2 &= (6x - 1)^2 - 1 \\ (6k - 1)^2 + (6j - 1)^2 &= (6y - 1)^2 + 1 } $$ Write \$K = 6k - 1\$, \$X = 6x - 1\$, and \$Y = 6y - 1\$. Add the two equations above, getting $$ 2K^2 = X^2 + Y^2. $$

So the plan is:

Iterate over all values for \$k\$ below the bound that you already established.
For each \$k\$, let \$K = 6k - 1\$, and enumerate all the ways to write \$2K^2\$ as the sum of two squares \$X^2 + Y^2\$.
For each such decomposition, if \$X ≡ -1 \pmod 6\$ and \$Y ≡ -1 \pmod 6\$ and \$J = \sqrt{K^2 - X^2 + 1} ≡ -1 \pmod 6\$, then this is a solution.

In step 2, the enumeration of \$2K^2\$ as sums of two squares can be computed efficiently by factorizing \$K\$ and then using the Brahmagupta–Fibonacci identity. Since you are going to be needing a lot of these factorizations, it will be most efficient to sieve for them.

This ought to be doable within your 10 second goal.

(Credit: I adapted this strategy from Daniel Fischer's post in the Project Euler forum.)


Nested "Select All" checkboxes So after answering a question about nested linked checkboxes I mentioned it at work and surprisingly it turned out to be similar to something we needed.

The requirements were:

a "Select all" checkbox
a nested "Select all" checkbox (only selects a small portion)
Cannot rely upon the hierarchy of the markup. The given fiddle is an example only. EDIT:- from Discussions with John I now see there was this requirement and so I've added it.

So realising it was different to the aforementioned question I set about writing my own.  I aimed for readability and minimal markup.

(function($) {
    $('input[type=checkbox][data-children]').unbind("change childchange childchangebubble").each(function() {
        var parent = $(this);

        var linkedChildren = $(parent.data("children"));

        //trigger a "childChange" event on the parent when any child is triggered.
        linkedChildren.bind("change childchangebubble", function() {
            parent.trigger("childchange");
        });

        parent.bind({
            "change": function(event) { // Bind change event to check all children
                linkedChildren.prop("checked", parent.prop("checked")).trigger("change");
            },
            "childchange": function() { // have to bind custom event seperately, there seems to be a jQuery bug.
                // When a child is changed recalculate if parent should be checked
                var noChildrenUnchecked = !linkedChildren.is(":not(:checked)");
                parent.prop("checked", noChildrenUnchecked).trigger("childchangebubble");
            }
        });
    });
})(jQuery);

the data-children attributes that are rendered to the page look like: data-children="#child2sub1, #child2sub2" on the parent and nothing extra needed on the children.

I'd love any feedback on readability/maintainability, performance etc.

current jsFiddle.

I have a suggestion.  Basically I'm not sure why you are adding so much to the HTML to handle the the parent and child checkboxes.  Isn't the hierarchy of the HTML enough to go on?  From the HTML you can already determine the parent and child relationships.  In additon why trigger more events.  Just handle the event as it bubbles up though the DOM.

Below is an example of some code that I believe to be far more reusable and much easier to implement.  In addition, I have modified this to work with future checkboxes added to the DOM.  It may be necessary to load some of the children via AJAX and/or appending to the DOM therefore your method would require you to bind your function on each append to the DOM, where as mine would handle it though delegation of events.

$.fn.nestedCheckboxes = function (stopPropogation) {
    this.addClass('nestedCheckboxes');
    this.click(nestedCheckboxHandler);
    this.delegate(':has( > input:checkbox)', 'click', nestedCheckboxHandler);
    function nestedCheckboxHandler (evt) {
        if ($(evt.target).is("input:checkbox")) {
            var parentContainer = $(this);
            var checkbox = parentContainer
                            .find(' > input:checkbox:eq(0)');
            var parentCheckbox = parentContainer
                                    .parent()
                                    .closest(':has( > input:checkbox)')
                                    .children('input:checkbox');

            if (evt.target == checkbox[0]) {
                $('input:checkbox', parentContainer)
                    .prop("checked", checkbox.prop("checked"));
            }
            var parentCheckboxValue = !parentCheckbox
                                        .parent()
                                        .find("input:checkbox")
                                        .not(parentCheckbox[0])
                                        .is("input:checkbox:not(:checked)");

            parentCheckbox.prop("checked",parentCheckboxValue);
        } else if (stopPropogation) {
            evt.stopPropagation();
        }
    };
};

$(function () {
    $('div').nestedCheckboxes();  //this is how you would use the function

    $('#addChildren').click(function () {
       $(this).after('

<input type="checkbox" />Child 2 SubChild 1

<input type="checkbox" />Child 2 SubChild2

');
       return false;
    });

});

In addition I have added an option for the nestedCheckboxes to stopPropogation if an element other than a checkbox is clicked.  By default this if set to false.

Finally have a look at the fiddle I created and play around.  You will see that the HTML Markup is far more simplistic.

http://jsfiddle.net/jfhartsock/WwtJr/


Creating a set of buffers for tiles I am currently converting a 2D game to 3D, never having worked with 3D before I am trying to learn as I go. I am wondering what the best way to use vertex/index buffers is.

I am currently looping through a grid and creating a set of buffers for each tile type, which are are then used in the draw method. I'm presuming I shouldn't use multiple sets of vertex buffers, but I don't know of another way to do this, as each tile is a different texture.

Grid loop

for (int i = Convert.ToInt32(xPos / 2) - 30; i < Convert.ToInt32(xPos / 2) + 30; i++)
{
    for (int a = Convert.ToInt32(yPos / 2); a < Convert.ToInt32(yPos / 2) + 50; a++)
    {
        if (mapXtile > 0 && mapYtile > 0 && mapXtile < Globals.mapsizex && mapYtile < Globals.mapsizey)
        {
            int currentTile = Globals.levelArray[level, mapXtile, mapYtile].TyleType;

            if (currentTile == tyleType.grass)
            {
                createCubeVertices(i * 2, a * 2, 1, 0f, count, grassVertices);
                SetUpBufferIndices(count, grassIndices);
                count++;
            }

            if (currentTile == tyleType.water)
            {
                createCubeVertices(i * 2, a * 2, 1, 0f, count, waterVertices);
                SetUpBufferIndices(count, waterIndices);
                waterCount++;
            }
        }
        mapYtile = mapYtile + 1;
        //mapYtile++;
    }
    mapXtile = mapXtile + 1;
    mapYtile = Convert.ToInt32(yPos / 2);
}

CopyToBuffers(grassVertices, grassIndices);
CopyToWaterBuffers(waterVertices, waterIndices);

Draw code

foreach (EffectPass pass in grassEffect.CurrentTechnique.Passes)
{
    pass.Apply();

    device.Indices = myIndexBuffer;
    device.SetVertexBuffer(myVertexBuffer);
    device.DrawIndexedPrimitives(PrimitiveType.TriangleList, 0, 0, grassVertices.Length, 0, grassIndices.Length/3);
}

foreach (EffectPass pass in mouseEffect.CurrentTechnique.Passes)
{
    pass.Apply();

    device.Indices = myWaterIndexBuffer;
    device.SetVertexBuffer(myWaterVertexBuffer);
    device.DrawIndexedPrimitives(PrimitiveType.TriangleList, 0, 0, 10000, 0, 10000);
}

You could batch by tiletype, so that the number of buffers submitted is only n (where n is the number of tile types).

e.g.

Dictionary<TileType,List<Tuple<int,int>>> tiles = new Dictionary<TileType,List<Tuple<int,int>>>();

for (int x = Convert.ToInt32(xPos / 2) - 30; x < Convert.ToInt32(xPos / 2) + 30; x++)
{
    for (int y = Convert.ToInt32(yPos / 2); y < Convert.ToInt32(yPos / 2) + 50; y++)
    {
        if (mapXtile > 0 && mapYtile > 0 && mapXtile < Globals.mapsizex && mapYtile < Globals.mapsizey)
        {
            int currentTile = Globals.levelArray[level, mapXtile, mapYtile].TyleType;

            if(!tiles.ContainsKey((TileType)currentTile))
            {
                tiles.Add((TileType)currentTile, new List<Tuple<int,int>>());
            }
            tiles[(TileType)currentTile].Add(new Tuple<int,int>(x*2,y*2));
        }
        mapYtile = mapYtile + 1;
        //mapYtile++;
    }
    mapXtile = mapXtile + 1;
    mapYtile = Convert.ToInt32(yPos / 2);
}

foreach(var tileCoords in tiles[TyleType.grass])
{
    //You would have to modify this to append to the buffer
    createCubeVertices(tileCoords.Item1, tileCoords.Item2, 1, 0f, count, grassVertices);
    SetUpBufferIndices(count, grassIndices);
    count++;
}

foreach(var tileCoords in tiles[TyleType.water])
{
    //You would have to modify this to append to the buffer
    createCubeVertices(tileCoords.Item1, tileCoords.Item2, 1, 0f, count, waterVertices);
    SetUpBufferIndices(count, waterIndices);
    count++;
}

CopyToBuffers(grassVertices, grassIndices);
CopyToWaterBuffers(waterVertices, waterIndices);

While this does involve additional looping, the extra looping will be considerably faster than clearing / sending buffers redundantly.

On the other hand since you seem to be rendering the same couple of meshes multiple times. have you considered Instanced Rendering? That will be a significant speed up, even over batching, and should result in some cleaner code too.


Comparing information to a directory of active enployees The code is doing everything that I need it to. It's taking much too long though upwards of a couple minutes to complete the macro. Does anyone know how I code optimize this VBA code to run quicker? I'm new to VBA and I'm not quite sure how to proceed. This code will be running on roughly 35,000 lines of data.

Public Sub matchRow()

    Dim dumpSheet, referencesheet, outputSheet As Worksheet
    Dim startRow, outputRow, tempDumpRow, tempActiveRow, referenceRowCount, finishedreferenceIndex As Integer
    Dim finishedreference() As String
    Dim isExist As Boolean

    'Set sheets
    Set dumpSheet = Sheets("Dump")
    Set referencesheet = Sheets("Active Directory")
    Set outputSheet = Sheets("Output")

    'Set start row of each sheet for data
    startRow = 5
    outputRow = 5

    'Get row count from Active Depository sheet
    referenceRowCount = referencesheet.Range("B5:D5").End(xlDown).Row

    'Set index
    finishedreferenceIndex = 5

    'Re-define array
    ReDim finishedreference(5 To referenceRowCount - 1)

    'Set the start row
    tempDumpRow = startRow

    'Here I looped with OR state, you can modify it to AND start if you want
    Do While dumpSheet.Range("B" & tempDumpRow) <> "" Or dumpSheet.Range("C" & tempDumpRow) <> "" Or dumpSheet.Range("D" & tempDumpRow) <> ""

        'Reset exist flag
        isExist = False

        'loop all row in Active Depository sheet
        For tempActiveRow = 5 To referenceRowCount Step 1

            'If row is not finished for checking.
            If UBound(Filter(finishedreference, tempActiveRow)) < 0 Then

                'If all cell are equal
                If dumpSheet.Range("B" & tempDumpRow) = referencesheet.Range("B" & tempActiveRow) And _
                   dumpSheet.Range("C" & tempDumpRow) = referencesheet.Range("C" & tempActiveRow) And _
                   dumpSheet.Range("D" & tempDumpRow) = referencesheet.Range("D" & tempActiveRow) Then

                    'Set true to exist flag
                    isExist = True

                    'Store finished row
                    finishedreference(finishedreferenceIndex) = tempActiveRow

                    finishedreferenceIndex = finishedreferenceIndex + 1

                    'exit looping
                    Exit For

                End If

            End If

        Next tempActiveRow

        'Show result
        outputSheet.Range("B" & outputRow) = dumpSheet.Range("B" & tempDumpRow)
        outputSheet.Range("C" & outputRow) = dumpSheet.Range("C" & tempDumpRow)
        outputSheet.Range("D" & outputRow) = dumpSheet.Range("D" & tempDumpRow)

        If isExist Then
            outputSheet.Range("E" & outputRow) = ""
        Else
            outputSheet.Range("E" & outputRow) = "Item found in ""Dump"" but not in ""Active Directory"""
        End If

        'increase output row
        outputRow = outputRow + 1

        'go next row
        tempDumpRow = tempDumpRow + 1

    Loop

    'loop all row in Active Depository sheet
    For tempActiveRow = 5 To referenceRowCount Step 1

        'If row is not finished for checking.
        If UBound(Filter(finishedreference, tempActiveRow)) < 0 Then

            'Show result
            outputSheet.Range("B" & outputRow) = referencesheet.Range("B" & tempActiveRow)
            outputSheet.Range("C" & outputRow) = referencesheet.Range("C" & tempActiveRow)
            outputSheet.Range("D" & outputRow) = referencesheet.Range("D" & tempActiveRow)
            outputSheet.Range("E" & outputRow) = "Item found in ""Active Directory"" but not in ""Dump"""

            'increase output row
            outputRow = outputRow + 1

        End If

    Next tempActiveRow

End Sub

This is the sample dataset I created.  The OP's code suggests that the Active Directory tab has an extra row.

Always Use Worksheets CodeNames Whenever Possible

Referencing worksheets by their code names will avoid naming conflicts while working with multiple workbooks and changing the worksheet name will not break any code.

Sheets("Active Directory") -> wsActiveDirectory
Sheets("Dump")-> wsDump
Sheets("Output") -> wsOutput

Use Constants for Magic Numbers

Using constants for values that should only be set once will make your code easier to read and maintain.  Constants will also throw an error if you try to change their values.

Before

startRow = 5
outputRow = 5

After

Const startRow As Long = 5, outputRow As Long = 5

Matching Lists

Dictionaries are optimised for fast lookups.  Using a Scripting.Dictionary will match the values will easily make the code run 100 times faster.

The trick is to create a composite key for all fields. Note: make sure to use a delimiter.

  1;Towney;Research and Development

Private Function getKey(ByVal rowIndex As Long, ByRef Target As Range) As String
    getKey = Target.Cells(rowIndex, 1) & ";" & Target.Cells(rowIndex, 2) & ";" & Target.Cells(rowIndex, 3)
End Function

SQL Solution

As Ryan Wildry stated "Using SQL is great for these type of set based comparisons." but this can be a little tricky.  The way I did it is I pasted my datasets into an Access Database as tables and use the Query Designer to help me write the code.

SELECT Dump.ID, Dump.Name, Dump.Department, "Item found in ""Dump"" but not in ""Active Directory""" AS [Found]
FROM Dump
WHERE (((Exists (SELECT NULL
  FROM [Active Directory]
  WHERE ([Active Directory].ID = Dump.ID) AND ([Active Directory].Name = Dump.Name) AND ([Active Directory].Department = Dump.Department)
))=False));
UNION ALL SELECT [Active Directory].ID, [Active Directory].Name, [Active Directory].Department, "Item found in ""Active Directory"" but not in ""[Active Directory]""" AS [Found]
FROM [Active Directory]
WHERE (((Exists (SELECT NULL
  FROM [Dump]
  WHERE ([Active Directory].ID = Dump.ID) AND ([Active Directory].Name = Dump.Name) AND ([Active Directory].Department = Dump.Department)
))=False));

I then aliased the tables to make it easier to replace the table names with the Excel Table Definition.

 SELECT t1.Name, t1.ID,  t1.Department, 'Item found in "Dump" but not in "Active Directory"' AS [Found]
 FROM [Dump$B4:E23] As t1
 WHERE (((Exists (SELECT NULL
   FROM [Active Directory] As t2
   WHERE (t2.ID = t1.ID) And (t2.Name = t1.Name) And (t2.Department = t1.Department)
 ))=False))
 UNION ALL
 SELECT t1.Name, t1.ID,  t1.Department, 'Item found in "Active Directory" but not in "Dump"' AS [Found]
 FROM [Active Directory$B4:E20] As t1
 WHERE (((Exists (SELECT NULL
   FROM [Dump] As t2
   WHERE (t2.ID = t1.ID) And (t2.Name = t1.Name) And (t2.Department = t1.Department)
 ))=False))

Now that I had the SQL working, I replaced the messages and created a single Select statement that I could modify to handle both selecting record in Dump and not in Active Directory or selecting records in Active Directory that are not in Dump.

SELECT t1.ID, t1.Name, t1.Department, "Message" AS [Found]
FROM [Dump] As t1
WHERE (((Exists (SELECT NULL
  FROM [Active Directory] As t2
  WHERE (t2.ID = t1.ID) AND (t2.Name = t1.Name) AND (t2.Department = t1.Department)
))=False));

Sub FindUnmatchedRowsCopyFromRecordset()

Create a recordset and use Range.CopyFromRecordset to transfer the records.

Sample SQl:

SELECT t1.Name, t1.ID,  t1.Department, 'Item found in "Dump" but not in "Active Directory"' AS [Found]
FROM [Dump$B4:E23] As t1
WHERE (((Exists (SELECT NULL
  FROM [Active Directory$B4:E20] As t2
  WHERE (t2.ID = t1.ID) And (t2.Name = t1.Name) And (t2.Department = t1.Department)
))=False))
UNION ALL
SELECT t1.Name, t1.ID,  t1.Department, 'Item found in "Active Directory" but not in "Dump"' AS [Found]
FROM [Active Directory$B4:E20] As t1
WHERE (((Exists (SELECT NULL
  FROM [Dump$B4:E23] As t2
  WHERE (t2.ID = t1.ID) And (t2.Name = t1.Name) And (t2.Department = t1.Department)
))=False))

Sub FindUnmatchedRowsAppend()

This is a slightly more complicated technique that appends the records directly to the Output tab.

Sample SQl:

INSERT INTO [Output$B4:E4] SELECT t3.* FROM (SELECT t1.Name, t1.ID,  t1.Department, 'Item found in "Dump" but not in "Active Directory"' AS [Found]
FROM [Dump$B4:E23] As t1
WHERE (((Exists (SELECT NULL
  FROM [Active Directory$B4:E20] As t2
  WHERE (t2.ID = t1.ID) And (t2.Name = t1.Name) And (t2.Department = t1.Department)
))=False))
UNION ALL
SELECT t1.Name, t1.ID,  t1.Department, 'Item found in "Active Directory" but not in "Dump"' AS [Found]
FROM [Active Directory$B4:E20] As t1
WHERE (((Exists (SELECT NULL
  FROM [Dump$B4:E23] As t2
  WHERE (t2.ID = t1.ID) And (t2.Name = t1.Name) And (t2.Department = t1.Department)
))=False))) as t3

Code

Option Explicit

Sub FindUnmatchedRowsAppend()
    Dim conn As Object
    Set conn = getThisWorkbookConnection
    conn.Open

    DeleteOutputResults
    Dim OutputDef As String
    OutputDef = getTableDefinition(wsOutput)

    Dim SQL As String
    SQL = "INSERT INTO " & OutputDef & " SELECT t3.* FROM (" & getOutputResultQuery & ") as t3"

    conn.Execute SQL
    conn.Close
End Sub

Public Sub FindUnmatchedRowsCopyFromRecordset()
    Dim conn As Object
    Set conn = getThisWorkbookConnection
    conn.Open
    Dim SQL As String
    SQL = getOutputResultQuery

    Dim rs As Object
    Set rs = conn.Execute(SQL)

    DeleteOutputResults
    wsOutput.Range("B5").CopyFromRecordset rs

    conn.Close

End Sub

Private Function getOutputResultQuery() As String
    Dim ActiveDirectoryDef As String
    ActiveDirectoryDef = getTableDefinition(wsActiveDirectory)

    Dim DumpDef As String
    DumpDef = getTableDefinition(wsDump)

    Const BaseSQl As String = "SELECT t1.Name, t1.ID,  t1.Department, '@Message' AS [Found]" & vbNewLine & _
        "FROM [xlTable1] As t1" & vbNewLine & _
        "WHERE (((Exists (SELECT NULL" & vbNewLine & _
        "  FROM [xlTable2] As t2" & vbNewLine & _
        "  WHERE (t2.ID = t1.ID) And (t2.Name = t1.Name) And (t2.Department = t1.Department)" & vbNewLine & _
        "))=False))"

    Dim SelectDump As String
    SelectDump = Replace(BaseSQl, "[xlTable1]", DumpDef)
    SelectDump = Replace(SelectDump, "[xlTable2]", ActiveDirectoryDef)
    SelectDump = Replace(SelectDump, "@Message", "Item found in ""Dump"" but not in ""Active Directory""")

    Dim SelectAD As String
    SelectAD = Replace(BaseSQl, "[xlTable1]", ActiveDirectoryDef)
    SelectAD = Replace(SelectAD, "[xlTable2]", DumpDef)
    SelectAD = Replace(SelectAD, "@Message", "Item found in ""Active Directory"" but not in ""Dump""")

    Dim SQL As String
    SQL = SelectDump & vbNewLine & "UNION ALL" & vbNewLine & SelectAD

    getOutputResultQuery = SQL
End Function

Private Sub DeleteOutputResults()
    Dim Target As Range
    With wsOutput
        Set Target = .Range("B4:E4", .Cells(.Rows.Count, "B").End(xlUp))
        Target.Offset(1).ClearContents
    End With
End Sub

Private Function getTableDefinition(ws As Worksheet) As String
    Dim Target As Range
    Select Case ws.Name
        Case wsActiveDirectory.Name, wsDump.Name
            With ws
                Set Target = .Range("B4:E4", .Cells(.Rows.Count, "B").End(xlUp))
            End With
            If ws Is wsActiveDirectory Then
                Rem Remove Summary Row
                Set Target = Target.Resize(Target.Rows.Count - 1)
            End If
        Case wsOutput.Name
            With ws
                Set Target = .Range("B4:E4", .Cells(.Rows.Count, "B").End(xlUp))
            End With
    End Select

    getTableDefinition = getTableDefinitionFromRange(Target)
End Function

Private Function getThisWorkbookConnection() As Object
    Dim conn As Object
    Set conn = CreateObject("ADODB.Connection")
    With conn
       .Provider = "Microsoft.ACE.OLEDB.12.0"
       .ConnectionString = "Data Source=" & ThisWorkbook.FullName & ";" & _
       "Extended Properties=""Excel 12.0 Xml;HDR=YES"";"
    End With

    Set getThisWorkbookConnection = conn
End Function

Private Function getTableDefinitionFromRange(Target As Range) As String
    Dim SheetName As String
    SheetName = Target.Parent.Name
    Dim Address As String
    Address = Target.Address(RowAbsolute:=False, ColumnAbsolute:=False)
    getTableDefinitionFromRange = "[" & SheetName & "$" & Address & "]"
End Function

Download Link

ADDump.xlsm