Go database helper code Today I've implemented a database helper in Go. I'm new in Go world so I just wanted someone with more experience to take a look and tell me what is good and especially, what is bad (and why, if possible).

package clinic

import (
    "fmt"
    "github.com/astaxie/beego/orm"
    _ "github.com/go-sql-driver/mysql"
)

func init() {
    orm.RegisterDriver("mysql", orm.DR_MySQL)
    orm.RegisterDataBase("default", "mysql", "user@pass/db-name?charset=utf8")
}

func NewPet(pet *Pet){
    id, err := orm.NewOrm().Insert(pet)
    if err == nil {
        fmt.Println(id)
    }
}

func UpdatePet(pet *Pet){
    if num, err := orm.NewOrm().Update(pet); err == nil {
     fmt.Println(num)
    }
}

func DeletePet(pet *Pet){
    if num, err := orm.NewOrm().Delete(pet); err == nil {
     fmt.Println(num)
    }
}

func AllPets() []interface{} {
    var pets []Pet
    all, err := orm.NewOrm().QueryTable("pet").All(&pets)
    fmt.Printf("Returned Rows Num: %s, %s", all, err)
    result := make([]interface{}, len(pets))
    for i, s := range pets {
        r := s;
        result[i] = &r
    }
    return result;
}

This code works and I am able to use it in the main package, for example, using clinic.NewPet(&Pet{field: "value"}). I am just wondering if this can be improved.

What do you think about error handling? I think that's good idea to provide some log messages for main operation with db (CRUD).

As example for Delete function:

func DeletePet(pet *Pet) {
    num, err := orm.NewOrm().Delete(pet)

    if err != nil {
        log.Println("DELETE", err)
    } else {
        fmt.Println(num)
    }
}


Multicolor bar above form legend I've implemented a multicolored bar as shown in the image below. It is a bar that is placed above the legend of an HTML form. (Ignore the border radius for now.)

I have accomplished this with a simple approach of using span elements.

*{
    margin: 0px;
    padding: 0px;
}

span{
    height: 5px;
    width: calc(100% / 7);
    background: black;
    display: inline-block;
}

span[data-color='green']{
    background: #C3E279;
}

span[data-color='pale-yellow']{
    background: #F7FEC8;
}

span[data-color='orange']{
    background: #FFD069;
}

span[data-color='red']{
    background: #F27669;
}

span[data-color='light-purple']{
    background: #DC9CBE;
}

span[data-color='purple']{
    background: #C59AE0;
}

span[data-color='blue']{
    background: #969DCC;
}

Is there a better way to do this to make the code smarter, presentable and easy to understand to other developers?

Unless your elements serve a legitimate purpose (ie. they have content), then you should avoid using empty elements whenever possible.  Instead, I recommend using a linear-gradient as a background on a content element.

Sass makes this easy to generate:

$colors: #C3E279, #F7FEC8, #FFD069, #F27669, #DC9CBE, #C59AE0, #969DCC;
$stops: ();
$current-stop: 0%;
@each $c in $colors {
    $stops: append($stops, $c $current-stop, comma);
    $current-stop: $current-stop + (100% / length($colors));
    $stops: append($stops, $c $current-stop);
}

body:before { /* or some other element*/
  content: '';
  height: 5px;
  display: block;
  background: linear-gradient(to right, $stops);
}

Output:

body:before {
  content: '';
  height: 5px;
  display: block;
  background: linear-gradient(to right, #C3E279 0%, #C3E279 14.28571%, #F7FEC8 14.28571%, #F7FEC8 28.57143%, #FFD069 28.57143%, #FFD069 42.85714%, #F27669 42.85714%, #F27669 57.14286%, #DC9CBE 57.14286%, #DC9CBE 71.42857%, #C59AE0 71.42857%, #C59AE0 85.71429%, #969DCC 85.71429%, #969DCC 100.0%);
}

http://sassmeister.com/gist/9665cd09dfab6b3e05e0


Applying function to entire dataframe I'm wondering if there is a faster way to do the following code, which works fine. It needs to be applied to every row of the DataFrame df. class_dct contains a different NLTK NaiveBayes classifier for each of 9 regions.

for region in regions:
    new_app=[]
    for title in df.booktitle:
        nlp_genre=class_dct['Classif_%s'% region].classify(features(title))
        new_app.append((title,region,nlp_genre))

I guess the Python interpreter will optimise most or all of this on its own, but you could try a list comprehension and removing an intermediate variable:

new_app = [(title, region, class_dct['Classif_%s'% region].classify(features(title))) for title in df.booktitle]


Array to map based on object property containing array items If i have a object like following
result  = [
 {
   phones : ["ABC", "DEF"],
   name: "Simon"
 },
 {
   phones : ["ABC", "XZY"],
   name: "John"
 }
]

Expected output
Map of key, value
{ABC,     ["Simon", "John"]}
{DEF,     ["Simon"]}
{XYZ,     ["John"]}

My try
map: Map = new Map();
for ( r of result ) {
   for( phone of r.phones) {
      if(map.get(phone)){
        map.set(phone, map.get(phone).concat(r.name))
      } else {
        map.set(phone, r.name);
      }
   }
}

Is there a ES6 way to perform the above double iteration in a minimised way ?

Overall Your code is quite reasonable, after fixing something that looks to be a typo. The transformation that needs to be applied to the data structure isn't entirely trivial, so the logic that needs to be implemented requires a handful of lines of code.
Typo? But your code also has a typo/bug: the map.set(phone, r.name); will set the value in the map to be a string, not an array. You probably meant to do map.set(phone, [r.name]); (maybe a miscopy?) There's also map: Map = new Map();, which isn't valid syntax in JS, remove the : Map part.
Conditional operator If you wanted to shorten your existing code, see how you're using .map.set(phone, someExpression) depending on a condition - this is a sign that you can use the conditional operator instead of the if/else.
map.set(phone, (map.get(phone) ?? []).concat(r.name));

But while that makes the code shorter, is it easier to read? I'm not entirely convinced. Maybe you'll like it, or maybe you won't.
There are a few other things that can improve the code quality:
Declare your variables Doing for (r of result) will implicitly create a global r variable and put it on the global object. If you're running in strict mode, this will throw an error too. Always declare variables before using them, preferably with const.
Destructure? With for (r of result) or for (const r of result), you create a variable named r, which isn't so intuitive - what's an r? Maybe destructure the name and phones properties immediately.

const result  = [
 {
   phones : ["ABC", "DEF"],
   name: "Simon"
 },
 {
   phones : ["ABC", "XZY"],
   name: "John"
 }
]
const map = new Map();
for (const { phones, name } of result) {
  for (const phone of phones) {
    map.set(phone, (map.get(phone) ?? []).concat(name));
  }
}
console.log([...map]);


LinQ wrapper to get distinct items from a IEnumerable based on a function of item I often need to perform LinQ queries to get distinct items from a IEnumerable based on one of its properties. I would like to know if this is a good way to wrap this operation:

<Extension()> Function DistinctOn(Of T)(iet As IEnumerable(Of T),
                                        identity_property As Func(Of T, Object)
                                       ) As IEnumerable(Of T)
    Return iet.GroupBy(identity_property).Select(Function(g) g.First)
End Function

Using example (pastas is a IEnumerable of Newtonsoft.Json.Linq.JObject):

Dim dpastas = pastas.DistinctOn(Function(jo) jo.Value(Of Integer)("id")).ToArray

It works Ok, but I'd like to know if it is good code also. Thank you very much!

That is a good definition for an IQueryable but I believe it is better to implement the operation directly for an IEnumerable (note: I don't use VB, so translated from C#):

<Extension()> Public Iterator Function DistinctBy(Of T, TKey)(src As IEnumerable(Of T), keyFun As Func(Of T, TKey)) As IEnumerable(Of T)
    Dim seenKeys As New HashSet(Of TKey)()
    For Each e As T In src
        If seenKeys.Add(keyFun(e)) Then
            Yield e
        End If
    Next
End Function


A simple Q and A quiz that reinforces Ruby learning I wrote a simple quiz which I am hoping to scale-up as I continue to learn about programming.

The code now stores questions and answers in separate arrays.

Please review this code to improve its efficacy.

questions = []
answers = []
q0 = "What is the command for a 'for' loop?"
a0 = "for"
q1 = "What is another name for a method?"
a1 = "function"
q2 = "what are the three types of loops that I know of?"
a2 = "while, for, and until"

questions.push(q0, q1, q2)
answers.push(a0, a1, a2)

question_counter = 0
index = 0

while true
  puts questions[index]
  question_counter += 1
  answer_attempt = gets.chomp.downcase
  if answer_attempt == answers[index]
    puts "Good job. Next question.."
  else
    puts "Incorrect. Next question"
  end
  index = index + 1
  if question_counter == 3
    puts "all done"
    break
  end
end

Store related data in a Hash

Also, looping like you've done is very uncommon in Ruby. You'll see Array#each (or Hash#each in this case) much more common than storing an index in a var while iterating through indexes.

@success_msg = 'Good job. Next question..'
@incorrect_msg = 'Incorrect. Next question..'
{
  "What is the command for a 'for' loop?" => 'for',
  'What is another name for a method?' => 'function',
  'What are the three types of loops that I know of?' => 'while, for, and until'
}.each do |q, a|
  puts q
  puts(gets.chomp.downcase == a ? @success_msg : @incorrect_msg)
end
puts 'All done.'

Also, I'd try to make this multiple choice or something if you're really trying to use it, theres no way your response is going to match those answers exactly. Perhaps you could look for certain terms in the response, but parsing English for semantic meaning isn't very easy hehe.


Breadth-first search on a tree in Python The following code performs breadth first search on a mockup tree

# some tree
#             A            - LAYER 0
#         B       C        - LAYER 1
#      D     E             - LAYER 2
#   F                      - LAYER 3

def get_something(x):
    if x == 'a':
        return ['b', 'c']
    if x == 'b':
        return ['d', 'e']
    if x == 'c':
        return []
    if x == 'd':
        return ['f']
    if x == 'e':
        return []
    if x == 'f':
        return []

and it is traversed by the following not so beautiful code

# I mean it works, but looking at it makes me feel sick

def recursive_get(x, loop_limit, count_limit):

    # stop condition 1: loop limit exceeded and nonzero
    # stop condition 2: count limit exceeded and nonzero
    # stop condition 3: we have reached the bottom

    current = 0
    seen_list = []
    todo_list = [x]

    while ((current < loop_limit or loop_limit == 0) and
           (len(seen_list) < count_limit or count_limit == 0) and
           todo_list):

        result = get_something(todo_list.pop(0))
        seen_list.extend(result)
        todo_list.extend(result)
        current += 1

    return seen_list

Is there any syntactic sugar to get the second part to look more sexy? (less lines, syntactically nicer looking)?

get_something is not a good name for the function. Obviously, it specifies your tree by returning the child nodes of an argument node; rename to, say, get_neighbors. Also, your implementation overkills breadth-first search a bit. See what I mean:

def get_neighbors(x):
    if x == 'a':
        return ['b', 'c']
    if x == 'b':
        return ['d', 'e']
    if x == 'c':
        return []
    if x == 'd':
        return ['f']
    if x == 'e':
        return []
    if x == 'f':
        return []

    return [] # Other nodes have no neighbors.

def bfs(start):
    queue = [start]
    visited = set(start)

    while queue:
        node = queue.pop(0)

        for neighbor in get_neighbors(node):
            if neighbor not in visited:
                queue.append(neighbor)
                visited.add(neighbor)

    return visited

print(str(bfs('a')))

Also, note that seen_list should be rather a Python set as operating on it is more efficient than on lists.


Find if a number is equal to the sum of its divisors A number is perfect if it is equal to the sum of all its divisors. For example divisors of 6 are 1, 2, 3. I want to read numbers, and for each number to print 1 if it is perfect and 0 if it's not perfect. Numbers are smaller than 10 ^ 19. How would I do it faster? My version:

#include <stdio.h>
#include <stdlib.h>
double perfectCheck( double number )
{
    double i, sum = 0;
    for( i = 1; i <= number / 2; i++ )
        if(( int )( number / i ) * i == number )
            sum += i;
    return sum;
}

int main()
{
    double n, i, m;
    scanf( "%lf", &n );
    for( i = 0; i < n; i++ )
    {
        scanf( "%lf", &m );
        if( perfectCheck( m ) == m )
            printf( "1 " );
        else
            printf( "0 " );
    }
    return 0;
}

Input:

5
28 7 8 9 6

Output:

1 0 0 0 1

I use C89.

Finding divisors faster

Instead of checking all numbers until number / 2,
it's enough to search until sqrt(number),
and count the divisor pairs.
For example, in the case of 6,
initialize sum = 1, because 1 will be always part of the sum,
and then when you see 2, also add 6 / 2 to the sum.
(You can get sqrt with #include "math.h".)
Also,
you can break out of the loop when sum exceeds the target value.

There is one more important point that can slow your implementation down,
is that all the variables are double instead of int.
This is not only unnatural for the given task,
floating point math is expensive in general.
So simply rewriting the program so that number, i, m, n are all integers, will make a big difference.

Make perfectCheck return boolean

Currently perfectCheck returns the sum of divisors.
You could make it return a bool:

return sum == number;

You can get the bool type in C99+ with #include "stdbool.h".

And then you can write the main function a bit simpler:

for (i = 0; i < n; i++)
{
    scanf("%d", &m);
    printf("%d ", perfectCheck(m));
}


Converting  tags to horizontal responsive barcharts I am writing my first jQuery plugin that converts li tags to horizontal responsive barcharts. I am looking for some feedback and suggestions regarding the implementation.

(function($) {
    $.fn.barcharts = function(options) {

        // Establish default settings
        var defaults = {
            animationSpeed  : 500
        };

        // Merge
        var options = $.extend({}, defaults, options);

        // Gather
        var $targets = $(this).find('.bar-group-item');

        // Render
        $.each($targets, function() {

            var $li = $(this),
                newWidth = $li.data('bar-value')/100*$li.width();

            // Construct the item to be displayed
            var $div = $('
', {
                width: 0,
                text: $li.text()
            });

            // Remove previous contents
            $li.empty();

            // Append to current item
            $div.appendTo($li);

            // Reveal
            $div.animate({width: newWidth}, options.animationSpeed);

        });
    };

}(jQuery));
.bar-group {
    list-style: none;
    padding-left:0;
}

.bar-group-item {
    width: 100%;
    background-color: #34495e;
    color: #ffffff;
    margin: 5px 0 5px 0;
}

.bar-group-item div {
    background-color: #2980b9;
    white-space: nowrap;
}

England

Spain

Zimbabwe

Lakers

Heat

Raptors

Los Angeles

Miami

Toronto

I'd recommend you to get rid of class selector here

// Gather
var $targets = $(this).find('.bar-group-item');

because you need you're HTML structure to use specific classes. You'd better find them by finding elements with data attributes inside plugin's target element.

Scalability!


Replace each whitespace in a string with "%20" My implementation:

string ReplaceAllSpaces(string input)
{
    StringBuilder builder = new StringBuilder();
    using(StringReader reader = new StringReader(input))
    {
        while(reader.Peek() != -1)
        {
            char c = (char)reader.Peek();
            if(char.IsWhiteSpace(c))
            {
                while(char.IsWhiteSpace(c))
                {
                    reader.Read();
                    c = (char)reader.Peek();
                }
                builder.Append("%20");
            }
            else builder.Append((char)reader.Read());
        }
    }
    return builder.ToString();
}

Input:

"My      Name Is  John"

Output:

"My%20Name%20Is%20John"

How could this be improved?  Only ASCII characters are permitted.

This one really fits well for a regular expression:

public static string ReplaceAllSpaces(string str) {
  return Regex.Replace(str, @"\s+","%20");
}

The expression \s+ consists of the pattern \s that matches a whitespace character (which is more that just spaces, as noted by @tinstaafl), and the quantifier + which means "at least once".

The pattern matches "one or more whitespace characters", so it will match each group of whitespace characters (for example the six spaces between My and Name in the example) and replace it with %20.


Converting Integers to roman between 1 and 1000 Below is my code for the problem:
ItoR = {
1: 'I',
4: 'IV',
5: 'V',
9: 'IX',
10: 'X',
40: 'XL',
50: 'L',
90: 'XC',
100: 'C',
400: 'CD',
500: 'D',
900: 'CM',
1000: 'M'
}

def convertItoR(number):
    number_str = str(number)
    number_len = len(number_str)

    rom_str = ''
    for i in range(number_len):
        # Extracting the digit
        digit = int(number_str[i])
        position = number_len - i - 1
        num_pos = digit * 10 ** position

        print(num_pos)

        # Converting digit to roman string
        if num_pos in ItoR.keys():
            rom_str += ItoR[num_pos]

        elif digit > 1 and digit < 4:
            first = 1 * 10 ** position
            rom_str += ItoR[first] * digit

        elif digit > 5 and digit < 9:
            first = 1 * 10 ** position
            fifth = 5 * 10 ** position
            rom_str += ItoR[fifth] + ItoR[first] * (digit - 5)

        print(rom_str)

    return rom_str

Can it be optimized?

You asked about optimization, which usually refers either to speed or memory
usage, neither of which apply in this case. Your code has negligible memory
cost. And if we truly cared about the ability to convert large volumes of
numbers at high speed, we would pre-compute a dict mapping every decimal number
of interest to its Roman equivalent.
If I were you, I would focus on optimizing this problem for simplicity of
understanding and code readability. Your current code appears to be fine, and
one can definitely figure it out, but it still requires some puzzling over. The
classic way that I've seen people solve this problem more simply is to build up
the Roman numeral by subtracting away even multiples of each divisor in your
dict mapping divisors to Roman symbols. The trick is to proceed from largest
divisor to smallest. Since Python 3 preserves order in dicts, we can
achieve that simply by reversing the order in which the data is set up.
The basic idea is illustrated below.
def decimal_to_roman(num):
    # The Romans had "integers" too, so this seems like a more
    # accurate function name.

    # You should add a check here to validate that the given decimal
    # number falls within the supported ranged.

    # Relies on Python 3 having ordered dicts.
    divisors = {
        1000: 'M',
        900: 'CM',
        500: 'D',
        400: 'CD',
        100: 'C',
        90: 'XC',
        50: 'L',
        40: 'XL',
        10: 'X',
        9: 'IX',
        5: 'V',
        4: 'IV',
        1: 'I',
    }

    # Build up the Roman numeral by subtracting multiples of
    # each divisor, proceeding from largest to smallest.
    roman = ''
    for div, sym in divisors.items():
        quotient = divmod(num, div)[0]
        roman += sym * quotient
        num -= div * quotient
        if num < 1:
            break
    return roman

When deciding on names for functions, variables, and so forth, aim for accuracy
and clarity in situations where the additional context is needed for
readability. This is a case where slightly longer names are appropriate (for
example, decimal_to_roman, divisors, and quotient). But when the
surrounding context and nearby names provide sufficient clues, you can aim for
brevity to reduce the visual weight of the code -- which also helps with
readability, but from a different direction. For example, within the narrow
context of this function, the short names like num, div, and sym are
easily understood without giving them fully explicit names.
Finally, how do you know your code is correct? If you're trying to learn, you
could consider writing the corresponding roman_to_decimal() function and then
make sure your converters can round-trip the data correctly. Another approach
is to find one of the many Python functions floating around the internet and use
it as your checker. Or you can install the
roman Python package to check
everything, as shown here:
import roman

for n in range(1, 4000):
    exp = roman.toRoman(n)
    got = decimal_to_roman(n)
    if exp != got:
        print(n, exp, got)


Validating credit card numbers - Haskell I'm learning Haskell and doing the exercises from http://www.seas.upenn.edu/~cis194/spring13/lectures.html.

This is my code to validate credit cards as described on http://www.seas.upenn.edu/~cis194/spring13/hw/01-intro.pdf.

I know very little about it and welcome any advice on how to improve.

Here's my solution:

toDigits :: Integer -> [Integer]
toDigits x
  | x <= 0 = []
  | otherwise = toDigits (div x 10) ++ [mod x 10]

toDigitsRev :: Integer -> [Integer]
toDigitsRev x
  | x <= 0 = []
  | otherwise = (mod x 10) : toDigitsRev (div x 10)

doEveryTwo' :: (t -> t) -> [t] -> [t]
doEveryTwo' _ [] = []
doEveryTwo' f (x:xs) = x : doEveryTwo f xs

doEveryTwo :: (t -> t) -> [t] -> [t]
doEveryTwo _ [] = []
doEveryTwo f (x:xs) = f x : doEveryTwo' f xs

doubleEveryOther :: [Integer] -> [Integer]
doubleEveryOther [] = []
doubleEveryOther xs
  | length xs `mod` 2 == 0 = doEveryTwo (*2) xs
  | otherwise = head xs : doEveryTwo (*2) (tail xs)

sumDigits :: [Integer] -> Integer
sumDigits [] = 0
sumDigits (x:xs) = sum (toDigits x) + sumDigits xs

validate :: Integer -> Bool
validate x
  | x < 10 = False
  | otherwise = sumDigits (doubleEveryOther (toDigits x)) `mod` 10 == 0

TL;DR:

toDigits can be expressed with toDigitsRev and vice-versa.
If you use both div and mod, use divMod to get both at the same time.
You can pattern match on two elements in a list with (x:y:xs). That way you only need a single doEveryTwo.
sumDigits is sum . map (sum . toDigits).
doubleEveryOther can be written with mapAccumR, but that might be a little bit too advanced.

All in all, your code will work, but we can improve it.

Write functions in terms of other functions

You have written both toDigits as well as toDigitsRev. However, you only need one of them. The other one is the reversed variant:

toDigits :: Integer -> [Integer]
toDigits = reverse . toDigitsRev

Use divMod if you need both results

You can think of divMod as

divMod x y = (div x y, mod x y)

but it's usually faster. Even better, if all your numbers are guaranteed to be positive, use quotRem, since div and mod have some constraints on their result:

toDigitsRev :: Integer -> [Integer]
toDigitsRev x
  | x <= 0 = []
  | otherwise = r : toDigitsRev q
 where
  (q,r) = x `quotRem` 10 -- x guaranteed to be positive

Use pattern matching to your advantage

You can match on multiple elements in a list at once. That way, you can write doEveryTwo without a helper that skips:

doEveryTwo :: (t -> t) -> [t] -> [t]
doEveryTwo f (x:y:xs) = f x : y : doEveryTwo f xs
doEveryTwo _ [x]      = f x
doEveryTwo _ []       = []

Note that a worker-wrapper approach is usually used here for performance:

doEveryTwo :: (t -> t) -> [t] -> [t]
doEveryTwo f = go
  where
    go (x:y:xs) = f x : y : go xs
    go [x]      = f x
    go []       = []

That way we don't have to carry f along. (Exercise: write doEveryTwo with zipWith. Have a look at cycle if you get stuck).

Use higher order function to your advantage

When you do the same for all elements in a list, it's usually time to use map. If we take a look at sumDigits, we can see that it uses toDigits for all numbers and then sums it:

sumDigits :: [Integer] -> Integer
sumDigits [] = 0
sumDigits (x:xs) = sum (toDigits x) + sumDigits xs

If we write that as list comprehension, it gets more obvious:

sumDigits xs = sum [sum (toDigits x) | x <- xs]

That's the same as

sumDigits xs = sum (map (sum . toDigits) xs)

or

sumDigits = sum . map (sum . toDigits)


Joining WorkTypes to WorkGroups via Channels and Users to check authorization I need to select some "work types" from a channel. These "channels" are in "work groups" and both channels and work groups need to have permissions to view them (tables are ChannelUsers and WorkGroupUsers to check this). The views that can be selected are as such:

  this.views = {
    "*" : ["*"],
    "index" : [
            "Timers.worktype_id",
            "WorkTypes.worktypename",
            "Timers.start",
            "Timers.end",
            "Users.username",
            "Timers.closed",
            "Users.username",
            "Users.id AS user_id",
            "(Timers.end - Timers.start) AS timer_length"
          ],
    "get" : [
            "Timers.worktype_id",
            "Timers.id AS timer_id",
            "Timers.start",
            "Timers.end",
            "Timers.user_id",
            "Timers.work_id",
            "Timers.closed",
            "ChannelUsers.user_id AS user_id",
            "Users.username"
          ]

The query, however is getting pretty grossly long. Being my first webapp ever I'm not used to chaining this many joins together. Is this at all acceptable? Should I be using some more efficient method of checking authorisation?

let checkQuery = `
            SELECT
              ${view}
            FROM Timers

            INNER JOIN WorkTypes
              ON Timers.worktype_id=WorkTypes.id
              AND WorkTypes.channel_id=?

            INNER JOIN ChannelUsers
              ON ChannelUsers.channel_id=WorkTypes.channel_id
              AND ChannelUsers.user_id=?

            INNER JOIN Channels
              ON Channels.id=ChannelUsers.channel_id

            INNER JOIN Users
              ON Users.id=?

            INNER JOIN WorkGroupUsers
              ON WorkGroupUsers.workgroup_id=Channels.workgroup_id
              AND WorkGroupUsers.user_id=?

            INNER JOIN WorkGroups
              ON WorkGroupUsers.workgroup_id=WorkGroups.id;
          `;
      checkQuery = mysql.format(checkQuery, [_channelID, _user.id, _user.id, _user.id]);

I've heard some conflicting opinions on "views" and "temporary tables" but nothing I'm certain could help here. Any opinions would be greatly appreciated!

(The views are separate as part of a system for "building" many-to-one relationships into the resulting object, i.e building all posts a user has made by their ID into the method result, I cut it here for simplicity).

I am understanding that your goal is to return a set of data that a user is authorized to see. I further understand that ChannelUsers and WorkGroupUsers are only used to validate the authorization. (I see you do select ChannelUsers.user_id but I don't think you really need it - you can take it from Users.id instead, I believe).

Given that, I believe you should test an alternative query to see whether this yield better performance. The SQL would be similar to this:

        SELECT
          ${view}
        FROM Timers
        INNER JOIN WorkTypes
          ON Timers.worktype_id=WorkTypes.id
          AND WorkTypes.channel_id=?
        INNER JOIN Channels
          ON Channels.id=ChannelUsers.channel_id
        INNER JOIN Users
          ON Users.id=?
        INNER JOIN WorkGroups
          ON WorkGroupUsers.workgroup_id=WorkGroups.id
        WHERE EXISTS (
            SELECT NULL
            FROM ChannelUsers
            WHERE ChannelUsers.channel_id=WorkTypes.channel_id
              AND ChannelUsers.user_id=Users.id
        ) AND EXISTS (
            SELECT NULL
            FROM WorkGroupUsers
            WHERE WorkGroupUsers.workgroup_id=Channels.workgroup_id
              AND WorkGroupUsers.user_id=Users.id
        );

The main advantage of using EXISTS is that it won't "duplicate" the results as a join would so you don't have to do a DISTINCT; it also stop searching as soon as it has found its match. If you have indices on ChannelUsers(channel_id, user_id) and WorkGroupUsers(workgroup_id, user_id this may help, as EXISTS can blow through the index and thus get an answer faster in that manner.

Profile the original SQL against this and see what you get. The results also will be less with the EXCEPT, so validate this is acceptable.


API Get Request to pull data from an online database This code gets detailed information about a particular game from an online database using an API Get request. The question I have doesn't pertain to the request but is more about the code itself. The code works. I just think it is long for what needs to be done though. I was wondering if this code is able to be refactored because there are a few places the code just looks the same. I have been trying for last couple of days to get this done with no avail. Any help will be appreciated.

def self.get_game_details(game_selection)
    if Game.all[game_selection.to_i - 1].description == nil
        response = RestClient.get("#{DETAIL_URL}#{Game.all[game_selection.to_i - 1].slug}", headers = {"x-rapidapi-host" => ENV["HOST"], "x-rapidapi-key" => ENV["API_KEY"]})
        data = JSON.parse(response.body)

        Game.all[game_selection.to_i - 1].description = data["description"]
        Game.all[game_selection.to_i - 1].released = data["released"]
        Game.all[game_selection.to_i - 1].metacritic_rating = data["metacritic"] unless data["metacritic"] == nil # default to 0 if no data

        data["ratings"].each do |rating|
            case rating["title"]
            when "recommended"
                Game.all[game_selection.to_i - 1].recommended_rating = rating["percent"] unless rating["percent"] == nil # # default to 0.0 if no data
            when "exceptional"
                Game.all[game_selection.to_i - 1].exceptional_rating = rating["percent"] unless rating["percent"] == nil # # default to 0.0 if no data
            when "meh"
                Game.all[game_selection.to_i - 1].meh_rating = rating["percent"] unless rating["percent"] == nil # # default to 0.0 if no data
            when "skip"
                Game.all[game_selection.to_i - 1].skip_rating = rating["percent"] unless rating["percent"] == nil # # default to 0.0 if no data
            end
        end
    end
end

My approach to your code:

# If this data isn't going to change, using constants would be good enough
HEADERS = { 'x-rapidapi-host' => ENV['HOST'],
            'x-rapidapi-key' => ENV['API_KEY'] }.freeze

def self.get_game_details(game_selection)
  # If Game is an ActiveRecord class, probably you should modify this to look
  # for the specific record instead of loading everything with .all
  # If is just a PORO, forget what I said.
  game = Game.all[game_selection.to_i - 1]
  return if game.description.present?

  response = RestClient.get("#{DETAIL_URL}#{slug}", HEADERS)
  data = JSON.parse(response.body)
  game.details = data
end

class Game
  ALLOWED_RATINGS = %w[recommended exceptional meh skip].freeze

  # Moved most of the assigning logic into the Game class. This is debatible as
  # I could use a Service class or another pattern for this, but as I don't
  # know your code's nature, to me looks like the quickest way.
  def details=(data)
    self.description = data['description']
    self.released = data['released']
    # .presence will return nil for blank strings, empty arrays, etc, then with
    # || you will default the value to 0, as:
    #
    #   nil || 0 # => 0
    self.metacritic_rating = data['metacritic'].presence || 0

    data['ratings'].each { |rating| self.rating = rating }
  end

  def rating=(rating)
    # .send method can run any method of this class, so better to check if the
    # value is valid first
    return unless ALLOWED_RATINGS.include? rating['title']

    send("#{rating['title']}_rating=", rating['percent'].presence || 0.0)
  end
end


Optimizing an IP generation method I have designed a proxy server which has the feature to manage miniature separate networks. Each network has space to have up to 500 nodes or 500 IPs. The code for getting a new virtual IP is based on a bit flag based approach. I have used an array of DWORD (Blocks) so as to skip entire 32 IP chunks, if the block is fully set. I have also divided the DWORD further into byte based checking. so as to forgo checking of all 32 bits as much as possible. The code also skips any addresses with the lower octet being 0 or 1. The code only gets the lower two octets while the higher two octets are fixed to 10.0.

I know the problem with this approach is that the higher the IP, the slower it gets found. So how can I diminish the slow down? Should I go higher towards LongWORD for one block? Should I check the Block byte by byte or in smaller/bigger chunks? Or just abandon this and find some other algorithm?

The limit to be 500 is more of a marketing move than a technical one. We currently have to manage 1800 such networks. These networks are academic in nature and have very anomalous behavior patterns, as such that we are seeing almost 10000 requests per second in peak times.

---

var
  fIPPool : array [0..15] of DWORD;

---

constructor xxxx;
begin

  fIPPool [0]  := $00000003; // Skips 0.0 and 0.1
  fIPPool [7]  := $80000000; // Skips 0.255
  fIPPool [8]  := $00000003; // Skips 1.0 and 1.1
  fIPPool [15] := $FE000000; // Skips 1.249-1.255

end;

---

function GetIP: DWORD;
var
  Block, Cell : Byte;
  Mask : DWORD

  function GetCell ( const ABlock: DWORD; var AMask: DWORD; const ACell : Byte )
                   : Byte; inline;
  begin
    Result := ACell;
    while ( ABlock and AMask ) <> 0 do
    begin
      Result := Result + 1;
      AMask := AMask shl 1;
    end;
  end;

begin

  Result := 0;
  for Block := 0 to 15 do
  begin

    if fIPPool [Block] <> $FFFFFFFF then
    begin

      if ( fIPPool [Block] and $000000FF ) < $000000FF then
      begin
        Mask := $00000001;
        Cell := GetCell ( fIPPool [Block], Mask, 0 )
      end
      else if ( fIPPool [Block] and $0000FFFF ) < $0000FFFF then
      begin
        Mask := $00000100;
        Cell := GetCell ( fIPPool [Block], Mask, 8 )
      end
      else if ( fIPPool [Block] and $00FFFFFF ) < $00FFFFFF then
      begin
        Mask := $00010000;
        Cell := GetCell ( fIPPool [Block], Mask, 16 );
      end
      else
      begin
        Mask := $01000000;
        Cell := GetCell ( fIPPool [Block], Mask, 24 );
      end;

      Result := ( Block * 32 ) + Cell;
      fIPPool [Block] := fIPPool [Block] or Mask;
      break;

    end;

  end;

end;

// one can use the following line to get the IPs if run in a for-loop for 500 times
Memo1.Lines.Add
( IntToStr ( ( IP and $FF00 ) shr 8 ) +'.'+ IntToStr ( IP and $00FF ));

I have performed some benchmarks to get the optimum combination of block and slice sizes using 32-bit and 64-bit compiler. 64-bit because we are also developing an optimized 64-bit implementation on the sidelines. Here are the results for generating 320000 IPs on our server with all the server applications also running, the values are in Milli-seconds and floored over 5 runs;

Block-Slice  32-bit 64-bit
32-0         3479   3120
32-16        3464   5912
64-0         2059   2980
64-16        2028   1606
64-32        2013   1622

The algorithm gave the worst results with 8-bit slices.
So the best bets are 64-32 for 32-bit and 64-16 for 64-bit for this algorithm.


Improving performance of moving arrays into an array I am loading a bunch of arrays and putting them into a single array.  The loading of the data is negligible, but putting it into combinedArray takes significant time (as in this code).

import time
import numpy as np
start_time = time.time()

   st,en = 0,0
   combinedArray = np.empty(shape = (30000*30000))
   #Get the array that each process wrote to
   for i in range(10):
       #load the array
       #ex loading
       X_np = np.empty(shape = (30000*30000)/10)
       st = en
       en += np.shape(X_np[:])[0]
       #add to the array list
       combinedArray[st:en] = X_np[:]

   print("Array Calc:",time.time()-start_time)

What I have found is often someone talking about not using append, so I tried with creating the array first, but just moving it is time consuming.  Any advice on how to optimize this is appreciated.

I tried some alternative approaches, and got this result:

Time for orig: 0.15900921821594238
Time for concat: 0.18401050567626953
Time for block: 0.16700983047485352
Time for stack_ravel: 0.1760098934173584
Time for stack_flatten: 0.32501864433288574

It's worth pointing out that I'm on a 32-bit machine with not enough RAM to hang with you. So I used smaller arrays. You'll want to correct the code and try for yourself. Also, my validation is really minimal, so you should probably improve that as well.

import time
import numpy as np

COMBINED_SHAPE = (3_000 * 3_000)

def time_func(f):
    start_time = time.time()
    ca = f()
    assert np.shape(ca)[0] == COMBINED_SHAPE
    print(f"Time for {f.__name__}:", time.time() - start_time)

def load_partial():
    partial = np.empty(shape=(COMBINED_SHAPE // 10))
    return partial

def orig():
    st, en = 0, 0
    combined_array = np.empty(shape=COMBINED_SHAPE)

    for i in range(10):
        X_np = load_partial()
        st = en
        en += np.shape(X_np[:])[0]
        combined_array[st:en] = X_np[:]

    return combined_array

def concat():
    ca = np.concatenate([load_partial() for _ in range(10)])
    return ca

def block():
    ca = np.block([load_partial() for _ in range(10)])
    return ca

def stack_ravel():
    ca = np.stack([load_partial() for _ in range(10)])
    ca = np.ravel(ca)
    return ca

def stack_flatten():
    ca = np.stack([load_partial() for _ in range(10)]).flatten()
    return ca

if __name__ == '__main__':
    time_func(orig)
    time_func(concat)
    time_func(block)
    time_func(stack_ravel)
    time_func(stack_flatten)


Guessing game where the first player inputs a word and a hint and the second player tries to guess it We have been given this project and I was wondering if someone could make my algorithm better using TASM and DOSBox.

This is what I have done so far:

ideal
model small
stack 10244
dataseg
msg1 db 'Enter Word: $'
msg2 db 'Enter Hint: $'
msg3 db 'Enter Guess(Press . to end guess)($'
wrnmsg db 'Wrong!$'
a3msg db 'Number of letters: $'
a4msg db 'First two letters: $'
a5msg db 'Last letter: $'
a6msg db 'Second to the last letter: $'
fail db ' Enter guess again($'
secret db 21 dup('')
hint db 101 dup('')
guess db 21 dup('')
endss db '): $'
win db 'That is correct! You win!$'
lose db 'Im sorry you lose$'
cnt dw 0
codeseg
PROC compare
mov bx, 0
mov di, 0

loop1:
    mov al, [secret+di]
    mov ah, [guess+di]

    cmp ah, al
    jne no
    cmp al, '$'
    je endi
no:
    mov bx, 1
endi:
    ret
ENDP

PROC main
mov ax, @data
mov ds, ax
mov es, ax

lea dx, [msg1]
mov ah, 9
int 21h

mov bx, 0
scan:
    mov ah, 1
    int 21h
    cmp al, '.'
    je com
    inc [cnt]
    mov [secret+bx], al
    inc bx
    jmp scan
com:
    mov [secret+bx], '$'

mov dx, 10
mov ah, 2
int 21h

lea dx, [msg2]
mov ah, 9
int 21h

lea dx, [hint]
mov ah, 10
int 21h

mov dx, 10
mov ah, 2
int 21h

lea dx, [msg3]
mov ah, 9
int 21h

lea dx, [hint]
mov ah, 9
int 21h

lea dx, [endss]
mov ah, 9
int 21h

mov cx, 0
loop2:
    inc cx
    mov bx, 0
    scan2:
        mov ah, 1
        int 21h
        cmp al, '.'
        je com2
        mov [guess+bx], al
        inc bx
        jmp scan2
    com2:
        mov [guess+bx], '$'

        mov dx, 10
        mov ah, 2
        int 21h

    call compare
    cmp bx, 0
    je right

    lea dx, [wrnmsg]
    mov ah, 9
    int 21h

    cmp cx, 3
    je three
    cmp cx, 4
    je four
    cmp cx, 5
    je five
    cmp cx, 6
    je six
    cmp cx, 7
    je fail

    man:

    mov dx, 10
    mov ah, 2
    int 21h

    lea dx, [fail]
    mov ah, 9
    int 21h

    lea dx, [hint]
    mov ah, 9
    int 21h

    lea dx, [endss]
    mov ah, 9
    int 21h

    jmp loop2

right:
    lea dx [win]
    mov ah, 9
    int 21h

    jmp end1

fail:
    lea dx, [lose]
    mov ah, 9
    int 21h

    jmp end1

three:
    lea dx, [a3msg]
    mov ah, 9
    int 21h

    lea dx, [cnt]
    mov ah, 2
    int 21h
    jmp man
four:
    lea dx, [a4msg]
    mov ah, 9
    int 21h

    mov di, 0
    mov si, 1
    mov dh, [secret+di]
    mov ah, 2
    int 21h
    mov dh, [secret+si]
    int 21h

    jmp man
five:
    lea dx, [a5msg]
    mov ah, 9
    int 21h

    mov di, [cnt]
    mov dl, [secret+di]
    mov ah, 2
    int 21h

    jmp man
six:
    lea dx, [a6msg]
    mov ah, 9
    int 21h

    mov di, [cnt]
    dec di
    mov ah, 2
    int 21h

    jmp man
end1:
    mov ah, 4ch
    int 21h
ENDP
END main

What was required of us was a program that will simulate a guessing game where the first player will input the word to be guessed by the second player including a hint such as what category and other description. (Description can have a maximum of 100 characters).

The second player will be given 6 chances to guess. If in the third try, the 2nd player still cannot guess the word, hints will be given for the succeeding unsuccessful guesses as cited below:

Third unsuccessful try – Number of letters will be displayed on screen
Fourth unsuccessful try – First two letters will be revealed
Fifth unsuccessful try – Last letter will be revealed
Sixth unsuccessful try- Second to the last letter will be revealed

You have a bit of code duplication that could be extracted and consolidated. But first with some error fixes (at least the code did not compile on my TASM).

Compilation issues

right:
    lea dx [win] ; <-- missing comma between dx and [win]

fail is defined twice. First as a string (fail db ' Enter guess again($') and the second time as a label. I would rename the first one to failmsg and use it in

lea dx, [failmsg] ; <-- instead of fail before

With those fixes the code compiles now. As for the code review I would start with

DOS functions

mov ah,09
int 21h

which is just Display string. So let's call it like that and make sure a requirement that when calling it we have dx

PROC display_string
   mov ah,09
   int 21h
   ret
ENDP

and instead of

lea dx, [a3msg]
mov ah, 9
int 21h

just do

lea dx, [a3msg]
call display_string

The same goes for other DOS functions

PROC get_char
   mov ah,01
   int 21h
   ret
ENDP

PROC put_char
   mov ah,02
   int 21h
   ret
ENDP

Using those instead of bare int-ing will make your code more readable. Having those it's clear what you try to do with this code

High-level functions

mov dx, 10
call put_char

but in DOS it should be 2 characters: 13,10 so it's better to do like this:

PROC new_line
  mov dx,13
  call put_char
  mov dx,10
  call put_char
  ret
ENDP

Buffered input

I don't know why you chose buffered input as your method of entering the hint, but in case of this command the data has a specific format.

hint db 100,?,101 dup(0)

The actual string starts on index 2 and before that you have max string length, and bytes read. Also there's no $ at the end. You have to put it there.

xor bx,bx
mov bl, [hint+1]
mov [hint+bx+2], '$'

So printing the hint should look like this:

lea dx, [hint+2]
call display_string

Printing the count

This is obviously wrong

lea dx, [cnt]
call put_char

as in the cnt will be the number of characters not the ascii representation of that number. If we could restrict ourselves to maximum 9 characters that would be easy. Just add 48(dec) to the cnt and you get the value. Since your secret is limited to 21 characters we could go with a simple method for printing.

PROC print_number
    push bx
    push dx
    mov bx,10
    xor dx,dx
    div bx
    mov bx,dx
    cmp ax,0
    je second
    mov dx,ax
    add dx,48
    call put_char
second:
    mov dx,bx
    add dx,48
    call put_char
    pop dx
    pop bx
    ret
ENDP

What we do here is just divide the number in ax by 10 and print the values stored in ax (result - only if >0) and the dx - reminder.

Printing the hints

On fourth attempt you are putting the character to be printed in dh. It should be in dl. You don't also have to use different registers so just:

mov dl, [secret]
call put_char
mov dl, [secret+1]
call put_char

To print the last char you need to keep in might that the last is '$' so just one before the last

mov di, [cnt]
mov dl, [secret+di-1]
call put_char

The same issue when you try to print the 6th hint. Also that you did not retrieve the char at all.

mov di, [cnt]
sub di,2
mov dl, [secret+di-1]
call put_char

Compare

Your compare method is kinda strange. You do not iterate at all and you don't use any opcodes that would do that for you. It works only when you have one-char secret. Not so cool :)

I would write it like this

PROC compare
  push cx
  lea si, [secret]
  lea di, [guess]
  rep cmpsb
  pop cx
  ret
ENDP

and change how to check for equality

call compare
je right

Other things that might be good to fix

You have the same loop for scanning the characters. Why not enclose it as another proc and just use in those two places? You can make assumption that for example di will point to the buffer that has to be filled. Also in this scanning you could handle for example backspace character and delete the characters.

PROC read_input
  push cx
  mov bx, 0
  mov cx, 0
scan:
  call get_char
  cmp al, '.'
  je com
  inc cx
  mov [di+bx], al
  inc bx
  jmp scan
com:
  mov [di+bx], '$'
  mov ax,cx
  pop cx
  ret
ENDP

And use it

lea di,[secret]
call read_input
mov [cnt],ax

and similarly in the second place.

Also some checks for input length might be good to do.

You should also comment more your code. I strongly believe the code should be self documented but not on such low level as asm :) So comment more.

GIST


Query database and check server details I have written bash script to query database and check server details.

Please let me know if the script is as per best standards:

if [ -z "$1" -o -z "$2" ]
then
  echo "FAIL : Put username and password"
else
  fulltoken=`curl -s --insecure -H "Content-Type: application/json" -X POST -d "{\"username\":\"$1\",\"password\":\"$2\"}" https://apiurl`
  echo $fulltoken | grep -q authToken > /dev/null 2>&1
  if [ $? -eq 0 ]
  then
    token=`echo $fulltoken | awk -F':' '{print $8}' | awk -F',' '{print $1}' | sed 's/"//g'`
    hostname=`uname -n | awk -F'.' '{print $1}'`
    serverdetails=`curl -s --insecure -H "Content-Type: application/json" -H "Authorization: $token" -X GET https://apiurl`
    echo $serverdetails | grep -iq "applnId"
    if [ $? -eq 0 ]
    then
      exactserverdetails=`echo $serverdetails`
      appInId=`echo $exactserverdetails | sed 's/,/\n/g' | grep "applnId" | awk -F':' '{print $2}'`

        if [ -n $appInId ] && [ $appInId != "null" ]
        then
            echo "PASS"
        else
            echo "FAIL"
        fi
        echo $exactserverdetails | sed 's/,/\n/g' | grep "environment_Name" | awk -F':' '{print $2}' | grep -q -e "Dev"
        if [ $? -eq 0 ]
        then
            echo "PASS"
        else
            echo "FAIL"
        fi
    else
        echo "sorry server is not present"
    fi
  else
      echo "all good"
  fi
fi

Add a shebang line

If you want to be able to run your program without explicitly specifying the interpreter, then you should make the script executable and start the script with a #! line:

#!/bin/sh

Usage reporting

If the arguments aren't specified, show how to use the program (and exit with an error status):

if [ -z "$1" ] || [ -z "$2" || [ "${3:-}" ]
then
  echo "Usage: $0 <username> <password>" >&2
  exit 1
fi

I've redirected the error message to the standard error stream, so it will be seen; and I've replaced the Bash test -o with a more portable alternation of test commands.

It's up to you whether an empty password (as opposed to an unset password) is allowable.

Care with variable expansions

You've been pretty diligent about quotes (well done), but there is one place where you might get tripped up:

fulltoken=`curl ... "{\"password\":\"$2\"}" ...`

Could $2 contain "?  If so, you might want to escape it (${2//\"/\\\"}) or detect it and abort before executing curl.

The if $? antipattern

Instead of running a command and then separately testing the status, it's more idiomatic to use a single if:

if echo "$fulltoken" | grep authToken >/dev/null 2>&1
then
  ...
fi

A couple of further observations on this particular line: you could replace echo with a <<< redirection; you could replace the grep with testing against a substituted string (both of these involve Bash extensions, so you might prefer to lean towards the portability above):

if <<<"$fulltoken" grep -q authToken 2>/dev/null

if [ "$fulltoken" != "${fulltoken/authToken}" ]

Related to this is

Keep the error handling close by

The program has more than one structure that looks like

if some_test
then
    ...
    lots
    ...
    of
    ...
    code
    ...
    here
    ...
    if second_test
    then
        ....
        more
        code
        ...
    else
        echo "INNER FAIL"
    fi
else
    echo "FAIL"
fi

This can make it harder to relate the else clause to the correct if.  My usual approach to this is to define a die function:

die(){
    echo "$@" >&2
    exit 1
}

Then the structure above becomes more linear and less indented:

some_test || die "FAIL"
...
lots
...
of
...
code
...
here
...
second_test || die "INNER FAIL"
....
more
code
...

This might not work for you if you want to report more than one problem per run, but it's certainly something to consider and possibly adapt for your purposes.

Use the simplest tool for the job

Although awk can pick out fields from a line, that's the sole purpose of cut, and I think it's clearer that that's all you're doing if you write like

token=$(echo "$fulltoken" | cut -d: -f8 | cut -d, -f1 | tr -d \")

Conversely, if you use awk, use it fully - the applnId pipeline (sed|grep|awk) can be done with a single awk invocation:

awk -F: 'BEGIN { RS = "," } $1 == "applnId" {print $2}'

Note that this is now a stricter test - it won't be fooled by a value containing applnId, for instance; it recognises only a key of exactly applnId.

Actually, since you have asked for JSON output, it's easier and more reliable to use jq than pure Bash to parse it:

applnId=$(jq -r applnId <<<"$exactserverdetails")

Similarly, to test the environment name:

test "$(jq -r environment_Name <<<"$exactserverdetails")" = Dev \
    || die "Environment name is not 'Dev'"

Be more specific when reporting errors

This test doesn't tell the user much:

    if [ -n $appInId ] && [ $appInId != "null" ]
    then
        echo "PASS"
    else
        echo "FAIL"
    fi

Instead, I'd write:

    case "${appInId-}" in
      '') echo "No applnId" >&2; exit 2;;
      null) echo "applnId is 'null'" >&2; exit 2;;
      *) echo "applnId is valid";; # (if you must)
    esac

I'd omit the last case, though - I'm not a fan of overly chatty programs.


Find path from source to destination in tilt maze For a tilt maze general reference, you can refer to this example. The previous related code review discussion could be referred here.

The problem I want to resolve is to find all possible path (so that in the future I can find minimal path) from source to destination.

My major idea is:

Represent the maze by flag which direction I can move from a specific cell
Using recursion, move left/right, then move top/down, then left/right, then top/down, until the destination is reached

I'm not sure if my implementation is elegant, move left/right, then move top/down, then left/right, then top/down, until reach to destination, seems a bit hard-coded way. I'm wondering if there are any functional/logical bugs in my code -- even if I tested, but might be potential issues which I do not find yet.

  __ __ __ __
 |__   |__   |
 |  |  |     |
 |  |   __|  |
 |     |   __|
 |__|__ __ __|

class MoveEnum:
    CAN_MOVE_LEFT = 1
    CAN_MOVE_RIGHT = 2
    CAN_MOVE_UP = 4
    CAN_MOVE_DOWN = 8

class Maze:
    def __init__(self, matrix):
        self.matrix = matrix
        self.results = []
    def move_left(self, x1, y1):
        while self.matrix[y1][x1] & MoveEnum.CAN_MOVE_LEFT:
            x1 -= 1
        return (x1, y1)
    def move_right(self, x1, y1):
        while self.matrix[y1][x1] & MoveEnum.CAN_MOVE_RIGHT:
            x1 += 1
        return (x1, y1)
    def move_up(self, x1, y1):
        while self.matrix[y1][x1] & MoveEnum.CAN_MOVE_UP:
            y1 -= 1
        return (x1, y1)
    def move_down(self, x1, y1):
        while self.matrix[y1][x1] & MoveEnum.CAN_MOVE_DOWN:
            print y1
            y1 += 1
        return (x1, y1)
    # find shortest move from x1 y1, to x2, y2
    def move_path(self, x1, y1, x2, y2, is_left_right, path):
        if x1 == x2 and y1 == y2:
            self.results.append(path[:])
        elif is_left_right:
            (x, y) = self.move_right(x1,y1)
            if x != x1:
                path.append((x, y))
                self.move_path(x, y, x2, y2, not is_left_right, path)
                path.pop(-1)
            (x, y) = self.move_left(x1,y1)
            if x != x1:
                path.append((x, y))
                self.move_path(x, y, x2, y2, not is_left_right, path)
                path.pop(-1)
        else:
            (x, y) = self.move_up(x1, y1)
            if y != y1:
                path.append((x, y))
                self.move_path(x, y, x2, y2, not is_left_right, path)
                path.pop(-1)
            (x, y) = self.move_down(x1, y1)
            if y != y1:
                path.append((x, y))
                self.move_path(x, y, x2, y2, not is_left_right, path)
                path.pop(-1)

if __name__ == "__main__":
    maze_matrix = [[MoveEnum.CAN_MOVE_RIGHT, MoveEnum.CAN_MOVE_LEFT|MoveEnum.CAN_MOVE_DOWN, MoveEnum.CAN_MOVE_RIGHT, MoveEnum.CAN_MOVE_LEFT|MoveEnum.CAN_MOVE_DOWN],
                   [MoveEnum.CAN_MOVE_DOWN, MoveEnum.CAN_MOVE_UP|MoveEnum.CAN_MOVE_DOWN, MoveEnum.CAN_MOVE_RIGHT|MoveEnum.CAN_MOVE_DOWN, MoveEnum.CAN_MOVE_LEFT|MoveEnum.CAN_MOVE_UP|MoveEnum.CAN_MOVE_DOWN],
                   [MoveEnum.CAN_MOVE_UP | MoveEnum.CAN_MOVE_DOWN, MoveEnum.CAN_MOVE_UP|MoveEnum.CAN_MOVE_DOWN|MoveEnum.CAN_MOVE_RIGHT, MoveEnum.CAN_MOVE_UP|MoveEnum.CAN_MOVE_LEFT, MoveEnum.CAN_MOVE_UP|MoveEnum.CAN_MOVE_DOWN],
                    [MoveEnum.CAN_MOVE_UP | MoveEnum.CAN_MOVE_DOWN | MoveEnum.CAN_MOVE_RIGHT, MoveEnum.CAN_MOVE_UP | MoveEnum.CAN_MOVE_DOWN | MoveEnum.CAN_MOVE_LEFT, MoveEnum.CAN_MOVE_DOWN|MoveEnum.CAN_MOVE_RIGHT, MoveEnum.CAN_MOVE_UP|MoveEnum.CAN_MOVE_LEFT],
                   [MoveEnum.CAN_MOVE_UP, MoveEnum.CAN_MOVE_UP|MoveEnum.CAN_MOVE_RIGHT, MoveEnum.CAN_MOVE_UP | MoveEnum.CAN_MOVE_RIGHT, MoveEnum.CAN_MOVE_LEFT]]
    maze = Maze(maze_matrix)
    maze.move_path(0,0,3,4,True,[])
    maze.move_path(0, 0, 3, 4, False, [])
    print maze.results

I'd still suggest using a closure, which I addressed in my last answer:

  Your code is WET. As you write pretty much the same thing four times. Instead I'd use a builder function to remove this duplicate logic. This is as you can set the special changes in the builder function, and mutate the data in the returned function.

  Take:

def _move(band, x_inc, y_inc):
    def inner(x, y):
        maze = self.maze
        while maze[y][x] & band:
            x += x_inc
            y += y_inc
        return x, y
    return inner

move_right = _move(2, 1, 0)

The name of MoveEnum could be changed to Moves, as the Enum part is useless when using it. I'd also change CAN_MOVE_* to just *, this is as it doesn't add any value. And so I'd much prefer it to be Moves.UP.

move_path is dodgy. You shouldn't have to pass a list to it, and it should really return results. Instead I would create another closure the outer function to create path, return results, and call the inner function. The inner function to perform the recursion as you currently have it.


Javascript / ES6 parse value from arrays of objects I want to write a generic parser which takes a value, and type and returns label of given value instead.

Currently, this is my code:

import a from "../constants/a"
import b from "../constants/b"
import c from "../constants/c"

const find = (value, fromArray) => fromArray.find((term) => term.value === value);
const getLabel = (of, fromArray) => {
    const value = find(of, fromArray);
    return (value && value.label) || 'No info';
};

export default (value, type) => {
    switch (type) {
        case 'paymentMethod':
            return getLabel(value, paymentMethods);
        case 'paymentTerm':
            return getLabel(value, paymentTerms);
        case 'dateType':
            return getLabel(value, dateTypes);
        default:
            return 'No info';
    }
}

What would be more ES6, faster and concise way to achieve this?

Granularity

All code can be said to have a level of granularity. This is a measure of how many functions it contains. Low granularity has fewer larger functions that has benefits of faster run-times at the expense of readability, while high granularity has many smaller functions, trading off performance for more readable and maintainable code.

Just how granular to make some code is something that comes from experience as there are no hard and fast rules to go by. However there is a general rule of thumb that increasing granularity should always reduce code complexity and source code size.

Too granular

Your code is too granular,

It is bloated with the source required only to define functions, call and pass arguments.
It adds un-needed complexity with arguments transforming the abstraction implied by naming from one value to another of and then back again to value.
It has forced you to repeat code.
It contains a function that is only an alias for existing functions find is an alias for Array.find and provides no additional processing.

The function getLabel processes  the result of find returning "No info" for undefined results. But this is repeated in the calling function.

Reducing granularity to improve code

Reducing granularity will not always improve code, well I should say very seldom and only in the case where the code is already too granulated.

Your module can be done in a single function

export default (value, type) => {
    var array, result;
    switch (type) {
        case 'paymentMethod':
            array = paymentMethods;
            break;
        case 'paymentTerm':
            array = paymentTerms;
            break;
        case 'dateType':
            array = dateTypes;
    }
    if (array) { result = array.find(item => item.value === value) }
    return result && result.label || "No info"
}

Or as there are only a few cases in the switch use if else

export default (value, type) => {
    var array, result;
    if(type === "paymentMethod") { array = paymentMethods }
    else if(type === "paymentTerm") { array = paymentTerms }
    else if(type === "dateType") { array = dateTypes }

    if (array) { result = array.find(item => item.value === value) }
    return result && result.label || "No info"
}

This can be even further reduced by using type to index the array via bracket notation, replacing the unwieldy switch or if else statements

const types = {paymentMethods, paymentTerms, dateTypes};
export default (value, type) => {
    var result;
    const name = type + "s";
    if (types[name]) { result = types[name].find(item => item.value === value) }
    return result && result.label || "No info";
}

Or if the names are not always pluralized with an s.

const types = {paymentMethod:paymentMethods, paymentTerm:paymentTerms, dateType:dateTypes};
export default (value, type) => {
    var result;
    if (types[type]) { result = types[type].find(item => item.value === value) }
    return result && result.label || "No info";
}

Your questions

  What would be more ES6, faster and concise way to achieve this?

More ES6? Not really, the  ES6 - eness of the code has not changed.
Faster? Yes there is less code to execute and few function states to create, push, and pop from the call stack.
Concise? Yes in my view it is much more concise. From 17 lines and three functions to 6 lines, one function, and no repeated code.


Find the highest unique element in an array Here is my approach. Unique element means that element should present only once in the array.

import java.io.IOException;
import java.util.Comparator;
import java.util.Map;
import java.util.Scanner;
import java.util.TreeMap;
import java.util.Map.Entry;

public class Unique {

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

        // Used TreeMap to store data in descending order to return maximum unique value
        Map<Integer, Integer> map = new TreeMap<Integer, Integer>(
                new Comparator<Integer>() {
                    public int compare(Integer o1, Integer o2) {
                        return o2.compareTo(o1);
                    }
                });
        // Add into map if input is not present. If already present, increase count
        for (int i = 0; i < n; i++) {
            int input = sc.nextInt();
            if (map.containsKey(input))
                map.put(input, map.get(input) + 1);
            else
                map.put(input, 1);

        }

        // Check for a entry with value 1. If not present print -1 else traverse
        // the map and print first key with value 1.
        if (map.containsValue(1)) {
            for (Entry<Integer, Integer> entry : map.entrySet()) {

                if (entry.getValue() == 1) {
                    System.out.print(entry.getKey() + " ");
                    break;
                }

            }
        } else
            System.out.println(-1);

    }
}

Sample input:

n=5
9 8 8 9 5

Desired Output:

5

Please critique my approach and suggest better way to solve this problem.

Your approach is a little bit complicated: you're building a TreeMap that is sorted in descending order of their keys. The value for each key is the number of times it appears in the input. Then you are iterating the map to find the first key having the value of 1. There are a couple of notes though.

Use built-in methods

You are constructing a TreeMap sorting in reverse order of their keys with

// Used TreeMap to store data in descending order to return maximum unique value
Map<Integer, Integer> map = new TreeMap<Integer, Integer>(
        new Comparator<Integer>() {
            public int compare(Integer o1, Integer o2) {
                return o2.compareTo(o1);
            }
        });

You actually do not need to create a custom comparator for that case. You can use the built-in Collections.reverseOrder(). Also, since Java 7, you can omit the type-declaration with the diamond operator. So you could just have:

Map<Integer, Integer> map = new TreeMap<>(Collections.<Integer>reverseOrder());

Use Java 8 constructs

Starting with Java 8, you can replace the following code

if (map.containsKey(input))
    map.put(input, map.get(input) + 1);
else
    map.put(input, 1);

with the built-in merge(key, value, remappingFunction) method. This will put the given key with the given value if there was no mapping for that key, else it will invoke then given remapping function with the old value and the new one. In this case, we can then replace that code with

map.merge(input, 1, Integer::sum);

which will put the mapping with 1 if there was no key or add 1 to the current value if the mapping already exists.

Add curly braces!

This may sound like a nitpick but it is in fact very important. There are part of your code that doesn't add curly braces, or have it partially, like

if (map.containsKey(input))
    map.put(input, map.get(input) + 1);
else
    map.put(input, 1);

with no curly braces, or

if (map.containsValue(1)) {
    // ...
} else
    System.out.println(-1);

with partial curly braces.

Better to make it consistent and use curly braces everywhere.

Simpler approach

In this problem, we're only interested in keeping the highest number that appear only one time. This means that every numbers which appears more than once can be ignored: we know that it won't be the result. As such, there is no need to store a map with the exact count of each number: we only want to know if it is 1 or not.

Consider the following approach:

We keep a Set with the running encoutered numbers.
We keep a Set with the running highest unique numbers.
For each input, those two sets are updated:

If the input was already encountered, we remove it from the set of uniques
If the input wasn't encoutered, we add it to both the encoutered and the uniques.

An implementation would be the following:

Scanner sc = new Scanner(System.in);
int n = sc.nextInt();
Set<Integer> encounteredInts = new HashSet<>(n);
Set<Integer> uniques = new HashSet<>(n);
for (int i = 0; i < n; i++) {
    int input = sc.nextInt();
    if (encounteredInts.contains(input)) {
        uniques.remove(input);
    } else {
        encounteredInts.add(input);
        uniques.add(input);
    }
}

if (uniques.isEmpty()) {
    System.out.println(-1);
} else {
    System.out.println(Collections.max(uniques));
}

There are clear advantages:

We do not store information that we don't need in a map: this is net-gain in memory usage.
We use a Set for look-up, insertion and deletion, which is constant-time: this is net-gain in performance.
From the final set, we only keep the highest according to the natural ordering of the integer. This is the only operation which runs in linear time, but it is done only once, at the end, when the set will have the minimum amount of numbers to consider.


Quick general purpose linked list in C I want to use it when some function needs to return a list of something. Is it working as I think it should or is there undefined behavior somewhere? What I really tried to achieve is something like a templated <list> from C++ - for any type. I'm not sure if there is a standard solution/include/library for this.

list.h

#pragma once

#include <stddef.h>

struct list_handler
{
    struct list_node *head;
    struct list_node *tail;
    size_t count;
};

struct list_node
{
    void *data;
    struct list_node *next;
};

void list_init(struct list_handler *list);
void list_add(struct list_handler *list, void *data);
void list_free(struct list_handler *list);
void list_foreach(struct list_handler *list, void (*foreach_function)(void *));

list.c

#include "list.h"

#include <stdlib.h>

void list_init(struct list_handler *list)
{
    list->head = NULL;
    list->tail = NULL;
    list->count = 0;
}

void list_add(struct list_handler *list, void *data)
{
    struct list_node *new_node = malloc(sizeof(struct list_node));

    new_node->data = data;
    new_node->next = NULL;

    if (list->count > 0)
    {
        list->tail->next = new_node;
    }
    else
    {
        list->head = new_node;
    }

    list->tail = new_node;

    ++list->count;
}

void list_free(struct list_handler *list)
{
    struct list_node *current = list->head;
    struct list_node *previous;

    while (current != NULL)
    {
        previous = current;
        current = current->next;

        free(current);

        list->head = current;
        --list->count;
    }

    list->tail = NULL;
}

void list_foreach(struct list_handler *list, void (*foreach_function)(void *))
{
    struct list_node *current = list->head;

    while (current != NULL)
    {
        (*foreach_function)(current->data);
        current = current->next;
    }
}

Usage:

void list_print(void *data)
{
    int *temp = (int*) data;
    printf("%d\n", *temp);
}

void list_free_data(void *data)
{
    int *temp = (int*) data;
    free(temp);
    data = NULL;
}

int main(int argc, char *argv[])
{
    struct list_handler list;
    list_init(&list);

    for (size_t i = 0; i < 3; ++i)
    {
        int *data = malloc(sizeof(int));
        *data = i;
        list_add(&list, data);
    }

    list_foreach(&list, list_print);
    list_foreach(&list, list_free_data);

    list_free(&list);

    return 0;
}

The list_free has a small error.
Whether the function should free the list is up to the API design. Maybe you did not want dangling pointers. Then the name should be better list_clear.

void list_free(struct list_handler *list)
{
    struct list_node *current = list->head;
    struct list_node *previous;

    while (current != NULL)
    {
        previous = current;
        current = current->next;

        //- free(current);
        free(previous); //+

        list->head = current;
        --list->count;
    }

    list->tail = NULL;
    free(list); //+
}

Freeing the data is a non-list issue, misleading, especially with a void*.

void list_free_data(void *data)
{
    //int *temp = (int*) data;
    //free(temp);
    //data = NULL;
    free(data);
}

Assigning NULL to data has no effect.

However if you want to free the list with freeing the data:

list_foreach(list, free);
list_free(list);
free(list);

The list_foreach is limited as it must work with side-effects, storing results or other context in global variables. Better provide a for-each with an additional parameter. So bad coding style is not inevitable.

For the remainder: I like APIs to provide a full set of operations, that is with a remove too.


Simple game of lotto in Python v 2.0 This is the second version of my lotto game, based on this thread before. I hope I used all the tips I was given, also I fixed it using only functions as promised.

Game rules

  Each player is given 15 numbers, in increasing order. This is why I
  use sorted() on the lists of player and computer. Also there is a
  common list with numbers in it from 1 to 90.

  A random number is picked and removed from this list and printed to
  players. The player who has this number in his card crosses it out or
  skips its turn. This continues until either player or computer crosses
  out all his numbers, or for some strange reason they are out of
  numbers. I think in other parts of the world it is also called BINGO
  but I might be wrong.

import random
import os

number_pool = list(range(1, 11))  # 1 , 101
player_card = sorted(random.sample(number_pool, 3))  # 15
computer_card = sorted(random.sample(number_pool, 3))  # 15

def random_choice(num_pool):
    choice = random.choice(num_pool)
    num_pool.remove(choice)
    # print("Current lotto: " + str(choice))
    return choice

def game_status_check():
    if len(player_card) == 0 or len(computer_card) == 0:
        print("Game over !")
        return True
    else:
        return False

def printout_cards():
    print("######################################\n")
    print("Player card: " + str(player_card))
    print("\n######################################\n")
    print("Computer card: " + str(computer_card))

def end_game():
    answer = input("Do you want to restart the game [y or n] ?: ")
    if answer == "y":
        main()
    elif answer == "n":
        print("Thank you for playing ! ")
        exit()
    else:
        print("Enter y or n !")

def main():
    while not game_status_check():
        printout_cards()
        random_number = random_choice(number_pool)
        print("Random lotto is: {}".format(random_number))
        answer = input("Do you want to cross out number [y or n]: ")
        if answer == "y":
            player_card.remove(random_number)
            if random_number in computer_card:
                computer_card.remove(random_number)
        elif answer == "n":
            if random_number in player_card:
                print("The number was in your card, you missed it and lost !")
                end_game()
            elif random_number in computer_card:
                computer_card.remove(random_number)
        os.system("cls")
        game_status_check()

if __name__ == '__main__':
    main()

For a basic program like this, everything is more or less okay. Here are some nits:

game_status_check() should only do checking. Put the print("Game over !") in your main function.

def game_status_check():
    return len(player_card) == 0 or len(computer_card) == 0

This also removes the need for you to do:

while not game_status_check():
    ...
    game_status_check()

What happens if a player doesn't remove a card that was in their set of player cards, and then enters an answer that's not y or n? Your end_game() function has a handler for that, but doesn't prompt the user again for input. Suggestion:

def end_game():
    answer = input("Do you want to restart the game [y or n] ?: ")
    while answer != "y" and answer != "n":
        answer = input("Do you want to restart the game [y or n] ?: ")
    if answer == "y":
        main()
    else:
        print("Thank you for playing ! ")
        exit()

This change is optional and is purely my opinion to make the output look better.

def printout_cards():
    print("######################################\n")
    print("Player card: {}".format(" ".join(player_card)))
    print("\n######################################\n")
    print("Computer card: {}".format(" ".join(computer_card)))

I would suggest not naming a function random_choice() for readability. It can easily be confused with random.choice().


Do I have too many regexes in my validation logic? The below function uses Javascript to validate a Belgian telephone number and format it according to the guidelines for phone number formatting.

function acm_mscrm2011_psa_FormatPhoneNumber(executioncontext) {
    //get the phone number from the field that starts the validation
    var telefoonnummer = executioncontext.getEventSource().getValue();
    //check if not empty
    if (telefoonnummer) {
        telefoonnummer = telefoonnummer.replace(/(\s|-|\/|\.)+/g, "");
        //check if it's a Belgian number
        //Optional country code of +32 or 0032, followed by optional 0, followed by a local part of either (an 8 digit number) OR (a 4 followed by 8 digits)
        var telefoonRegex = /^(?:\+32|0032)?0?([1-9][0-9]{7}|4[0-9]{8})$/;
        if (telefoonRegex.test(telefoonnummer)) {
            var telefoonMetFormaat;
            //get the first captured group of the previous test, i.e. the local part
            var localpart = RegExp.$1;
            if (localpart.length == 8) {
                //landline or fax
                //group in format # ### ## ##
                telefoonMetFormaat = localpart.replace(/(\d)(\d\d\d)(\d\d)(\d\d)/, "+32 $1 $2 $3 $4");
            } else {
                //cellphone number
                //group in format ### ## ## ##
                telefoonMetFormaat = localpart.replace(/(\d\d\d)(\d\d)(\d\d)(\d\d)/, "+32 $1 $2 $3 $4");
            }
            //put the validated and formatted number back into the system
            executioncontext.getEventSource().setValue(telefoonMetFormaat);
        }
    }
}

As you can see, I first use a Regex to strip any excess delimiters from the code, then I use another Regex to check if the number is valid, then I use another Regex to format the number in a way that's more easily readable.Now, it works, most of it is readable (the validation regex is tricky but has a comment to explain it), but I'm not sure if using Regex everywhere is a good idea.

However, I don't immediately see a different solution that doesn't involve a large amount of nested ifs, fors and substrings that ultimately would be just as tricky to read and maintain as these 3 regexes.

Is there an alternative to these regexes?

For one, I wouldn't worry about the number of regexps here. There's nothing terribly complex going on here. I'd say regular expressions are the right tools for this job, so no reason to change that.

I do have some other comments though:

I'd advice you to always use English in your code, so phoneNumber, formattedPhoneNumber etc.. I'm not a native English-speaker myself, but I never write code in any other language. Just for consistency.
You can combine the first step of sanitizing the input and checking if there is any. To avoid indenting the code, you can also just return early.
While you're at it, you may want to simplify the regexp a bit. You basically just want to replace everything that isn't a digit or a plus sign:

var input = executioncontext.getEventSource().getValue();
var phoneNumber = String(input).replace(/[^\d+]/g, '');
if(!phoneNumber) return;

The large regexp may be simplified a bit (and be made more consistent by always using \d in place of 0-9). I'd write it as follows, and use match instead of test

var match = phoneNumber.match(/^(?:\+32|0032)?0?([1-9]\d{7}|4\d{8})$/);
if(!match) return; // again we can return if the number wasn't matched

You've already got the formatting part handled well. I've changed it a little, but nothing major. I've swapped out \d for simply . (since we know we're only dealing with digits at this point) but that's not necessary - \d would do equally well.

function acm_mscrm2011_psa_FormatPhoneNumber(executioncontext) {
  var input, phoneNumber, match, formattedNumber;

  input = executioncontext.getEventSource().getValue();
  phoneNumber = String(input).replace(/[^\d+]/g, '');

  if(!phoneNumber) return;

  match = phoneNumber.match(/^(?:\+32|0032)?0?([1-9]\d{7}|4\d{8})$/);

  if(!match) return;

  if(match[1].length === 8) {
    formattedNumber = match[1].replace(/(.)(...)(..)(..)/, "+32 $1 $2 $3 $4");
  } else {
    formattedNumber = match[1].replace(/(...)(..)(..)(..)/, "+32 $1 $2 $3 $4");
  }

  executioncontext.getEventSource().setValue(formattedNumber);
}

You could also combine a couple of steps, and do

function acm_mscrm2011_psa_FormatPhoneNumber(executioncontext) {
  var input, match, formattedNumber;

  input = executioncontext.getEventSource().getValue();
  match = String(input)
    .replace(/[^\d+]/g, '')
    .match(/^(?:\+32|0032)?0?([1-9]\d{7}|4\d{8})$/);

  if(!match) return;

  if(match[1].length === 8) {
    formattedNumber = match[1].replace(/(.)(...)(..)(..)/, "+32 $1 $2 $3 $4");
  } else {
    formattedNumber = match[1].replace(/(...)(..)(..)(..)/, "+32 $1 $2 $3 $4");
  }

  executioncontext.getEventSource().setValue(formattedNumber);
}


Passing "This", but not really So i am developing an Android application as a side project/hobby and I have come across the situation that I have to pass an Activity into a method for use a context (Don't question it)

The problem here is that I have to pass it into a PreferenceOnClickListener and so I am unable to use this. I have come up with the following code snippet, that you all might just wanna stab me for writing... Sorry, I feel bad for creating this as well:

Preference rebuildDB = pf.findPreference(getString(R.string.preference_rebuildDatabase));
    Activity thisContext = this;
    rebuildDB.setOnPreferenceClickListener(new Preference.OnPreferenceClickListener() {
        @Override
        public boolean onPreferenceClick(Preference preference) {
            SongLoader.generateXml(thisContext);
            return true;
        }
    });

The thing I don't like is setting thisContext to the value of this, so my question is the following:

Is this really as bad as I think it is, and is there any better way to achive it?

You can do it in 2 way.

The first is by implementing the interface on your main class, so you don't have to create an anonymous class on the fly but just pass your this.

UPDATE

Here is an example on how to do it:

public class MyActivity extends AppCompatActivity
                        implements Preference.OnPreferenceClickListener {

        // You implement the listener method in your main
        // class, so you just pass the current this object
        @Override
        public boolean onPreferenceClick(Preference preference) {
           SongLoader.generateXml(this);
           return true;
        }

        // ... other code

       // when you record the listener your main class is the
       // listener now.
       rebuildDB.setOnPreferenceClickListener(this);

}

The second is by creating a specific class that implement the interface.

This class is going to take in the constructor a parameter that is an instance of your main class.

So you will pass a new instance of this class instead of an instance of an anonymous class.

Here is an example of what I mean.

private static class MyOnPreferenceClickListener implements Preference.OnPreferenceClickListener {

    MyActivity myActivity;

    public MyOnPreferenceClickListener(MyActivity myActivity) {
        this.myActivity = myActivity;
    }

    @Override
    public boolean onPreferenceClick(Preference preference) {
        SongLoader.generateXml(myActivity);
        return true;
    }
}

I defined private and static considering to declare this class inside your main class.

UPDATE

The fist method is the quick and fast way, not considering the anonymous listener class, but could drive on very fat classes implementing many different listeners, depends on the Activity you're writing.

The second method, helps to decouple the listeners from your business code, and organize it in a better way.


Counting ways to fit bottles in crates I'm calculating how many possibilities there are to fit \$n\$ identical bottles inside \$k\$ crates of various capacities.

n = Number of bottles
k = Number of crates
K = List of the number of bottles each crate can fit

Each crate may be filled with any number of bottles up to \$K_i\$, including being empty.

This is an instance of the Donut Shop Problem.

Example 1

Let's say we have:

7 Bottles (\$n = 7\$)
2 Crates (\$k = 2\$)
Crate 1 fits 3 bottles, crate 2 fits 5 bottles (\$K_1 = 3, K_2 = 5\$)

There are 2 possibilities to fit the bottles inside the crates.

Example 2

7 Bottles (\$n = 7\$)
3 Crates (\$k = 3\$)
Crate 1 fits 2 bottles, crate 2 fits 3 bottles, crate 3 fits 4 bottles (\$K_1 = 2, K_2 = 3, K_3 = 4\$)

6 possibilities

I have a recursive solution that works fine for small integers:

def T(n, k, K):
    if k==0: return n==0
    return sum(T(n-i, k-1, K) for i in xrange(0, K[k-1]+1))

But when I try it with \$n = 30, k= 20, K_i = i\$, it takes FOREVER, so I'm asking you: how could I improve the above code/function?

You should 'spread out' your code more, k==0 kinda looks like a variable,
and if k==0: return n==0 doesn't really look like an if statement at first.

Single letter variable names are bad, amount_bottles is better then n.
You could even use bottles and it would still be better.

T is not an allowed name for a function according to PEP8 Python's style guide.
Capital letters are only used in constants, and classes.

As for your performance concern,
I would recommend using memoization in a wrapper.
memoization remembers output, and so you re-use the output rather then re-find it.
It's really good in recursive functions.

def memoize(fn):
    outputs = {}
    def inner(*args):
        try:
            return outputs[args]
        except KeyError:
            outputs[args] = fn(*args)
            return outputs[args]
    return inner

def permutations(bottles, crates, BOTTLES_CRATE):
    @memoize
    def inner(bottles, crates):
        if crates == 0:
            return bottles == 0
        crates -= 1
        return sum(
            inner(bottles - i, crates)
            for i in xrange(BOTTLES_CRATE[crates] + 1)
        )
    return inner(bottles, crates)

print(permutations(30, 20, range(1, 21)))

This took under a second to get 6209623185136.

Just to note, memoization requires no side effects to happen.
So if you use an outside variable then that can break the memoization.

For example, if we were to memoize addition of a number plus a global variable,
it is prone to breakage.

inc = 4

@memoize
def add(num):
    return num + inc

add(4) # 8
inc = 0
add(4) # 8
add(0) # 0

If you were to want to expand on your above code,
and you make BOTTLES_CRATE a variable,
then you would have to pass BOTTLES_CRATE via the memoizer too.
Just like we should inc in the above.

@memoize
def add(a, b):
    return a + b

add(4, 4) # 8
add(4, 0) # 4

As I use a dictionary to store the arguments,
you will need to cast BOTTLES_CRATE to a hash-able type, e.g. a tuple.
This may lead to a non-transparent solution, where you can just add @memoize.

Another bad use-case is if add 'returns' from it's internals to another variable.

list_ = []

@memoize
def push(num):
    list_.append(num)

push(2) # list_ == [2]
push(2) # list_ == [2]

And so if there are possible side effects, you may want to chose a different method in future.


C/C++ code to do customized string comparing It's an interview question, I was asked to compare 2 strings, and the number in the string are evaluated to integers when comparing.

I can use C/C++ and can not use stl or stdlib. Some test cases are showed in the code.

I submitted the code below but failed. The interviewer said that the code is not clean enough. Can anybody review the code?

#include <cstdio>
#include <cmath>
bool is_num(char x) {return (x>='0' && x<='9');}

int get_num(const char* s, int len)
{
    int j = 0;
    int ret = 0;
    while (len > 0)
    {
        ret += (s[j++] - '0') * pow(10, len-1);
        len --;
    }
    return ret;
}

int my_compare(const char* s1, const char* s2)
{
    while((*s1)&&(*s2))
    {
        int value1 = 0;
        int value2 = 0;
        bool is_t1_num = false;
        bool is_t2_num = false;
        int len_t1 = 0;
        int len_t2 = 0;
        const char*  t1;
        const char*  t2;
        if (is_num(*s1))
        {
            t1 = s1;

            while(is_num(*t1)&&(*t1))
            {
                len_t1 ++;
                t1++;
            }
            value1 = get_num(s1, len_t1);
            is_t1_num = true;
        }
        if (is_num(*s2))
        {
            t2 = s2;
            while(is_num(*t2)&&(*t2))
            {
                len_t2 ++;
                t2++;
            }
            value2 = get_num(s2, len_t2);
            is_t2_num = true;
        }

        if ((!is_t1_num) && (!is_t2_num))
        {
            if ((*s1) < (*s2)) return -1;
            if ((*s1) > (*s2)) return 1;
            s1++;
            s2++;
        }

        if ((is_t1_num) && (is_t2_num))
        {
            if (value1 < value2) return -1;
            if (value1 > value2) return 1;
            s1 += len_t1;
            s2 += len_t2;
        }

        if ((is_t1_num) && (!is_t2_num))
        {
            if (value1>0) return 1;
            s1 += len_t1;
        }

        if ((!is_t1_num) && (is_t2_num))
        {
            if (value2>0) return -1;
            s2 += len_t2;
        }
    }//end while
    if (!(*s1) && !(*s2)) return 0;
    if (!(*s1)) return -1;
    if (!(*s2)) return 1;
}

int main()
{
    //TEST CASE 0
    printf("%d\n", my_compare("ab", "1"));
    printf("%d\n", my_compare("123", "456"));
    printf("%d\n", my_compare("abc", "def"));
    printf("%d\n", my_compare("abc123def", "abc000def"));
    printf("%d\n", my_compare("ab", "abc"));

    //TEST CASE 1
    printf("%d\n", my_compare("abc000def", "abcdef"));
    printf("%d\n", my_compare("abc101def", "abcdef"));

    //TEST CASE 2
    printf("%d\n", my_compare("abc101def", "abceef"));
}

First pick a language.

Either use C or use C++. Do NOT use a mixture of the two (its sloppy and can introduce bugs). So first off you should have picked a language. You seem to have picked both:

 #include <cmath>                         // C++
 printf("%d\n", my_compare("ab", "1"));   // Why use printf in C++
                                          // It is obtuse dangerous (not type safe).

The two languages have a basic similar syntax but they are different languages treat them as such.

Know what is available from the standard libraries:

bool is_num(char x) {return (x>='0' && x<='9');}

This exists in the standard library as std::isdigit()

If this is C++ why are you passing C-Strings around.

int get_num(const char* s, int len)

Much better to write:

int get_num(std::string const& s)

Also there is no point in writing this function (especially since it is error prone to do so.

In c++ use operator>> to stream to an int.
In C use sscanf(s, "%d%n", &value, $length).

Also your variable names are bad. Pick names that make it easy to read the code and understand what you are doing.

    int j = 0;
    int ret = 0;
    while (len > 0)
    {
        // Why add the extra complexity of `j++` here.
        // You have made the expression hard enough to read as it is.
        // You don;t get points for being clever in an interview its readability
        // we want to see.
        ret += (s[j++] - '0') * pow(10, len-1);
        len --;
    }
    return ret;
}

Yes in C it is traditional to put all the variables at the top of the function (so fine for C). But in C++ this is considered bad practice and sloppy. Declare variables as close to the point you want to use them (preferably just before you use them). That way you can see the type at the point you are using them.

        int value1 = 0;
        int value2 = 0;
        bool is_t1_num = false;
        bool is_t2_num = false;
        int len_t1 = 0;
        int len_t2 = 0;
        const char*  t1;
        const char*  t2;

You are repeating the same processes twice.
First you scan over the string finding the length of the number.
Second you scan over the string calculating the value of the number.
There is no need to do that. Use one pass.

            while(is_num(*t1)&&(*t1))
            {
                len_t1 ++;
                t1++;
            }
            value1 = get_num(s1, len_t1);

Also you have the same boilerplate code for both t1 and t2. If you have code that does the same thing factor it out into a function.


Find a Sum of Particles in A Set of Boxes Given a Number of Rows and Columns This is a code challenge, from Codewars, the details of the specific challenge in depth can be found here: https://www.codewars.com/kata/magnet-particules-in-boxes/javascript.

The summary of it though is a function which gives a sum of particles in a set of boxes given a number of rows and a number of columns to work with. You are given a specific function that calculates the number of particles in the box based off of the specific box that is being calculated. Here is the equation:

$$ v(k,n) = \frac{1}{k(n + 1)^{2k}} $$

$$
doubles (k_{max}, n_{max}) = \sum_{k=1}^{k_{max}} \sum_{n=1}^{n_{max}} v(k,n)
$$

Here is the function I came up with:

function doubles(maxk, maxn) {

  let total = 0

  for(let k = 1; k <= maxk; k++){
    const twoK = 2*k
    for(let n = 1; n <= maxn; n++){
      total += 1/(k*Math.pow(n+1, twoK))
    }
  }

  return total
}

My function does the job perfectly, and comparing it to other responses marked as 'best practices' it is very similar. But I would like to know if anyone sees obvious ways to improve my function, or I would really love to see something that can improve the time complexity of the function, if that is possible.

Inside the inner loop, \$1/k\$ is a constant term. So you could extract that multiplication from the inner loop, and apply it on the computed subtotal. This won't change the order of complexity though.

I was wondering if there is a closed form for computing the sum of reciprocal powers (to replace the summing and thereby speed things up), but I couldn't find.

I was also wondering if you inverted the direction of computations to sum by columns instead of summing by rows, would there be a closed form to replace the summing, but I couldn't find that either.

Other than some clever math optimizations, the computation by rows or columns could run in parallel, but the input parameters would have to be large enough to be worth the overhead.


Efficient way to extract the first row in a Group By group I have a large Sql Server view with this schema:

[archive_ID] (int, not null)
[archive_date] (datetime, not null)
[archdata_path_ID] (varchar(50), not null)
[archdata_value] (int not null)

I need to group the records by the Date, and I need to extract just the first record for each group.

This is the current query:

WITH cte
AS (
        SELECT *
               ,CAST(archive_date AS DATE) AS C
               ,ROW_NUMBER() OVER (
                       PARTITION BY CAST(archive_date AS DATE) ORDER BY CAST(archive_date AS DATE) ASC
                       ) AS ad
        FROM ArchiveData
        WHERE archdata_path_ID = @PathID
        )
SELECT DISTINCT C
        ,archdata_value AS val
FROM cte
WHERE ad = 1
ORDER BY C ASC

The main problem is to improve the readability.
Would be great to optimize also the performance, but it's not mandatory.

I believe that DISTINCT is redundant, since the CTE should produce only one row for each date whose ROW_NUMBER() is 1.

Avoid selecting * in the CTE, and list the columns you want explicitly.

Your naming is poor: CTE, C, val, ad.  Please find more descriptive names.

If you are using any SQL Server ≥ 2012, then FIRST_VALUE() is the function you want.

SELECT CAST(archive_date AS DATE) AS C
     , FIRST_VALUE(archdata_value) OVER (
           PARTITION BY CAST(archive_date AS DATE)
               ORDER BY CAST(archive_date AS DATE)
       ) AS val
    FROM ArchiveData
    WHERE archdata_path_ID = @PathID
    ORDER BY C;


Custom password hashing I have been creating a class for users on my account. OOP and classes are not my strong point.

Below is an extract from my class which I use to create a hash (that will be stored in a database) and the second function is used to check a value against a hash.

I know that I should just use bcrypt for my hashing but I was experimenting with adding a cost factor to other hashing algorithms.

// cryptography settings
private $cost = 10; // the amount of times to run the hashing algo
private $algo = "bcrypt"; // the hash to use on the passwords
private $salt = "mySalt"; // put whatever function you want here to create a random salt
private $pepper = "myPepper"; // a pepper is a secret key not stored anywhere (except here)

/**
 * Use the specified hash in order hash the password
 * @param  string $password The password in need of being hashed
 * @return string           The hashed version of the password
 */
public function hashPassword($password)
{
    $this->algo = strtolower($this->algo); // lowerise the algo
    $hash = ""; // the hashed password
    $password .= $this->salt . $this->pepper; // add salt and pepper to the password

    // BCrypt
    if ($this->algo == "bcrypt")
        $hash = password_hash($password, PASSWORD_BCRYPT, ["cost" => $this->cost]);

    // hash algorithms
    else if (in_array($this->algo, hash_algos()))
    {
        $hash = hash($this->algo, $password, false); // hash the password
        // loop to hash the password $cost times
        for ($i = 0; $i < $this->cost; $i++)
            $hash = hash($this->algo, $hash, false);
    }

    // the hashed password
    return $hash;
}

/**
 * Check if the hashed password matches hash
 * @param  string $pass The password that is in need of hashing
 * @param  string $hash The already hashed password
 * @return bool         $pass == $hash
 */
public function checkHash($pass, $hash)
{
    // cannot have anything other than integer to loop
    if (!is_int($this->cost))
        throw new Exception("Cost must be an integer.", 1);

    // use lowercase for if statement
    $this->algo = strtolower($this->algo); // lowerise the algo
    $success = false; // if hash matches password hash
    $pass .= $this->salt . $this->pepper; // add salt and pepper to the password

    // BCrypt
    if ($this->algo == "bcrypt")
        $success = password_verify($pass, $hash);

    // hash algorithms
    else if (in_array($this->algo, hash_algos()))
    {
        $temp = hash($this->algo, $pass, false); // hash the password
        // do same amount of looping
        for ($i = 0; $i < $this->cost; $i++)
            $temp = hash($this->algo, $temp, false);

        // check if the new hash equals the original hash
        $success = $temp == $hash;
    }

    return $success;
}

The two functions can be used as such. Note that $user is an instance of my class.

$hash = $user->hashPassword("test"); // might produce: $2y$10$lVSgcHrtuWmRIbRryAlXE.gJQZZhFKjI49RI5bnm.4k9DyV0uqHUi

The above will be used so that I can insert that into the database.
Then I would check when logging in using the below.

// get the hash from the database
$query = $this->conn->prepare("SELECT password FROM users WHERE username = ?");
$query->bind_param("s", $_POST["username"]);
$query->execute();
$query->store_result();
$query->bind_result($hash);
$query->fetch();

// check the hash against the posted password
if ($user->checkHash($_POST["password"], $hash))
    echo "You are logged in";
else
    echo "Login failed";

Firstly, is this a good idea or should I just use password_hash? The reason I decided to create these methods was in case my passwords got compromised and then I could easily change the hashing algorithm and ask users to change their passwords.

Secondly, am I checking the password correctly? That is a lot of lines to extract the hash. It seems wrong but I don't know any other way.

Security

General Approach

Your reason for this approach are not reasonable. The only reason to provide a different hash function is to deal with systems which do not support password_hash. And really, those systems should be updated.

You definitely do not want to switch to a weaker hash function after a breach. You can ask users to change their passwords via other mechanisms.

tl;dr: Yes, just use bcrypt.

Salt vs Pepper

Based on your comments, you seem to understand the difference between a salt and a pepper. But your code doesn't seem to differentiate, both your salt and your pepper are a pepper (this is also not as easy to fix as "replace this string with a random function", as you not only need to generate the salt, but also store and retrieve it).

A salt should be unique for each hash, so that it actually slows down attacks.

Bcrypt usage

The bcrypt part of your approach is fine; you are using password_hash and password_verify correctly.

Note that bcrypt actually handles salts for you though, so adding your own salt is unnecessary. As bcrypt can only handle passwords up to 72 characters, your approach may actually be harmful, as a long salt + password + pepper may cut of (parts of) the pepper. I would at least comment on this, but ideally, just remove the salt for bcrypt.

Custom Implementation: Empty Hash

If you set your $algo to something invalid, you will - silently - store an empty string instead.

Currently, you actually return false on the check, so it's not catastrophic, but it can lead to usability issues. I would throw an exception here.