Generate alternate saturday dates list within range in R library(tidyr)
d = seq(as.Date("2018-01-01"),Sys.Date()+365,by='day')
saturdayList = d[weekdays(d)=='Saturday']

altSaturdayList = data.frame(lapply(split(saturdayList, format(saturdayList, "%Y-%m")), function(x)
  na.omit(x[c(2,4)])))

altSaturdayList = subset(gather(altSaturdayList),select = c(value))
HolidayList = rbind(HolidayList, setNames(altSaturdayList, "Holidays"))
HolidayList = rbind(HolidayList, setNames(as.data.frame(as.Date("2018-11-06")), "Holidays"))
HolidayList = arrange(unique(HolidayList),Holidays)

rm(d)
rm(saturdayList)
rm(altSaturdayList)

The above code generates a list of dates which contain 2nd and 4th saturday of every month within that date range.

As you can tell I have generated and deleted intermediate variables like d, saturdayList and altSaturdayList. I was wondering how I can make this code better and efficient?

I learned about %>% operator but I don't know how I can do it with no input.

Any tips to improve the code is welcomed. Thanks

A pipe (%>%) solution is the following. I also use the dplyr-package, which you are probably already using as you have the arrange-function in your code. First I create a tibble with the days as input and then we can pipe ahead ;)

library(tidyr)
library(dplyr)

dates <- tibble(days = seq(as.Date("2018-01-01"), Sys.Date() + 365, by = 'day'))

HolidayList <- dates %>%
  # Only keep Saturdays
  filter(weekdays(days) == 'Saturday') %>%
  # Create a group variable (year_month) within we want to select the 2nd and 4th value
  group_by(year_month = format(days, "%Y-%m")) %>%
  # Only keep the 2nd and 4th value in each year_month
  slice(c(2, 4)) %>%
  # Get rid of the grouping
  ungroup() %>%
  # Only select the days column and rename it to Holidays
  select(Holidays = days) %>%
  # Add the extra holiday 2018-11-06
  add_row(Holidays = as.Date("2018-11-06")) %>%
  # similar to unique.data.frame(), but considerably faster:
  distinct() %>%
  # Arrange ascending by Holidays
  arrange(Holidays)

HolidayList
# A tibble: 54 x 1
#      Holidays
#        <date>
#  1 2018-01-13
#  2 2018-01-27
#  3 2018-02-10
#  4 2018-02-24
#  5 2018-03-10
#  6 2018-03-24
#  7 2018-04-14
#  8 2018-04-28
#  9 2018-05-12
# 10 2018-05-26
# ... with 44 more rows


Command-line search for web-scraped TV Guide listings This is a project that I am looking to clean up.  The code is fully functioning and it has already been turned in, so I figured I can let Code Review help me.  The git repository can be found here: https://github.com/TylerP33/sports_today

Here is my user input handler for my Command Line interface:

def user_input

    input = nil
    input = gets.strip.downcase

    case input

    when  "football"
        info = SportsToday::Schedule.football
        show_info(info)
        post_search_options

    when "baseball"
        info = SportsToday::Schedule.baseball
        show_info(info)
        post_search_options

    when "college basketball"
        info = SportsToday::Schedule.college_basketball
        show_info(info)
        post_search_options

    when "nba basketball"
        info = SportsToday::Schedule.nba_basketball
        show_info(info)
        post_search_options

    …

The code above calls these methods in the SportsToday::Schedule class, which search for specific strings in attributes (derived from scraped HTML):

def self.select_by(sport)
    all.select do |object|
        ATTRIBUTES.any? { |attr| object.public_send(attr).include?(sport) }
    end
end

def self.football
    select_by("Football")
end

def self.baseball
    select_by("Baseball")
end

def self.college_basketball
    select_by("College Basketball")
end

def self.nba_basketball
    select_by("NBA")
end

…

And then, the results are displayed using this:

def show_info(info)
    info.collect {|info|
        puts""
        puts""
        puts "\nEvent: #{info.sport}"
        puts "\nTime: #{info.time}"
        puts "\nDescription: #{info.description}"
        puts "\nMatchup: #{info.matchup}"
        puts ""
        puts ""
    }
end

I have some nasty code repetition going on — 10 similar methods in my CLI and Schedule classes — and I have no idea how to get around it.  There HAS to be a way for me to make some type of container for keywords, and be able to access them through my select_by method.

Refactoring #1

The first thing I noticed is the repetition in user_input. You're using the case statement as flow control--it's no different than a series of if/else statements. Instead, use the case statement to set your info variable and move the other statements out:

info = case input
       when "football"           then SportsToday::Schedule.football
       when "baseball"           then SportsToday::Schedule.baseball
       when "college basketball" then SportsToday::Schedule.college_basketball
       when "nba basketball"     then SportsToday::Schedule.nba_basketball
       end

show_info(info)
post_search_options

Refactoring #2

After the first refactoring, it becomes clear that the case statement is simply translating a String to a method name (which gets translated back to a String in the method itself). This is inefficient. It might be tempting to generate the method name:

SportsToday::Schedule.call(input.tr(" ","_"))

but all that does is mask the underlying problem: you have a method for every possible search string.

Instead, consider changing the interface of the class. Instead of one method per search string, use one for all:

#SportsToday::Schedule

def self.sport(sportname)
  select_by(sportname)
end

Not only does this simplify your SportsToday::Schedule class, it simplifies your CLI:

info = SportsToday::Schedule.sport(input)
show_info(info)
post_search_options

Note that I "cheated" a little on this refactoring, as the interface changes slightly. The user needs to enter the case of the sport name differently. However, it's not much different than before, as an exact match was required in your original code too. If the intent is to make it accommodate user input, you can adjust for user input sloppiness in your Schedule class. I wouldn't do it in the CLI, because it requires knowledge of the Schedule's underlying data format and that should be kept encapsulated in your Schedule class.

[I don't have time to add more refactorings right now, I'll try to come back to this and add more.]


Project Euler Problem #4 - Palindromic number (Revision 1) I've cobbled together as best I could an answer using the format found here provided. I lack full understanding of how the LINQ query works and don't want to use it without understanding well the how and why of what's occurring.

Regarding NumberIsAPalindrome, I tried to use number.ToString().Reverse().ToString() but was unable to achieve the results I expected, so I reverted to a for...loop. Is this achievable?

class PalindromNumber
{
    public string GetPalindromeNumber(int maxNumber = 999)
    {
        var query = from first in ThreeDigitNumbers()
                    from second in ThreeDigitNumbers()
                    let product = first * second
                    where NumberIsAPalindrome(product)
                    select product;
        return query.Max().ToString();
    }

    public int[] ThreeDigitNumbers()
    {
        int[] numberArray = new int[900];
        for (int i = 0; i < numberArray.Length; i++)
        {
            numberArray[i] = 999 - i;
        }
        return numberArray;
    }

    public bool NumberIsAPalindrome(int number)
    {
        int reversedValue=0;
        string stringNumber = number.ToString();
        for (int i= stringNumber.Length-1; i >= 0; i--)
        {
            reversedValue += (int.Parse(stringNumber[i].ToString()) * (int)Math.Pow(10,i) );
        }
        return number == reversedValue;
    }
}

You were on the right track starting with number.ToString().  However, you don't need to duplicate the string, just check whether the value at each index is the same as the value at the index the same distance from the other end:

bool IsPalindrome(int number)
{
    string num = number.ToString();
    for (int i = 0, j = num.Length - 1; i < num.Length / 2; i++, j--)
    {
        if (num[i] != num[j])
        {
            return false;
        }
    }
    return true;
}


IIdentity and IPrincipal Interfaces I've always seen the IPrincipal and IIdentity interfaces separately, but I haven't seen any compelling reason for it, so I have my own interface that combines the two:

public interface IUser : System.Security.Principal.IIdentity, System.Security.Principal.IPrincipal {
    string Role { get; set; }
}

Then I implement the interface as:

public class User : IUser {
    public string Role { get; set; }

    public System.Security.Principal.IIdentity Identity {
        get { return this; }
    }

    public string AuthenticationType {
        get { throw new System.NotImplementedException(); }
    }

    public bool IsAuthenticated {
        get { throw new System.NotImplementedException(); }
    }

    public string Name {
        get { throw new System.NotImplementedException(); }
    }

    public bool IsInRole(string role) {
        throw new System.NotImplementedException();
    }
}

Does this smell?  Do I have a security issue here?

While there are circumstances where you need to decouple the Principal from the Identity, in many applications this is not the case, and the distinction will only complicate the code.  The way security is best handled in an application depends very much on the individual needs of the particular app.

Martin Fowler's article Dealing with Roles provides an in depth analysis on to the different patterns that can be used to implement role-based security, and the indications of necessity for each pattern.

In general, you can't make an assumption that a solution that separates the principal from the identity is the optimal solution for all applications.  I have encountered many scenarios where it was unnecessary and added nothing but additional classes.  In that case, it is better to merge the two interfaces.

Furthermore, these are interfaces not classes.  An interface denotes a "has a" relationship between the instance and the definition, where as a class denotes an "is a" relationship.  An instance of a User in the OP's example is an object that has both an Identity and a Principal (Security Context).  You can always break things down to further levels of complexity, but if the application functionality doesn't indicate a reasonable need to separate the class, then Why would you do it?


Sort an array of 0s, 1s and 2s in Java Write a program to sort an array of 0's,1's and 2's in ascending
  order.

  Input:

  The first line contains an integer 'T' denoting the total number of
  test cases. In each test cases, First line is number of elements in
  array 'N' and second its values.

  Output:

  Print the sorted array of 0's, 1's and 2's.

  Constraints:

  1 <= T <= 100
  1 <= N <= 105
  0 <= arr[i] <= 2

  Example:

  Input :

2
5
0 2 1 2 0
3
0 1 0

  Output:

0 0 1 2 2
0 0 1

My approach:

import java.io.InputStreamReader;
import java.io.BufferedReader;
import java.io.IOException;

class GFG {

    private static void printSortedArr (int[] arr) {
        int count0 = 0, count1 = 0, count2 = 0;

        for (Integer elem : arr) {
            if (elem == 0) {
                count0++;
            }
            else if (elem == 1) {
                count1++;
            }
            else {
                count2++;
            }
        }

        while (count0 != 0) {
            System.out.print(0 + " ");
            count0--;
        }

        while (count1 != 0) {
            System.out.print(1 + " ");
            count1--;
        }

        while (count2 != 0) {
            System.out.print(2 + " ");
            count2--;
        }
        System.out.println();
    }

    public static void main (String[] args) throws IOException {
        BufferedReader br = new BufferedReader(new InputStreamReader(System.in));
        String line = br.readLine();
        int numTests = Integer.parseInt(line);

        for (int i = 0; i < numTests; i++) {
            String line2 = br.readLine();
            int size = Integer.parseInt(line2);
            int[] arr = new int[size];
            String line3 = br.readLine();
            String[] inps = line3.split(" ");

            for (int j = 0; j < size; j++) {
                arr[j] = Integer.parseInt(inps[j]);
            }

            printSortedArr(arr);
        }
    }
}

I have the following questions with regards to the above code:

How can I further improve my approach?
Is there a better way to solve this question?
Are there any grave code violations that I have committed?
Can space and time complexity be further improved?

Reference

Descriptive names

    private static void printSortedArr (int[] arr) {

I don't like these names.  I would rather write out array if that's what's being said.  E.g.

    private static void printSortedArray(int[] numbers) {

Now I don't have to figure out that Arr is short for array.  And numbers is more descriptive than array anyway.

Favor arrays over numbered variables

        int count0 = 0, count1 = 0, count2 = 0;

So here you have three variables that hold different values based on their names.  However, we have a data type that enforces that relation, the array.

As proposed here:

    private static void printSortedArray(int[] array, int maxElement) {
        int[] counts = new int[maxElement + 1];

Now you have a more reusable method.  It's no longer limited to just sorting an array of 0s, 1s, and 2s.  It can sort any array of 0, ..., maxElement.  Further, it is now self-organizing.  Instead of

            if (elem == 0) {
                count0++;
            }
            else if (elem == 1) {
                count1++;
            }
            else {
                count2++;
            }

You can just say

            counts[element]++;

You no longer have to expand the if/else each time you add a new element value.

In a comment, you said

  Isn't storing the values in an array a space overhead?

The difference between your original three variables and this array is the length variable.  Other than that, both use the same space.  The cost of the extra variable is more than made up with the reduction in errors from replacing the if/else monster with the simple array update.

For example, your original has a bug if the array contains numbers other than 0, 1, or 2.  It silently counts any other number as a 2.  The revised version would throw an array out of bounds exception in that case, and it will do so without any added checking on your part.

In another comment, you say

  I thought that according to the problem statement, there was no explicit need to store the sorted elements in the array causing me to just print them. Also, I thought that using an array would be a space overhead.

In that case, the array duplicated the original array and was a space overhead.  But the thing that you gain there is that you can reuse the created array for other things.

You are correct in pointing out that the current problem statement does not require separate action and display.  What we are trying to tell you though is that in the real world, problem statements change.  Your code is fragile.  If the problem statement changes, you have to rewrite it.  We want to encourage you to write robust code that will continue to work even if the goal changes.  This is because the goal in a project tends to change a lot.

Donald Knuth said:

  The real problem is that programmers have spent far too much time worrying about efficiency in the wrong places and at the wrong times; premature optimization is the root of all evil (or at least most of it) in programming.

You are generally going to be better off writing for good engineering principles and then, in those rare instances where it matters, optimizing.  Don't fall in love with your optimized solutions, as user requirements will stomp all over them.

What if you were asked to print out the counts for each element?  You would have to modify this code to do that.  However, if your method was

    // numbers can only contain values from 0 to maximumValue
    private static int[] countElementsByValue(int[] numbers, int maximumValue) {
        int[] counts = new int[maximumValue + 1];

        for (int element : numbers) {
            counts[element]++;
        }

        return counts;
    }

Then you could easily display the counts or the array.  Or both.  Because you have that information right there.  With the original method, you would either need to rewrite it like this or recount the elements to do that display.  Because the original method discards that information immediately.  Or you could modify the original method.  But then what happens when someone says that they want just the array without the counts?

In general, we prefer small methods that do one thing and only one thing over more complicated methods that do multiple things.  This is especially true about mixing generation and display.  That fails quite often, as we have found that requiring multiple displays over the same basic data is quite common.  So there is a specific instance (don't mix generation and display) of the more general rule to keep difference actions separate.

Your original method might become something like

        int[] counts = countElementsByValue(numbers, 2);
        int[] sorted = expandCounts(counts);
        System.out.println(Arrays.toString(sorted));

Note how the last line doesn't have to know that it's displaying a sorted array.  It doesn't care.  Whatever the array is, it displays.

We could probably improve performance by writing a special display routine that works off the counts directly.  But it wouldn't be that much of an improvement.  And most of the time, it won't matter.  With your example input, the time difference is going to be trivial.  We'll spend more time compiling the program than running it.

Performance

If you're really worried about storage performance, then why read the data into an array at all?

            String line3 = br.readLine();
            String[] inps = line3.split(" ");

            for (int j = 0; j < size; j++) {
                arr[j] = Integer.parseInt(inps[j]);
            }

            printSortedArr(arr);

Could be

            for (String token : br.readline().split(" ")) {
                 char digit = token.charAt(0);
                 if (digit == '0') {
                     System.out.print("0 ");
                 } else {
                     counts[digit - '0']++;
                 }
            }

            int value = '0';
            for (int count : counts) {
                while (count > 0) {
                    System.out.print(value + " ");
                    count--;
                }

                value++;
            }
            System.out.println();

One reason not to do this is that, counter-intuitively, it is actually slower to mix input and output like this than to use more storage.  In particular, System.out.print is not high performance with printing two characters at a time.  You'd usually be better off using a StringBuilder to generate the string and then outputting that.

Why use readline?  The resulting array could potentially be huge, and you are committing yourself to two arrays, one of strings and one of integers.  Consider

            int read = br.read();
            while (read >= 0) {
                if (read != ' ') {
                    counts[read - '0']++;
                }

                read = br.read();
            }

This saves us two arrays and a string at the cost of one smaller array.  If you are really space constrained, mixing input and generation like this is better than mixing generation and output.

Of course, it is quite possible that this is slower than your original.  That's a common trade-off, speed for space.

Then with Java 8, you can use something like (from Stack Overflow):

            for (i = 0; i < counts.length; i++) {
                String value = Integer.toString(i);
                System.out.print(String.join(" ", Collections.nCopies(counts[i], value)));
            }
            System.out.println();

This will display with only four print statements.

Or if that uses too much space (it's unclear to me if it creates a temporary collection or not), stick to your original output method.  That may be slow, but it uses very little space.

Remember though, that this kind of optimization is something you should do after you establish that you need it.  Most of the time this is going to be unnecessary and it may even be counter-productive.  E.g. if you're worrying about space constraints when you are actually time constrained.


Finding the 10001st prime in C# In an effort to learn  C# / improve my coding skills, I've started working on the Project Euler problems. I see that I may have been overthinking things a bit after finishing problem 7:

  By listing the first six prime numbers: 2, 3, 5, 7, 11, and 13, we can
  see that the 6th prime is 13.

  What is the 10 001st prime number?

But even so I believe that my solution should be faster than what is presented in the problem overview. While this may not be an issue when searching for a "small" number of primes, the previous problems have focused quite a bit on optimizing the codes so I am wondering if I have missed something  obvious.

In other words, how does my code compare to the pseudocode presented in the overview of the problem? Are there any obvious mistakes I make in my code?

using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Threading.Tasks;
using System.Diagnostics;

namespace ProjectEulerTasks
{
    class Program
    {
        static void Main(string[] args)
        {
            int no_of_primes_found = 0;
            int current_number = 0;
            List<int> found_primes = new List<int>();
            bool n_minus_found = false;
            bool n_plus_found = false;

            found_primes.Add(2);
            found_primes.Add(3);
            found_primes.Add(5);
            found_primes.Add(7);
            found_primes.Add(11);
            found_primes.Add(13);

            current_number = 12;
            no_of_primes_found = 6;

            while (no_of_primes_found < 10001)
            {
                current_number += 6;
                n_minus_found = false;
                n_plus_found = false;
                foreach (int current_prime in found_primes)
                {
                    if (!n_minus_found && (current_number - 1) % current_prime == 0)
                    {
                        n_minus_found = true;
                    }
                    if (!n_plus_found && (current_number + 1) % current_prime == 0)
                    {
                        n_plus_found = true;
                    }
                    if (n_minus_found && n_plus_found)
                    {
                        break;
                    }
                }
                if (!n_minus_found)
                {
                    found_primes.Add(current_number - 1);
                    no_of_primes_found = no_of_primes_found + 1;
                }
                if (!n_plus_found && no_of_primes_found < 10001)
                {
                    found_primes.Add(current_number + 1);
                    no_of_primes_found = no_of_primes_found + 1;
                }
            }
            System.Diagnostics.Debug.Print(Convert.ToString(no_of_primes_found));
            System.Diagnostics.Debug.Print(Convert.ToString(found_primes[found_primes.Count - 1]));
        }
    }
}

Project Euler pseudocode:

Your solution looks massively overcoded and woefully under-engineered - as if you had fixed on one particular solution facet (striding in the manner of the mod 6 wheel with unrolling of one revolution) of a particular approach (trial division) before analysing the problem.

Also, you didn't specify with regard to which quality you want your code to be compared to others. It is way too convoluted for scoring on elegance and simplicity, and it is way too slow for scoring on speed. Hence it is impossible to guess what you think its merit might be...

You obviously overlooked the bit about only scanning up to the square root of the current number in the pseudo code. This alone makes your code run much more slowly than the simplest, cleanest, most straightforward algorithm without any optimisations whatsoever. In other words, all that nonsense with wheeled striding and unrolling is for nought.

To answer your question: compared to the pseudo code your solution does not fare well at all.

If you are looking for speed in contexts that involve the enumeration of non-trivial numbers of primes then you might want to look at sieves. The Sieve of Eratosthenes is simple but offers an unbeatable bang for buck ratio.

For the piddling small numbers that Project Euler tasks deal in, I'd recommend going for simplicity, cleanness and elegance. At least that will give you are more thorough understanding of the algorithms. Forget all the half-understood supposed optimisations that you may have seen somewhere. They are only optimisations if they are needed, applicable, and applied correctly. In all other cases they are pessimisations, and they also keep you from understanding your own code, obviously.

Start simple. Measure. Add complications one by one and measure the effect.

Also, the work that is done fastest is the work that you don't do at all. Hence it always pays to look at a problem with an eye for opportunities not to do things. This includes donning first the glasses of the mathematician, then those of the algorithmist, and only last the loupe of the performance coder.

Answer to eirikdaude's question in the comment (posted here to give it space):

Wheeled striding can cut the workload by a certain (small) constant factor, by way of skipping a certain part of the numbers to be scanned.

Double striding cuts the load in half by skipping multiples of two; mod 6 striding skips a further third, mod 30 (2*3*5) striding a further fifth, and so on.

Effectiveness decreases as you add more small primes and the effort increases explosively. Double striding has the best bang-for-buck ratio in many cases, and it has other advantages (like regular strides, as opposed to the 2,4,2,4 step of the mod 6 wheel or the 48 step sequence of the mod 210 wheel). Higher wheel orders than 2 require careful coding and engineering to realise at goodish part of the theoretical speedup. Even the highest-order wheel with the most perfect coding can realise only a small constant speedup, typically not more than half an order of magnitude (without even considering the slow-down due to the much more complex code).

The column 'modulus' gives the wheel circumference, 'spokes' gives the number of hops in an increment sequence, 'ratio' gives the ratio of how many numbers need to be scanned compared to scanning all candidates.

The mod 30 wheel has a lot of nice advantages (starting with the fact that it maps nicely to eight-bit bytes) but cannot even double the performance of the mod 2 wheel, despite all the effort required for implementing it. Mod 210 wheels are occasionally found in heavy-duty industrial-grade code, and wheels higher than 13 (starting with 17, i.e. mod 510,510) definitely belong in the pyrrhic oddball category.

Use the 'ratio' column to gauge the limit on possible speedups due to workload reduction, and use the 'spokes' column to gauge the required code complexity (unrollings, sizes of lookup tables etc.).

Have a look at my answer in the topic Printing all the prime numbers between two bounds to see benchmarks for actual speedups gained from wheeled striding in a roughly similar setting. Compare actual speedups to the theoretical limits gleaned from the table, to the increase in code complexity despite the tricks employed, and how little this all accomplishes compared to algorithmic optimisations.

Morale: outside of academic papers, wheels can give you only a small constant boost.

On the other hand, scanning only candidate factors up to the square root of the current number instead of all candidates gives exponential benefits, several orders of magnitude already with the small Euler target (100001st prime).

I found it very rewarding to treat the Euler problems with respect, giving them the full math ponderation and pencil+paper preparation before throwing down the first line of code - and then striving for the simplest-possible, cleanest, most elegant code that I can manage (some might say this comes a couple decades too late, and they might not be entirely wrong about that...).


Finding objects with even references tidier As the title says, I'm trying to make my code less horrible looking! For a university practical, I've to use recursive methods to return the even references in a collection of Integer objects stored in an ArrayList.

For example, if the input is [1, 2, 3, 4], my method should return [1, 3] (i.e. elements 0 and 2, the even elements).

My code works, but I feel is horrible:

import java.util.*;

public class ListMethods {

public static ArrayList<Integer> evenList (ArrayList<Integer> tList) {
    ArrayList<Integer> newList = ListMethods.deepClone(tList);
    int tempStorage = newList.size();

    if (newList.size() <= 0)
        return newList;
    else
    {
        if (newList.size()%2==0)
            tempStorage = newList.remove(newList.size()-2);
        newList.remove(newList.size()-1);
        newList = ListMethods.evenList(newList);
        if (tempStorage!=0)
            newList.add(tempStorage);
    }
    return newList;
}

public static ArrayList<Integer> deepClone (ArrayList<Integer> tList) {
    ArrayList<Integer> list = new ArrayList<Integer>();

    for (Integer i : tList)
        list.add(new Integer(i));
    return list;
}

public static void main (String[] args) {
    ArrayList<Integer> tempList = new ArrayList<Integer>();
    tempList.add(1);
    tempList.add(2);
    tempList.add(3);
    tempList.add(4);
    tempList.add(5);
    tempList.add(6);

    for (Integer i : tempList)
    { System.out.println (i); }

    ArrayList<Integer> evenList = ListMethods.evenList(tempList);
    System.out.println();

    for (Integer i : evenList)
    { System.out.println (i); }
}
}

The cloning section I have to include, at our lecturer's request. The real part I'm looking to tidy up is the if/else inside the evenList method.

Does anybody have any ideas?

I'm not sure if you're tied to having the method signature public static ArrayList<Integer> evenList(ArrayList<Integer> tList) and if you have to use the deepClone method you've got here, but if you aren't tied down to these restrictions, there are cleaner ways of doing this.

Firstly, one of the main points of recursion is using the stack to store state - this doesn't seem to really come through in your example. Let's think about the problem a bit: we want to go over each element in our list, adding that element to another list if the index of that element is even. How would we do this in "normal" code? Well, we'd just loop over the List with a for loop, probably:

(Note: I use List<Integer> here instead of ArrayList<Integer>. This may not make sense to you yet - if so, just mentally replace every List<Integer> with ArrayList<Integer>.)

public static List<Integer> evenList(List<Integer> tList)
{
    List<Integer> result = new ArrayList<Integer>();
    for(int i = 0; i < tList.size(); ++i) {
        if(i % 2 == 0) {
            result.add(tList.get(i));
        }
    }
    return result;
}

So converting this to recursive code, what state do we store in the function that we can store on the stack instead? Well, both the result and the index, i. So instead of creating a new List that we return, let's pass both of those in as parameters instead:

public static void evenList(List<Integer> tList, List<Integer> result, int index)

So, with this recursive method, how will we know when to stop? Well, exactly like in our for loop: when our index is at tList.size():

public static void evenList(List<Integer> tList, List<Integer> result, int index)
{
    if(index < tList.size()) {
       ...
    }
}

Ok, so what about the logic? Well, it hasn't really changed much - we still want to do the exact same thing, that is, if index is even, add the element at that index to our result list:

public static void evenList(List<Integer> tList, List<Integer> result, int index)
{
    if(index < tList.size()) {
        if(index % 2 == 0) {
            result.add(tList.get(index));
        }
    ...
}

Now, we need a call to the function itself (otherwise it wouldn't be recursive!), but what parameters do we pass through? Well, we always need tList, so that should go through. We want to keep adding to the same result, so that should go through. However, our index needs to change - we want to test the next element - so that should be ++index. So our final function looks like:

public static void evenList(List<Integer> tList, List<Integer> result, int index)
{
    if(index < tList.size()) {
        if(tList.get(index) % 2 == 0) {
            result.add(tList.get(index));
        }
        evenList(tList, result, ++index);
    }
}

I'm not really sure why your professor has you using clone methods here - it's really inefficient. Every single recursive call, you call deepClone on the List you pass in - this is a lot of wasted effort.

In fact, if we were being really clever here, we would see that every second element gets added to our return list, so we can skip one of the if checks:

public static void evenList(List<Integer> tList, List<Integer> result, int index)
{
    if(index < tList.size()) {
        result.add(tList.get(index));
        index += 2;
        evenList(tList, result, index);
    }
}

Of course, this only works properly when the user passes in an even initial value, presumably 0. So let's make sure that happens. Let's make this method private and supply the starting index:

private static void evenList(List<Integer> tList, List<Integer> result, int index)
{
    if(index < tList.size()) {
        result.add(tList.get(index));
        index += 2;
        evenList(tList, result, index);
    }
}

public static void evenList(List<Integer> tList, List<Integer> result)
{
    evenList(tList, result, 0);
}

If you are tied to the original method signature, well, I've typed a lot of stuff for not much good I suppose, although I'd have to question why your professor had made you do it this way.


Extract code and convert special characters to HTML entities I want users to enter their code into my blog and keep original styling tags intact. So I had to develop a function that extracts user's code (between two tags) and convert special characters to HTML entities, then remove unwanted tags using purifyHTML.

I want to know if it is safe to use it like that, and if there are any bad issues with this function.

public function extracter($string, $start, $end){
    function get_string_between($string, $start, $end){
        $delimiters = array();
        $ini = strpos($string,$start);
        if ($ini == 0) return FALSE;
        $delimiters['start'] = substr($string, 0, $ini);
        $ini += strlen($start);
        $last = strpos($string,$end,$ini);
        $len =  $last - $ini;
        $delimiters['inside'] = substr($string,$ini,$len);
        $delimiters['end'] = substr($string, $last+strlen($end));
        return $delimiters;
    }
    function reconstruct($strings = array(), $filter){
        if( count($strings) == 0 || !$strings )
            return false;
        $count = count($strings);
        $i = 0;
        $str = '';
        $code = array();
        $uniq = uniqid();//i create a unique key to prevent confusion if users insert the same token i'm using
        foreach ($strings as $key => $val) {
            $i++;
            foreach ($val as $k => $v) {
                if( $k == 'start' )
                    $str .= $v;
                if( $k == 'inside' ){
                    $str .= '['.$uniq.$i.']'; //creat a token in the string
                    $code[$i] = htmlspecialchars($v);
                }
                if( $i == $count )
                    if( $k == 'end' )
                        $str .= $v;
            }
        }
        $purify = $filter->purifyHtml($str);//i'm using purifyhtml to remove undesirable tags and leave TinyMce tags
        for ($j=1; $j <= $i; $j++) {
            $purify = str_replace('['.$uniq.$j.']', $code[$j], $purify); //replace the token with the escaped code
        }
        return $purify;
    }
    /* main */
    $string = " ".$string;
    $components = array();
    $exists = TRUE;
    while ( $exists ) {
        $st = get_string_between($string, $start, $end);//decompose if token found
        if( !$st )
            return $string;// return the original string if nothing found
        $string = $st['end'];
        $components[] = $st;
        $exists = strpos($st['end'], $start) !== FALSE;//check if another code exists in the string
    }

    return reconstruct($components, $this->InputFilter);
}

"Extracter" should be spelled "extractor".

It's nice that you broke up the functionality into smaller functions.

In get_string_between(), you don't appear to be properly handling the $start and $end strings not being found.  Consider:

$ini = strpos($string,$start);
if ($ini == 0) return FALSE;

Did you mean to write

if ($ini === FALSE) return FALSE;

instead?  strpos() is tricky, in that you have to carefully distinguish between 0 and FALSE return values.

For $last = strpos($string,$end,$ini), you don't check the return value at all, and just use it.

Flag variables are an ineffective way to control execution flow.  If possible, restructure the code to avoid them.  For example,

$components = array();
$exists = TRUE;
while ( $exists ) {
    $st = get_string_between($string, $start, $end);//decompose if token found
    if( !$st )
        return $string;// return the original string if nothing found
    $string = $st['end'];
    $components[] = $st;        $exists = strpos($st['end'], $start) !== FALSE;//check if another code exists in the string
}

… should be written as an equivalent do-while loop:

$components = array();
do {
    $st = get_string_between($string, $start, $end);//decompose if token found
    if( !$st )
        return $string;// return the original string if nothing found
    $string = $st['end'];
    $components[] = $st;
} while (strpos($st['end'], $start) !== FALSE); //check if another code exists in the string

However, even that is dissatisfying, since the strpos() test is redundant with the strpos() operation within get_string_between().

$components = array();
while (($st = get_string_between($string, $start, $end))) {
    $components[] = $st;
    $string = $st['end'];
}
return empty($components) ? $string    // return the original string if nothing found
                          : reconstruct($components, $this->InputFilter);

In reconstruct(), your loops are too complex.  The original code…

$count = count($strings);
$i = 0;
$str = '';
$code = array();
$uniq = uniqid();//i create a unique key to prevent confusion if users insert the same token i'm using
foreach ($strings as $key => $val) {
    $i++;
    foreach ($val as $k => $v) {
        if( $k == 'start' )
            $str .= $v;
        if( $k == 'inside' ){
            $str .= '['.$uniq.$i.']'; //creat a token in the string
            $code[$i] = htmlspecialchars($v);
        }
        if( $i == $count )
            if( $k == 'end' )
                $str .= $v;
    }
}

… could be simplified to

$str = '';
$code = array();
$uniq = uniqid();
foreach ($strings as $i => $component) {
    $str .= $component['start'];
    $str .= "[$uniq" . ($i + 1) . "]";  // Create a token in the string
    $code[$i + 1] = htmlspecialchars($component['inside']);
}
$str .= $component[count($component) - 1]['end'];

It's unconventional that $code is a 1-based array, and there doesn't seem to be any advantage to starting from 1 rather than 0, so just use $i instead of $i + 1 everywhere.  Just adjust $j accordingly.

Nobody likes to read low-level string manipulation code, especially when it's not clear what the code is supposed to do.  Now that I've understood it, I would write a comment on the function like this:

/**
 * Runs HTML purifier on $string.  Sections of the string between $start
 * and $end delimiters are taken literally: instead of being filtered through
 * the HTML purifier, they are escaped using htmlspecialchars() instead.
 * The $start and $end delimiters themselves are discarded.
 */


Implementing a Stack with Templates and Smart pointers I'm trying to understand the concepts of smart pointers and templates. To get a better understanding of these, I tried to implement these two concepts on Stack. I have created a Stack which can hold objects of any type. For dynamic memory allocation, I have used smart pointers.

Please suggest how I could improve the code. Am I missing any cases while using smart pointers and templates? Since I'm using smart pointers, can I rest assure that free store (heap) memory will be deleted once the Stack variable goes out of scope?

#include "stdafx.h"
#include<iostream>
#include<string>
#include<memory>

using namespace std;

// Template Node
template <typename T>
struct Node
{
    T data;
    shared_ptr<Node> next;
    Node(T d, shared_ptr<Node> n) : data(d), next(n) {}
};

// Template Stack
// Note: T type is used to set the Node type
template <typename T>
class Stack {
private:
    shared_ptr<Node<T>> head;
public:
    Stack() : head(NULL) {}
    Stack(const Stack<T> &rhs) = delete;
    Stack& operator=(const Stack<T> &rhs) = delete;
    void push(const T&);
    void pop();
    T top() const;
    void print() const;
};

// Notice <T> after the class name, it is the syntax requirement
template <typename T>
void Stack<T>::push(const T &data) {
    shared_ptr<Node<T>> temp = make_shared<Node<T>>(data,nullptr);
    if (head == nullptr) {
        head = temp;
    }
    else {
        temp->next = head;
        head = temp;
    }
}

template <typename T>
void Stack<T>::pop() {
    //shared_ptr<Node<T>> temp = head;
    head = head->next;
}

template <typename T>
T Stack<T>::top() const {
    return head->data;
}

template <typename T>
void Stack<T>::print() const {
    // shared_ptr<Node<T>> temp;
    for (auto temp = head; temp != nullptr; temp = temp->next)
        cout << temp->data << '\n';
}

int main()
{
    cout << "String data" << '\n';
    {
        Stack<string> strStack;
        strStack.push("Test1");
        strStack.push("Test2");
        strStack.push("Test3");
        strStack.print();
        strStack.pop();
        strStack.print();
    }
    cout << "Integer data" << '\n';
    {
        Stack<int> intStack;
        intStack.push(10);
        intStack.push(20);
        intStack.push(30);
        intStack.print();
        intStack.pop();
        intStack.print();
    }
    return 0;
}

Readability

Please put a space after the include and before the <

#include<iostream>
#include<string>
#include<memory>

Don't do this

You can read any other C++ revue on this site.

using namespace std;

See: Why is “using namespace std” considered bad practice?

Don't add useless comments

// Template Node
template <typename T>
struct Node

Thanks but I can read the code I can see it is a template and a class called Node. Useless comments are worse than no comment.

// Template Stack
// Note: T type is used to set the Node type
template <typename T>

Again you are describing what the code does. If you write nice well formatted code it should explain itself (with good method and variable names).

// Notice <T> after the class name, it is the syntax requirement
template <typename T>
void Stack<T>::push(const T &data) {

Don't explain the compiler rules. The compiler does not care nor do I.

Pass parameters by reference

Here you pass the parameters by value.

    Node(T d, shared_ptr<Node> n) : data(d), next(n) {}

This means you are copying the data to the parameters than you are making a second copy of the data into the member variables.

Try this:

    Node(T const& d, shared_ptr<Node>& n) : data(d), next(n) {}

You can even use move semantics. I'll cover that more in the push() below.

    Node(T&& d, shared_ptr<Node>& n) : data(std::move(d)), next(n) {}

Push

You don't need that if block.

void Stack<T>::push(const T &data) {
    shared_ptr<Node<T>> temp = make_shared<Node<T>>(data,nullptr);
    if (head == nullptr) {
        head = temp;
    }
    else {
        temp->next = head;
        head = temp;
    }
}

You always assign temp to head. The value assigned to temp.next is nullptr if the head is nullptr or it is head. In both cases the value of head is assigned to next.

void Stack<T>::push(const T& data) {
    head = make_shared<Node<T>>(data, std::move(head));
}

You pass the data by reference, which is good. Make sure that node also takes the parameter by reference.

In some situations we also want to add move semantics to the class. So it can also be useful to add a push() that takes an r-value reference. This allows for a much more efficient push when T is large (like an vector).

void Stack<T>::push(T&& data) {
    head = make_shared<Node<T>>(std::move(data), std::move(head));
}

Also in the vain of push. Sometimes T is expensive to build and copy but its arguments can easily be passed. In this case it is worth allowing the user to pass the parameters to the constructor of T and then building the T object in place (rather than passing it around).

template<typename... Args>
void Stack<T>::push<Args>(Args...&& args) {
    head = make_shared<Node<T>>(std::move(args));
}

template<typename... Args>
Node<T>::Node<Args...>(Args...&& args)
    : data(args...) // Use the constructor of T
{}

Top

Here you return the result by value. This means a copy is probably generated. This could be expensive so it I would return a reference rather than a value. The user can then decide if they want to copy the reference explicitly into a value.

T Stack<T>::top() const {
    return head->data;
}

Change to

  T const& Stack<T>::top() const {
//  ^^^^^^
      return head->data;
  }

Print

Looks fine.

void Stack<T>::print() const {
    // shared_ptr<Node<T>> temp;
    for (auto temp = head; temp != nullptr; temp = temp->next)
        cout << temp->data << '\n';
}

The thing I would change is to pass a stream as a parameter (std::cout is not the only stream you want to print to). It can default to std::cout for backward compatibility.

Also the standard way of printing in C++ is using operator<< so it would be nice to define that in addition.

void Stack<T>::print(std::ostream& str = std::cout) const
{
    for (auto temp = head; temp != nullptr; temp = temp->next)
        str << temp->data << '\n';
}

std::ostream& operator<<(std::ostream& str, Stack<T> const& value)
{
    value.print(str);
    return str;
}


Controller class pattern The GetDropdownList() function is being called on a couple of controllers based on this example and it may be called in many other controllers, so this code is being repeated. I was thinking of encapsulating this portion of code to the base class and call that base class in these controllers, but I got stuck and don't know how to implement it. Can you please show me a way using SOLID principles?

public interface IDropdownListEntity
{
    string Value { get; set; }
    string Text { get; set; }
}

public sealed class DropdownListEntity: IDropdownListEntity
{
    public string Value { get; set; }
    public string Text { get; set; }
}

public class CountryController : ApiController
{
    //Returns the list of countries
    public IEnumerable<IDropdownListEntity> GetDropdownList()
    {
        CountryStore store = new CountryStore();

        return store.SimpleSortedListByName<DropdownListEntity>();
    }
}

public class CityController : ApiController
{
    //Returns the list of cities
    public IEnumerable<IDropdownListEntity> SortedListForDropdown()
    {
        CityStore store = new CityStore();

        return store.SimpleSortedListByName<DropdownListEntity>();
    }
}

Web API should not be bound to a particular UI

API calls should not bound to a user interface requirement, so that it could be used in multiple places without any addition requirement. In the above code, it is returning a list of dropdownlist values rather than a list of entities.

Your controller code may look like this:

public class ConstantsController : ApiController
{
    private readonly IConstantRepository _constantRepository;

    public ConstantsController(IConstantRepository constantRepository)
    {
        _constantRepository = constantRepository;
    }

    [Route("Constants/City")]
    [HttpGet]
    public IEnumerable<City> GetCity()
    {
        return _constantRepository.GetCity();
    }

    [Route("Constants/Country")]
    [HttpGet]
    public IEnumerable<Country> Country()
    {
        return _constantRepository.GetCountry();
    }
}

Note: I am using Web API 2 for this code.

This code can be used by any kind of application like this:

  http://yoururl/Constants/Country

Use a DI container to inject dependencies into your controller

Use Unity or a simple injector to inject dependencies. It will help you to isolate components.

public interface IConstantRepository
{
    IEnumerable<City> GetCity();

    IEnumerable<Country> GetCountry();
}

Also, include unit tests in your applications.


Java rolling dice + csv array output I have made a program to take x number of die and rolls them y number of times, then stores the data into an array so that I may output a CSV file. Everything works as intended, but I am having trouble figuring out how to increase the number of die to anything substantial. Right now I am using a switch, but linearly adding code like this seems inefficient, not to mention it will crash with amounts larger than 4 die. Is there some shortcut for adding variable number of switch statements? Any other methods would work as well, I am just not clever enough to come up with any as of yet.

import javax.swing.JOptionPane;

public class histogram {
public static void main(String[] M83cluster) {

    // # of die
    String N = JOptionPane.showInputDialog("How many dice would you like to roll?");
    int numofDie = Integer.parseInt(N);
    // # of rolls
    String M = JOptionPane.showInputDialog("how many times would you like to roll?");
    int numofRolls = Integer.parseInt(M);
    int maxValue = numofDie*6;
    int[] taco = new int[maxValue]; // for every die there will be at most 6 values.

    // rolls the die and obtains a value.
    for (int i=0;i<numofRolls; i++) {
        int oneTotalRoll = 0;
        for (int k=0;k<numofDie; k++) {
             oneTotalRoll += (int)(1+6*Math.random());
        }

        //int oneTotalRoll = (int) (valueofDice * numofDie);
        System.out.println("ROLL: " + oneTotalRoll);

        // for each roll, increment taco[] array.
        switch (oneTotalRoll) {
            case 4: taco[0] += 1;
                break;
            case 5: taco[1] += 1;
                break;
            case 6: taco[2] += 1;
                break;
            case 7: taco[3] += 1;
                break;
            case 8: taco[4] += 1;
                break;
            case 9: taco[5] += 1;
                break;
            case 10: taco[6] += 1;
                break;
            case 11: taco[7] += 1;
                break;
            case 12: taco[8] += 1;
                break;
            case 13: taco[9] += 1;
                break;
            case 14: taco[10] += 1;
                break;
            case 15: taco[11] += 1;
                break;
            case 16: taco[12] += 1;
                break;
            case 17: taco[13] += 1;
                break;
            case 18: taco[14] += 1;
                break;
            case 19: taco[15] += 1;
                break;
            case 20: taco[16] += 1;
                break;
            case 21: taco[17] += 1;
                break;
            case 22: taco[18] += 1;
                break;
            case 23: taco[19] += 1;
                break;
            case 24: taco[20] += 1;
                break;
            case 25: taco[21] += 1;
                break;
        }
    }
    System.out.println("-------");

    String gorgon = null;  // prints outcome
    for (int g=0; g<maxValue ; g++) {
        String gigabolt = (taco[g] + ",");
        gorgon += gigabolt;
        // System.out.print(gigabolt);
    }
    if (gorgon.endsWith(","))
        gorgon = gorgon.substring(4, gorgon.length() - 1);
    System.out.print(gorgon);
}
}

Indentation

Your code's indentation is wrong, the class and method declaration should not be on the same line.

Whitespace

Your code has incorrect whitespace in places, for example:

int maxValue = numofDie*6;

and here:

case 4: taco[0] += 1;
    break;

The first one is missing whitespace between the operators, and the second one should have the taco[0] statement on a new line.

switch:

Your entire switch statement could be condensed to this:

taco[oneTotalRoll - 4] += 1;

All of your cases were testing if oneTotalRoll's value was 4 more than the array position, so in that case, you could just minus four from oneTotalRoll and use that as an index position.

Naming

You have variables named like taco , gorgon, N and M.

These should be named to things that are closer to their usage and what they are, for example:

taco into rollCount
gorgon into condensedList
N into stringDieCount
M into stringRollCount

String concatenation

Instead of string concatenating everything at the end, consider using an Array join to "glue" everything together with a common join (,).


Codilty, binary gap get the highest gap I have written a solution for the codility task which is Binary Gap.
finding the highest gap in Binary. for example 1041 is the input number and convert it to binary the result is 5. here is the code :

package codility

import(
    "strconv"
)

func Solutions(N int) int{
    binary := convertNumberToBinary(N)
    result := getHighestGap(binary)

    return result
}

func convertNumberToBinary(N int)string{
    result := ""

    for N > 0 {
        binary := N%2;
        result = strconv.Itoa(binary) + result
        N = N/2;
    }

    return result
}

func getHighestGap(binary string) int{
    result := 0
    count := 0
    isCount := false
    stringArray := []byte(binary)

    for i:=0; i < len(stringArray); i++{
        if stringArray[i] == '1' {
            if result < count{
                result = count
            }
            count = 0
            isCount = true
        }else if isCount{
            //found 0
            count ++
        }
    }

    return result
}

and the test file :

package codility

import(
    "testing"
)

func TestBinrayGap(testing *testing.T){
    var testObjects = []struct{
        input int
        expected int
    }{
        {1041, 5},
        {529, 4},
        {20, 1},
    }

    for _, t := range testObjects {
        actual := Solutions(t.input)
        if actual != t.expected{
            testing.Errorf("Test failed expected : %s, actual : %s", t.expected, actual)
        }
    }
}

here I'm converting the input number to binary first and then count the highest length of the gap by converting the string to array byte `[]byte'. And for the test I'm using table driven test.

It would have been a good idea to link to the problem statement in your question.
Please run gofmt on your code, typically have your editor automatically format your files every time you save your source file.
Your naming conventions aren't very Go-like.

Solutions is too vague, BinaryGap would be better.
convertNumberToBinary is really long; this isn't Java — toBinary would largely be enough
stringArray is very wrong: this variable is a byte slice, plus, it doesn't give you extra information from its type
TestBinryGap should probably be TestBinaryGap
testObjects is weird and slightly misleading; Go test files usually name this variable test or tc, and expected is pretty much always called want

When you want to append repetitively to a string like in convertNumberToBinary, use string.Buffer. But here, why not just use a []bool that you can name isOne or something?
I think the function is simple enough that you can just put it in one function, and compute the binary representation at the same time you compute the binary gap. This would make your memory complexity $(1) instead of O(log(n)) (no need to allocate a slice or a string anymore) and simplify your code.
You don't need to convert your string (or your []bool) into a []byte later on; you can just iterate on it directly.
You should probably test that the input is a nonnegative number and return an error if that's the case (although maybe this isn't necessary in a competitive programming setting).
On the positive side, your test is a good idiomatic example of struct-based tests =)


Iterating pattern (1-10 then 10-1) with only 1 loop I want to obtain the following output:

1 2 3 4 5 6 7 8 9 10
10 9 8 7 6 5 4 3 2 1
1 2 3 4 5 6 7 8 9 10

The code am running is as follows:

#include <stdio.h>

int main(void) {
    int i=1,flag=0,lines=0;  //0 for forward, 1 for reverse
    while(i!=0 && lines<3){
        if(!flag){
            printf("%d ",i);
            if(i==10){
                flag=1;
                printf("\n");
                lines++;
            }
            else
                i++;
        }
        else if(flag){
            printf("%d ",i);
            if(i==1){
                lines++;
                flag=0;
                printf("\n");
            }
            else
                i--;
         }
    }
    return 0;
}

Am getting the desired output from the above code but not sure if it's an optimal code. Any other method/suggestion? Considering unlimited space but time complexity should be kept minimum.

Condition: Use only one loop

I don't see any compelling reason for doing this in one loop, but it's certainly possible in a simpler way.  First, though let's look at some other aspects of the code.

Declare one variable per line

Generally, it's better for readability if we declare just one variable per line instead of cramming them all together on a single line.

Use more whitespace

Lines like this

while(i!=0 && lines<3){

Are rendered much more readable if we use more whitespace:

while (i != 0 && lines < 3) {

Use braces consistently

I'd recommend using braces around the else clause statement, even though it isn't technically required.  In addition to making things more readable (there's that word again!) it helps to prevent bugs.

Use better variable names

In the current code, lines is a good variable name because it suggests the purpose of the variable, but flag is not a very good name.  It would be better to make it bool and call it reverse.

Use named constants

Named constants make it easier for readers of the program to understand what's being done and why.

Simplify your code

Essentially, what the program is intended to do is make three traversals from 1 to 10 and back.  I'd write it like this:

int main(void) {
    const int lowerlimit = 1;
    const int upperlimit = 10;
    int i = lowerlimit;
    int delta = +1;  // start counting up
    for (int traversals=3; traversals; ) {
        printf("%d ", i);
        i += delta;
        if (i > upperlimit || i < lowerlimit) {
            printf("\n");
            delta *= -1;   // change direction
            i += delta;    // come back in bounds
            --traversals;
        }
    }
}

Omit return 0

When a C or C++ program reaches the end of main the compiler will automatically generate code to return 0, so there is no need to put return 0; explicitly at the end of main.

Note: when I make this suggestion, it's almost invariably followed by one of two kinds of comments:  "I didn't know that." or "That's bad advice!"  My rationale is that it's safe and useful to rely on compiler behavior explicitly supported by the standard.  For C, since C99; see ISO/IEC 9899:1999 section 5.1.2.2.3:

  [...] a return from the initial call to the main function is equivalent to calling the exit function with the value returned by the main function as its argument; reaching the } that terminates the main function returns a value of 0.

For C++, since the first standard in 1998; see ISO/IEC 14882:1998 section 3.6.1:

  If control reaches the end of main without encountering a return statement, the effect is that of executing return 0;

All versions of both standards since then (C99 and C++98) have maintained the same idea.  We rely on automatically generated member functions in C++, and few people write explicit return; statements at the end of a void function.  Reasons against omitting seem to boil down to "it looks weird".  If, like me, you're curious about the rationale for the change to the C standard read this question.  Also note that in the early 1990s this was considered "sloppy practice" because it was undefined behavior (although widely supported) at the time.

So I advocate omitting it; others disagree (often vehemently!)  In any case, if you encounter code that omits it, you'll know that it's explicitly supported by the standard and you'll know what it means.


Updating multiple membership plan expiration dates at once I have model action in RequestAdmin like this:

def offer_plan(modeladmin, request, queryset):
    class MembershipForm(forms.Form):
        plan = forms.ModelChoiceField(queryset=Plan.objects.all(), required=True,empty_label=None)
        _selected_action = forms.CharField(widget=forms.MultipleHiddenInput)
        expires = forms.DateField(initial="click to select date")

    selected = request.POST.getlist(admin.ACTION_CHECKBOX_NAME)
    links = {}
    if request.POST.get('submit'):
        form = MembershipForm(request.POST)
        if form.is_valid():
            plan = form.cleaned_data['plan']
            expires = form.cleaned_data['expires']
            for _id in selected:
                Request.objects.filter(id=_id).update(offered_membership_plan=plan,expires=expires)
                links[Request.objects.filter(id=_id).get().email] = 'www.xyz.com/plan/' + str(int(_id))
            return render_to_response("success.html", {'links': links,
                }, context_instance=RequestContext(request))

    else:
        form = MembershipForm(initial={'_selected_action': selected})
    return render_to_response("offer.html", {'form': form,
        'selected_action': selected,}, context_instance=RequestContext(request))

How can I improve the above code (I think it's always better not to run database calls in for loops) by removing database calls in loops?

You want to set the same values on multiple rows.  The query you want should be UPDATE Requests SET offered_membership_plan=…, expires=… WHERE id IN (…, …, …), and a way to achieve that would be:

Request.objects.filter(id__in=selected).update(
    offered_membership_plan=plan,
    expires=expires
)


Software drivers for the mcu peripherals in C++ I have been developing software drivers for the mcu peripherals in C++. My approach how to implement the drivers is following.
Each peripheral has its software driver modeled by C++ class with its interface
Peripheral.h
#include <cinttypes>
#include "DriversCfg.h"

class Peripheral {
public:

    Peripheral(const PeripheralCfg& peripheral_cfg, uint32_t base_address);

    void enable();
    void toggleReset();
    bool isEnabled();

private:

    struct ControlReg
    {
        uint32_t enable_bit:    1;
        uint32_t reset_bit:     1;
        uint32_t cfg_bit_01:    1;
        uint32_t cfg_bit_02:    1;
        uint32_t cfg_bit_03:    1;
    };

    struct PeripheralRegs
    {
        volatile ControlReg control_reg;
    };

    PeripheralRegs* regs;

};

and its implementation
Peripheral.cpp
#include <new>
#include "Peripheral.h"

Peripheral::Peripheral(const PeripheralCfg& peripheral_cfg, uint32_t base_address) {
    regs = new (reinterpret_cast<void*>(base_address)) PeripheralRegs;

    regs->control_reg.cfg_bit_01 = static_cast<uint32_t>(peripheral_cfg.cfg_bit_1);
    regs->control_reg.cfg_bit_02 = static_cast<uint32_t>(peripheral_cfg.cfg_bit_2);
    regs->control_reg.cfg_bit_03 = static_cast<uint32_t>(peripheral_cfg.cfg_bit_3);
}

void Peripheral::enable()
{
    regs->control_reg.enable_bit = 1;
}

bool Peripheral::isEnabled()
{
    return ((regs->control_reg.enable_bit) != 0);
}

void Peripheral::toggleReset()
{
    if(regs->control_reg.reset_bit)
    {
        regs->control_reg.reset_bit = 0;
    }
    else
    {
        regs->control_reg.reset_bit = 1;
    }
}

Each peripheral has fixed part of its configuration i.e. part of configuration which is not changeable  during run-time
PeripheralCfg.h
enum class FunctionUsage
{
  kNotUsed,
  kUsed
};

struct PeripheralCfg
{
  FunctionUsage cfg_bit_1;
  FunctionUsage cfg_bit_2;
  FunctionUsage cfg_bit_3;
};

There is a "container" containig the fixed part of the configuration for all the peripherals
DriversCfg.h
#include "PeripheralCfg.h"

struct DriversCfg
{
    PeripheralCfg periph_cfg_01 = {FunctionUsage::kAdcNotUsed, FunctionUsage::kAdcUsed,
                                   FunctionUsage::kAdcUsed};
    PeripheralCfg periph_cfg_02 = {FunctionUsage::kAdcNotUsed, FunctionUsage::kAdcNotUsed,
                                   FunctionUsage::kAdcUsed};
};

There is a "container" containing instances of all the drivers
Hal.h
#include "Peripheral.h"
#include "DriversCfg.h"

class Hal {
public:

    Peripheral *periph_01;
    Peripheral *periph_02;

    Hal();

private:

    DriversCfg configuration;

};

Hal.cpp
#include "Hal.h"
#include "DriversCfg.h"
#include "Registers.h"

Hal::Hal() {
    periph_01 = new Peripheral(configuration.periph_cfg_01, PERIPH_01_BASE_ADDRESS);
    periph_02 = new Peripheral(configuration.periph_cfg_02, PERIPH_02_BASE_ADDRESS);
}

The intended usage is
main.cpp
int main(int argc, char** argv)
{
    Hal hal;

    hal.periph_01->enable();
    hal.periph_02->enable();
}

I have tested this approach and it works. Nevertheless I have some doubts regarding namely following aspects:

container DriversCfg with configuration for the peripherals in which the individual configurations are public - is it a problem here?
order of instantiation of the private attribute DriversCfg configuration in the class Hal more precisely whether there is a chance that the DriversCfg configuration will not be created at the moment when a reference to it will be passed into the peripheral drivers?

Thanks in advance for any remarks and suggestions.

Avoid the placement new
The placement new you are doing looks redundant. PeripheralRegs is a POD type, so its member values will probably not be initialized, but to avoid any doubt I would just write:
Peripheral::Peripheral(const PeripheralCfg& peripheral_cfg, uint32_t base_address):
    regs{reinterpret_cast<PeripheralRegs*>(base_address)}
{
    regs->control_reg.cfg_bit_01 = static_cast<uint32_t>(peripheral_cfg.cfg_bit_1);
    regs->control_reg.cfg_bit_02 = static_cast<uint32_t>(peripheral_cfg.cfg_bit_2);
    regs->control_reg.cfg_bit_03 = static_cast<uint32_t>(peripheral_cfg.cfg_bit_3);
}

Toggle using XOR
You can rewrite toggleReset() by using the XOR operation:
void Peripheral::toggleReset()
{
    regs->control_reg.reset_bit ^= 1;
}

Why the k prefix for enum class values?
I see the values of enum class FunctionUsage are prefixed with a k. This looks like Hungarian notation, but I don't see Hungarian notation used anywhere else in this code. I suggest you drop it. Since it's an enum class, there is no chance that these values would conflict with anything else.
Use arrays where appropriate
I see several variables being repeated with just a different number at the end. Those are often a candidate to be replaced by arrays. Consider writing:
struct PeripheralCfg
{
  FunctionUsage cfg_bits[3];
};

...

struct DriversCfg
{
    PeripheralCfg periph_cfgs[2] = {
        {FunctionUsage::kAdcNotUsed, FunctionUsage::kAdcUsed, FunctionUsage::kAdcUsed},
        {FunctionUsage::kAdcNotUsed, FunctionUsage::kAdcNotUsed, FunctionUsage::kAdcUsed},
    };
};

...

class Hal {
    ...
    Peripheral *peripherals[2];
    ...
};

And so on. You can always rename the variables you used before to refer to an array element, for example periph_01 becomes periph[0]. Also, it opens up possibilities to use for-loops to avoid repetition:
for (auto &periph: hal.peripherals)
    periph->enable();

Although it looks silly now with an array of only two elements, it will quickly become more useful if you have to support hardware which supports more peripherals.
Make Hal::configuration static const
It looks like Hal::configuration is only meant to be read once, and never changed. You should change it into a static const variable:
class Hal {
    ...
    static const DriversCfg configuration;
    ...
};

Although it does look a bit weird this way. If it's only used by the constructor of Hal, then this should not be a (static) class member at all. You could move it completely into Hal.cpp:
static const struct
{
    PerpheralCfg periph_configs[2] = {...};
} configuration;

Hal::Hal() {
    perhiperal[0] = new Peripheral(configuration.periph_cfgs[0], PERIPH_01_BASE_ADDRESS);
    perhiperal[1] = new Peripheral(configuration.periph_cfgs[1], PERIPH_02_BASE_ADDRESS);
}

You could also keep the configuration in DriversCfg.h, but then still exactly as I've shown here.
As for the order of instantiation: global variables are instantiated before main() runs, so configuration will be valid before the variable hal is constructed. If it was still a regular member variable, then it would also have been instantiated before the body of the constructor was run, so your code is fine in that respect.
Avoid unnecessary memory allocations
Why does Hal need to allocate the Peripherals on the heap? Especially on an MCU you want to avoid dynamic memory allocations. So instead of having pointers to Peripherals inside class Hal, just have them by value:
class Hal {
public:
    Peripheral peripherals[2];
    Hal();
};

...

Hal::Hal():
    peripherals{
        {configuration.periph_cfgs[0], PERIPH_01_BASE_ADDRESS},
        {configuration.periph_cfgs[1], PERIPH_02_BASE_ADDRESS},
    }
{}

And then use it like so:
int main(...)
{
    Hal hal;
    for (auto &periph: hal.peripherals)
        periph.enable();
}


Writing a JTable to a tab delimited file I have written this class that will write a JTable to a tab delimited file so that it can be opened in Excel. It works great, but is there any way to make it better or have it directly open in Excel without having to go through the import screens all of the time? How can I tidy up this code to work or operate more efficiently?

import java.io.File;
import java.io.FileWriter;
import java.io.IOException;
import javax.swing.JTable;
import javax.swing.table.TableModel;

public class excel {
    String columnNames[] = { "Column 1", "Column 2", "Column 3" };
    String dataValues[][] =
    {
        { "12", "234", "67" },
        { "-123", "43", "853" },
        { "93", "89.2", "109" },
        { "279", "9033", "3092" }
    };

    JTable table;

    excel() {
        table = new JTable( dataValues, columnNames );
    }

    public void toExcel(JTable table, File file){
        try{
            TableModel model = table.getModel();
            FileWriter excel = new FileWriter(file);

            for(int i = 0; i < model.getColumnCount(); i++){
                excel.write(model.getColumnName(i) + "\t");
            }

            excel.write("\n");

            for(int i=0; i< model.getRowCount(); i++) {
                for(int j=0; j < model.getColumnCount(); j++) {
                    excel.write(model.getValueAt(i,j).toString()+"\t");
                }
                excel.write("\n");
            }

            excel.close();

        }catch(IOException e){ System.out.println(e); }
    }

    public static void main(String[] o) {
        excel cv = new excel();
        cv.toExcel(cv.table,new File("H:\\cs.tbv"));
    }
}

Most of the issues with this code are stylistic and violations of Java conventions rather than functional.

public class excel {

Class names should always be capitalized (e.g., public class Excel).  Your class should probably also be named more descriptively and appropriately.  Your class isn't Microsoft Excel.  It's a class which converts JTable data into a CSV.  So it would make more sense to name your class JTableConverter or something like that.

String columnNames[] = { "Column 1", "Column 2", "Column 3" };

Class fields should always have an explicit access modifier (public, private, or protected).  Generally, if they're constants (as you use them), they are either public static final or private static final with the name in ALL_CAPS_WITH_UNDERSCORES.  For an understanding of static, you can see this StackOverflow Q&A.

So... private static final String[] COLUMN_NAMES = { "Column 1", "Column 2", "Column 3" };

excel() {
    table = new JTable( dataValues, columnNames );
}

Constructors and methods should also have an explicit access modifier.  (public JTableConverter())

public void toExcel(JTable table, File file) {

Method names should generally be verbs or verb phrases (such as convertToExcel).  Also, since you're passing in the JTable to be used, it would make more sense for this to be a static method rather than an instance method.  If you want it to be an appropriate instance method, there's no need to pass in the JTable at all, since the instance will already have access to the JTable table field.

excel.close();

Releasing or closing resources should pretty much always be done in a finally block, which is guaranteed* to run.  i.e.,

FileWriter writer = new FileWriter(file);
try {
    //stuff
}
catch(IOException ioe) {
    //handling
}
finally {
    try {
        writer.flush();
        writer.close();
    }
    catch(IOException ioe) {
        //handling
    }
}

And, finally ...

public static void main(String[] o) {
    excel cv = new excel();
    cv.toExcel(cv.table,new File("H:\\cs.tbv"));
}

It's a bit nitpicky, but it's usually better to have your main method in a separate driver class (like JTableConverterDriver).


Computing the circumference of two circles This is a program which computes the circumference of two circles and gives some information such as the difference between circumferences.  I want to know whether the method declarations and method calling in my code is correct.  I didn't face any errors.

import java.text.*;
import javax.swing.*;
class Q5Main{
    public static void main(String args[]){
        double smallCircum;
        double largeCircum;
        InputHandler input = new InputHandler();
        Question5 myq = new Question5();

        double a = input.getDouble("Radius for the smaller circle:");

        smallCircum = (myq.start(a));//calculate the small circle circumference

        a = input.getDouble("Radius for the larger circle: ");
        largeCircum = (myq.start(a));//calculate the large circle circumference

        myq.showDetails(smallCircum, largeCircum);

    }

}

class Question5{
    DecimalFormat df = new DecimalFormat("0.000");

    private double circumference;//to store circumference

    //carry out the full process
    public double start(double value){
        return circum(value);
    }

    //compute the circumference
    private double circum(double radius){
        circumference = 2 * Math.PI * radius;
        return circumference;
    }

    public void showDetails(double smallCircum, double largeCircum){
        System.out.println("Circumference of smaller circle: "+df.format(smallCircum));
        System.out.println("\n");
        System.out.println("Circumference of larger circle: "+df.format(largeCircum));
        System.out.println("\n");
        System.out.println("Difference: "+df.format((largeCircum-smallCircum)));

    }

}

class InputHandler{
    private static final String DOUBLE_DEFAULT_PROMPT = "Enter integer: ";

    public double getDouble(){
        return getDouble(DOUBLE_DEFAULT_PROMPT);
    }

    public static double getDouble(String prompt){
        String inStr;
        inStr = JOptionPane.showInputDialog(null,prompt);
        return Double.parseDouble(inStr);
    }
}

Although your code works, it's not so good.

Declare variables as late as possible

Here you declare some variables at the top, and use them much later, for example smallCircum:

double smallCircum;
double largeCircum;
InputHandler input = new InputHandler();
Question5 myq = new Question5();

double a = input.getDouble("Radius for the smaller circle:");

smallCircum = (myq.start(a));//calculate the small circle circumference

It would be better to declare as late as possible, right before you need it:

InputHandler input = new InputHandler();
Question5 myq = new Question5();

double a = input.getDouble("Radius for the smaller circle:");
double smallCircum = (myq.start(a));//calculate the small circle circumference

Pointless field

In Question5, the variable circumference doesn't need to be a field.
It's only used here:

private double circum(double radius){
    circumference = 2 * Math.PI * radius;
    return circumference;
}

But there is no need to store that value in a field, and no need for even a local variable, you can simply return it directly:

private double circum(double radius) {
    return 2 * Math.PI * radius;
}

static method

It's unclear why this method in InputHandler is static:

public static double getDouble(String prompt){

I suggest to drop the static keyword, make it non-static.

Poor naming

Looking at this:

smallCircum = (myq.start(a));//calculate the small circle circumference

A method named start returns the calculated circumference.
That's really not intuitive.
Why not rename start to calculateCircum?
If its containing Question5 class needs to be kept general purpose and not imply circumference, then perhaps you can rename the method to just calculate instead.
Probably almost anything is better than start.

Another thing, a is a poor name to store radius.

Make private what you can

In Question5, df should be private.

Make final what you can

In Question5, df can be final.

Suggested implementation

With the above suggestions applied, the solution becomes:

package com.janosgyerik.practice.oj.leetcode.hard.SubstringWithConcatenationOfAllWords;

class Q5Main {
    public static void main(String args[]) {
        InputHandler input = new InputHandler();
        Question5 myq = new Question5();

        double smallRadius = input.getDouble("Radius for the smaller circle:");
        double smallCircum = myq.calculate(smallRadius);

        double largeRadius = input.getDouble("Radius for the larger circle: ");
        double largeCircum = myq.calculate(largeRadius);

        myq.showDetails(smallCircum, largeCircum);
    }
}

class Question5 {
    private final DecimalFormat df = new DecimalFormat("0.000");

    public double calculate(double value) {
        return circum(value);
    }

    private double circum(double radius) {
        return 2 * Math.PI * radius;
    }

    public void showDetails(double smallCircum, double largeCircum) {
        System.out.println("Circumference of smaller circle: " + df.format(smallCircum));
        System.out.println("\n");
        System.out.println("Circumference of larger circle: " + df.format(largeCircum));
        System.out.println("\n");
        System.out.println("Difference: " + df.format((largeCircum - smallCircum)));
    }
}

class InputHandler {
    private static final String DOUBLE_DEFAULT_PROMPT = "Enter integer: ";

    public double getDouble() {
        return getDouble(DOUBLE_DEFAULT_PROMPT);
    }

    public double getDouble(String prompt) {
        String inStr = JOptionPane.showInputDialog(null, prompt);
        return Double.parseDouble(inStr);
    }
}


Inserting records into a database I wrote the following code to insert records into a database.  The user selects the rows from the RadGrid and the insert command is executed when that user clicks the button.  I've been spending a lot of time trying to make this work (nothing currently happens when I click the button).  However, I just realized my approach is probably very inefficient due to the fact that it will result in the application repeatedly opening and closing database connections.  Even though I'm dealing with only a few hundred records I'd like to follow best practices as much as possible.

How would you rewrite this code to be more efficient? My gut tells me it would be better to gather all of the new records and then deal with them as a collection.

    protected void RadButton2_Click(object sender, EventArgs e)
{
    foreach (GridDataItem item in RadGrid1.SelectedItems)
    {
        //GridDataItem item = (GridDataItem)RadGrid1.SelectedItems;
        int UserID = Convert.ToInt16(item["UserID"].Text);
        string Type = "D";
        DateTime Date = DateTime.Now;
        string Description = "Monthly Storage Fee - Tag: " + item["PackageTag"].Text + Label3.Text;
        Int32 AmountDue = Convert.ToInt32(item["AmtDue"].Text);

        string connectionString = ConfigurationManager.ConnectionStrings["Foo"].ConnectionString;
        SqlConnection connection = new SqlConnection(connectionString);

        try
        {

            SqlCommand cmd = new SqlCommand("INSERT INTO Billing (UserID, Type, Date, Description, Amount) VALUES (@UserID, @Type, @Date, @Description, @AmountDue)", connection);
            cmd.Parameters.AddWithValue("@UserID", UserID);
            cmd.Parameters.AddWithValue("@Type", Type);
            cmd.Parameters.AddWithValue("@Date", Date);
            cmd.Parameters.AddWithValue("@Description", Description);
            cmd.Parameters.AddWithValue("@AmountDue", AmountDue);

            connection.Open();
            cmd.ExecuteNonQuery();
        }

        catch
        {
            Label4.Text = "uh oh";
        }

        finally
        {
            connection.Close();
        }

This is not best practice but simple logic.

Why you have add inside the loop this (non change) parameters ?

    string Type = "D";
    DateTime Date = DateTime.Now;
 string connectionString =
       ConfigurationManager.ConnectionStrings["Foo"].ConnectionString;

Get them out of the loop.

Second place the connection inside using, and totally outside the loop, why you need to open it and close it all the time.

protected void RadButton2_Click(object sender, EventArgs e)
{
    //GridDataItem item = (GridDataItem)RadGrid1.SelectedItems;
    string Type = "D";
    DateTime Date = DateTime.Now;

    string connectionString = ConfigurationManager.ConnectionStrings["Foo"].ConnectionString;
    SqlConnection connection = new SqlConnection(connectionString);

    using(SqlConnection connection = new SqlConnection(connectionString))
    {
        connection.Open();
        foreach (GridDataItem item in RadGrid1.SelectedItems)
        {
            try
            {
                string Description = "Monthly Storage Fee - Tag: " + item["PackageTag"].Text + Label3.Text;
                Int32 AmountDue = Convert.ToInt32(item["AmtDue"].Text);
                int UserID = Convert.ToInt16(item["UserID"].Text);

                using (SqlCommand cmd = new SqlCommand("INSERT INTO Billing (UserID, Type, Date, Description, Amount) VALUES (@UserID, @Type, @Date, @Description, @AmountDue)", connection))
                {
                    cmd.Parameters.AddWithValue("@UserID", UserID);
                    cmd.Parameters.AddWithValue("@Type", Type);
                    cmd.Parameters.AddWithValue("@Date", Date);
                    cmd.Parameters.AddWithValue("@Description", Description);
                    cmd.Parameters.AddWithValue("@AmountDue", AmountDue);

                    cmd.ExecuteNonQuery();
                }
            }
            catch
            {
                Label4.Text = "uh oh";
            }
        }
    }
}

Need some extra test for check if the connection is open or fail


Memory usage in Reversi board state I'm coding a Reversi game, with an artificial intelligence using the MinMax as the search algorithm. My concern is that (most) search algorithms needs to store a lot of instances of "states", in my case, board states. What I want is to represent a BoardState with 64 SlotState (Empty, White Disk or Black Disk), with the minimum RAM usage as possible. My current implementation uses a System.Collections.BitArray for storage.

public enum SlotState : byte {
    Empty = 0,
    White = 1,
    Black = 2
}
public sealed class BoardState {

    private const int BitsPerSlot = 2;
    private const int SlotsPerRow = 8;
    private const int SlotsInBoard = 64;
    private const int BitsPerRow = SlotsPerRow * BitsPerSlot;
    private const int BitsInBoard = SlotsInBoard * BitsPerSlot;

    // stores state in big-endian row-major
    private BitArray State;

    public BoardState() {
        State = new BitArray(BitsInBoard);
        this[3, 3] = SlotState.White;
        this[4, 4] = SlotState.White;
        this[3, 4] = SlotState.Black;
        this[4, 3] = SlotState.Black;
    }

    public BoardState(BoardState original) {
        State = new BitArray(original.State);
    }

    public SlotState this[int x, int y] {
        get {
            if (!(IsValidCoordinate(x) && IsValidCoordinate(y))) {
                throw new IndexOutOfRangeException();
            }
            int position = GetPosition(x, y);
            bool majorBit = State[position];
            bool minorBit = State[position + 1];
            if (majorBit && !minorBit) {
                return SlotState.Black;
            } else if (!majorBit && minorBit) {
                return SlotState.White;
            } else {
                return SlotState.Empty;
            }
        }
        set {
            if (!(IsValidCoordinate(x) && IsValidCoordinate(y))) {
                throw new IndexOutOfRangeException();
            }
            int position = GetPosition(x, y);
            State[position] = false;
            State[position + 1] = false;
            if (value == SlotState.White) {
                State[position + 1] = true;
            } else if (value == SlotState.Black) {
                State[position] = true;
            }
        }
    }

    private int GetPosition(int x, int y) {
        return BitsPerSlot * x + BitsPerRow * y;
    }

    private bool IsValidCoordinate(int coordinate) {
        return coordinate >= 0 && coordinate <= 7;
    }

}

More specific questions could be: Is there any way to reduce memory usage even further (does BitArray have a memory overhead)? Is it excessive const declarations? Should I break the get/set into smaller pieces of code? Are the naming and general coding style all right?

I also tried this implementation of the get method. I didn't like it, but couldn't find a reason to use either over the other. Is it any better/worse than the other implementation, in terms of ease of understanding?

get {
    if (!(IsValidCoordinate(x) && IsValidCoordinate(y))) {
        throw new IndexOutOfRangeException();
    }
    int position = GetPosition(x, y);
    bool majorBit = State[position];
    bool minorBit = State[position + 1];
    byte value = 0;
    if (majorBit) value += 2;
    if (minorBit) value += 1;
    return (SlotState)value;
}

If you're going to suggest a library or anything like that, keep in mind this is a XNA project (thus the memory concern).

I agree with other commenters that you should have your memory usage measured first, so you know for sure you need to solve this. That said, I don't think MinMax requires storing all board states of the tree. Why do you need to have more boards in the memory than the level of the tree search? That should be 10-15 boards in the memory tops, no? I would even think about storing just one board in the memory and passing just the "moves" down the recursion. You will perform the move when you go down the recursion and undo the move when going up.


Store variables function-wide and access them in an anonymous function At the moment, I have this code:

(function() {
  var lastSelectedIndex = -1;
  function onSelTypeSelected() {
    // #selType is a <select> tag
    // var selType = document.getElementById("selType");
    var selType = this;
    // finished(onSuccess, onError)
   loader.finished(function(data) {
      lastSelectedIndex = selType.selectedIndex;
    }, function(err) {
      selType.selectedIndex = lastSelectedIndex;
    });
  }
})();

So lastSelectedIndex is not in the global context, however I want to put it into eventListener()'s context. inside eventListener(), so you can't access it in the outer anonymous function.
That's cleaner in my opinion and also faster (okay, a few nanoseconds) because of the scope chains hierarchy (Google Tech Talk: Speed Up Your JavaScript is a good introduction).

I thought of adding a property to the function (as I learnt here) but I don't have access to eventListener() in the anonymous functions.

The next idea I had was to use HTML 5 data-* attributes:

function onSelTypeSelected() {
  // #selType is a <select> tag
  // var selType = document.getElementById("selType");
  var selType = this;
  // finished accepts two arguments: onSuccess and onError
  loader.finished(function(data) {
    selType.setAttribute("data-lastIndex", selType.selectedIndex);
  }, function(err) {
    selType.selectedIndex = selType.getAttribute("data-lastIndex");
  });
}

(The default value for data-lastIndex will be set in the HTML Markup)

lastSelectedIndex is already visible from eventListener. It is in fact visible from everywhere inside the immediately invoked anonymous function.

Try putting console.log("lastSelectedIndex is", lastSelectedIndex); in there and you'll see.

This is due to the concept of closures which in technical-ish terms means that when a variable is not found inside a function's context, it looks in the parent context (the context the function was in when created) and so on all the way up to the window object. In plain-english terms this means that you should forget all your Java/C#/Php training and the variables visible inside a function are exactly those that you intuitively think should be.

That being said, there is going to be only one instance of lastSelectIndex for this block of code, so if the eventlistener is ever attached to more than a single element all instances of the listener will be referencing the same lastSelectIndex.

There are plenty of ways to skin this cat and you really did not give enough details on what your trying to achieve to give advice on best practices but let me try to give some advice anyways

DO the simplest thing that can possibly work first
DON'T name your functions after how they're used (eventlistner)
DO name your functions after what they do
DON'T use data- attributes directly. Use a library like jquery, at the very least use element.dataset to retrieve them, though keep in mind the data api is not available in all browsers (which libraries handle for you)
CONSIDER having everything set up in response to a user action that triggers it so in pseudocode, the following would be all inline (and with js can be written gracefully and very compact).

when the user changes the option
 memoize the previous value
 query the server
   on success continue
   on error restore the previous value
Finally, if you do this more than a few times, CONSIDER coming up with an abstraction (possibly a custom control) for this pattern so you don't have to do it every time


Confirm code is correct - crossover methods in Java I did this code for somebody but need it to be double checked before I pass it onto them. Code seems fine but I need someone to confirm I have coded the crossover methods correctly.

Would be great if somebody that is familiar with genetic algorithms and crossover methods, could confirm that I have the correct logic and code behind each crossover method.

 //one crossover point is selected, string from beginning of chromosome to the
 //crossover point is copied from one parent, the rest is copied from the second parent

 // One-point crossover

public void onePointCrossover(Individual indi) {
    if (SGA.rand.nextDouble() < pc) {
        // choose the crossover point
        int xoverpoint = SGA.rand.nextInt(length);

        int tmp;
        for (int i=xoverpoint; i<length; i++){
            tmp = chromosome[i];
            chromosome[i] = indi.chromosome[i];
            indi.chromosome[i] = tmp;
        }
    }
}
//two crossover point are selected, binary string from beginning of chromosome to
//the first crossover point is copied from one parent, the part from
// the first to the second crossover point is copied from the second parent
// and the rest is copied from the first parent

// Two-point crossover
    public void twoPointCrossover(Individual indi) {
        if (SGA.rand.nextDouble() < pc) {
            // choose the crossover point
            int xoverpoint = SGA.rand.nextInt(length);
            int xoverpoint2 = SGA.rand.nextInt(length);

            int tmp;
            //swap
            if (xoverpoint > xoverpoint2){
                tmp = xoverpoint;
                xoverpoint = xoverpoint2;
                xoverpoint2 = tmp;
            }

            for (int i=xoverpoint; i<xoverpoint2; i++){
                tmp = chromosome[i];
                chromosome[i] = indi.chromosome[i];
                indi.chromosome[i] = tmp;
            }
        }
    }

    //  For each gene, create a random number in [0,1]. If
    // the number is less than 0.5, swap the gene values in
    // the parents for this gene; otherwise, no swapping

// Uniform Crossover
    public void UniformCrossover(Individual indi) {
        if (SGA.rand.nextDouble() < pc) {
        for (int i= 1; i<length; i++){
            boolean tmp =  SGA.rand.nextFloat() < 0.5;
            if(tmp){
                chromosome[i] = indi.chromosome[i];

            }
            }

        }

Parent 1 = chromosome    Parent 2= indi.chromosome

I am turning the parents into children inplace.

It's difficult to just look at, but a few things jump out. More improvements than bugs:

For single point crossover using System.arraycopy() would be much more reliable. For example:

System.arraycopy(parent1Genes, 0, childGenes, 0, xoverPoint);
System.arraycopy(parent2Genes, xoverPoint, childGenes, xoverPoint, geneSize-xoverPoint);

You could use the same method for two point, but it would be slightly more complex.
In Uniform crossover you can use rand.nextBoolean(), which returns true or false, instead of  comparing the float.


Intersection of two lists in Python I am currently working on an algorithm, which involves finding the intersection of two lists. That is, given the following inputs [1,2,2,1],[2,2] the result would be [2,2].

I have implemented it in two ways which hopefully should be easy to understand

Method 1

def intersect(nums1, nums2):
    dict_count = {}
    for num1 in nums1:
        if num1 not in dict_count:
            dict_count[num1] = 1
        else:
            dict_count[num1] += 1

    inter_arr = []

    for num2 in nums2:
        if num2 in dict_count and dict_count[num2] > 0:
            inter_arr.append(num2)
            dict_count[num2] -= 1

    return inter_arr

Method 2

def intersect_v2(nums1, nums2):
    arr_one = sorted(nums1)
    arr_two = sorted(nums2)

    p1, p2 = 0, 0
    inter_arr = []

    while p1 < len(arr_one) and p2 < len(arr_two):
        if arr_one[p1] < arr_two[p2]:
            p1 += 1
        elif arr_one[p1] > arr_two[p2]:
            p2 += 1
        else:
            inter_arr.append(arr_one[p1])
            p1 += 1
            p2 += 1

    return inter_arr

I have two questions about time complexity:

What if nums1's size is small compared to nums2's size? Which algorithm is better? (The original algorithm problem is from Leetcode and this is a follow up question to the problem)

It feels like the pointer method always win, but I can't articulate why.

If the inputs always came in sorted, which algorithm would be more efficient with respect to time and space? (This would mean that first two lines to intersect_v2 would not be needed)

From my understanding intersect would be O(M+N) since you would have to iterate through the two arrays not matter what. The second one is a bit confusing since it feels like the it would be O(Z) where Z is less than Max(M,N) and greater than Min(M,N), is that about right?

Blockquote

What if nums1's size is small compared to nums2's size? Which
algorithm is better? (The original algorithm problem is from Leetcode
and this is a follow up question to the problem)

It feels like the pointer method always win, but I can't articulate why.

Granted, in many cases, the pointer method might be faster because it might break out of the while after exhausting the elements of the smaller list without having to consider the vast majority of the elements of the larger list.
However, think of a pathological worst case where max(nums1) > max(nums2) such as nums1 = [1_000_000] and nums2 = list(range(100_000)). The pointer approach as shown would have to examine all 100_000 elements of nums2.
What are some ways you can mitigate this?
Taking advantage of the fact that nums1 and nums2 are sorted, you can get the min and max of each list in O(1) and use those values as additional conditions to break out of the while loop earlier.
Given that the input is sorted, binary search might speed things up considerably. bisect.bisect_left is python's own implementation of binary search. Given that sorted input, binary search would allow you to potentially skip over larges swaths of the input as follows:
while p1 < len(nums1) and p2 < len(nums2):
    if nums1[p1] < nums2[p2]:
        # advance p1 to an index s.t. nums1[p1] >= nums2[p2]
        p1 = bisect.bisect_left(nums1, nums2[p2])
    elif nums1[p1] > nums2[p2]:
        # advance p2 to an index s.t. nums2[p2] >= nums1[p1]
        p2 = bisect.bisect_left(nums2, nums1[p1])
    else:
        inter_arr.append(arr_one[p1])
        p1 += 1
        p2 += 1

P.S.:
bisect_left has two optional keyword arguments, lo and hi which might be useful here as well.