PowerShell - Loop Through Nested Objects I'm writing a PowerShell script to loop through an organization's PowerBI workspaces and spit out the report IDs, names, and URLs for each workspace - so, a nested loop.

Here's my working script thus far:

Install-Module -Name MicrosoftPowerBIMgmt

$myArray = @()

Login-PowerBIServiceAccount
Get-PowerBIWorkspace -Scope Organization | ForEach-Object {
        $ws = $_
        Get-PowerBIReport -WorkspaceId $_.Id | ForEach-Object {
            $x = [pscustomobject]@{
                workspaceId = $ws.Id
                workspaceName = $ws.Name
                reportId = $_.id
                reportName = $_.Name
                reportUrl = $_.WebUrl
             }
        $myArray += $x
        }
}

$myArray | Export-Csv "PowerBIWorkspaceReports.csv"

Four questions:

1) Is there a better way to add the module dependency at the top? I can't find any info on how PowerBI looks for existing module dependencies.

2) Are the ForEach-Object calls redundant? I see in simple code people assume whatever command runs after the pipe runs just fine whether on a single object or a collection of objects.

3) Is there a better way to manage my objects? Since I need data from the first loop ( the Workspace name and ID) and the second loop, is there an easier way to join them (or export them as a flattened list) than looping?

4) Should I just append my rows directly to the CSV? There doesn't seem to be the equivalent of While(File.Open() ...) in PowerShell.

And any other advice appreciated.

I rejigged your code a little to make it tidier. I don't have PowerBI, so I haven't tested it, and I can't guarantee that I haven't done anything dumb.

I got rid of the nested loop. Note the -PipelineVariable ws. What that does is put the output of that stage of the pipeline into a variable called $ws, which we can refer to later. It means that we don't need to do $ws = $_.

-PipelineVariable is one of the "Common Parameters", and is available on many or most cmdlets. You can abbreviate it to -PV, by the way.

I also got rid of the intermediate array $myArray since it wasn't needed.

Install-Module -Name MicrosoftPowerBIMgmt

Login-PowerBIServiceAccount

Get-PowerBIWorkspace -Scope Organization -PipelineVariable ws |
    ForEach-Object { Get-PowerBIReport -WorkspaceId $_.Id } |
    ForEach-Object {
            [pscustomobject]@{
                workspaceId   = $ws.Id
                workspaceName = $ws.Name
                reportId      = $_.id
                reportName    = $_.Name
                reportUrl     = $_.WebUrl
             }
        } |
    Export-Csv "PowerBIWorkspaceReports.csv"


Produce all multiples of a set of primes up to a certain limit I have some Python code that generates all possible multiples and combinations of a given set of primes up to a certain limit. The code runs and does exactly what I intend it to do, now I am trying to optimize it in order to improve run-time.

import math
from collections import defaultdict
import itertools

lim = 10**13 #This limit can be changed if you'd like
primes = [2,3,5,7,11,13,17,19,23,29,31,37,41,43,47] #So can these primes
res = defaultdict(list)
res[()] = [1] #Maybe not this

for n in range(1, len(primes) + 1):
    for combo in itertools.combinations(primes, n):
        for c in res[combo[:(n - 1)]]:
            stop = int(math.log(lim / c, combo[-1]))
            for x in range(1, stop + 1):
                res[combo[:n]].append(c * combo[-1] ** x)

multiples = []
for key in res:
    multiples += res[key]

multiples.sort()
print(multiples)

If you don't feel like reading through all that, the basic idea is to take increasingly large combinations of the set of primes, starting with a single prime, and computing all the permutations of the subset with increasing powers up to the chosen limit. The way I have it coded, I am initializing the dictionary with an empty tuple that maps to the number 1 as it makes writing the rest of the for loop much easier.

For example: Let's say your list of primes is \$[2,3,5]\$ and your limit is 30. First, it's going to take each number in that list and compute all the powers below 30, storing each in a dictionary key that maps to a list. So, after one loop, you would have:

res = {(): [1], (2):[2,4,8,16], (3):[3,9,27], (5):[5,25]}

The second time through the combinations would be \${2 \choose 3}\$, \${2 \choose 5}\$, and \${3 \choose 5}\$. In each combo, it chops off the last value and searches for the key associated with the remaining part of the tuple.  So for \${2 \choose 3}\$ it will look for the (2) key, i.e. the powers of two, and multiply them by increasing powers of three until they go above thirty.  So you will have:

\${2 \choose 3}\$ = \$[2 \cdot 3, 2 \cdot 9, 4 \cdot 3, 8 \cdot 3]\$ = \$[6, 18, 12, 24]\$

This process continues until you are out of combinations.

For lower limits the code actually runs fairly quickly.  For high limits (\$10^{13}\$, 20 primes) like I have listed above the code runs in about 5 minutes. I would like to get that down to under a minute if possible. I was wondering if I could turn the main for loop into a dictionary comprehension but I'm not sure if that's possible since I've initialized res outside of the main for loop, and need the () = [1] for my code to run.

I am not completely married to this idea, but I do think it's slick (I'm still pretty new to programming so we might have different ideas on what that means), so if I could preserve it that would be nice. Any ideas?

Give variables better names, exponent is better than x.
List comprehensions make your code more understandable.
You can use dictionary.values() to get a list of the values.
You should make this a function. If you can change; limit, and primes.
You're not just getting multiples, you're also getting exponents too. So you may want a better function / variable name.

This can get you:

from collections import defaultdict
from itertools import combinations
from math import log

def prime_multiples(limit, primes):
    res = defaultdict(list)
    res[()] = [1]

    for n in range(1, len(primes) + 1):
        for combo in combinations(primes, n):
            base = combo[-1]
            res[combo] = [
                multiple * base**exponent
                for multiple in res[combo[:-1]]
                for exponent in range(1, int(log(limit / multiple, base)) + 1)
            ]

    multiples = [i for val in key.values() for i in val]
    multiples.sort()
    return multiples

print(prime_multiples(10**13, [2,3,5,7,11,13,17,19,23,29,31,37,41,43,47]))

It's 5AM, and I've not tested the following in anyway.
But you could probably change the code to use the modulo operator, %.
To check if the number is devisable.
And so if you write a generator function, you may be able to use:

def prime_multiples(limit, primes):
    yield 1
    for num in range(1, limit):
        if any(num % prime == 0 for prime in primes):
            yield num


Printing out Armstrong numbers This is my first Java program to print out the Armstrong numbers.

  An Armstrong number is a number that is the sum of its own digits each raised to the power of the number of digits.

I want to get better, so be harsh and straightforward.

public class Armstrong {

    public static void main (String arg[]){

        int Max=2000000000;
        for (int i=0; i<Max; i++){
            if (isarmstrong(i)){
                System.out.println(i);
            }

        }
    }

    private static boolean isarmstrong(int x) {
        int n=x;
        if (n == getdig(n)){
            return true;
        }
        else return false;

    }

    private static int getdig(int sp){

        int d=sp;
        int r=0, dg=0;
        int length = String.valueOf(sp).length();
     if (length>1){
        for (int count=length; count>=0; count--){

            dg = (int) (d/(Math.pow(10, count)));
            d= (int) (d % (Math.pow(10, count)));
            dg =(int) Math.pow(dg, length);
            r = r + dg;
        }

        }
     else {
         r=sp;
     }
     return r;

        }
}

Your indenting is weird. In getdig, you deindent, then reindent.

Variables should always be camelCase or, if they're constants, UPPER_SNAKE_CASE -- int Max should be int max or final int MAX, depending on if you want to make it clear that it's a constant. I also think it should be a class variable, not a local variable in a method.

Put a space on either side of your binary (two-argument) operators -- so int n=x becomes int n = x, i<max becomes i < max, etc. Note that things like ++ and ! are not binary, since they only take one argument.

Method names are camelCase as well, where the humps are on word boundaries. isarmstrong should be isArmstrong.

Use meaningful names! sp, getdig, n, sp -- none of these mean anything. It's fine to have long names -- don't use any abbreviations but the most obvious ones. Id sit, getIP would be fine; you don't need to write getInternetProtocolAddress.

if (/* condition */) {
    return true;
}
return false;

is a pretty common way to add lots of characters with no meaning. Just write

return /* condition */;

replacing /* condition */ with the actual code. In this case, that's n == getDig(n).

You have a bunch of lines with nothing but whitespace; the convention I've seen is to separate methods and logically distinct blocks of code. You have a bunch of whitespace just before the method ends, or just after it begins.

In isArmstrong, you assign n to be equal to x, then never use x again. Why not ditch n and just use the original variable?

In getdig's for, a couple of things.

Firstly, a = a + b can be shortened to a += b and it's understood what that means.

Secondly, since none of the other code relies on the changing of d -- in fact, you moved one line to keep it from interfering -- why not visually separate it by moving it to the bottom and plopping a blank line between?

As for your actual math, though, it looks good. It's rather hard to tell, since your names mean absolutely nothing, but no matter.

With all of these applied (except the better names -- you should do that yourself, since you know what they are) here's how your code looks:

public class Armstrong {
    public static void main (String arg[]){
        int max = 2000000000;
        for (int i = 0; i < max; i++){
            if (isArmstrong(i)){
                System.out.println(i);
            }
        }
    }

    private static boolean isArmstrong(int n) {
        return n == getDig(n);
    }

    private static int getDig(int sp){
        int d = sp;
        int r = 0, dg = 0;
        int length = String.valueOf(sp).length();
        if (length > 1){
            for (int count = length; count >= 0; count--){
                dg = (int) (d / Math.pow(10, count));
                dg = (int) Math.pow(dg, length);
                r += dg;

                d = (int) (d % Math.pow(10, count));
            }

        }
        else {
            r = sp;
        }
        return r;
    }
}


String processing in JavaScript We have a lot of phone numbers that contain dashes and start with a zero which are from another country.

I am trying to strip the dashes and the one leading zero and select the country code from a drop-down box.

I'm using "raw" JS with DOM here (for the sake of not having to pull in a framework):

<html>
<head><title>Dash Remover</title></head>
<body>
<form method="POST">
<select id="countryCallingCode">
    <option value="0043">+43</option>
    <option value="0044">+44</option>
    <option value="0049">+49</option>
    <option value="">Other</option>
</select>
<input id="numbertodial" type="text" size="16"
       oninput="prepareTelNo()" />

<button name="callBtn" onclick="callTelNo();">Call!</button>
</form>

<script>
function getCCC() { return document.getElementById('countryCallingCode').value; }

function prepareTelNo() {
    var wd = document.getElementById('numbertodial');
    wd.value = wd.value.replace(/-/g, ''); // strip - from the tel. no.

    // Cut off leading zeros
    if(wd.value.charAt(0) === '0' & getCCC() != "")
        wd.value = wd.value.substring(1);
}

function callTelNo() {
    var telNo = document.getElementById('numbertodial').value;
    window.alert('Calling '+getCCC()+telNo);
}
</script>
</body>
</html>

Any room for improvement? I know that oninput is only supported in some browsers.

That's ok, it's for an intranet app that is usually used with IE 10.

Just a few points

You're missing a & in your if conditional. Right now you're doing a bitwise AND, rather than a logical AND (which would be &&).
I'd say you should always use curly braces, even for one-liners. JS accepts one-liners just fine, but as a code-hygiene thing it's better to have braces there, I think.
You could rewrite the reg exp to remove everything that isn't a numerical digit (instead of only removing dashes). I don't know if that's what you need, but if it is

wd.value = wd.value.replace(/[^\d]/g, ""); // remove all non-digit characters

You also could use a little more reg exp to make sure you remove all leading zeros (even if you say there's only going to be 1). Incidentally, this allows you to skip the "starts with zero" check, and only have the getCCC() != "" check. Point is, you can do a replace for leading zeros specifically, rather than "blindly" chopping something off of the string.

function prepareTelNo() {
    var wd = document.getElementById('numbertodial');
    wd.value = wd.value.replace(/-/g, ''); // strip - from the tel. no.

    // Cut off leading zeros if a country code is set
    if(getCCC() != "") {
        wd.value = wd.value.replace(/^0+/, ""); // cuts off all leading zeros
    }
}

If you really, truly only want to remove 1 leading zero rather than all of them, use /^0/ (without the plus sign) as the reg exp pattern.


Int stack implementation #include <stdio.h>
#include <stdlib.h>

struct Stack {
  int value;
  int is_empty;
  struct Stack *next;
};

int pop(struct Stack **stack) {
  if ((*stack)->is_empty == 1) {
    printf("Stack is empty!\n");
    abort(); // not sure what this does
  } else {
    int value = (*stack)->value;
    // outer pointer (static) points to new stack now
    *stack = (*stack)->next;
    return value;
  }
}

void push(struct Stack **stack, int value) {
  // malloc is used so this memory can be accessed outside of scope
  struct Stack *head = malloc(sizeof(struct Stack*));
  if (head != NULL) {
    head->value = value;
    // head's next is pointer to stack
    head->next = *stack;
    head->is_empty = 0;
    // stack now holds pointer to head
    *stack = head;
  } else {
    printf("push memory no alloc");
  }
}

struct Stack *empty() {
  struct Stack *head = malloc(sizeof(struct Stack *));
  if (head != NULL) {
    head->value = 0;
    head->is_empty = 1;
    head->next = NULL;
    return head;
  } else {
    return NULL;
  }
}

int main() {
  struct Stack *init = empty();
  struct Stack **stack = &init;
  push(stack, 3);
  push(stack, 2);
  push(stack, 1);
  int one = pop(stack);
  printf("one: %d\n", one);
  int two = pop(stack);
  printf("two: %d\n", two);
  int three = pop(stack);
  printf("three: %d\n", three);
  //int hmm = pop(stack);
}

I'm not sure pointers to pointers is the best way to do this. I also feel like I'm writing code in a Java-y way, and would like to know how to improve my C style.

Also, are there any edge cases I am missing?

Things you did well:

Overall the code looks very nice and well organized.
You used comments well.

Things you could improve:

Syntax:

typedef your structs.

struct Stack {
  int value;
  int is_empty;
  struct Stack *next;
};

The typedef means you no longer have to write struct all over the place. That not only saves some space, it also can make the code cleaner since it provides a bit more abstraction.

typedef struct
{
    int value;
    int is_empty;
    struct Stack *next;
} Stack;

You can simplify your NULL checks.

if (head != NULL)

Since NULL is defined as (void *)0, we can treat is as a comparison to 0, making the != NULL a redundant check.

if (head)

If you don't take in any variables as parameters, you should declare them as void.

int main(void)

Error handling:

Don't use abort().  The function raises the SIGABRT signal. This, if uncaught, causes the program to terminate returning a platform-dependent unsuccessful termination error code to the host environment. The program is terminated without destroying any object and without calling any of the functions passed to atexit or at_quick_exit.  It would be much better to return an error indicator and have the calling function handle it.

return -1; // we have an error

Memory usage:

You never free() your init value.

struct Stack *init = empty();

// within empty()
struct Stack *head = malloc(sizeof(struct Stack *));
return *head;

You should probably free() it before your program terminates.  Failure to deallocate memory using free() leads to buildup of non-reusable memory, which is no longer used by the program. This wastes memory resources and can lead to allocation failures when these resources are exhausted.

free(init);

Returning:

You don't return a value from main(), even though you state you do.

int main() {
    // no return value... no indication of successful exit
}

Returning 0 is usually a typical indication of a successful exit.

return 0;


Solving the N-queens puzzle the Racket or Scheme way My final version is a direct translation from Python. Thanks to build-in support for generator, its speed is almost the same as Python. As I quite like using generator and list comprehension in Python, I am not sure I get the right way in Racket.

Moreover, I've only found explicit support for generator in Racket these days, other scheme languages like MIT scheme and chez scheme seem not. So maybe there are some better way or more "schemeic" way to solve this problem and I am very curious about it.

This is my racket code, if needed:

#lang racket
(require racket/generator)

(define (safe? x y sln)
    (if (null? sln)
        #t
        (let ((px (caar sln)) (py (cadar sln)))
            (if (or (= y py) (= (- py y) (- px x)) (= (- py y) (- x px)))
                #f
                (safe? x y (cdr sln))))))

; input-> n, output-> a generator that yields
;            every possible list of n queens' coordinates
(define (nqueen n)
    (let recur ((x (- n 1)))
        (generator ()
            (if (= x -1)
                (yield '())
                (for* ([sln (in-producer (recur (- x 1)) (void))]
                       [y   (range n)])
                    (and (safe? x y sln)
                         (yield (cons (list x y) sln)))))
        (void))))

(for ([e (in-producer (nqueen 8) (void))])
  (displayln e))

Not sure if this is "the Racket way" but this one is faster, using Racket's sets and for loops:

(define (queens n)    ; n-queens, see http://en.wikipedia.org/wiki/Eight_queens_puzzle

  (define res null)   ; accumulator for results

  (define (s lst diags1 diags2 row columns)
    (if (= row n)
        (set! res (cons lst res))        ; got a result, remember it
        (for ((col (in-set columns)))    ; for every column left
          (define diag1 (+ col row))     ; compute diagonal 1
          (define diag2 (- col row))     ;     and diagonal 2
          (unless (or (set-member? diags1 diag1) (set-member? diags2 diag2)) ; if diagonals not already taken
            (s (cons (cons row col) lst)
               (set-add diags1 diag1)
               (set-add diags2 diag2)
               (add1 row)
               (set-remove columns col))))))

  (s null (set) (set) 0 (apply set (range n))) ; initial call
  res)

(time (for-each displayln (queens 8)))


SFINAE with several condition [Refactoring] How can I write this function shorter?

template<typename U, typename = typename std::enable_if<std::is_fundamental<U>::value>::type,
    typename = typename std::enable_if<!std::is_same<char, U>::value>::type,
    typename = typename std::enable_if<!std::is_same<void, U>::value>::type
>
void multiply(U&& u){};

I want exclude several type like void or char, which don't must be number. I wrote code like this, but I think, that is better solution. (code of course is correctly)

Seems short enough to me.

It's impossible to know how to refactor any code without first knowing what it is supposed to do. All we know about this no-op function is that you "want [to] exclude several type[s] like void or char". How many types? Which types? Why?

The obvious shortening is

template<class U, class = std::enable_if_t<std::is_fundamental_v<U>>,
    class = std::enable_if_t<!std::is_same_v<char, U> && !std::is_same_v<void, U>>
>
void multiply(U&&) {}

i.e. just remove all the superfluous ::type and ::value, and replace typename by class because it's shorter (and you did ask for us to make it shorter!).

Your naming the function multiply implies that maybe what you want is

template<class U>
auto multiply(U&& u) -> decltype(u*u, void()) {}

Your comments about "don't must be number" imply that maybe what you want is

template<class U, class = std::enable_if_t<std::is_arithmetic_v<U>>>
void multiply(U&&) {}

But we really can't know for sure unless you tell us what the code is supposed to do.


Exception-handling optimisation I have posed another question here and I am writing this one for some code review:
class ConfigFile
{
private:
  PTree m_ptConfig;

public:
  ConfigFile(const std::string& name)
  {
    json::read_json(name, m_ptConfig);
  }

  ~ConfigFile();

  double getVal1()
  {
    std::string path = "path.to.val1";
    double val1 = m_ptConfig.get<double>(path, -1);

    if (val1 < 0)
    {
      std::string msg = "getVal1() failed or val1 < 0";
      throw MyConfigFileException(msg);
    }

    return val1;
  }
  double getVal2() {/*the same as before*/};
  // ... more values the same way
};

The exception class is here:
class MyConfigFileException : public MyIOException // MyIOException is derived from std::exception
{
private:
  std::string m_msg;

public:
  MyConfigFileException(const std::string msg) : msg(msg) {}
  virtual const char* what() const throw()
  {
    return m_msg.c_str();
  }
};

The handling for this kind of exception I have done it in main function:
int main(int argc, char **argv)
{
  const std::string configFileName = argv[1];
  try
  {
    RunAlgo algo(configFileName);
    algo.runAlgorithm();
  }
  catch (MyConfigFileException& mcfe)
  {
    std::cout << "Config File Exception: " << mcfe.what() << std::endl;
    return -1;
  }
  catch (MyIOException& mioe)
  {
    std::cout << "IOException: " << mioe.what() << std::endl;
    return -2;
  }
  catch (std::exception& e)
  {
    std::cout << "Unknown exception: " << e.what() << std::endl;
    return -3;
  }

  return 0;
}

I use the chatch blocks to see if it has crashed because of me or not, that is why I just print the message and return a value. (Maybe this is not necessary since it returns a different value, but I let it there for the moment)
In the runAlgorithm() function I have a loop, where I treat another kind of exceptions:
// ...
void RunAlgo::runAlgorithm()
{
  ConfigFile cf("config.json")
  double v1 = cf.getVal1();
  // ...
  for (int i = 0; i < 100; i++)
  {
    // ...
    try
    {
      foo(cf);
    }
    catch (MyOtherExceptions & moe)
    {
      std::cout << moe.what() << std::endl;
    }
    // ...
  }
  // ...
}

MyOtherExceptions are going to be treated separately, like: for the current iteration, there is a problem with a buffer, send a message to a queue.
foo is implemented here:
void foo(const ConfigFile& cf)
{
  double v2 = cf.getVal2();

  // some other code that may throw MyOtherException exceptions
}

Do you think I am using the exceptions correctly?

I admit that the handling is not really done, but I am asking if I am doing right with the throws and try-catch blocks, supposing that instead of just printing messages the handling is done (as "sending message to a queue").

That is not really exception handling:

In your main() method:

You are branching on three different types of exceptions, but your action is the same: Print a message.
Doing so is fine, but the point for different catch blocks is not obvious to me.
Anyway:

Use std::cerr instead of std::cout for errors.
return 1 (or any value different from 0) if your program fails.

In runAlgorithm()

Generally you should only catch exceptions you want to handle.
And printing a message is not really handling an exception, that is more like cheap debugging.

You are catching the MyOtherException exception and ignoring it.
Why? Is it irrelevant to the algorithm? If so, why print a message? If not, why continue the algorithm? You might want to move the catch block out of the loop if it is fatal, or possibly remove it at all if you can't deal with it.

As a rule of thumb: Only catch an exception if

you can do something about it, this might be trying a different way to achieve the goal of your function
you want to wrap it inside a more appropriate exception, for example you could catch a low level io exception and wrap it in a parse exception explaining in more detail what went wrong
it is irrelevant to the calling function
it is the main method. C++ behaves ugly on uncaught exceptions.
But you should probably return 1 instead of 0.


Styling android widgets programmatically I have code which styles standard Android widgets programmatically (not from XML). The code is working, but I have this bunch of if instanceof else if instanceof block which is ugly.

I have read discussions on Stack Overflow, and my conclusion is instanceofis better on my case, but I'm not so sure, maybe I'm missing something?

public static void styleAllControls2(Context c, List<View> controls, CxpResources cxpr) {
    if (cxpr == null) {
        return;
    }
    File xfiles = c.getExternalFilesDir(null);
    if (xfiles == null) {
        Log.e(TAG, "couldnt get external files dir!");
        return;
    }
    Resources res = c.getResources();
    String filepath = xfiles.getAbsolutePath() + File.separator;
    for (View view : controls) {
        ViewModifier vModfr = null;
        if (view instanceof RatingBar) {
            // viewModifier = new RatingBarModifier(res, cxpr, filepath);
        } else if (view instanceof SeekBar) {
            vModfr = new SeekBarModifier(res, cxpr, filepath);
        } else if (view instanceof ProgressBar) {
            vModfr = new ProgressBarModifier(res, cxpr, filepath);
        } else if (view instanceof CheckBox) {
            vModfr = new CheckBoxModifier(res, cxpr, filepath);
        } else if (view instanceof RadioButton) {
            vModfr = new RadioButtonModifier(res, cxpr, filepath);
        } else if (view instanceof Switch) {
            vModfr = new SwitchModifier(res, cxpr, filepath);
        } else if (view instanceof ToggleButton) {
            vModfr = new ToggleButtonModifier(res, cxpr, filepath);
        } else if (view instanceof Button) {
            vModfr = new ButtonModifier(res, cxpr, filepath);
        } else if (view instanceof Spinner) {
            vModfr = new SpinnerModifier(res, cxpr, filepath);
        } else if (view instanceof EditText) {
            vModfr = new EditTextModifier(res, cxpr, filepath);
        }
        if (vModfr != null) {
            vModfr.modify(view);
        }
    }
}

ViewModifier class:

public abstract class ViewModifier<T extends View> {
    private static final String TAG = "ViewModifier";
    protected final CxpResources cxpResources;
    protected final String basedir;
    protected final Resources resources;
    protected final TypedValue typedValue;

    public ViewModifier(Resources resources, CxpResources cxpResources, final String basedir) {
        this.resources = resources;
        this.cxpResources = cxpResources;
        this.basedir = basedir;
        typedValue = new TypedValue();
        typedValue.density = resources.getDisplayMetrics().densityDpi;
    }

    public abstract void modify(T v);

    // some helper codes....
}

All xxxModifier class inherit from ViewModifier class.

My questions:

Is a bunch of instanceofs really the best way? As far as I know, I can't use visitor pattern because I can't modify Android widgets.
Am I using generics right? Particularly in ViewModifier class signature.
Which is better, my current implementation, or to make modify an interface and then have xxxModifier classes implement that? ViewModifier has a bunch of helper methods though.
Review of my code in general, according to Android and Java best practices and conventions.
Intellij Idea / Android Studio tricks to help self-review perhaps?

I already answered some questions in a comment here is the code review.

Some tips and suggestions:

Use full names for params and local variables to improve readability
Don't use numbers in the names of methods
Try to use Iterable interface above the List if possible
Follow naming conventions for Android
Use annotations to improve code quality and usage
Use final modifier if possible

'

public static void styleControls(@NonNull final Context context, @NonNull final Iterable<View> controls, @NonNull final Object object) {
    if (object == null) {
        return;
    }
    final File file = context.getExternalFilesDir(null);
    if (file == null) {
        Log.e(TAG, "couldnt get external files dir!");
        return;
    }
    final Resources resources = context.getResources();
    final String filepath = file.getAbsolutePath() + File.separator;
    for (final View view : controls) {
        final Modify modify = null;
        //... instance of checks
        if (modify != null) {
            modify.modify(view);
        }
    }
}

public abstract class ViewModifier implements Modify {

    private static final String TAG = "ViewModifier";

    protected final Object     mObject;
    protected final String     mBasedir;
    protected final Resources  mResources;
    protected final TypedValue mTypedValue;

    public ViewModifier(@NonNull final Resources resources, @NonNull final Object object, @NonNull final String basedir) {
        mResources = resources;
        mObject = object;
        mBasedir = basedir;
        mTypedValue = new TypedValue();
        mTypedValue.density = resources.getDisplayMetrics().densityDpi;
    }

    // some helper codes....
}

public interface Modify<T extends View> {
    void modify(final T t);
}


Find if cookie name exists in the cookie string I need to determine whether a cookie name is available or not in the cookie string. I have achieved this. We could use the cookie-parser package but I don't want to use that package so I have written the below code. Can I reduce the code in a better way that is more optimised?

    function checkCookie(cookie, cookieToBeSearched){
                if(cookie === "" || cookie === undefined){
                    return false
                }
                let res = cookie.split(";").some(cookie => {
                    let eachCookie = cookie.split("=");
                    return eachCookie[0].trim() === cookieToBeSearched
                });
                return res;
            }

    let cookie = "_ga=GA1.2.2091695351.1539084164; __qca=P0-338702612-1539084164095; __gads=ID=770d92bcdac8de40:T=1539084164:S=ALNI_MbsRKpoSJdn8tsdShMHMZUAR17uZA; _gid=GA1.2.798724103.1539582973";

    console.log("Cookie is available - ", checkCookie(cookie, "_gid"))

    console.log("Cookie is available - ", checkCookie(cookie, "_giddd"))

Option 1: Same functionality, smaller size

  Can I reduce the code in a better way more optimised?

If you want to optimize for size, then here's a suggestion for a more compact version:

function checkCookie(cookies, name) {
    return (cookies || '')
        .split(/;\s*/)
        .some(cookie => cookie.split('=', 1)[0] === name)
}

(cookies || '') produces the same function result as the original if(…) return false
split uses a regular expression that matches the space after the semicolon, saving the trim in the lines below
the assignments for eachCookie and res can be removed, leading to a single line some call

Benefits: short & concise

Limitations: only checks for existence of a cookie, not its value

Option 2: More versatility, similar size

There is an opportunity for a different design that allows for more generic use cases: you can first parse the cookie string into a plain object and then check for the existence of the cookie in question.

function parseCookie(cookie) {
    return (cookie || '')
        .split(/;\s*/)
        .reduce((result, entry) => {
            const [key, value] = entry.split('=', 2)
            result[key] = value
            return result
        }, {})
}

function checkCookie(cookies, name) {
    return Boolean(parseCookie(cookie)[name])
}

Granted, this a solution may be a bit "slower" than the original implementation. This shouldn't be an actual issue unless you intend to parse millions of cookie strings and time is a constraint.

What you gain in return though is a lean, readable, reusable function that lets you check for the existence of a cookie and in addition can also return the cookie’s value.

Benefits: check for cookie existence and get values; faster than original solution when checking multiple cookies

Limitations: no savings in size

Option 3: Universal key-value parser, a bit larger

The above solution is based on code that I use to parse URL query strings. Both cookie- and query strings are key-value pairs, except cookies use a semicolon ; as a separator, while query strings use an ampersand &. If we adapt our function to accept the separator and equals sign as (optional) parameters, we can parse query strings and cookies with the same function:

function parseValues(cookie, sep, eq) {
    sep = sep || /;\s*/
    eq = eq || '='

    return (cookie || '')
        .split(sep)
        .reduce((result, entry) => {
            const [key, value] = entry.split(eq, 2)
            result[key] = decodeURIComponent(value)
            return result
        }, {})
}

function parseCookies(cookies) {
    return parseValues(cookies)
}

function parseQuery(query) {
    return parseValues(query, '&')
}

function checkCookie(cookies, name) {
    return Boolean(parseCookie(cookie)[name])
}

Option 4: Consider using libraries

I understand that you wanted to write your own function as opposed to introducing cookie-parser, which does a lot more than what you actually need - especially signing & signature verification - and is thus larger in size than what you aim for.

Other options would be cookie or lightcookie, which are smaller in size and might be a good choice over writing your own function.

const lightcookie = require('lightcookie')

function checkCookie(cookies, name) {
    return Boolean(lightcookie.parse(cookies)[name])
}

Benefits: save time; avoid mistakes; benfit from tested & verified code

Limitations: can be larger than what you are willing or able to include


A fast approach to prime number sieving (non-threaded array) I am posting this question for input on comparisons between other sieving methods and opinions on what I have discovered. I believe this might be the fastest (singular, non-threaded) array implementation of prime sieving to date, correct me if I am wrong.

My rudimentary benchmark tests stand as follows:

My optimized code: 4 seconds   N = 1,000,000,000
My original code: 7 seconds   N = 1,000,000,000
Standard Sieve-of-Eratosthenes: 16 seconds  N = 1,000,000,000

I have seen PrimeSieve and as I understand it utilizes better means to compute faster, but as it seems - it must dump the data somewhere and retrieve it to utilize it again, because it cycles the computations through the cache only.

My original thread with same pseudo-code is here, however, my original Java code did not match the pseudo-code I developed (but essentially performed similar functions at a lesser speed and easier complexity to code).

Pseudo-code as follows:

Initialize list with initial values \$(2, 3)\$.
Use the formula \$6k ± 1\$ where \$k = 1\$ and must increment by \$1\$. Store values to list.
Compute step two up to a product \$n\$, where \$n > 7\$ if \$k = 1\$.
Starting with \$x = 5\$, remove all products of \$x \times y\$ (up to \$n\$),
where \$y\$ starts at \$x\$ and
increments to the next number in the list as \$x\$ remains the same.
Repeat step four by assigning \$x\$ to the next number in the list.
Stop when \$x \times y > n\$.
Final step: We must remove all factors of the square of every prime number \$(25, 49)\$

//-----------------------------------------------------------------
//  Class:   SixesSieve.java
//  Author:  Alex Lieberman
//  Purpose: Finds all prime numbers up to a specified
//           maxNumber (exclusive) and marks them as true in the boolean array.
//-----------------------------------------------------------------

public class SixesSieve {
  public static void main (String[] args) {

   int maxNumber = 1000000000;
   boolean[] isPrime = new boolean [maxNumber];
   isPrime[2] = true;
   isPrime[3] = true;

   for (int i = 5, j = 7; i < maxNumber; i+=6, j+=6) {
     isPrime[i] = true;
     if (j < maxNumber)
      isPrime[j] = true;
    }

   for (int a = 5; a*a <= maxNumber; a+=2) {
     if (isPrime[a]) {

       for (int b = a, z = a*a; b*a <= maxNumber; ) {
        if(b*a == z && z < maxNumber/2) {
         while (true){
           if (z <= maxNumber) {
            isPrime[z] = false;
            z = z+(a*a*2);
           }
           else
            {b+=2; break;}
           }
         }
        else {
         isPrime[b*a] = false;
         b+=2;
         while (!isPrime[b])
         {b+=2;}
        }
       }
      }
    }

    for (int i = 0; i < maxNumber; i++) {
      if (isPrime[i])
       System.out.print(i + " ");
    }
  }
 }

Just some minor comments to the code style. Let's start with your fileheader...in these times and days it should be unnecessary. First it doesn't make sense to have the file/classname in the comment, it's only something that will get out of date because refactoring can't reach it. Second, the other parts should be in the JavaDoc of the class.

Variable names. I know it is hard to find good names for such...mathematical stuff, but try to find them. 'a', 'b', 'z' are meaningless. It is hard to read the code if variables have meaningless names.

The code formatting is partly a bloody mess. If you use an IDE, make use of these formatting features. It is so much easier to read code if it is well formatted.

First, don't omit braces, even if it is just a one line 'if' or 'for' block. Second, be consistent about your brace style, and be consequent.

System.out.print(i + " ");

I still don't understand why the Java compiler does allow this. This is not a "cast", this is a hack in the best of cases.

System.out.print(Integer.toString(i) + " ");


Walking between points I have written a function that fills in the values between points and creates one continuous path (essentially walks between the points) and I'd like some feedback on it's implementation.

If two points are provided that are not "in line" with one another then an error is thrown, as I do not want to support diagonal movement.

Here is an example of the code's input and output:

The route to walk

|_Point_|_X_|_Y_|
| A     | 0 | 0 |
| B     | 2 | 0 |
| C     | 2 | 2 |
| D     | 2 | 4 |
| E     | 4 | 4 |

Diagram of the route in 2D space

  0_1_2_3_4_X
0|A   B
1|
2|    C
3|
4|    D   E
Y|

Calling the walk function

const output = walk(
  { x: 0, y: 0 }, // A
  { x: 2, y: 0 }, // B
  { x: 2, y: 2 }, // C
  { x: 2, y: 4 }, // D
  { x: 4, y: 4 }  // E
);

The output from the walk function

[
  { x: 0, y: 0 }, // A
  { x: 1, y: 0 }, // A -> B
  { x: 2, y: 0 }, // B
  { x: 2, y: 1 }, // B -> C
  { x: 2, y: 2 }, // C
  { x: 2, y: 3 }, // C -> D
  { x: 2, y: 4 }, // D
  { x: 3, y: 4 }, // D -> E
  { x: 4, y: 4 }  // E
]

And the function itself:

function walk(...points) {
  const result = [];
  let lastPoint = null;

  points.forEach((point, index) => {
    if (lastPoint === null) {
      result.push(point);
    } else {
      const direction = {
        x: Math.sign(point.x - lastPoint.x),
        y: Math.sign(point.y - lastPoint.y)
      };

      if (direction.x !== 0 && direction.y !== 0) {
        throw new Error(`points are misaligned at ${index}`);
      } else if (direction.x % 1 !== 0 || direction.y % 1 !== 0) {
        throw new Error(`points must be whole values at ${index}`);
      } else if (direction.x === 0 && direction.y === 0) {
        return;
      }

      const axis = direction.x !== 0 ? 'x' : 'y',
            otherAxis = axis === 'x' ? 'y' : 'x';

      const start = lastPoint[axis] + direction[axis],
            comparator = direction[axis] > 0 ? (a, b) => a <= b : (a, b) => a >= b,
            delta = direction[axis];

      for (let value = start; comparator(value, point[axis]); value += delta) {
        result.push({
          [axis]: value,
          [otherAxis]: point[otherAxis]
        });
      }
    }

    lastPoint = point;
  });

  return result;
}

Things I think could be improved

I don't like how the comparator variable works — it seems a bit of a hack and there's most likely a more mathematical way of working it out.
Are there more "ES6-y" ways of doing what I've done? For example using reduce or similar functions.

This condition

else if (direction.x % 1 !== 0 || direction.y % 1 !== 0)

Doesn't check for integer and is (almost) always false, since direction.x is the result of Math.sign(), which returns an integer (1, 0, or -1) or NaN, which is the only time the condition can be true.

Use point.x instead. I would also use Number.isInteger() to check for integers. This check should also be at the start of your loop, since the first point of the input is currently not getting checked.

I'm not sure what this code is supposed to accomplish.

else if (direction.x === 0 && direction.y === 0) {
  return;
}

What is the desires behavior when you get two identical point after each other? Is it illegal? Throw an error. Should it keep both points? Don't do anything. Should duplicates be deleted? Replace return with continue so it won't get added to the result.


ES6 classes and ReactJS: implementing propTypes I had a problem to define propTypes for my React class. I ran into solution that doesn't feel right:

let React = require('react')
let CallerCard = require('caller-card')

class CallerDetailsPanel {
  render() {
    return (
      <div className='caller-details-panel'>
        <CallerCard person={this.props.callerData.owner} />
        <CallerCard person={this.props.callerData.user} />
      </div>
    )
  }
}

CallerDetailsPanel.prototype.propTypes = {
    callerData: React.PropTypes.object
}

module.exports = React.createClass(CallerDetailsPanel.prototype)

Is this correct approach or how propTypes should be defined? If I try to define them inside class, I get a Parse error from 6to5 / esprima on console.

I think you're just supposed to do this:

CallerDetailsPanel.propTypes = { callerData: React.PropTypes.object }

This feels a little backwards, but the React team has done this with the hope that in the future, with ES7, you will be able to use a syntax that looks something like this:

class CallerDetailsPanel {
  static propTypes = { callerData: React.PropTypes.object }
  render() {
    return (
      <div className='caller-details-panel'>
        <CallerCard person={this.props.callerData.owner} />
        <CallerCard person={this.props.callerData.user} />
      </div>
    )
  }
}


Given Sum of sub sums of an integer find the original integer If we are given an integer \$\text{SumofSubSums}(N)\$, and number of digits \$D\$ that \$N\$ has, we are asked to find the original  integer \$ N \$.

Example:
 If we are given \$2148\$ , and \$D = 4\$.
We need to find \$1935\$ , because
$$2148 = 1935 + \frac{1935}{10}+\frac{1935}{100}+\frac{1935}{1000}$$

Now I have a code which solves the task but it takes a lot of time to do so...

#include <iostream>
#include <cmath>
using namespace std;

    long long int Sum_of_Sub_Sums(long long int a) {
        long long int Sum = 0;
        for (a = a; a > 0; a /= 10) {
            Sum += a;
        }
        return Sum;
  }

    int main() {
        long long int maxSum[15] = { 9, 108, 1107, 11106, 111105, 1111104, 11111103, 111111102, 1111111101, 11111111100, 111111111099, 1111111111098, 11111111111097, 111111111111096, 1111111111111095 };
        long long int number;
        int numDigits;
        cin >> number >> numDigits;
        if (number < 10 && numDigits < 2) {
            cout << number;
        }
        else {
                long long int l = pow(10,numDigits-1);
                long long int r = maxSum[numDigits-1];
                long long int mid= l + (r-l)/2;
                while(Sum_of_Sub_Sums(mid)!= number){
                if(Sum_of_Sub_Sums(mid) > number){
                    r = mid;
                }
                else {
                    l = mid;
                }
                mid = l + (r-l)/2;
            }
            cout<<fixed<<mid;
        }
            return 0;
    }

how can I decrease the complexity of my code ?

\$EDIT: \$
If anyone is interested here is the working solution

#include <iostream>
#include <cmath>
using namespace std;

long long int Sum_of_Sub_Sums(long long int a) {
    long long int Sum = 0;
    for (a = a; a > 0; a /= 10) {
        Sum += a;
    }
    return Sum;
}

int main() {
    long long int number;
    long double tmp;
    int numDigits;
        cin >>number >>numDigits;
        if (number < 10 && numDigits < 2) {
      cout << number;
    } else {
        tmp=(pow(10,numDigits)-1)/(9*pow(10,numDigits-1));
        long long int current =  number/tmp;
        while (Sum_of_Sub_Sums (current) != number) {
      if (Sum_of_Sub_Sums (current) > number) {
          current--;
        } else {
          current++;
        }
        Sum_of_Sub_Sums (current);
    }
      cout << fixed << current;
    }
        return 0;
}

It seems to me that you could find an approximate solution easily.
Maybe you could then use this knowledge to iterate through less numbers.

When checking your number, you do this operation for D = 4: a + truncate(a/10) + truncate(a/100) + truncate(a/1000), which can be quite close to the float a + a/10 + a/100 + a/1000, but not exactly the same value as any of the terms might not be an integer value.

Basically, the float computation is a * 1.111, and the result is probably not too far from your number N.

If you take N / 1.111, that should bring you pretty close to your desired number. In your example, 2148 / 1.111 = 1933.39.

You also know that the float value that you find with this division is lower than the one you want, because of truncations.

A naive way would be to start from this value, and iterate until you find a solution. I am not sure how well this would be compared to your binary search, but this could already be something worth trying.

EDIT

Just another thought: you can also take into account that the sum of digits must match the last digit of N during your iteration (1 + 9 + 3 + 5) % 10 = 8 = 2148 % 10, this will help iterate only 9 by 9 instead of 1 by 1.


Binary search a sorted list of integers in Python 3 y is the value I am searching for. Is this the correct implementation for binary search?

y= 3
x= range(0,100)
left = 0
right = len(x)-1
mid=int((left+right)/2)

while x[mid]!=y:
    if x[mid]>y:
        right = mid
        mid=int((left+right)/2)
    if x[mid]<y:
        left = mid
        mid=int((left+right)/2)

PEP-8

PEP-8 is a style guide for Python.
It contains several practices which you should follow, like having spaces around operators.  Use a tool, like pylint, to ensure you follow these practices.

Integer Division

mid=int((left+right)/2)

Python comes with a built-in integer division operator: //.  You should use it.

mid = (left + right) // 2

Termination

while x[mid] != y:

You are searching until you find the desired value.  What if the desired value is not present?  You will search forever???  Consider adding a different stopping condition.

Redundancy

    if x[mid]>y:
        #...
        mid=int((left+right)/2)
    if x[mid]<y:
        #...
        mid=int((left+right)/2)

Since you are looping while x[mid] != y, your only real choices are for x[mid] > y or x[mid] < y.  Instead of testing the second condition, how about using else:?

Since you enter will either the x[mid] > y then clause, or the x[mid] < y then clause, you will always be executing mid=int((left+right)/2).  You can safely move those two statements out if the if, and unconditionally execute it at the end.  As in:

    if x[mid] > y:
        #...
    else:
        #...
    mid = (left + right) // 2

Efficiency

If you start with left=0, right=99, mid=49, and find x[mid]>y is true, you proceed to search in the range left=0, right=49.  But you've already tested x[49] and found the value wasn't there; you don't need to include it in your search range anymore.  Similarly, when you find x[mid]<y, you don't need to include that mid point as the left end of your range.

    if x[mid] > y:
        right = mid - 1
    else:
        left = mid + 1

Bug

Your existing algorithm will not terminate for certain numbers.  If you search for 99, the value of mid will take on the following values:

49, 74, 86, 92, 95, 97, 98, 98, 98, 98, 98, 98, 98, 98, ...


List all repeated substrings with a fixed length While fiddling around, I've made a very simple and naive function to retrieve any repeated sub-string within a certain string.

function getRepetitions(str, length){
    if(isNaN(length))
    {
        throw new TypeError('length must be a number');
    }

    if(length < 1 || length > 2147483647)
    {
        throw new RangeError('length must be between 1 and 2147483647');
    }

    var copy = str.toString();

    if(length > (copy.length / 2))
    {
        length = Math.ceil(copy.length / 2);
    }

    var x = {};
    for(var i = str.length - length - 1; i > 1; i--)
    {
        piece = copy.substr(i, length);
        if(!x[piece] && copy.indexOf(piece) !== i)
        {
            x[piece] = copy.split(piece).length - 1;
        }
    }

    return x;
}

It accepts anything that can be converted into a string, and expects the length of the substring to be found.

The length must be withing a certain range and must be a valid number. It will be converted into a 32-bit signed integer later on.

This will be part of a larger project, and just want to make sure it is as polished as it can be.

Performance:

Performance is quite satisfactory. It takes around 2.5s to search 9-bytes long repetitions, on a 8.58MB string, on my machine, using Google Chrome v47.0.2526.106.

Using IE11, it takes over twice the time (around 5.6s)!

Screenshots: Google Chrome, IE11.

The time it takes is always around the same (with very small variations), which is a good thing.

Running this code on a string that is 10% the size will take 10% of the time. That is, for a string that is 0.86MB it will take around 250ms.

This is the code used to test:

var str = Array(1000001).join('123456789');
console.time('x');
getRepetitions(str, 9);
console.timeEnd('x');
console.log((str.length / 1024 / 1024).toFixed(2) + 'MB');

Note: There are a few edge cases, such as (very rarely) substrings like prototype and __proto__ going untouched. I am aware of those and I have no idea how to avoid it.

In terms of performance, readability, boundary checking and reliability, what can be improved?

Styling and readability
You are using a variable length. It is unclear what this variable is, or does, without looking at your explanation in the question. It appears that length is a number which represents the length of the substrings you want to find. I would recommend changing the name of this variable to better represent what it is.
You are using a variable str. When reading this, I expect this variable to be a string, but from your description this function accepts "anything that can be converted into a string". To avoid confusion I would recommend renaming this variable to something that better represents what it contains, such as source.
You are using a variable x which does not describe what it contains.
You check the validity of length, but you do not do this for str. You can check the validity of str with str.toString !== undefined. Instead of a cryptic TypeError (variable.toString is not a function) you can give a better error description.
Javascript processes variable declarations in a context before executing the code. This process is called variable hoisting. This means that when you declare variables in the middle of a function, they are moved to the top. To avoid confusion why variables are defined before you declare them, or why local variables are used when you expected global variables to be used, I would recommend declaring all variables at the beginning of the context.
You are dropping the variable piece into the global scope by forgetting to declaring it with var. This is another reason to declare them at the top!
Logic errors
You have an if-statement in your code that seems to serve no purpose other than causing the result to be incorrect. If the substring you want to find is larger or equal to the length of the data passed, you cannot find repeated substrings. You should remove this if-clause.
if(length > (copy.length / 2))
{
    length = Math.ceil(copy.length / 2);
}

You use the .toString() method on a variable of an unknown type. The result is a string. At a later stage, you request the length of str, which is still of an unknown type. This causes the code to be incorrect when str is not actually a string. This error is likely caused by using a variable name that does not represent the type, which is a good argument why you should use a better name for that variable.
var copy = str.toString();
//...
for(var i = str.length - length - 1; i > 1; i--)

If your string contains reserved words with length length, your code will fail as you experienced. You can work around this by prepending a character such as # in keys. You'll need to remove that character again when you use the key. As far as I am aware, no reserved properties beginning with a # exist in javascript/ES.
Your implementation has two off-by-one errors in the following line. With a length length and a string s, you start your loop at length + 1 before the end of the string, causing you to ignore the last character in s. Your code fails to produce the right result for the string "test.........test", with a substring length of 4. You end your loop with i > 1, and this will cause the code to produce the incorrect result when the substring starting at 0 and the substring starting at 1 are the same. Your code will fail for the string "11111...........". You can safely ignore the substring starting at index 0, because indexOf will always be 0.
for(var i = str.length - length - 1; i > 1; i--)

Note: You use !x[piece], which would fail if it contained a falsy value. Since it contains a value that is guaranteed >= 1 this is not an issue in this case.
Note: Your algorithm will find repeating substrings even if the substrings overlap. I am assuming this is intended behaviour. When substrings overlap copy.split(piece).length - 1 will not be equal to the amount of times the substring appears in the string.
Performance
Your performance tests give a skewed result. The data you passed is highly repetitive, causing more than 99% of your code to skip the indexOf test. If indexOf was implemented in a very naive way, the worst case would need to do (n * n) / 2 comparisons. indexOf seems to be implemented slightly better, but only slightly.

var str = Array(10000000).join("1");
var lookup1 = "2222222222222222";
var lookup2 = "1111111111111112";

console.time( "Lookup 1" );
str.indexOf( lookup1 );
console.timeEnd( "Lookup 1" );
console.time( "Lookup 2" );
str.indexOf( lookup2 );
console.timeEnd( "Lookup 2" );
console.log( "Some lookups are handled quite fast" );

If I execute your code on 10 random string of 100.000 alphanumeric characters, finding substrings of 9 characters, it costs about 2.959 ms per string to find all repetitions. Executing your code on 10 random strings of 200.000 alphanumeric characters, finding substrings of 9 characters, it costs about 11.815 ms. This is significantly worse than "twice as long".
Your code can be easily improved by using hashmaps. Browser vendors use hashmaps to store properties of objects1. Using an object to store found substrings will cost more memory, but will improve performance significantly. For 10 random strings of 100.000 alphanumeric characters, finding substrings of 9 characters, it costs about 126 ms per string to find all repetitions. For 10 random strings of 200.000 alphanumeric characters, finding substrings of 9 characters, it costs about 313 ms per string to find all repetitions. For 10 random strings of 1.000.000 alphanumeric characters, finding substrings of 9 characters, it costs about 1.659 ms per string to find all repetitions.
Testing on your fixed strings will make the improved versions perform worse, because two lookups have to be done instead of one. The difference for random strings is between seconds and several hours.

function getRepetitions(source, substringLength) {
  var copy;
  var piece;
  var output;
  var i;

  if (isNaN(length)) {
    throw new TypeError('length must be a number');
  }

  if (substringLength < 1 || substringLength > 2147483647) {
    throw new RangeError('length must be between 1 and 2147483647');
  }
  if (source.toString === undefined) {
    throw new TypeError('must be able to convert source to string via toString method');
  }

  copy = source.toString();
  output = {};

  for (i = copy.length - substringLength; i > 0; i--) {
    piece = copy.substr(i, substringLength);
    if (!output[piece] && copy.indexOf(piece) !== i) {
      output[piece] = copy.split(piece).length - 1;
    }
  }

  return output;
}

function getRepetitions2(source, substringLength) {
  var copy;
  var piece;
  var output;
  var i;
  var foundSubstrings;

  if (isNaN(length)) {
    throw new TypeError('length must be a number');
  }

  if (substringLength < 1 || substringLength > 2147483647) {
    throw new RangeError('length must be between 1 and 2147483647');
  }
  if (source.toString === undefined) {
    throw new TypeError('must be able to convert source to string via toString method');
  }

  copy = source.toString();
  output = {};
  foundSubstrings = {};

  for (i = copy.length - substringLength; i >= 0; i--) {
    piece = copy.substr(i, substringLength);
    if (!foundSubstrings[piece]) {
      foundSubstrings[piece] = true;
    } else if (!output[piece]) {
      output[piece] = copy.split(piece).length - 1;
    }
  }

  return output;
}

function getRepetitions2WithoutKeywords(source, substringLength) {
  var copy;
  var piece;
  var output;
  var i;
  var foundSubstrings;
  var key;

  if (isNaN(length)) {
    throw new TypeError('length must be a number');
  }

  if (substringLength < 1 || substringLength > 2147483647) {
    throw new RangeError('length must be between 1 and 2147483647');
  }
  if (source.toString === undefined) {
    throw new TypeError('must be able to convert source to string via toString method');
  }

  copy = source.toString();
  output = {};
  foundSubstrings = {};

  for (i = copy.length - substringLength; i >= 0; i--) {
    piece = copy.substr(i, substringLength);
    key = "#" + piece;
    if (!foundSubstrings[key]) {
      foundSubstrings[key] = true;
    } else if (!output[key]) {
      output[key] = copy.split(piece).length - 1;
    }
  }

  return output;
}

function test() {
  //https://stackoverflow.com/a/1349426/2209007
  var possibleChars = 'abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789 ';
  var str = '';
  var nrOfTests = 10;
  var strLength = 100000;
  var i;
  var testNr;
  var substrLength = 9;

  console.log('== Testing random strings ==');
  for (testNr = 1; testNr <= nrOfTests; testNr++) {
    str = '';
    console.time('Test ' + testNr + ': Generating string');
    for (i = 0; i < strLength; i++) {
      str += possibleChars.charAt(Math.floor(Math.random() * possibleChars.length));
    }
    console.timeEnd('Test ' + testNr + ': Generating string');

    console.log('Test ' + testNr + ': Working with string "' + str.substring(0, 100) + '...\"');

    console.time('Test ' + testNr + ': Default repetitions');
    getRepetitions(str, substrLength);
    console.timeEnd('Test ' + testNr + ': Default repetitions');

    console.time('Test ' + testNr + ': Improved repetitions');
    getRepetitions2(str, substrLength);
    console.timeEnd('Test ' + testNr + ': Improved repetitions');

    console.time('Test ' + testNr + ': Improved repetitions without keywords');
    getRepetitions2WithoutKeywords(str, substrLength);
    console.timeEnd('Test ' + testNr + ': Improved repetitions without keywords');
  }

  console.log('== Testing fixed string ==');
  str = Array(1000001).join("123456789");
  console.log('Test ' + testNr + ': Working with string \"' + str.substring(0, 100) + '...\"');

  console.time('Test ' + testNr + ': Default repetitions');
  getRepetitions(str, 9);
  console.timeEnd('Test ' + testNr + ': Default repetitions');

  console.time('Test ' + testNr + ': Improved repetitions');
  getRepetitions2(str, 9);
  console.timeEnd('Test ' + testNr + ': Improved repetitions');

  console.time('Test ' + testNr + ': Improved repetitions without keywords');
  getRepetitions2WithoutKeywords(str, 9);
  console.timeEnd('Test ' + testNr + ': Improved repetitions without keywords');
}

test();


How to refactor this function to solve cyclomatic issue? I'm trying to fix this error on codeclimate

  Cyclomatic complexity for resource_icon is too high. [7/6]

What is the best way to refactor this function?

def resource_icon(resource)
    case resource.to_sym
    when :locations
      'fa-building'
    when :monitored_places
      'fa-globe'
    when :attendances
      'fa fa-bicycle'
    when :gym_distribuitions
      'fa fa-users'
    when :ranking_activities
      'fa fa-list-ol'
    when :attendance_by_group_class_trainers
      'fa fa-graduation-cap'
    else
      'fa-diamond'
    end
  end

You could try using a hash that maps "locations" to "fa-building", "monitored_places" to "fa-globe", and so on, with a default value of "fa-diamond".


Lightshot Print Screen key linux handler I haven't found any Lightshot Print Screen key linux handler publicly available.

So, I decided to write one today. It is a standard POSIX shell script.

I believe there is always space to get better, so any and all reviews are welcome.

#!/bin/sh

is_number()
{
    # check if exactly one argument has been passed
    test "$#" -eq 1 || print_error_and_exit 5 "is_number(): There has not been passed exactly one argument!"

    # check if the argument is an integer
    test "$1" -eq "$1" 2>/dev/null
}

# ------------------------------------------------------------------------------

print_error_and_exit()
{
    # check if exactly two arguments have been passed
    test "$#" -eq 2 || print_error_and_exit 3 "print_error_and_exit(): There have not been passed exactly two arguments!"

    # check if the first argument is a number
    is_number "$1" || print_error_and_exit 4 "print_error_and_exit(): The argument #1 is not a number!"

    # check if we have color support
    if test -x /usr/bin/tput && tput setaf 1 > /dev/null 2>&1
    then
        bold=$(tput bold)
        red=$(tput setaf 1)
        nocolor=$(tput sgr0)
        echo "$bold$red$2 Exit code = $1.$nocolor" 1>&2
    else
        echo "$2 Exit code = $1." 1>&2
    fi

    exit "$1"
}

# ------------------------------------------------------------------------------

check_for_prerequisite()
{
    # check if exactly one argument has been passed
    test "$#" -eq 1 || print_error_and_exit 2 "check_for_prerequisite(): There has not been passed exactly one argument!"

    # check if the argument is a program which is installed
    command -v "$1" > /dev/null 2>&1 || print_error_and_exit 6 "check_for_prerequisite(): I require $1 but it's not installed :-("
}

# ------------------------------------------------------------------------------

# check if no arguments have been passed to the script
test "$#" -gt 0 && print_error_and_exit 1 "$0: You have passed some unexpected argument(s) to the script!"

# ------------------------------------------------------------------------------

# check for prerequisites
check_for_prerequisite "pgrep"
check_for_prerequisite "xdotool"

# ------------------------------------------------------------------------------

# global constants
lightshot_key="Print"
lightshot_name="Lightshot"

# ------------------------------------------------------------------------------

# get the lightshot pid
lightshot_pid=$(pgrep "$lightshot_name")

# test if a pid has been successfully acquired
is_number "$lightshot_pid" || print_error_and_exit 7 "lightshot_pid: The argument is not a number!\\nLightshot is most probably not running."

# ------------------------------------------------------------------------------

# get the window id from lightshot pid
lightshot_wnd=$(xdotool search --limit 1 --all --pid "$lightshot_pid" --name "$lightshot_name")

# test if a window handler has been successfully acquired
is_number "$lightshot_wnd" || print_error_and_exit 8 "lightshot_wnd: The argument is not a number!\\nLightshot is most probably not running."

# ------------------------------------------------------------------------------

# simulate a print screen key press on the lightshot window
xdotool key --window "$lightshot_wnd" "$lightshot_key"

In Linux Mint 18.3 you can simply put this into your custom keyboard shortcuts:

Instruct shell how to treat unset variables

set -o nounset

In this script, there should not be possible to encounter an unbound variable, so setting this will prove useful for debugging.

Redirection style

 9     test "$1" -eq "$1" 2>/dev/null

in contrast with:

23     if test -x /usr/bin/tput && tput setaf 1 > /dev/null 2>&1

It should be unified either with, or without a space.

Constants should be moved to the top

61 lightshot_key="Print"
62 lightshot_name="Lightshot"

It is obvious, that everyone might not want to use Print, so this really has to be moved to the top in order for each user to quickly customize the keyboard combination for the print screen.

Those constants should be commented and clarified

In order for the users to quickly know, how to define key combinations, there should be some example added.

Consider replacing echo with printf

28         echo "$bold$red$2 Exit code = $1.$nocolor" 1>&2
29     else
30         echo "$2 Exit code = $1." 1>&2

Basically, it's a portability (and reliability) issue. Read more about this topic in this answer.

Problematic code

I notice now, there is a new line inside these strings:

70 is_number "$lightshot_pid" || print_error_and_exit 7 "lightshot_pid: The argument is not a number!\\nLightshot is most probably not running."
78 is_number "$lightshot_wnd" || print_error_and_exit 8 "lightshot_wnd: The argument is not a number!\\nLightshot is most probably not running."

Which according to Shellcheck.net:

  echo won't expand escape sequences

Read more about this topic in this article.

This code, as it is, works though, in spite of this error. Tested in bash version 4.3.48(1)-release and POSIX dash version 0.5.8-2.1ubuntu2.

Better variable naming convention

These are of little value to a foreign script reader:

61 lightshot_key="Print"
62 lightshot_name="Lightshot"

There are only two constants. Why not to give them some real meaning, this would further enhance the script reader's experience.

The variables:

67 lightshot_pid
75 lightshot_wnd

could use some love too.

Tell the user what arguments are (not) expected

50 test "$#" -gt 0 && print_error_and_exit 1 "$0: You have passed some unexpected argument(s) to the script!"

Let's add there something like:

  No arguments expected!

Problematic space in the comments (block separators)

12 # ------------------------------------------------------------------------------

There should be no space between # and -, because imagine, that you have a larger project than this, having these block separators will prevent you from searching for long arguments beginning (--).

Re-consider using smileys in the output and using grammar shortcuts

44     command -v "$1" > /dev/null 2>&1 || print_error_and_exit 6 "check_for_prerequisite(): I require $1 but it's not installed :-("

It may look a little unprofessional. Hard to say without knowing the people reading your script, though. (It might be totally OK for personal usage.) This script will likely be spread across the internet, so better not use that. Also, avoid using grammar shortcuts like:

  it's

xdotool: documenting all passed arguments would be greatly appreciated by the script readers (and the order of arguments in lightshot_wnd could be better a little)

75 lightshot_wnd=$(xdotool search --limit 1 --all --pid "$lightshot_pid" --name "$lightshot_name")

Re-writen code would look as follows

#!/bin/sh

# treat unset variables as an error when substituting
set -o nounset

# global constants for an easy set-up:
    # lightshot_printscreen_hotkey: set this to the same hotkey which you have set up in Lightshot
    #                               example: for left control and print screen key -> type Control_L+Print
lightshot_printscreen_hotkey="Print"
    # lightshot_process_name: do not change this one, it is a case-sensitive name of the Lightshot process
lightshot_process_name="Lightshot"

#------------------------------------------------------------------------------

is_number()
{
    # check if exactly one argument has been passed
    test "$#" -eq 1 || print_error_and_exit 5 "is_number(): There has not been passed exactly one argument!"

    # check if the argument is an integer
    test "$1" -eq "$1" 2> /dev/null
}

#------------------------------------------------------------------------------

print_error_and_exit()
{
    # check if exactly two arguments have been passed
    test "$#" -eq 2 || print_error_and_exit 3 "print_error_and_exit(): There have not been passed exactly two arguments!"

    # check if the first argument is a number
    is_number "$1" || print_error_and_exit 4 "print_error_and_exit(): The argument #1 is not a number!"

    # check if we have color support
    if test -x /usr/bin/tput && tput setaf 1 > /dev/null 2>&1
    then
        bold=$(tput bold)
        red=$(tput setaf 1)
        nocolor=$(tput sgr0)
        printf "%s%s%s Exit code = %s.%s\\n" "$bold" "$red" "$2" "$1" "$nocolor" 1>&2
    else
        printf "%s Exit code = %s.\\n" "$2" "$1" 1>&2
    fi

    exit "$1"
}

#------------------------------------------------------------------------------

check_for_prerequisite()
{
    # check if exactly one argument has been passed
    test "$#" -eq 1 || print_error_and_exit 2 "check_for_prerequisite(): There has not been passed exactly one argument!"

    # check if the argument is a program which is installed
    command -v "$1" > /dev/null 2>&1 || print_error_and_exit 6 "check_for_prerequisite(): I require $1 but it is not installed!"
}

#------------------------------------------------------------------------------

# check if no arguments have been passed to the script
test "$#" -gt 0 && print_error_and_exit 1 "$0: You have passed some unexpected argument(s) to the script. No arguments expected!"

#------------------------------------------------------------------------------

# check for prerequisites
check_for_prerequisite "pgrep"
check_for_prerequisite "xdotool"

#------------------------------------------------------------------------------

# get the Lightshot process id
lightshot_process_id=$(pgrep "$lightshot_process_name")

# test if a process id has been successfully acquired
is_number "$lightshot_process_id" || print_error_and_exit 7 "lightshot_process_id: The argument is not a number! Lightshot is most probably not running."

#------------------------------------------------------------------------------

# get the window id from the Lightshot process id
    #--all   : Require that all conditions be met.
    #--limit : Stop searching after finding N matching windows.
    #--pid   : Match windows that belong to a specific process id.
    #--name  : Match against the window name. This is the same string that is displayed in the window titlebar.
lightshot_window_id=$(xdotool search --all --limit 1 --pid "$lightshot_process_id" --name "$lightshot_process_name")

# test if a window id has been successfully acquired
is_number "$lightshot_window_id" || print_error_and_exit 8 "lightshot_window_id: The argument is not a number! Lightshot is most probably not running."

#------------------------------------------------------------------------------

# simulate the above pre-defined print screen hotkey press on the Lightshot window
xdotool key --window "$lightshot_window_id" "$lightshot_printscreen_hotkey"


Incrementing Integers in Map I have a HashMap<Token, Integer>, which counts occurrences of Token. Each time Token is found, the value in the map should be incremented.

Map<Token, Integer> occurrences = new HashMap<Token, Integer>();
// ...

public void tokenFound(Token token) {

    Integer numberOfOccurs = occurrences.get(token);
    Integer newNumberOfOccurs = new Integer((numberOfOccurs == null) ? 1 : numberOfOccurs.intValue() + 1);
    occurrences.put(token, newNumberOfOccurs);
}

Is there a more elegant way to do this?

You have several different options for this:

Guava

Google's Guava Library introduces the idea of a Multiset which is capable of counting the occurrences, and also provides a couple of other features.

Java 8

If you are using Java 8 (which I highly recommend if you have the ability to do so), your tokenFound method can simply be this:

occurrences.merge(token, 1, (oldValue, one) -> oldValue + one);

Or this:

occurrences.compute(token, (tokenKey, oldValue) -> oldValue == null ? 1 : oldValue + 1);

Note that as of Java 7, you can initialize the map with the "diamond operator":

Map<Token, Integer> occurrences = new HashMap<>();

Without Java 8, no libraries

If you are unable to use Java 8 and don't want to add Guava as a third party library to your project, there are a small part you can do to simplify your existing code:

Integer previousValue = occurrences.get(token);
occurrences.put(token, previousValue == null ? 1 : previousValue + 1);

More specifically:

Using the new Integer constructor is not necessary, Java automatically uses boxing to do this. For Integer values close to zero, this will actually save you a little bit because Java keeps some integers cached.
You don't need the newNumberOfOccurs variable as it's only used once.


Counting seconds until auto-log-off I've just created a teeny little script to count the seconds until I get auto-logged off.  I already solved my issue with my ssh client settings, but I'd still like any help making the bash script nicer to read, or just tips in general.

#!/bin/bash

count=0
while ( [ : ] )
do
  count=$(($count+1))
  n=${n:-0}
  for i in `seq 0 $n`; do echo -en '\b'; done
  n="${#count}"
  echo -n $count
  sleep 1
done

count=0
while true; do
  printf "\r%d" $((++count))
  sleep 1
done

Your while condition is doing a lot of work just to return true: you launch a subshell, and evaluate  a test giving a 1-character string (which will always return true). Make it simpler and more readable by just executing the true program.

Note that variable names inside an arithmetic expression do not require the leading dollar sign in most cases. That's quietly documented here:

  "Within an expression, shell variables may also be referenced by name without using the parameter expansion syntax."

printf is more portable than echo, if that's a concern for you. I also find it makes your intentions more obvious.

Instead of backing up the right number of characters, just carriage return to the first column and overwrite the previous contents.


As far as optimizing the tolower logic itself for a single ASCII character, your table is about as fast as it gets.

Alternatives would be:
 A) the standard tolower function
 B) this bit trick if you have only ASCII alpha characters

(unsigned char)((c) | (unsigned char)0x20)

C) this bit trick for all ASCII characters

// set the most significant bit in value X if greater than or equal to 'A'
// set the most significant bit in value Y if less than or equal to 'Z'
// bitwise AND X and Y to determine if both are true and mask out all other bits
// shift the set bit into the 0x20 place and bitwise OR in the bit
(unsigned char)(((((unsigned char)0x40 - (c)) & ((c) - (unsigned char)0x5b) & (unsigned char)0x80) >> 2) | (c))

I ran 300 iterations on a pre-generated random string of one-billion bytes (including the null terminator) of some x64 code under full optimization and no debugger attached from Visual Studio 2013.

your table lookup took about 22.4 seconds
alternative A took about 106.0 seconds (the call was not inlined)
alternative B took about 15.1 seconds (but it only works for strings only containing letters)
alternative C took about 31.9 seconds (so the table beats the bit tricks)

For whatever it's worth, replacing the tolower logic with a NOP and simply copying the source string took 15.0 seconds, so that's what's being spent in loop overhead.

But no matter, your real opportunity for optimization here is in vectorization and parallelization.

However this cannot happen as the signature and contract to the StrToLowerCase function exists now. This is for two reasons:

Any vectorized instructions will need to access data in larger chunks than one byte, and reading (and writing) past the null is bad if you don't know the arrays are padded out the appropriate amount.
You don't know the length of the string without doing a byte-by-byte scan for the null terminator. So you can't break the string into chunks easily and spin off threads to process separate chunks, and you're going to have to check for the null terminator in the middle of vectorized code (if you go that route), and you will lose much of if not all of what you gain.

So, for the sake of moving forward. Let's assume our signature now looks like this:
void StrToLowerCase(const char *src, char *dst, unsigned len)
where for the sake simplicity, len is however many chunks of data the function must process whatever its size may be for the given implementation.

Lets explore vectorized approaches:

If you have a processor that supports AVX2 instructions, then you can use the VGATHERDPS instruction which performs 8 vectorized lookups. However I don't have this instruction so I couldn't try it out. In fact few CPUs will since it first showed up in 2013. So moving on...

Almost everyone has SSE2 now days, and luckily the above bit trick can be vectorized with SSE2. Here is an implementation that operates on 128-bit chunks:

__forceinline void StrToLowerCaseSSE2(const char *src, char *dst, unsigned len)
{
   __m128i const *src128 = (__m128i const*)src;
   __m128i       *dst128 = (__m128i*)dst;
   while (len--)
   {
      __m128i const c16  = _mm_load_si128(src128++);
      __m128i const abv  = _mm_sub_epi8(_mm_set_epi32(0x40404040, 0x40404040, 0x40404040, 0x40404040), c16);
      __m128i const blw  = _mm_sub_epi8(c16, _mm_set_epi32(0x5B5B5B5B, 0x5B5B5B5B, 0x5B5B5B5B, 0x5B5B5B5B));
      __m128i       msk  = _mm_and_si128(abv, blw);
      msk                = _mm_and_si128(msk, _mm_set_epi32(0x80808080, 0x80808080, 0x80808080, 0x80808080));
      msk                = _mm_srli_epi16(msk, 2);
      msk                = _mm_or_si128(msk, c16);
      _mm_store_si128(dst128++, msk);
   }
}

The above SSE2 implementation finishes the test in just 5.3 seconds, roughly 4.5 times faster than the table lookup code that was previously that fastest. However, keep in mind some of this is in the loop overhead we eliminated by effectively unrolling the loop 16 times (each iteration of the SSE2 loop lower-cases 16 ASCII chars). If we unroll the loop 16 times for the lookup code, it finishes in 15.3 seconds. So loop overhead aside, the SSE2 implementation is about 3 times faster. Furthermore, if we employee the alpha-character only tolower in the unrolled loop it finishes in 8.9 seconds, and the SSE2 code that handles non-alpha ASCII characters as well as extended ASCII (codes greater than 127) is still faster.

The SSE2 implementation will be the fastest presented here, but lets say you don't want to use SSE2, you have 64-bit support, and you only have standard ASCII in the range [0, 127] and not extended ASCII in the range [0, 255] (the most significant bit is always zero). In that case you can use a 64-bit vectorized approach that hijacks the 64-valued bit in the same way the above bit trick operations hijack the 128-valued bit. The subtractions can be done on 8 characters shoved into a 64-bit integer, but the trick will be ensuring carries and borrows don't escape the byte you're working in. This can be done by keeping in mind the 128-valued bit in every byte will be zero. Here is the 64-bit implementation where len is the number of 64-bit chunks:

 __forceinline void StrToLowerCase64(const char *src, char *dst, unsigned len)
{
   unsigned const __int64 *src64 = (unsigned const __int64 *)src;
   unsigned       __int64 *dst64 = (unsigned __int64 *)dst;
   while (len--)
   {
      unsigned const __int64 x = *src64++;
      *dst64++ = (((0xC0C0C0C0C0C0C0C0 - x) & (x + 0x2525252525252525) & 0x4040404040404040) >> 1) | x;
   }
}

The above 64-bit implementation finishes the test in 6.3 seconds, and is almost as fast as the 128-bit SSE2 implementation that finished in 5.3 seconds.

Whats next?

From here one can adapt these tricks to higher bit-width vectorization instructions so long as one can be confident the target platform has them available.

There are two other places to go with this that will certainly give even better performance if the strings or number of string to process are large enough. One, is breaking this up into threads either within the function or in the functions that call this function. The other is to resort to offloading the task to the GPU or other more drastic hardware solutions.

EDIT
Here is an SSE2 approach adapted to work with the null terminator. The input still needs to be 128-bit aligned, but special logic could easily be added to the function to handle that. It finishes in 5.8 seconds, so it's only marginally slower than the version that does not perform a null check which finished in 5.3 seconds.

__forceinline void StrToLowerCaseSSE(const char* src, char* dst)
{
    __m128i const *src128  = (__m128i const*)src;
    __m128i       *dst128  = (__m128i*)dst;
    for (;;)
    {
        __m128i const c16  = _mm_load_si128(src128);
        __m128i const null = _mm_cmpeq_epi8(c16, _mm_set_epi32(0, 0, 0, 0));
        if (_mm_movemask_epi8(null))
            break;
        ++src128;
        __m128i const abv  = _mm_sub_epi8(_mm_set_epi32(0x40404040, 0x40404040, 0x40404040, 0x40404040), c16);
        __m128i const blw  = _mm_sub_epi8(c16, _mm_set_epi32(0x5B5B5B5B, 0x5B5B5B5B, 0x5B5B5B5B, 0x5B5B5B5B));
        __m128i       msk  = _mm_and_si128(abv, blw);
        msk                = _mm_and_si128(msk, _mm_set_epi32(0x80808080, 0x80808080, 0x80808080, 0x80808080));
        msk                = _mm_srli_epi16(msk, 2);
        msk                = _mm_or_si128(msk, c16);
        _mm_store_si128(dst128++, msk);
    }
    src = (const char*)src128;
    dst = (char*)dst128;
    while (*src)
    {
        const unsigned char c = *src++;
        *dst++ = (unsigned char)(((((unsigned char)0x40 - (c)) & ((c)-(unsigned char)0x5b) & (unsigned char)0x80) >> 2) | (c));
    }
    *dst = '\0';
}

And here is similar null check adaptation of the 64-bit code. It finishes in 6.4 seconds where the original 64-bit version took 6.3.

__forceinline void StrToLowerCase64(const char* src, char* dst)
{
    unsigned const __int64 *src64 = (unsigned const __int64 *)src;
    unsigned       __int64 *dst64 = (unsigned __int64 *)dst;
    for (;;)
    {
        const unsigned __int64 x  = *src64;
        if ((x - 0x0101010101010101) & 0x8080808080808080)
            break;
        ++src64;
        *dst64++ = (((0xC0C0C0C0C0C0C0C0 - x) & (x + 0x2525252525252525) & 0x4040404040404040) >> 1) | x;
    }
    src = (const char*)src64;
    dst = (char*)dst64;
    while (*src) {
        const unsigned char x = *src++;
        *dst++ = (unsigned char)(((((unsigned char)0x40 - (x)) & ((x)-(unsigned char)0x5b) & (unsigned char)0x80) >> 2) | (x));
    }
    *dst = '\0';
}


String Similarity using fuzzywuzzy on big data I have a file in which I was to check the string similarity within the names in a particular column. I use fuzzywuzzy token sort ratio algorithm as it is required for my use case. here is the code, is there a way to speed it up? It is taking a lot of time for 30000 records.

import csv
import itertools
import pandas as pd
from fuzzywuzzy import fuzz

data = pd.read_csv('D:\\Sim_Input.csv')

Dishes = data['Product_Name']

threshold_ratio = 85

with open('D:\\Sim_Score.csv', 'w') as f1:
    writer = csv.writer(f1, delimiter='\t', lineterminator='\n', )
    writer.writerow(['tag4'])
    for str_1, str_2 in itertools.permutations(Dishes, 2):
        list = []
        ratio = (fuzz.token_sort_ratio(str_1, str_2))
        if ratio > threshold_ratio:
            row = (str_1, str_2, ratio)
            list.append(row)
            print(list)
            writer.writerow(list)

I need the output in a csv file with the names of ItemA, ItemB and similarity score, where the similarity should be above 85.

The first algorithmic recommendation is to use itertools.combinations instead of .permutations, since you don't care about order. This assumes fuzz.token_sort_ratio(str_1, str_2) == fuzz.token_sort_ratio(str_2, str_1). There are half as many combinations as there are permutations, so that gives you a free 2x speedup.

This code also lends itself easily to parallelization. On an i7 (8 virtual cores, 4 physical), you could probably expect this to give you ~4-8x speedup, but that depends on a lot of factors. The least intrusive way to do this is to use multiprocessing.Pool.map or .imap_unordered:

import multiprocessing as mp

def ratio(strings):
    s1, s2 = strings
    return s1, s2, fuzz.token_sort_ratio(s1, s2)

with open('D:\\Sim_Score.csv', 'w') as f1:
    writer = csv.writer(f1, delimiter='\t', lineterminator='\n', )
    writer.writerow(['tag4'])

    with mp.Pool() as pool:
        for s1, s2, r in pool.imap_unordered(ratio, itertools.combinations(Dishes, 2)):
            if r > threshold_ratio:
                writer.writerow([(s1, s2, r)])

All told, I'd expect these changes to give you 5-10x speedup, depending heavily on the number of cores you have available.

For reference, a generator comprehension version of this might look something like:

with mp.Pool() as pool:
    writer.writerows((s1, s2, r)
                     for s1, s2, r in pool.imap_unordered(ratio, itertools.combinations(Dishes, 2))
                     if r > threshold_ratio)

That version has a some, but very little, performance improvement over the non-comprehension version, and IMO is harder to read/maintain.

One other minor thing I noticed in testing my code was that fuzzywuzzy recommends installing python-Levenshtein in order to run faster; when I did so, it ran about 20x slower than when it used the built-in SequenceMatcher. When I uninstalled python-Levenshtein it got fast again. That seems very odd to me, but it's certainly something worth trying.

Finally, if performance is important, you could consider digging into what fuzz.token_sort_ratio does to see if you could remove some repeated work. For example, it's tokenizing and sorting each string again every time you pass it in, so maybe you could pre-tokenize/sort the strings and only run the ratio logic inside the main loop. A quick dig tells me that token_sort_ratio is two main steps:

Preprocess each string using fuzz._process_and_sort
Run fuzz.partial_ratio against the processed strings

I'm not able to get this to run faster at the moment, but I'll edit and update this answer if I can get that approach to work well.


Python CSV to XML converter I'm creating an application that reads in data from a CSV file and creates an XML file using LXML. The below code works as expected. However, before I develop it further I would like to refactor it and make it more DRY. I will need to start creating the additional XML snippets for the other code types in the CSV file and I believe, in its current state, there is too much duplication.

I am attempting to learn Python while using a real user case, please can someone provide any guidance on how I could make the code more eloquent and more DRY. Do I need to be using classes and functions or is my approach OK for the type of file (XML) and creating?

import pandas as pd
from lxml import etree as et
import uuid

df = pd.read_csv('assets.csv', sep=',')

root = et.Element('SignallingSchemeData', xmlns='boo')
timeframe = et.SubElement(root,'TimeFrame')
timeframe.attrib["fileUID"] = str(uuid.uuid4())
timeframe.attrib["name"] = str("timeframe 1")

for index, row in df.iterrows():
    if row['CODE'] == 'S1':
        equipment = et.SubElement(root, 'Equipment')
        signalEquipment = et.SubElement(equipment, 'SignalEquipment')
        signalEquipment.attrib["fileUID"] = str(row["ID"])
        signalEquipment.attrib["name"] = str(row["DESC"])
        equipment = et.SubElement(root, 'Equipment')
        equipmentSupportEquipment = et.SubElement(equipment, 'EquipmentSupportEquipment')
        equipmentSupportEquipment.attrib["fileUID"] = str(uuid.uuid4())
        equipmentSupportReference = et.SubElement(signalEquipment, 'EquipmentSupportReference').text = equipmentSupportEquipment.attrib["fileUID"]
    else:
        equipment = et.SubElement(root, 'Equipment')

source = et.SubElement(root, 'Source')
view = et.SubElement(root, 'View')
view.attrib["fileUID"] = str(uuid.uuid4())
view.attrib["name"] = str('Coordinates')

for index, row in df.iterrows():
    viewCoordinatesList = et.SubElement(view, 'ViewCoordinatesList')
    viewCoordinates = et.SubElement(viewCoordinatesList, 'ViewCoordinates')
    itemFileUID = et.SubElement(viewCoordinates, 'ItemFileUID')
    itemFileUID.attrib['fileUID'] = str(row['ID'])
    viewCoordinatePairLon = et.SubElement(viewCoordinates, 'ViewCoordinatePair', name = 'longittude')
    viewCoordinatePairLon.attrib['Value'] = str(row['Y'])
    viewCoordinatePairLat = et.SubElement(viewCoordinates, 'ViewCoordinatePair', name = 'latitude')
    viewCoordinatePairLat.attrib['Value'] = str(row['X'])
    viewCoordinatePairH = et.SubElement(viewCoordinates, 'ViewCoordinatePair', name = 'height')
    viewCoordinatePairH.attrib['Value'] = str(row['Z'])

et.ElementTree(root).write('test.xml', pretty_print=True, xml_declaration = True, encoding='UTF-8', standalone = None)

assets.csv is as follows:

ID,CODE,ELR,TRID,DIR,MILEAGE,X,Y,Z,DESC,IMAGE
30734,S1,LEC1,1100,,008+0249 (9-1497),518169.12,185128.27,37.52,,Asset30734.jpg
31597,S10,LEC1,1100,,008+0286 (9-1460),518151.38,185157.1,36.7,IRJ at 8 miles and 0289 yards,Asset31597.jpg
31598,S10,LEC1,1100,,008+0286 (9-1460),518150.4,185156.11,36.7,IRJ at 8 miles and 0289 yards,Asset31598.jpg
31596,S10,LEC1,1100,,008+0287 (9-1458),518149.76,185157.14,36.7,IRJ at 8 miles and 0289 yards,Asset31596.jpg

The real issue with repeating yourself here is that you are iterating over your dataframe twice when you don't have to. Fortunately, the devs for lxml make the elements behave like lists, so you can create everything that you need to in one go, then just do root.append(sub_element) at the end like you would with a list.

Whenever you find yourself directly iterating over a pandas dataframe, that's usually a sign that it's not the correct data structure for the task. There are certainly exceptions, but I think using the csv module here might be a bit better of a fit. The reason being that you are technically iterating over the whole file with pd.read_csv, only to iterate again. csv.reader will allow you to iterate and process at the same time.

First, declare all of the trees you might need:

import csv
from lxml import etree as et

# configure your elements here. lxml acts as a list,
# so make them their own trees until you are ready to merge
root = et.Element('SignallingSchemeData', xmlns='boo')
source = et.Element('Source')
view = et.Element('View')

Note that I haven't used the SubElement factory class yet. Since Element can be treated as a list, we can leave the linking of elements to root to the end of the script.

Moving forward:

# you want this inserted first, so SubElement is
# perfectly fine here
timeframe = et.SubElement(root,'TimeFrame')
timeframe.attrib["fileUID"] = str(uuid.uuid4())
# you don't need the str function call here, it's already a string
timeframe.attrib["name"] = "timeframe 1"

# open your file here
with open('assets.csv') as fh:
    # the separator by default is ','
    # so no need to specify it
    reader = csv.DictReader(fh)

Now, csv.DictReader allows you to keep the row[key] paradigm because it will create an OrderedDict for each row:

with open('assets.csv') as fh:
    reader = csv.DictReader(fh)
    row = next(reader)

row
OrderedDict([('ID', '30734'), ('CODE', 'S1'), ('ELR', 'LEC1'), ('TRID', '1100'), ('DIR', ''), ('MILEAGE', '008+0249 (9-1497)'), ('X', '518169.12'), ('Y', '185128.27'), ('Z', '37.52'), ('DESC', ''), ('IMAGE', 'Asset30734.jpg')])

row['ID']
'30734'

row['CODE']
'S1'

This requires minimal changes from your original script. You can iterate over the reader just like before except the index doesn't have to be there anymore:

with open('assets.csv') as fh:
    reader = csv.DictReader(fh)

    for row in reader:
        # rest of loop

Refactoring the loop

Equipment

First, you can create a function to handle everything that's happening in the if block. This makes the core of the loop more readable, as well as logically separates functions of the code. Overall, the core logic is good. Your variable names are descriptive, if a little lengthy, with the only change really being formatting:

def add_equipment(root, row):
    '''Add equipment elements and supporting child elements to root'''
    equipment = et.SubElement(root, 'Equipment')

    # I'm grouping blocks of code/variables that group together
    signal_equipment = et.SubElement(equipment, 'SignalEquipment')
    signal_equipment.attrib["fileUID"] = row["ID"]
    signal_equipment.attrib["name"] = row["DESC"]

    # The names have been changed to snake_case with lowercased
    # letters, as is the naming conventions of variables and functions
    # in python (but not classes)
    equipment = et.SubElement(root, 'Equipment')
    eq_support = et.SubElement(equipment, 'EquipmentSupportEquipment')
    eq_support.attrib["fileUID"] = str(uuid.uuid4())

    reference = et.SubElement(signal_equipment, 'EquipmentSupportReference').text = eq_support.attrib["fileUID"]

I don't need to return anything because root is being modified in-place by these operations.

Now, the first portion of your loop looks like this:

with open('assets.csv') as fh:
    reader = csv.DictReader(fh)

    for row in reader:
        if row['CODE'] == 'S1':
            # now all of this code sits
            # in a function, and it's much easier to tell what's
            # going on
            add_equipment(root, row)
        else:
            equipment = et.SubElement(root, 'Equipment')

View

The view can be cleaned up in pretty much the same manner. Again, I don't really see anything egregious other than just some logical grouping and formatting. Your code and logical flow looks good, it's clear and easy to read

def add_view(view, row):
    '''Add supporting child elements to view. Pass in row for value extraction'''
    view_list = et.SubElement(view, 'ViewCoordinatesList')

    view_coords = et.SubElement(view_list, 'ViewCoordinates')

    item_file_uid = et.SubElement(view_coords, 'ItemFileUID')
    item_file_uid.attrib['fileUID'] = row['ID']

    longitude = et.SubElement(view_coords, 'ViewCoordinatePair', name='longittude')
    longitude.attrib['Value'] = row['Y']

    latitude = et.SubElement(view_coords, 'ViewCoordinatePair', name='latitude')
    latitude.attrib['Value'] = row['X']

    height = et.SubElement(view_coords, 'ViewCoordinatePair', name='height')
    height.attrib['Value'] = row['Z']

You don't need the str to wrap row['X'] anymore because csv.DictReader doesn't infer datatypes, everything is a string.

The loop as a whole now looks like this:

with open('assets.csv') as fh:
    reader = csv.DictReader(fh)

    for row in reader:
        if row['CODE'] == 'S1':
            # refactor to a function here that
            # adds to root and takes row as an argument
            add_equipment(root, row)
        else:
            equipment = et.SubElement(root, 'Equipment')

        # add the rest of your loop in a function here to
        # deal with the view tree, again, take row as an argument
        add_view(view, row)

Now, your loop can just be read as a loop, and the logic inside the if blocks is abstracted into functions.

Constructing your tree

Now, to get everything in order, it's just a call to root.append:

root.append(source)
root.append(view)

And everything is in order with one pass over the csv file. Your last line can remain perfectly intact:

et.ElementTree(root).write('test.xml', pretty_print=True, xml_declaration=True, encoding='UTF-8', standalone=None)

Timing Results

Original Script

python -m timeit 'import file'
10 loops, best of 3: 0.118 usec per loop

Refactored Script

python -m timeit 'import file2'
10000000 loops, best of 3: 0.0809 usec per loop