Building a "LIKE" query in Yii2 This code makes an SQL query. The last andWhere() method adds a LIKE condition with one percentage sign at the right side of the string.
I'm wondering if there is a better way to do it.
public function prepareDataProvider()
{
    /* @var $modelClass \yii\db\ActiveRecord */
    $modelClass = $this->modelClass;

    $query = $modelClass::find()
        ->where('published=1')
        ->select([
            'id',
            'city_id',
            'city_district_id',
            'seller_net_id',
            'name',
            'address',
            'type',
        ]);

    // filters
    if ($city_id = \Yii::$app->request->getQueryParam('city_id'))
        $query->andWhere('city_id = :city_id',[':city_id'=>$city_id]);
    if ($city_district_id = \Yii::$app->request->getQueryParam('city_district_id'))
        $query->andWhere('city_district_id = :city_district_id',[':city_district_id'=>$city_district_id]);
    $city_name = \Yii::$app->request->getQueryParam('name');
    if ($city_name && mb_strlen($city_name) >= \Yii::$app->params['minSearchChars'])
        $query->andWhere(['LIKE' ,'name',strtr($city_name,['%'=>'\%', '_'=>'\_', '\\'=>'\\\\']).'%', false]);

    return new ActiveDataProvider([
        'query' => $query
    ]);
}

The question

The last andWhere() method adds a LIKE condition with one percentage sign at the right. I'm wondering if there a better way to do it.

Hmm... let us look at that last addWhere() line:

$query->andWhere(['LIKE' ,'name',strtr($city_name,['%'=>'\%', '_'=>'\_', '\\'=>'\\\\']).'%', false]);

That is quite a long line - and appears to have manual character escaping, which isn't a bad thing but not something you want to handle manually if you can help it. Since you are using bound parameters for the first two parameters you likely know that they can help prevent SQL injection attacks and it would be wise to find a way to bind a parameter for that last where condition.
As this StackOverflow answer suggests, a bound parameter could still be used with the LIKE condition - then you might not need to escape as many characters:
$query->andWhere(new \yii\db\Expression('name LIKE :city_name', [':city_name' => $city_name . '%']));

But if the presence of certain characters like % in the city name still present a problem then continue to escape them
Other feedback
The code is very easy to read. Perhaps it was written to be compliant with PSR-2, which is now deprecated1, though it mostly is compliant with PSR-12. I like the fact that the short array syntax is used- I wish I had started doing that back then!
I see it contains assignment within the conditionals - e.g.

    if ($city_id = \Yii::$app->request->getQueryParam('city_id'))

While it doesn't take much to notice that the variables are later used within the statement that follows, it might be nice to help readers with a comment above the group that says something like intentional assignment.


Playing “craps” for the win v0.3 Continud from Playing “craps” for the win v0.2

  You roll two dice. Each die has six faces, which contain one, two,
  three, four, five and six spots, respectively. After the dice have
  come to rest, the sum of the spots on the two upward faces is
  calculated. If the sum is 7 or 11 on the first throw, you win. If the
  sum is 2, 3 or 12 on the first throw (called “craps”), you lose (i.e.,
  the “house” wins). If the sum is 4, 5, 6, 8, 9 or 10 on the first
  throw, that sum becomes your “point.” To win, you must continue
  rolling the dice until you “make your point” (i.e., roll that same
  point value). You lose by rolling a 7 before making your point.

// Craps.java

package co.vu.xxx.craps;

import java.security.SecureRandom;

public class Craps {
    public static enum Outcome {UNDEFINED, WIN_FIRST, WIN_POINT, LOSE_FIRST, LOSE_POINT};
    private int currentNumber;
    private int point;
    private Outcome gameResult = Outcome.UNDEFINED;
    private static SecureRandom randomNumbers = new SecureRandom();

    public static int rollDice() {
        int randN = 2 + randomNumbers.nextInt(6) + randomNumbers.nextInt(6);
        return randN;
    }

    public void firstRoll() {
        currentNumber = rollDice();
        switch (currentNumber) {
            case 7:case  11:
                gameResult = Outcome.WIN_FIRST;
                break;
            case 2:case 3:case 12:
                gameResult = Outcome.LOSE_FIRST;
                break;
            default:
                point = currentNumber;
        }
    }

    public void moreRolls() {
        currentNumber = rollDice();
        if(currentNumber == point) {
            gameResult = Outcome.WIN_POINT;
        } else if(currentNumber == 7) {
            gameResult = Outcome.LOSE_POINT;
        }
    }

    public Outcome getGameResult() {
        return gameResult;
    }

    public int getPoint() {
        return point;
    }

    public int getCurrentNumber() {
        return currentNumber;
    }
}

// UseCraps.java

package co.vu.xxx.craps;

import java.io.IOException;
import java.util.Scanner;

public class UseCraps {
    public static void main(String[] args) throws IOException {
        Craps crapObj = new Craps();
        waitUser();
        crapObj.firstRoll();
        declareResult(crapObj);
        while(crapObj.getGameResult() == Craps.Outcome.UNDEFINED) {
            waitUser();
            crapObj.moreRolls();
            declareResult(crapObj);
        }
    }
    public static void declareResult(Craps pCrap) {
        System.out.printf("You got a %d\n", pCrap.getCurrentNumber());
        int point = pCrap.getPoint();
        if(point != 0) {
            System.out.printf("You need to make your point: %d\n", point);
        }
        switch (pCrap.getGameResult()) {
            case LOSE_FIRST:
                System.out.println("You have lost because of hitting crap in the first round.");
                break;
            case WIN_FIRST:
                System.out.println("You have won, you got a lucky number in the first round!");
                break;
            case LOSE_POINT:
                System.out.println("You have lost, you hit 7 before making your point!");
                break;
            case WIN_POINT:
                System.out.println("You have won, you made your point!");
                break;
            case UNDEFINED:
                System.out.println("Game continues...\n");
                break;
        }
        System.out.println();
    }

    private static void waitUser() {
        Scanner inputScan = new Scanner(System.in);
        System.out.print("Press enter to start rolling: ");
        String input = inputScan.nextLine();
    }
}

Much of this is nitpicking, but since you keep posting revisions, you seem to want a really thorough review.

The parameter name pCrap to UseCraps.declareResult() isn't explanatory. Try renaming it to something like game instead.
I suggest doing the same for crapObj in main().
getGameResult() should probably be named getState() or something similar, since result implies that it has finished, and there is an UNDEFINED state.
Speaking of which, the state isn't really undefined, it's just unfinished. It should be renamed to something that doesn't imply an error condition.
To be super finicky, you could be declaring your local variables randN, currentNumber, crapObj, point, and inputScan as final.
In Craps.rollDice() you could just return the value instead of storing it in a variable and then immediately returning. The method should also be private.
UseCraps.declareResult() should be private.
In UseCraps.waitUser() you don't need to store the line in input since you don't use it.
Your switch in UseCraps.declareResult() doesn't have a default.
The Craps class should only expose one roll method. It may make sense to keep the two as separate private ones, but you should simplify what your callers need to do to use your class. Consider what happens if someone calls firstRoll and never calls moreRolls, or if they call moreRolls first, or if they go back and forth.
Consider adding a reset method to Craps so you can start a new game without having to create a new instance.
You should check if you are already in a win state when someone calls roll.


Enhance MVC implementation I know that there are several ways to develop the MVC pattern. To improve consistently my knowledge, I would like to get some advice and feedback. How could I enhance my code? I knew that there are several lines of code, but it would be great if you help me improve my skills.

The Model store the object Person and notify the view if there are any changes. The view implements the interface IModelPersonObserver to receive the notification. The view notify the controller about any changes. The controller starts a new task for long calculations. The result is passed to the model.

To pass different events I am able to use multiple events, like ModelHandler and ViewHandler based on the real action or just one event and extend the EventArgs class.

IModelPerson

public delegate void ModelHandler<IModelPerson>(IModelPerson sender, ModelEventArgs e);

public class ModelEventArgs : EventArgs
{
    public Person person;
    public ModelEventArgs(Person p) { person = p; }
}

public interface IModelPersonObserver
{
    void Add(IModelPerson model, ModelEventArgs e);
}

public interface IModelPerson : IModel
{
    void Add(Person person);

    void Remove(Person person);

    void Observe(IModelPersonObserver obsever);
}

ModelPerson

public class ModelPerson : IModelPerson
{
    private readonly ArrayList persons = new ArrayList();

    public event ModelHandler<ModelPerson> change;

    public void Add(Person person)
    {
        this.persons.Add(person);
        this.change.Invoke(this, new ModelEventArgs(person));
    }

    public void Remove(Person person)
    {
        this.persons.Remove(person);
    }

    public void Observe(IModelPersonObserver obsever)
    {
        this.change += obsever.Add;
    }
}

IView

public interface IView
{
    Controller Controller { set; }
}

IViewPerson

public delegate void ViewHandler<IViewPerson>(IViewPerson sender, ViewEventArgs e);

public class ViewEventArgs: EventArgs
{
    public Person person;
    public ViewEventArgs(Person p) { person = p; }
}

public interface IViewPerson : IView
{
    event ViewHandler<IViewPerson> change;
}

ViewPerson

public partial class ViewPerson : Form, IViewPerson, IModelPersonObserver
{
    private Controller controller;

    public event ViewHandler<IViewPerson> change;

    public ViewPerson()
    {
        InitializeComponent();
    }

    public Controller Controller
    {
        set { this.controller = value; }
    }

    private void button1_MouseClick(object sender, MouseEventArgs e)
    {
        var person = new Person();
        person.Name = "Name";
        person.Surname = "Surname";

        this.change.Invoke(this, new ViewEventArgs(person));
    }

    public void Add(IModelPerson model, ModelEventArgs e)
    {
        MessageBox.Show("e");
    }
}

Controller

public abstract class Controller
{
    protected readonly IModel model;

    protected readonly IView view;

    protected Controller(IModel model, IView view)
    {
        this.model = model;
        this.view = view;

        this.view.Controller = this;
    }
}

ControllerPerson

public class ControllerPerson : Controller
{
    private static ModelPerson modelPerson;

    public ControllerPerson(ModelPerson model, ViewPerson view) : base(model, view)
    {
        ControllerPerson.modelPerson = model;
        ControllerPerson.modelPerson.Observe(view);

        view.change += ViewOnChange;
    }

    private static void ViewOnChange(IViewPerson sender, ViewEventArgs viewEventArgs)
    {
        Task.Factory.StartNew(() => Add(viewEventArgs.person));
    }

    private static void Add(Person state)
    {
        Thread.Sleep(5000);
        ControllerPerson.modelPerson.Add(state);
    }
}

What do you think?

public interface IView
{
    Controller Controller { set; }
}

NO!!
You have just coupled the View with the Controller, which defeats the entire purpose.
This is also broken:

public abstract class Controller
{
    protected readonly IModel model;

    protected readonly IView view;

    protected Controller(IModel model, IView view)
    {

Now you have a circular dependency, with the controller knowing about the view, and the view knowing about the controller - but the controller knowing about the view and model is fine:

I don't understand this line:

Thread.Sleep(5000);

Why sleep for five whole seconds?

I'll just add that the naming convention, as noted by BCdotNET, is off: ControllerPerson should be PersonController - not because that's how ASP.NET MVC has it, but because C# and the .NET framework in general does it that way; it's just a matter of consistency... and it's what someone maintaining your code would expect to see.


Pointers and string reverse function implementation char * reverse_string(const char *s1)
{
int count = 0;
char *output;
char *o;
while(*s1 != '\0')
    s1++;

s1--;
output = new char[strlen(s1)];
o = output;

while(*s1)
    *output++ = *s1--;

if(*s1 == '\0')
    cout<<"yes";
return o;
}

int main()
{
const char *s1 = "hello world";
char *result;
result = reverse_string(s1);
cout<<result;
}

This is a way of implementing string reverse.  I know the code uses extra space and strlen(), which is not optimal.

But, my question is, when I decrement the s1 pointer one location from the beginning of the input string, it has '\0' as its value. I do not understand how that could be feasible? For testing, I am printing an "yes" (at the end of function strrevImp()).

Could someone point out if I am lost anywhere here?

indent your code. Indentation should normally reflect control flow:

char * reverse_string(const char *s1)
{
    int count = 0;
    char *output;
    char *o;
    while (*s1 != '\0')
        s1++;

    s1--;
    output = new char[strlen(s1)];
    o = output;

    while (*s1)
        *output++ = *s1--;

    if (*s1 == '\0')
        cout << "yes";
    return o;
}

int main()
{
    const char *s1 = "hello world";
    char *result;
    result = reverse_string(s1);
    cout << result;
}

When you're manipulating strings in C++, prefer std::string over pointers to char. Likewise, prefer standard algorithms to duplicating their functionality. Using std::string, and a standard algorithm, your core function can reduce to something like this:

std::string reverse_string(std::string in) {
    std::reverse(in.begin(), in.end());
    return in;
}

Prefer to initialize variables rather than creating them uninitialized, then assigning a value later. For example, this:

char *result;
result = reverse_string(s1);
cout<<result;

...would almost certainly be better off as:

 char *result = reverse_string(s1);

Avoid intermediate variables unless they actually contribute something useful. The sequence above would probably be just as well off being written as:

std::cout << reverse_string(s1);

Avoid using namespace std;. Although you don't actually show this in your code, using cout without std:: indicates that you probably have it elsewhere in your code. If you use this at all, you generally want to restrict it to a relatively small scope (but it's generally better to avoid it completely).
If you insist on allocating space for the string "by hand" (i.e., directly invoking new char[len], be sure to include enough space for the NUL terminator. Right now you have:

output = new char[strlen(s1)];

...which allocates space for the characters in the string, but no space for the zero-byte that signals the end of the string. Along with this, when you copy the data, you need to ensure that you put a zero-byte at the end of the result to signal where it ends. But, see point number 2 above--despite its (many) flaws, you're much better off using std::string, and avoiding this problem entirely.
A function should have a single intent. Right now, your reverse_string not only reverses its input, but also prints out "yes". This restricts that function to being using only under very specific circumstances. By strong preference, a function that reverses a string should do only that, without printing out anything (and the same applies in general, not just to this specific function).


Advent of Code - Day One I tried to write a foolproof function for the first part of the first problem on adventofcode, which asks for the count of ( characters minus the count of ) characters in a string.

The function intentionally ignores any other characters in the input string.

function findEndFloor(floors, upFloorSymbol, downFloorSymbol) {
  if (typeof floors !== 'string')
      throw 'parameter 1 is expected to be a string';
  if (typeof upFloorSymbol !== 'string')
      throw 'parameter 2 is expected to be a string';
  if (typeof downFloorSymbol !== 'string')
      throw 'parameter 3 is expected to be a string';

  var count = 0;
  var chars = floors.split('');
  for (var i = 0; i <  chars.length; i++) {
      if(chars[i] === upFloorSymbol || chars[i] === downFloorSymbol)
          count += (chars[i] === upFloorSymbol ? 1 : -1);
  }

  return count;
}

Any feedback will be appreciated.

Alternate solution

First off, I want to provide an alternate solution.

Let's analyze the sample inputs and outputs provided:

  Input:

(()(()(

  Output:

3

Now, let's count how many of each character there is:

(: 5 occurrences
): 2 occurrences

Well, if ( is the "up" character and ) is the down character, subtracting their occurrences gives you the floor number.

Don't believe me? Here's another example:

  Input:

))(((((

  Output:

3

(: 5 occurrences
): 2 occurrences

5 - 2 = 3; again, the floor number is revealed.

Basically, it doesn't matter the order of characters; you are always going to end up on the same floor. Now your implementation can be simplified: count up the occurrences of each character and subtract them for your result.

var upOccurrences = floors.match(new RegExp(upFloorSymbol, "g")).length;
var downOccurrences = floors.match(new RegExp(downFloorSymbol, "g")).length;

return upOccurrences - downOccurrences;

Note that JavaScript may yell at you for using the above code depending on the character passed in. For example, if you use parentheses, you will get an error even if they are escaped in the argument list. Here is how to fix that:

  If you want to fix that regex error then just provide the parentheses
  as "\\(" and "\\)". That will change the regex to be /\(/g and /\)/g
  :)

  -tkellehe

An alternative solution for finding the amount of characters in a string would be this:

var occurrences = string.split(character).length - 1;

However, I am unsure if this is good practice in JavaScript (I know it's not in Java).

Review

Now, for a review on your code.

Splitting an array into the same array

var chars = floors.split('');

In JavaScript, strings are just like arrays with the exception that strings are immutable. Since you are not modifying this chars array as you loop through it, it is unnecessary to split it as you can just treat the string like an array.

Stop throwing strings at me! I can't trace them!

throw 'parameter 1 is expected to be a string';

This is bad practice. When an error is thrown like this, there is no way to search the stack trace for where the problem occurred. Also, you get a pretty weird console message:

  Uncaught parameter 1 is expected to be a string

That doesn't even make sense (grammatically).

You should be throwing an instance of an Error, like this:

throw new Error(...);

Now, when this error is thrown, a better message and a stack trace are provided. This makes tracking down where the error occurred a lot more easy:

  Uncaught Error: message (stack trace)

Even better, you can be more specific and JavaScript's built-in errors like TypeError, especially since you are dealing with the types of the arguments.

Half the size for twice the conditionals

  if(chars[i] === upFloorSymbol || chars[i] === downFloorSymbol)
      count += (chars[i] === upFloorSymbol ? 1 : -1);

I see what you are trying to do here. You are trying to make your code shorter by first squishing two conditionals into one, and then using a ternary.

This is a bad idea. With how you did it, you are now forced to create two more conditions (creating a total of 4, or something like that) to check when you really only need two. These extra conditions unnecessarily slow down your code.

Instead of combining two conditionals here:

  if(chars[i] === upFloorSymbol || chars[i] === downFloorSymbol)

Split it up like this:

if(chars[i] === upFloorSymbol) {
    count += 1;
} else if(chars[i] === downFloorSymbol) {
    count -= 1;
}

Now you don't need that extra ternary.

Braces keep your code straight

Always use braces with conditionals, loops, etc.


Dictionary-based dispatch in Python with multiple parameters Is there a better way to express the dispatch (switch) code without using a lambda? I think I need the lambda to act as an intermediary because the functions have varying numbers of parameters.

def printf(arg):
    print arg

def printf2(arg1, arg2):
    print arg1
    print arg2

def printn(arg):
    print arg

action = 'op2'
functions = {
    'op1': lambda: printf('op1 called'),
    'op2': lambda: printf2('op2 called', 'param 2'),
    }
functions.get(action, lambda: printn('default op'))()

You could change it to just store the functions and the arguments.  Then use the argument unpacking to fill in the correct arguments to the function call.

action = 'op2'
functions = {
    'op1': (printf, ('op1 called',)),
    'op2': (printf2, ('op2 called', 'param 2')),
}

fn, args = functions.get(action, (printn, ('default op',)))
fn(*args) # call the function unpacking the provided arguments

A better solution in this particular case IMHO, rather than defining multiple functions with varying number of arguments, you could just define one function accepting variable arguments to achieve the same thing.

def printf(*args):
    if args:
        for arg in args:
            print arg
    else:
        print 'default op'

Then it's just a matter of storing only the arguments to the call rather than the functions themselves:

action = 'op2'
arguments = {
    'op1': ('op1 called',),
    'op2': ('op2 called', 'param 2'),
}

printf(*arguments.get(action, ()))


Display factorial with user-entered number This needs some optimizing without any additional variables or loops. The difficult part of the task was no additional variables subroutines/functions or loops. If that seems (near) impossible then break those rules.

The output should look like:

  5 X 4 X 3 X 2 X 1 = 120

Private Sub FactorialCalculation()

 Dim Number, Factorial As Integer

 Console.WriteLine("Please enter a number")
 Number = CInt(Console.ReadLine)
 Factorial = 1

 Console.WriteLine("Your number is " & Number)

 If Number > 0 Then
     Console.Write(Number & " X ")
     For Count As Integer = 1 To Number
         If (Number - Count) > 0 And (Number - Count) <> 1 Then
             Factorial *= Count
             Console.Write("{0} X ", Number - Count)
         ElseIf (Number - Count) = 0 Then
            Factorial *= Count
         ElseIf (Number - Count) = 1 Then
             Factorial *= Count
             Console.Write(Number - Count)
         End If

     Next
     Console.WriteLine(" = {0}", Factorial)
 Else
     Console.WriteLine("Please enter a positive number greater than 0")
 End If
End Sub

The difficult part of the task was no additional variables, subroutines/functions, or loops.

I'll keep that in the back of my head, but it's far more important to know what tools to use when, than being able to implement functionality x using language features y or z.

It's also much more important to know how to separate concerns and why - this "no additional subroutines/functions" so-called requirement is a very questionable idea, especially if that's something someone is tasking you to do.

In the real world, your client (or boss, or project manager, or whatever) tells you what the program needs to do: how it does it is at your [team's] discretion, and you[r team] will want to write code that's easy to maintain and extend, because that client never seems to stop adding new requirements - and if you do it wrong, one day a new requirement will come and shatter some fundamental assumption your code was making, and you need to rewrite a big part of it, and then doing so you introduce bugs that you will want to identify and fix quickly.

Writing a "god procedure" that literally does everything goes against every sane design principle, so I'm going to toss that non-functional bogus requirement away - because what you really want is to write maintainable, extensible code that's easy to read and easy to debug.

The "god procedure" also fails at abstraction, which is one of the four pillars of Object-Oriented Programming (OOP), and happens to be the paradigm VB.NET was designed for.

The functional requirements are:

User needs to be able to input a number, which must be a positive integer.
The program needs to compute the factorial of that number.
The program needs to output the result and the calculations made to get there.

I'll take the specified output as a firm functional requirement, and add:

For input 5, the program should output 5 X 4 X 3 X 2 X 1 = 120.

That's what the program needs to do - these are the main concerns. We'll see if additional variables or loops are needed as we see fit. Let's start.

Review

I like starting with the signature:

Private Sub FactorialCalculation()

Procedures do something; they're actions. By convention, procedure names start with a verb, and describe what's going on. Their parameters have meaningful names, like local variables; nouns that describe their purpose.

Naming is already hard, but a procedure that does too many things is even harder to name accurately: we know FactorialCalculation has something to do with calculating factorials, but what's going on here is impossible to tell just by reading the name of the procedure: even CalculateFactorial would be a misleading name, because it doesn't talk about user input. Looking at the code, the only name that comes to mind is PromptAndValidateUserInputAndCalculateFactorialAndOutputResult, which sounds ridiculous for a reason.

Dim Number, Factorial As Integer

If you're writing VBA as well as VB.NET, this is a dangerous way to declare your variables: in VBA Number would be an implicit Variant, and while it's great that VB.NET fixed that annoyance, it's better to declare variables on separate instructions, as close as possible to their first use - and ideally assigning them a value.

Console.WriteLine("Please enter a number")
Number = CInt(Console.ReadLine)

CInt is perhaps a VB-ish way to do this, but as a C# dev looking at it, you wonder how that conversion function is going to do with a non-numeric string input, for example - whereas more .NET-idiomatic conversion methods leave no ambiguity about what's going on:

Dim input As String = Console.ReadLine()
Dim value As Integer
Dim isInteger As Boolean = Integer.TryParse(input, value)

So, the output needs to be in descending order. Why is the loop ascending then?

For Count As Integer = 1 To Number

Seeing how almost every reference to Count is for computing Number - Count, I would have reversed the loop:

For Count As Integer = Number To 1 Step -1

Lastly, in order to know whether the output matches what's expected, we need to mentally iterate that loop and "build" the output - that's not ideal.

There's a better way.

Testability

Let's say you want to test whether your program fulfills all the requirements. You can run the program and see it with your own eyes - and that's a mandatory thing to do every once in a while. But testing your code like this, for every single edge case, and re-doing all the tests every time you make a change to the code, gets tedious. Professional code is testable, meaning that you can write code that executes your program and verifies that it works correctly: here there's only a handful of points to clear, but in a real code base there could be thousands of tests that run every time something changes in the code. These tests not only help identifying regression bugs, they also document the requirements and assumptions the code is making.

  QA engineer walks into a bar. Orders a beer. Orders 0 beers. Orders 999999999 beers. Orders a lizard. Orders -1 beers. Orders a sfdeljknesv.

  https://twitter.com/JimFraeErskine/status/527564769302433792

Before you even write a single line of code, you can look at the requirements and, knowing that a 32-bit integer input is maxed at 12 and a 64-bit integer is maxed at 20, start thinking of what you'll need to test:

Whether the user input is deemed valid with a negative integer
Whether the user input is deemed valid with sfdeljknesv for input
Whether the user input is deemed valid with a positive integer under 12
Whether the user input is deemed valid with a positive integer under 20
Whether the user input is deemed valid with a positive integer above 20
Whether the program logic correctly computes the factorial of 0
Whether the program logic correctly computes the factorial of a positive integer under 12
Whether the output format matches the specifications

That's 8 tests we know we can write right away. There's a development methodology called Test-Driven Development that works exactly like this: you actually write the tests, then you write the code that makes these tests pass. One of the benefits of that methodology is that it's impossible to write untestable code, since you're only ever implementing just enough code to make a test pass.

Your implementation will throw unhandled exceptions with several of these tests, but the problem is that it's impossible to test a single procedure that does everything.

Separating Concerns

If we wanted to merely be able to write these tests, we would need separate procedures for:

Validating user input
Computing the factorial of a given integer
Generating the output

One implementation that comes to my mind would leverage an iterator to Yield a counter, and an <Out()> ByRef parameter to output the current result. Something like this:

Public Iterator Function CalculateFactorial(ByVal value As Integer, <Out()> ByRef result As Integer) As IEnumerable(Of Integer)
    If result < 0 OrElse result > 12 Then Throw New ArgumentException()

    result = 1
    For i As Integer = value To 1 Step -1
        result *= i
        Yield i
    Next

End Function

The calling code could look like this:

Dim input As Integer = 12
Dim result As Integer
Dim values As IEnumerable(Of Integer) = CalculateFactorial(input, result)

Notice how decoupling the user input from the factorial-computing logic now allows us to test the logic with any given valid or invalid Integer input. Also notice that the logic is separated from the formatted output we're going to end up with.

If we wrote unit tests for it, we could already test all these:

Whether and how the program logic handles invalid input (throws an ArgumentException)
Whether the program logic correctly computes the factorial of 0 (returns 1 as expected)
Whether the program logic correctly computes the factorial of a positive integer under 12 (does it?)

To test the input validation logic, we would have to implement a separate function - one that takes a String input and returns a Boolean, for example:

Public Function IsValidInput(ByVal input As String, <Out()> ByRef result As Integer) As Boolean
    If String.IsNullOrEmpty(input) OrElse Not IsNumeric(input) Then Return False

    Dim value As Integer
    Dim isInteger As Boolean = Integer.TryParse(input, value)
    If isInteger AndAlso value > 0 AndAlso value < 12 Then
        result = value
        Return True
    End If
End Function

With that function we can test all these:

Whether the user input is deemed valid with a negative integer
Whether the user input is deemed valid with sfdeljknesv for input
Whether the user input is deemed valid with a positive integer under 12
Whether the user input is deemed valid with a positive integer under 20
Whether the user input is deemed valid with a positive integer above 20

And now the calling code looks like this:

Dim input As Integer
If IsValidInput(Console.ReadLine(), input) Then
    Dim result As Integer
    Dim values As IEnumerable(Of Integer) = CalculateFactorial(input, result)
Else
    Console.WriteLine("Invalid input.")
End If

All that's left to do, is to assemble the yielded values into a String, and output it. A dedicated function can do this:

Public Function GenerateOutput(ByVal IEnumerable(Of Integer) values, ByVal result As Integer) As String
    Return String.Format("{0} = {1}", String.Join(" X ", values), result)
End Function

And now this test can pass:

Whether the output format matches the specifications

Result

Putting it all together, the resulting code might look like this - notice how the higher abstraction level of the Main procedure makes it easy to instantly grasp what's going on, with as little effort as it takes to just read it; the output is sent to the console once, and all the lower-level code can be tested independently:

Public Sub Main()
    Dim input As Integer
    If IsValidInput(Console.ReadLine(), input) Then
        Dim result As Integer
        Dim values As IEnumerable(Of Integer) = CalculateFactorial(input, result)
        Console.WriteLine(GenerateOutput(values, result))
    Else
        Console.WriteLine("Invalid input.")
    End If
End Sub

Public Function IsValidInput(ByVal input As String, <Out()> ByRef result As Integer) As Boolean
    If String.IsNullOrEmpty(input) OrElse Not IsNumeric(input) Then Return False

    Dim value As Integer
    Dim isInteger As Boolean = Integer.TryParse(input, value)
    If isInteger AndAlso value > 0 AndAlso value < 12 Then
        result = value
        Return True
    End If
End Function

Public Iterator Function CalculateFactorial(ByVal value As Integer, <Out()> ByRef result As Integer) As IEnumerable(Of Integer)
    If result < 0 OrElse result > 12 Then Throw New ArgumentException()

    result = 1
    For i As Integer = value To 1 Step -1
        result *= i
        Yield i
    Next

End Function

Public Function GenerateOutput(ByVal IEnumerable(Of Integer) values, ByVal result As Integer) As String
    Return String.Format("{0} = {1}", String.Join(" X ", values), result)
End Function

Is this the best way to implement it? Probably not. But it's testable, so go ahead and implement these tests, and then refactor the code to get rid of the <Out()> parameter in CalculateFactorial - if you break anything in the process, the tests will tell you.


Type-safe JSON deserialization In a Python (3.6) application I receive messages from Kafka in JSON format. (The code base makes heavy use of static type annotations, and every file is automatically checked using mypy --strict to catch type errors as early as possible.)

So I try to deserialize the received messages into objects immediately in order to avoid working with Dict[str, Any] instances in the downstream code. I'd like the deserialization to be type-safe, i.e., to not only fail if not all class members are defined in the JSON string, but also if one or more of them have an incorrect type.

My current approach looks as follows:

(The class Foo in the unit tests is an example of a typical target_class.)

#!/usr/bin/env python3

"""
Type-safe JSON deserialization
"""

import inspect
from typing import Any, List
import json
import unittest

def get_all_member_variable_names(target_class: Any) -> List[str]:
    """Return list of all public member variables."""
    all_members = [name for name, _ in inspect.getmembers(target_class,\
                       lambda a: not inspect.isroutine(a))]
    return list(filter(lambda name: not name.startswith("__"), all_members))

def deserialize_json(target_class: Any, object_repr: str) -> Any:
    """Constructs an object in a type-safe way from a JSON strings"""
    data = json.loads(object_repr)
    members = get_all_member_variable_names(target_class)
    for needed_key in members:
        if needed_key not in data:
            raise ValueError(f'Key {needed_key} is missing.')
    dummy = target_class()
    for needed_key in members:
        json_type = type(data[needed_key])
        target_type = type(getattr(dummy, needed_key))
        if json_type != target_type:
            raise TypeError(f'Key {needed_key} has incorrect type. '
                            '({json_type} instead of {target_type}')
    return target_class(**data)

class Foo():
    """Some dummy class"""
    val: int = 0
    msg: str = ''
    frac: float = 0.0

    def __init__(self, val: int = 0, msg: str = '', frac: float = 0.0) -> None:
        self.val: int = val
        self.msg: str = msg
        self.frac: float = frac

class TestDeserialization(unittest.TestCase):
    """Test with valid and invalid JSON strings"""

    def test_ok(self) -> None:
        """Valid JSON string"""
        object_repr = '{"val": 42, "msg": "hello", "frac": 3.14}'
        a_foo: Foo = deserialize_json(Foo, object_repr)
        self.assertEqual(a_foo.val, 42)
        self.assertEqual(a_foo.msg, 'hello')
        self.assertEqual(a_foo.frac, 3.14)

    def test_missing(self) -> None:
        """Invalid JSON string: missing a field"""
        object_repr = '{"val": 42, "msg": "hello"}'
        with self.assertRaises(ValueError):
            deserialize_json(Foo, object_repr)

    def test_incorrect_type(self) -> None:
        """Invalid JSON string: incorrect type of a field"""
        object_repr = '{"val": 42, "msg": "hello", "frac": "incorrect"}'
        with self.assertRaises(TypeError):
            deserialize_json(Foo, object_repr)

It works and the unit tests succeed, but I'm not sure if I am missing some potential problem or other opportunities to improve this. It would be cool if you could give me some hints or general criticism.

You need to:

Get the annotated signature of a target_class' __init__ method;
Apply whatever arguments come from the provided JSON string;
Check that all arguments are present;
Check that all arguments conform to the annotations of the class' __init__.

All except the last step is pretty straightforward using the inspect module:

import json
import inspect
from typing import Callable, Any, T

def deserialize_json(target_class: Callable[[Any], T], object_repr: str) -> T:
    data = json.loads(object_repr)
    signature = inspect.signature(target_class)
    bound_signature = signature.bind(**data)
    bound_signature.apply_defaults()
    return target_class(**bound_signature.arguments)

inspect.Signature.bind validate arguments pretty much the same as a real call to target_class.__init__ would; raising TypeError if a positionnal argument is missing or if an extra keyword argument is found.

Now you "just" need to validate the type of the arguments based on the annotations. A simple but potentially sufficient way of doing would check the annotation attribute of each parameter of the Signature and check it is the right type using isinstance:

def deserialize_json(target_class: Callable[[Any], T], object_repr: str) -> T:
    data = json.loads(object_repr)
    signature = inspect.signature(target_class)
    bound_signature = signature.bind(**data)
    bound_signature.apply_defaults()

    for name, value in bound_signature.arguments.items():
        expected_type = signature.parameters[name].annotation
        if not isinstance(value, expected_type):
            raise TypeError('<error message>')

    return target_class(**bound_signature.arguments)

Note that this simple example would most likely have troubles validating Generic types or parametrized Generic types. Adapting the code to fit those needs is left as an exercise for the reader (but Python 3.7 have better support for such checks).

As a side note, if all target classes ressemble the Foo example, and you don't mind having them immutable, you can make use of typing.NamedTuple:

class Foo(typing.NamedTuple):
    val: int
    msg: str
    frac: float = 1.0

print(Foo(1, 'bar'))  # Foo(val=1, msg='bar', frac=1.0)
print(Foo(2, 'baz', 0.0))  # Foo(val=2, msg='baz', frac=0.0)
print(Foo(3))  # TypeError: __new__() missing 1 required positional argument: 'msg'

or switch to Python 3.7 and use full-blown dataclasses.


Is this a valid implementation of a 'future'? I'm not very experienced in C++, but I am trying to circumvent a kind of 'constructor order' situation, where I have class A wants class B in its constructor, and vice versa. They don't really want eachother, but both implement an interface and need each other. I decided to give object A a future. Is this even remotely good practice in C++11?

I'm thinking that the code below should be fine - the shared_ptr simply serves to make the future passable by value - multiple future objects can represent the same 'future'. Since I am not experienced in C++, I'm not sure of any potential trouble. I think that I don't need to declare any special copy constructor or such, since I don't do any allocation and std::shared_ptr should be copy-constructible automatically.

What do you think?

#ifndef LIB_FUTURE
#define LIB_FUTURE

#include <stdexcept>
#include <memory>

namespace lib {
    template <typename TValue>
    class future
    {
    public:
      future() : value_ptr(std::make_shared<TValue*>())
      { }

      void set(TValue &value)
      {
        *value_ptr.get() = &value;
      }

      TValue& get()
      {
        assert_valid_access();
        return **value_ptr;
      }

      TValue* operator->()
      {
        assert_valid_access();
        return *value_ptr;
      }

      bool has_value()
      {
        return value_ptr.get() != nullptr;
      }

    private:
      std::shared_ptr<TValue*> value_ptr;

      void assert_valid_access()
      {
        if(!has_value())
          throw std::runtime_error("Attempting to access a non-set value from a future");
      }
    };

  }

#endif

UPDATE
The use case I have is a little more complicated than the above though. Here goes:

class view : public log_sink
{
    view(tcp_client &client);
    virtual sink members...
};

class tcp_client
{
    tcp_client(log) //a sink should be provided before this, since it will do some logging.
};

class log
{
  log(log_sink sink); //So this one needs view.
};

int main()
{
     log log(view);
     tcp_client client(log);
     view the_view(client);
}

The above is not possible. View needs client, but client needs log, but log needs access to an interface on view.

Instead, I opted to give log a promise of a sink in the future. Log implements a tiny buffering mechanism, on the off chance that it doesn't have a sink by the time someone wants logging done. When it finally gets a sink, everything works fine. :)

Like this:

int main()
{
  future<log_sink> promise;
  log log(promise);
  tcp_client client(log);
  view the_view(client);

  promise.set(view);
}

What's your inheritance relationship between A and B? If A is the parent of B, it's bad design if A needs to do ANYTHING with B in the constructor/destructor because B will not be valid until after A's construction.

If you require RAII design, look at lazy resource acquisition to avoid re-acquisition going up the constructor chain (vector).

Why do you want a smart pointer to a pointer. Smart pointers help you when there is no clear custodian of an instance (or shared primitive type), and you want to destroy an instance only when there are no more users of it. The way you are using the smart pointer means that only a memory location of a pointer is freed, not the memory of a type instance. Plus when you construct this class, the pointer held by the 'value_ptr' is random, which is bad.

I'm not sure where you're going with this 'future' class and how it solves your inter-dependance between A and B.

If A and B need a similar member, make them inherit from the same parent class. If you want to hide this 'shared' member or you want a pure-virtual interface, simply declare the interface as well as defining some sort of linking parent.

If A and B need access to the same instance of something, use a singleton that is inherited from a shared parent.

If you do need a shared instance, and you need to free any resources (IE files) held by the singleton, it's better to write an explicit free function. That way you will easily see where it's being cleaned up.

If you mean you want to avoid initializing a resource that's defined in a parent class, until your child class A or B does, perhaps look at making an explicit, polymorph function.

Hope that helps.


Asynchronous method and collection binding I have a WPF application, in which I'd like to bind a collection to a combobox using an asynchronous method :

<ComboBox  Margin="2,0,5,0" Width="178" ItemsSource="{Binding Animateur}" DisplayMemberPath="nom"   SelectedIndex="0"  />

the viewmodel class

_service.GetAnimateur((item, error) =>
                      {
                          if (error != null)
                          {
                              // TODO : traitement d'erreur
                          }
                          else
                          {
                              _Animateur.Clear();
                              item.ForEach(Elem =>
                              {
                                  _Animateur.Add(Elem);
                              });

                          }
                      });

the Asynchrounous method :

public async void GetAnimateur(Action<List<fiche>, Exception> callback)
        {
            try
            {
                Task<List<fiche>> data = (Task<List<fiche>>)Task.Run(
                    () =>
                    {
                        DataEntities _db = new DataEntities();
                        var dpcs = _db.fiche;
                        return new List<fiche>(dpcs);
                    });
                var result = await data;
                callback(result, null);
            }
            catch (Exception ex)
            {
                callback(null, ex);
            }
        }

I have 20128 items in the table fiche, the problem is that GetAnimateur takes a lot of time to fill the combobox.

What are the errors that I commited in this code?
How can I improve it?

First, I want to point out the behavior of method that is declared as async void. When the code reaches this line:

var result = await data;

The code following after _service.GetAnimateur is executed. async void is there only for event handlers. So using this signature creates more problems than it solves.

Second, I may not see the whole picture, but your GetAnimateur looks weird. I think it is because the problem I've mentioned earlier. The most readable and understandable may be code like:

public Task<IEnumerable<fiche>> GetAnimateur()
{
        return Task.Factory.StartNew(() =>
            {
                DataEntities _db = new DataEntities();
                return (IEnumerable<fiche>) _db.fiche;
            });
}

and consume like:

try
{
    var list = await GetAmateur();
    // success
}
catch (Exception ex)
{
    // fail
}

or even:

GetAmateur().ContinueWith(t =>
    {
        if (t.IsCompleted)
        {
            var list = t.Result;
            // success
        }
        else
        {
            var ex = t.Exception;
            // fail
        }
    }, TaskScheduler.FromCurrentSynchronizationContext());

Third, when you add or remove an item from ObservableCollection, the UI gets updated, slowing down performance. One common way to create an observable collection that supports painless multi-adding is

public class RangeObservableCollection<T> : ObservableCollection<T>
{
    private bool _suppressNotification = false;

    protected override void OnCollectionChanged(NotifyCollectionChangedEventArgs e)
    {
        if (!_suppressNotification)
            base.OnCollectionChanged(e);
    }

    public void AddRange(IEnumerable<T> list)
    {
        if (list == null)
            throw new ArgumentNullException("list");

        _suppressNotification = true;

        foreach (T item in list)
        {
            Add(item);
        }
        _suppressNotification = false;
        OnCollectionChanged(new NotifyCollectionChangedEventArgs(NotifyCollectionChangedAction.Reset));
    }
}

So you simply use RangeObservableCollection instead of ObservableCollection.

Fourth, just don't forget about tuning your EF properly for better performance.


Scaling and incrementing non-zero elements of a NumPy matrix I have a NumPy matrix C and want create a copy of it cPrime, which has some operation of the original matrix to all non-zero values of it. In the code below, for each non-zero element of C, I multiply by 30 and then add 1:

import numpy as np

size = 6
C = np.zeros((size,size), dtype=int)

C[4][0] = 2
C[4][1] = 5
C[4][2] = 3
C[0][3] = 1
C[1][3] = 1
C[2][3] = 1
C[3][5] = 3

cPrime = np.zeros((size, size),dtype=int)
for i in range(size):
    for j in range(size):
        if C[i][j] != 0:
            cPrime[i][j] = C[i][j]*30 + 1

This code works, but it feels inefficient. I'm about 99% sure that there's a really efficient way to achieve this goal, maybe using a masked array, but I haven't been able to figure it out.

This is a typical use case for numpy.where:

cPrime = np.where(C, C * 30 + 1, C)

This is about twice as fast as (30 * C + 1) * (C != 0) and generalizes more easily to other conditions.


Traversing a tile-based 2D-array map using keyboard controls I've produced the beginnings of an RPG-style game using HTML5 and canvas. So far you can use the keyboard controls WASD to move an avatar around a simple tile-based map. The map is constructed of individual 'room' maps which form a larger 'world' map. The use of a 'world' map allows the avatar to walk off the screen and onto a new map. If the avatar has reached the 'edge' of the 'world', it will behave as though it has encountered a non-traversable tile.

So far I'm happy with how it works, but I am aware that the code itself has a lot of duplication and I'd like to refactor it to be more DRY. So far, what has stopped me doing this is confusion over how best to efficiently pass values between different functions without having tons of global variables.

One example of what I mean is my keyDownHandler function. I created variables such as nextTileNorth (and so on) to let me calculate the nature of the grid tile onto which the avatar is attempting to move. There is a lot of repeated behaviour which I think should be in a separate function, but then how would I pass on the nextTileNorth (etc) information to the new function without further repeating myself?

I have included the keyDownHandler function below, but in line with the rules of Code Review I am happy to receive feedback on any aspects of the code, which can be found in this JSFiddle.

function keyDownHandler(event) {
    var keyPressed = String.fromCharCode(event.keyCode);

    var nextTileSouth;
    (currentMap[currentRowTile + 1] === undefined) ? nextTileSouth = undefined : nextTileSouth = currentMap[currentRowTile + 1][currentColumnTile];

    var nextTileNorth;
    (currentMap[currentRowTile - 1] === undefined) ? nextTileNorth = undefined : nextTileNorth = currentMap[currentRowTile - 1][currentColumnTile];

    var nextTileEast = currentMap[currentRowTile][currentColumnTile + 1];
    var nextTileWest = currentMap[currentRowTile][currentColumnTile - 1];

    if (keyPressed == "W") {

        worldDirection = 'N';
        // if the next tile is undefined (because we're at the edge of the screen) ...
        if (nextTileNorth === undefined) {
            drawNewMap();
        } else {
            // otherwise, check if we can continue to move forward (is the next tile traversable?)
            if (traversableTiles.indexOf(nextTileNorth) > -1) {
                isMoving = true;
            } else {
                // if we're blocked by a non-traversable tile, stop moving
                isMoving = false;
            }
        }
    } else if (keyPressed == "D") {

        worldDirection = 'E';
        if (nextTileEast === undefined) {
            drawNewMap();
        } else {
            if (traversableTiles.indexOf(nextTileEast) > -1) {
                isMoving = true;
            } else {
                isMoving = false;
            }
        }
    } else if (keyPressed == "S") {

        worldDirection = 'S';

        if (nextTileSouth === undefined) {
            drawNewMap();
        } else {
            if (traversableTiles.indexOf(nextTileSouth) > -1) {
                isMoving = true;
            } else {
                isMoving = false;
            }
        }
    } else if (keyPressed == "A") {

        worldDirection = 'W';

        if (nextTileWest === undefined) {
            drawNewMap();
        } else {
            if (traversableTiles.indexOf(nextTileWest) > -1) {
                isMoving = true;
            } else {
                isMoving = false;
            }
        }
    }

    for (var i = 0; i < avatarContainer.length; i++) {
        avatarContainer[i].facing = worldDirection;
        avatarContainer[i].isMoving = isMoving;
    }
}

You could just create a function like this:

function move(directionNextTile) {
    if (directionNextTile === undefined) {
        drawNewMap();
    } else {
        // otherwise, check if we can continue to move forward (is the next tile traversable?)
        if (traversableTiles.indexOf(directionNextTile) > -1) {
            isMoving = true;
        } else {
            // if we're blocked by a non-traversable tile, stop moving
            isMoving = false;
        }
    }
}

Or simplified:

function move(directionNextTile) {
    if (directionNextTile === undefined) {
        drawNewMap();
    } else {
        isMoving = traversableTiles.indexOf(directionNextTile) > -1;
    }
}

And use it like this:

if (keyPressed == "W") {
    worldDirection = 'N';
    move(nextTileNorth);
} else if (keyPressed == "D") {
    worldDirection = 'E';
    move(nextTileEast);
} else if (keyPressed == "S") {
    worldDirection = 'S';
    move(nextTileSouth);
} else if (keyPressed == "A") {
    worldDirection = 'W';
    move(nextTileWest);
}

And your initial assignment:

var nextTileSouth;
(currentMap[currentRowTile + 1] === undefined) ? nextTileSouth = undefined : nextTileSouth = currentMap[currentRowTile + 1][currentColumnTile];

could be:

var nextTileSouth = (currentMap[currentRowTile + 1] === undefined) ? undefined : currentMap[currentRowTile + 1][currentColumnTile];

I didn't look at all the code you linked to, but I think that you would benefit from creating a map class. That way, you can encapsulate a lot of code, and get rid of a lot of global variables.


Comic book conversion I have two small structs (might change to classes later) that get loaded into Generic lists. The loading of the lists is what I'm asking about. Can it be done better or more object-oriented?

You'll notice I'm using MS Enterprise library for data access to a SqlCe database.

I'm just looking for some opinions on this code. Is there a more efficient way to do what I've written so far? My goal is to make the code as efficient and small as possible while still being maintainable and expandable.

 public class ConversionUtility
{

    private List<ComicBook> _books;
    private List<ComicPublisher> _publishers;

    private void Load()
    {
        try
        {
            _books = GetBooks();
            _publishers = GetPublishers();
        }
        catch (Exception ex)
        {
            Console.WriteLine(ex.Message);
        }
    }

    public void PerformConversion()
    {
        Load();

       //still working on this

    }

    private List<ComicPublisher> GetPublishers()
    {
        List<ComicPublisher> tmp = new List<ComicPublisher>();

        Database db = EnterpriseLibraryContainer.Current.GetInstance<Database>("CeConnectionString");

        IDataReader rdr = db.ExecuteReader(CommandType.Text, "SELECT * FROM Publisher");

        while (rdr.Read())
        {
            tmp.Add(new ComicPublisher { Id = (int)rdr["PublisherId"], Name = rdr["PublisherName"].ToString() });
        }
        return tmp;

    }

    private List<ComicBook> GetBooks()
    {
        List<ComicBook> tmp = new List<ComicBook>();

        Database db = EnterpriseLibraryContainer.Current.GetInstance<Database>("CeConnectionString");

        IDataReader rdr = db.ExecuteReader(CommandType.Text, "SELECT * FROM Book");

        while (rdr.Read())
        {
            tmp.Add(new ComicBook { Id = (int)rdr["BookId"], Title = rdr["Title"].ToString(), Company = rdr["Company"].ToString() });
        }

        return tmp;

    }

}

public struct ComicPublisher
{
    public int Id { get; set; }
    public string Name { get; set; }
}

public struct ComicBook
{
    public int Id { get; set; }
    public string Title { get; set; }
    public string Company { get; set; }
}

Firstly:

Close your DataReaders!!!

Secondly:

You may want to consider abstracting boilerplate code like this:

  Database db = EnterpriseLibraryContainer.Current.GetInstance<Database>("CeConnectionString");
  IDataReader rdr = db.ExecuteReader(CommandType.Text, "SELECT * FROM Book");

Into some helper methods.


Function to find all occurrences of substring This function returns a list of all the beginning indices of a substring in a string. After finding the index of a substring, the search for the next one begins just after this index.

def find_substring(substring, string):
    """
    Returns list of indices where substring begins in string

    >>> find_substring('me', "The cat says meow, meow")
    [13, 19]
    """
    indices = []
    index = -1  # Begin at -1 so index + 1 is 0
    while True:
        # Find next index of substring, by starting search from index + 1
        index = string.find(substring, index + 1)
        if index == -1:
            break  # All occurrences have been found
        indices.append(index)
    return indices

I would turn this function into a generator so that an iteration over its values does not unnecessarily build a list into memory. If the caller trully needs a list, they can still call list(find_substring(...)). I would also rename this function substring_indexes as I feel it better convey what this function is about; also index could be something like last_known_position or last_found. But naming is hard and I might be wrong on this one.

def substring_indexes(substring, string):
    """
    Generate indices of where substring begins in string

    >>> list(find_substring('me', "The cat says meow, meow"))
    [13, 19]
    """
    last_found = -1  # Begin at -1 so the next position to search from is 0
    while True:
        # Find next index of substring, by starting after its last known position
        last_found = string.find(substring, last_found + 1)
        if last_found == -1:
            break  # All occurrences have been found
        yield last_found


Python path-breaking script for use in R R does not support line continuation as Python, and it's disturbing to have long file paths like

  /media/user/data/something-not-very-important-but-super-long/some-curious-secret-file.pdf

and can't break it into multiple lines.

The workaround is to use the file.path function:

file.path(
  "/root",
  "folder",
  "file.ods")

Manually editing is tedious, so here is a Python script that does the job for you:

#!/usr/bin/env python3

import sys

x = sys.argv[1]
# split by unix path separator
y = x.split("/")
# get rid of empty strings
y = [i for i in y if i]

# preserve / if it's an absolute path
if x.startswith("/"):
    y[0] = "/" + y[0]

# quote everything
y = ["'" + i + "'" for i in y]

res = "file.path(" + ",\n    ".join(y) + ")"
print(res)

Always throw things in main:

def main():
    # code

main()

It might seem pointless but it does help with preventing global pollution, which is good if you ever decide to add a function somewhere.

Using

x = sys.argv[1]

is acceptable for a trivial script, but it's not much harder to use docopt and implement a proper command-line parser:

"""
Name Of Program.

Usage: prog.py <pathname>
"""

import docopt

def main():
    args = docopt.docopt(__doc__)

    x = args["<pathname>"]

Trivial, but it gives you --help, error reporting and sane handling of bad input, nearly for free.

This can be simplified a little with pathlib:

parts = pathlib.PurePosixPath(args["<pathname>"]).parts

if len(parts) >= 2 and parts[0] == "/":
    parts = ("/" + parts[1],) + parts[2:]

You can actually use just pathlib.Path but it may assume a Windows format on Windows machines.

Your quoting:

parts = ["'" + i + "'" for i in parts]

should be done with repr:

parts = map(repr, parts)

and the formatting should use .format:

res = "file.path({})".format(",\n    ".join(parts))

This gives

#!/usr/bin/env python3

"""
Name Of Program.

Usage: prog.py <pathname>
"""

import docopt
import pathlib

def main():
    args = docopt.docopt(__doc__)
    parts = pathlib.PurePosixPath(args["<pathname>"]).parts

    if len(parts) >= 2 and parts[0] == "/":
        parts = ("/" + parts[1],) + parts[2:]

    args = ",\n    ".join(map(repr, parts))
    print("file.path({})".format(args))

main()

Technically PEP 8 says docopt should be separated from pathlib, the second being in the stdlib, but it looks nicer this way for now.


Efficiently determining maximum allowed euclidean distance between lists of colors I was recently tasked with determining which hex RGB colors in list  color_list are nearest to each hex RGB color in list target_colors, using euclidean distance as the measuring stick, and only returning items that are within 10 distance from each target color.  My first pass at this was for each item in target_colors, check the distance between it and everything in color_list, and return the colors at or below the target distance. This works well enough when color_list is small. But when color_list is large, looping over the entire list for each item in target_colors is slow. I am wondering if there is a better way to go about doing this, so that I don't have to loop over the entire list every time.

My first thought was to cache the results, but given that there are 16 million potential colors, that is potentially 2*16m^2 combinations, it would not be very space efficient.

My next thought was that perhaps the source color list can be sorted in a way that would make searching it faster, but I have to loop over all of the colors to get the distance as the thing to sort on, and I would not be in any better of shape.

My last thought was that given an r, g, or b color, perhaps I could eliminate colors that are obviously going to be out of range. If I have a color like #AABBAA, I instinctively know that #001100 is going to be way too far away, so I could sort the source list of colors l, then eliminate anything that smells like its out of range. But, I get back to the "well I would calculate the distance to figure out if it smells out of range", and I'm back to square one.

distance.rb:

TARGET_DISTANCE = 10.0

# calculate the euclidiean distance between two hex colors
def color_distance(c1, c2)
  c1_parts = c1.split('').each_slice(2).map{|c| c.join}.to_a
  c2_parts = c2.split('').each_slice(2).map{|c| c.join}.to_a
  r_diff = c1_parts[0].to_i(16) - c2_parts[0].to_i(16)
  g_diff = c1_parts[1].to_i(16) - c2_parts[1].to_i(16)
  b_diff = c1_parts[2].to_i(16) - c2_parts[2].to_i(16)
  Math.sqrt(r_diff**2 + g_diff**2 + b_diff**2)
end

def random_color
  3.times.map{rand(254).to_s(16).rjust(2, '0')}.join()
end

color_list = []
1.upto(10000) { color_list << random_color }

target_colors  = []
1.upto(100) { target_colors << random_color }

results = {}
target_colors.each do |t|
  results[t] ||= []
  color_list.each do |c|
   distance = color_distance(c, t)
   if distance <= TARGET_DISTANCE
     results[t] << [c, distance]
   end
  end
end

As @vnp says, a solution involving an octree would a more efficient way to find neighboring points.

Before you implement one, though, there are some huge time savings you can get by making some trivial changes:

Computing square roots is computationally intensive.  Do it only when you need to.
Parsing hex values into integers is somewhat slow, especially when you do it using a lot of string and array operations.  It would be more efficient to parse the entire 6-hex-digit string into an integer, then use bit manipulation.
The same goes for generating random colors.  I don't know why you used rand(254) rather than rand(256).  If you allow color components FE and FF, then it becomes a simple matter of generating a 24-bit random number, and formatting it as hex.  (Better yet, avoid converting the colors into their hex representation altogether, and just store them as integers.)

In addition, 1.upto(10000) { color_list << random_color } is poor Ruby style.  Write it using .map instead (like you did in the implementation of random_colors).

I don't know why you wrote results[t] ||= [] instead of results[t] = [].

With just those changes, I'm able to reduce the execution time by 90% already.

def color_dist_sq(c1, c2)
  c1 = c1.to_i(16)
  c2 = c2.to_i(16)
  [0, 8, 16].map do |bits|
    ((c1 >> bits & 0xff) - (c2 >> bits & 0xff)) ** 2
  end.inject(:+)
end

def random_color
  '%06x' % rand(1 << 24)
end

TARGET_DISTANCE_SQ = 10**2
color_list = 10000.times.map { random_color }
target_colors = 100.times.map { random_color }

results = {}
target_colors.each do |t|
  results[t] = color_list.inject([]) do |result, c|
    dist_sq = color_dist_sq(c, t)
    result << [c, Math.sqrt(dist_sq)] if dist_sq <= TARGET_DISTANCE_SQ
    result
  end
end


Time limit exceeded in K-th symbol in Grammar (LeetCode) I am taking efforts to solve problem K-th Symbol in Grammar - LeetCode

  K-th Symbol in Grammar

  On the first row, we write a 0. Now in every subsequent row, we look at the previous row and replace each occurrence of 0 with 01, and each occurrence of 1 with 10.

  Given row N and index K, return the K-th indexed symbol in row N. (The values of K are 1-indexed.) (1 indexed).

Examples:
Input: N = 1, K = 1
Output: 0

Input: N = 2, K = 1
Output: 0

Input: N = 2, K = 2
Output: 1

Input: N = 4, K = 5
Output: 1

Explanation:
row 1: 0
row 2: 01
row 3: 0110
row 4: 01101001

  Note:

  N will be an integer in the range [1, 30].
  K will be an integer in the range [1, 2^(N-1)].

My solution

class Solution:
    def kthGrammar(self, N: int, K: int) -> int:
        #replace function
        def replace(row: "List[int]") -> "List[int]":
            """
            rtype: row
            """
            for i in range(len(row)):
                print(row[i])
                if row[i] == 0: #0 -> 01
                    row.insert(2*i+1, 1)
                elif row[i] == 1: #1 -> 10
                    row.insert(2*i+1, 0)
            return row

        #helper function
        def helper(N: int) -> "List(int)":
            """
            rtype:the last row
            """
            #error case
            if N < 1: return []
            #base case
            if N == 1:
                res = [0]
                return res
            elif N > 1:
                return replace(helper(N-1))

        #error cases
        if N < 1 or K < 1 or K > 2**(N-1) : return None
        #recur
        row = helper(N)
        #logging.debug(f"row: {row}")
        return row[K-1]

Unfortunately it  reported Time Limit Exceeded

  Last executed input:
  30
  434991989

Review my solution:

employed tail recursion return replace(helper(N-1))
did not create a new row

It's a relatively fine solution,

What might be the reason of TLE?

It makes little difference how the row is constructed, the problem is intentionally designed such that constructing the row at all will lead to TLE. Consider that for N=30, the row would have 229 elements, that's no good. This is usually the case with competitive programming problems, they are made to make the straightforward solutions fail, that is what makes it competitive.

There are a few ways you could go about solving it, for example, you could recognize that the sequence produced is the Thue-Morse sequence, which has a simple formula to calculate an element at a given index without generating the whole sequence up to that index. The result does not depend on N.


GUI Application for counting visitors This code was to create how many people enter the "building" so there will be someone clicking the button "count". I am still improving. Please let me know your honest opinions!

from tkinter import *
import math

win = Tk()

global counter
counter = 0

def closewindow():
    exit()

def continuewindow():
    global counter
    counter+=1
    print ("Total number of people entering: ")
    print (counter)

b1 = Button(win,text="Count",command=continuewindow)
b2= Button(win,text="Exit",command=closewindow)

b1.pack(side=LEFT)
b2.pack(side=LEFT)
mainloop()

Personally, when building GUI applications I like to wrap my code into a class, and handle each action into each own method. I'll start from top to bottom.

I don't use wildcard imports. I import the package as "tk", which requires that I prefix all my tkinter commands with tk. This prevents global namespace pollution, plus it makes the code completely obvious when you are using tkinter classes, ttk classes, or some of your own.

So, I'd write the import as:

import tkinter as tk

The main application should be a class. This will give you a private namespace for all your callbacks and private functions (if any), and just generally makes it easier to organize the code. The way you did (procedural style) you have to code top-down, defining functions before using them, etc. With this method you don't, since you don't actually create the main window until the very last step. (as you'll see).

Let's start building our class step by step:

class CountVisitors:
    """
    GUI Application which counts how many visitors enter the building.
    The application prints the count of visitors in the console
    """

I've started by giving our class a name, and writing  just below it a docstring which tells the users what the class is supposed to do.

After we've defined our class, we should initialize some of the components of our application and we're gonna use a constructor, which by convention is the __init__ method:

def __init__(self, master):
    self.master = master

    self.button1 = tk.Button(self.master, text="Count", command=self.count_visitors)
    self.button2 = tk.Button(self.master, text="Exit", command=self.close_window)

    self.button1.pack(side=tk.LEFT)
    self.button2.pack(side=tk.LEFT)

    self.button_clicks = 0

Now, whether or not __init__ is a constructor or not is not our concern here. If you're not familiar with what a constructor it is, well...it's nothing more than a special kind of method that Python calls when it instantiates an object using the definitions found in some class. Basically, Python relies on the constructor to perform tasks such as initializing (assigning values to) any instance variables that the object will need when it starts.

So, in the above code, we've initialized our parent app, our buttons, and the counter that will keep track of the visitors.

Moving forward, I'll create two separate methods which will handle the two actions we'll have in our app:

counting / printing the visitors
closing the application

def count_visitors(self):
    self.button_clicks += 1
    print(self.button_clicks)

def close_window(self):
    self.master.destroy()

If you didn't noticed, it wasn't necessary to force the application to close using exit() (which by the way, is frowned upon) because tkinter has its own way of doing exactly that using the destroy() method.

Now, all you have to do is to create a main() method where you'll instantiate the class and start it:

def main():
    root = tk.Tk()
    CountVisitors(root)
    root.mainloop()

The entire code:

import tkinter as tk

class CountVisitors:
    """
    GUI Application which counts how many visitors enter the building.
    The application prints the count of visitors in the console
    """
    def __init__(self, master):
        self.master = master

        self.button1 = tk.Button(self.master, text="Count", command=self.count_visitors)
        self.button2 = tk.Button(self.master, text="Exit", command=self.close_window)

        self.button1.pack(side=tk.LEFT)
        self.button2.pack(side=tk.LEFT)

        self.button_clicks = 0

    def count_visitors(self):
        self.button_clicks += 1
        print(self.button_clicks)

    def close_window(self):
        self.master.destroy()

def main():
    root = tk.Tk()
    CountVisitors(root)
    root.mainloop()

if __name__ == '__main__':
    main()


Shift Operator Multiplier Please suggest a way to optimize the below code. I want to reduce the execution time. This is a program to multiply using the shift operator.

import java.util.Scanner;

public class Test2 {

    public static void main(String args[] ) throws Exception {

        Scanner in = new Scanner(System.in);
        int N = in.nextInt();
        long[] [] list=new long[N][2];
        for(int i=0;i<N;i++){
            list[i][0]=in.nextLong();
            list[i][1]=in.nextLong();
        }
        for(long[] s:list){
            System.out.println(calc(s[0], s[1]));
        }
    }

    public static String calc(long n1, long n2) {
        long temp, i = 0, result = 0;
        String s=null;
        while (n2 != 0) {
            if ((n2 & 1) == 1) {
                temp = n1;
                if(s==null){
                    s="("+i+"-"+i+") ";
                }else{
                    s=s+"("+i+"-"+i+") + ";
                }
                result += (temp<<=i);
            }
            n2 >>= 1;
            i++;
        }
        return s;
    }
}

The first thing I see is concatenating a string in a loop. This will create a new string every loop. You don't want that.

Instead use a StringBuilder:

public static String calc(long n1, long n2) {
    long temp, i = 0, result = 0;
    StringBuilder s=new StringBuilder();
    while (n2 != 0) {
        if ((n2 & 1) == 1) {
            temp = n1;
            if(s.length()!=0)s.append("+ ");
            s.append("(").append(i).append("-").append(i).append(") ");
            result += (temp<<=i);
        }
        n2 >>= 1;
        i++;
    }
    return s.toString();
}


Cyclomatic complexity for method is too high How do i refactor my code to fix the lint error i am getting?

  def value=(val)
    if val
      if [:char].include?(@type) && val.length >= args[:limit] && val == 'true'
        val = 'Y'
      elsif [:char].include?(@type) && val.length >= args[:limit] && val == 'false'
        val = 'N'
      elsif [:bit].include?(@type) && val.length >= args[:limit] && val == 'true'
        val = 1
      elsif [:bit].include?(@type) && val.length >= args[:limit] && val == 'false'
        val = 0
      elsif [:varchar, :char, :nvarchar].include?(@type) && @args[:limit]
        unless val.length <= args[:limit]
          msg = "The length of #{@name} exceeds the max limit #{@args[:limit]}"
          raise Programmability::BadRequestError.new(nil, msg)
        end
      elsif [:numeric].include?(@type) && @args[:precision] && @args[:scale]
        valid_numeric?(val, @args[:precision], @args[:scale])
      elsif [:datetime].include?(@type)
        valid_date?(val)
      end
    end
    @value = val
  end

[x].include?(y) is the most roundabout way I've ever seen of saying x == y.
You're repeating val.length >= args[:limit] for almost every. single. case.
Why is there an instance variable called @args? That smells like you didn't bother to actually handle things in the constructor, and instead just store everything for later. Also, why are you using @args directly, when it seems you also have a reader/accessor method, args? Using the accessor is better, though you can use either - just don't mix them.
Why isn't there code after the calls to valid_numeric? and valid_date?? The prefix valid_... and the ? in the names indicate that those methods (should!) return boolean values. But if they do, you're ignoring it - which doesn't make sense. So it smells like those methods don't so much validate anything, but actually set @value to something. No thanks.
You're overwriting the argument val, which isn't pretty. Make a new local variable instead, or write to @value directly.
What if val is just false? Not the word, but the boolean? In that case all the logic will be skipped, and you'll go straight to @val = false. It doesn't matter whatever @type is, or whatever args[:limit] is. I think you want your if to check for val.nil? specifically.

It seems like what you really want is 4-6 different classes to handle different datatypes. Whatever you're trying to do, you're doing too much of it in one method and one class.


CLI to bump package.json version and add git tags I'm writing a command line app in node.js. At this stage, the app does the following:

Get the version from package.json
Update the package.json with a new version depending on the command line argument
Commit the changes
Tag the commit
Push commits and tags to the remote

Some comments:

I'm really new to async/await and promises so I may be using those incorrectly in my code.
A lot of my code is wrapped in try-catch blocks which I understand is not supposed to be the case with async/await or Promises. I'm not sure how to fix this.
There's a lot of duplication with the spinner initialisation code. I am aware the ora has an ora.promise method as well. I couldn't get this to work.
Another example of duplication is with the process.exit(1) line within each catch block. Is this really needed or are there better ways to achieve this?
Do I really need to use the async IIFE at the end or is there a better way to do this?
I would also like feedback on whether I've divided my code into functions adequately.
I haven't added much error checking yet. For example, later, I'd like to check if a package.json exists, and if it doesn't, get the current version from git tags. How can I structure my code in such a way that I can easily add such functionality in the future?

Any other feedback is also welcome. If you can address even some of the above, I would appreciate it.

Usage:

ship <major|premajor|minor|preminor|patch|prepatch|prerelease>

(I named the script index.js, and created a symbolic link ship to it in /usr/local/bin)

#!/usr/bin/env node
'use strict';

const fs = require('fs');
const semver = require('semver');
const readPkg = require('read-pkg');
const writePkg = require('write-pkg');
const git = require('simple-git/promise')(process.cwd());

const ora = require('ora');
const chalk = require('chalk');

const env = process.env;

const validArgs = [
    'major', 'premajor', 'minor', 'preminor', 'patch', 'prepatch', 'prerelease'
];

const argv = require('yargs').argv;
const arg = argv._[0];

let spinner;

if (arg === undefined) {
    console.error(chalk.red(`Missing argument.`));
    console.error(chalk.red(`Should be one of ${validArgs.join(', ')}.`));
    process.exit(1);
}

if (!validArgs.includes(arg)) {
    console.error(chalk.red(`Invalid argument ${arg}`));
    process.exit(1);
}

async function nextVersion() {
    let current;

    try {
        const data = await readPkg();
        current = data.version;
    } catch(err) {
        process.exit(1);
        // todo: get version from git tags
    }

    return semver.inc(current, arg);
}

async function updatePkg(version) {
    spinner = ora('Writing package.json').start();

    try {
        const data = await readPkg();
        delete data._id;
        data.version = version;
        await writePkg(data);
        spinner.succeed();
    } catch (err) {
        spinner.fail();
        process.exit(1);
    }
}

async function commit(msg) {
    spinner = ora('Commiting changes').start();

    try {
        await git.add('package.json');
        await git.commit(msg);
        spinner.succeed();
    } catch (err) {
        spinner.fail();
        process.exit(1);
    }
}

async function makeTags(name) {
    spinner = ora('Adding tag').start();

    try {
        await git.addTag(name);
        spinner.succeed();
    } catch (err) {
        spinner.fail();
        process.exit(1);
    }
}

async function push() {
    spinner = ora('Pushing changes').start();

    try {
        // both of these may run together
        await git.push();
        await git.pushTags();
        spinner.succeed();
    } catch (err) {
        spinner.fail();
        process.exit(1);
    }
}

(async () => {
    const next = await nextVersion();

    console.log(); // gap between input and spinners

    await updatePkg(next);
    await commit(next);
    await makeTags(next);
    await push();
})();

(Disclaimer: I'm not a JavaScript expert and I don't have much experience with async/await either, so take my advices with a grain of salt. I hope you will find it helpful anyway.)

Running async calls in parallel

If you assume these will run in parallel, they won't:

// both of these may run together
await git.push();
await git.pushTags();

To make them run in parallel, you could use Promise.all:

await Promise.all([git.push(), git.pushTags()]);

Or save the result of the calls without await, and await on the results:

const pushResult = git.push();
const pushTagsResult = git.pushTags();
await pushResult;
await pushTagsResult;

Not too much async and spinners?

Most of the operations are fast,
such as updating the file, git add, and git commit,
and I don't see a good reason to make them run in parallel and with a progress indicator.

The only operation that may be slow and where the progress indicator makes sense is the git push.

Since the main logic is not packaged as reusable code that can be imported and called from another module,
wrapping it in async (in the IIFE at the end) seems unnecessary too.
If the updating of the package was separated from the command line parsing,
then it could make sense to make it possible to run async,
if you had a use case for that.
Without a use case, I think it's over-engineering.

Addressing your questions

  A lot of my code is wrapped in try-catch blocks which I understand is not supposed to be the case with async/await or Promises. I'm not sure how to fix this.

I don't see anything wrong with your try-catch blocks (but see the disclaimer at the top).

  There's a lot of duplication with the spinner initialisation code. I am aware the ora has an ora.promise method as well. I couldn't get this to work.

As mentioned earlier, I think the spinners are overkill.
However, even if you remove them from most places,
much duplication will still remain.

I don't think you need the granularity of catching exception at every step.
You could wrap those steps in a single try-catch block,
and that will reduce the duplication and boilerplate code.

  I haven't added much error checking yet. For example, later, I'd like to check if a package.json exists, and if it doesn't, get the current version from git tags. How can I structure my code in such a way that I can easily add such functionality in the future?

The way you did so far seems fine to me.
If later you want to get tags from Git,
you can add a function for that,
and call it from nextVersion as needed.


Computing the integer square root of large numbers The "integer square root" of a
non-negative integer \$ n \$ is defined as the largest integer not greater than
 \$ \sqrt{n} \$:
$$
 \operatorname{isqrt}(n) = \lfloor \sqrt{n} \rfloor
 = \max \{ k \in \Bbb N_0 \mid k^2 \le n \}
$$
It is for example needed in prime factorization, as an upper bound for the possible
factors.

A simple approach is to compute the floating point square root and truncate the result to an integer. In Swift that would be

func isqrt_simple(_ n: Int) -> Int {
    return Int(Double(n).squareRoot())
}

As observed in
Computing the square root of a 64-bit integer,
this can produce wrong results for large numbers, because an IEEE 64-bit floating point
number with its 53 bit significand cannot represent large integers exactly.
Here is an example:

let n = 9223371982334239233
let r = isqrt_simple(n)

print(r)           // 3037000491
print(r * r <= n)  // false

The correct result would be 3037000490, since
$$
  \sqrt{9223371982334239233} \approx 3037000490.9999996957524364127605120353
$$
(computed with PARI/GP).

The following implementation uses the ideas from
DarthGizka's answer
to the above mentioned question to implement a "correct" integer square root function in Swift 4:

func isqrt(_ n: Int) -> Int {
    precondition(n >= 0, "argument of isqrt() must be non-negative")

    var r = Int(Double(n).squareRoot()) // Initial approximation
    // Try to increase:
    while case let prod = r.multipliedReportingOverflow(by: r),
        !prod.overflow && prod.partialValue < n  {
            r += 1
    }
    // Decrease if necessary:
    while case let prod = r.multipliedReportingOverflow(by: r),
        prod.overflow || prod.partialValue > n  {
            r -= 1
    }
    return r
}

Example:

let n = 9223371982334239233
let r = isqrt(n)

print(r)                 // 3037000490
print(r * r <= n)        // true
print((r+1) * (r+1) > n) // true

Remarks:

I have chosen Int as argument and result type even if the square root is defined
only for non-negative integers. The reason is that Int is the prevalent integer
type in Swift and already used for quantities that can not be negative (e.g.
the count of an array, or the MemoryLayout<T>.size of a type).
Int can be a 32-bit or 64-bit quantity, therefore I cannot check against
a constant for overflow (as r < UINT32_MAX in
DarthGizka's C++ solution).
multipliedReportingOverflow() is used instead to check if squaring a candidate causes an overflow.

The code worked correctly in all my tests. Here are some tests which all succeed

func testSqrt(_ n: Int) {
    let r = isqrt(n)
    if r * r > n {
        print("Too large:", n, r)
    } else if (r+1) * (r+1) <= n {
        print("Too small:", n, r)
    }
}

testSqrt(4503599761588224)
testSqrt(4503599895805955)
testSqrt(4503600030023688)
testSqrt(4503600164241423)

testSqrt(9223371982334241080)
testSqrt(9223371982334239233)
testSqrt(9223372024852248003)
testSqrt(9223372024852247041)
testSqrt(9223372030926249000)
testSqrt(9223372030926247424)

These tests fail if isqrt_simple() is used instead.

All feedback is welcome, in particular suggestions how to improve the performance.

Your function is rather unnecessary. When converted to double, all 64-bit integers retain 1 sign bit and at least 52 precision bits. So the error is at most 11 bit; which means for any number N, the error $$dN<=\frac{2^{11}}{2^{63}}N=2^{-52}N$$
With simple calculus, the error of sqrt() is $$d(\sqrt{N})=\frac{dN}{2\sqrt{N}}<=\frac{2^{-52}N}{2\sqrt{N}}=2^{-53}\sqrt{N}<2^{-53}*2^{32}=2^{-21}<<1$$
In any case the error must be smaller than 1. With rounding/truncation, it can only be -1, 0, or 1.

So your code can be

func isqrt(_ n: Int) -> Int {
    var r = Int(Double(n).squareRoot()) // Initial approximation
    if r * r > n { r -= 1 }
    else if (r + 1) * (r + 1) <= n { r += 1}
    return r
}

edit:
There will be time that (r + 1) * (r + 1) be larger than Int.max and causes error. Instead of using multipliedReportingOverflow, I would do it this way

func isqrt(_ n: Int) -> Int {
    if n >= 3037000499 * 3037000499 {return 3037000499}
    var r = Int(Double(n).squareRoot()) // Initial approximation
    if r * r > n { r -= 1 }
    else if (r + 1) * (r + 1) <= n { r += 1}
    return r
}

edit 2:
To deal with different possible Int sizes:

func isqrt(_ n: Int) -> Int {
  #if arch(i386) || arch(arm)
    if n >= 46340 * 46340 {return 46340}
  #else
    if n >= 3037000499 * 3037000499 {return 3037000499}
  #endif

  var r = Int(Double(n).squareRoot()) // Initial approximation
  if r * r > n { r -= 1 }
  else if (r + 1) * (r + 1) <= n { r += 1}
  return r
}


Bash script for log rotation Okay, first things first:
Yes, I am aware of the logrotate tool, which would usually be used for something like this.
However, since the logfile i want to rotate already contains a timestamp in the filename, logrotate will never delete old logs.
This script is supposed to:

tell the application to create a new logfile (via SIGHUP)
find all existing, unprocessed logfiles of the application
compress all logfiles except the one currently in use by the application
keep the most recent 7 logs and delete the rest.

From what I can tell, everything seems to work just fine, but I'm curious if anything could be improved.
#!/usr/bin/bash

procname="foo"
srcpath="/var/log"
srcname="*${procname}.log"  # example: 2021-02-05_1200_foo.log
count=7

echo "rotate $procname logs: ${srcpath}/${srcname} ($count rotations)"

pid="$(pidof $procname)"
if [[ ! $? == 0 ]]
then
    # don't rotate anything if the application is not running
    echo "$procname process not running"
    exit 1
fi

# ask the application to create a new logfile
kill -HUP "$pid"

sleep 1 #probably not necessary?

# get array of all logfiles (should be exactly 2 in most cases)
mapfile -t list <  <(find "$srcpath" -maxdepth 1 -name "$srcname" | sort)
size=${#list[@]}

# don't do anything unless there are at least 2 files
if [[ $size -lt 2 ]]
then
    echo "nothing to do"
    exit 0
fi

# find the active logfile (should be the last one) and delete it from the array
for ((i=size-1; i>=0; i--));
do
    if [[ $(find -L /proc/"$pid"/fd -inum "$(stat -c '%i' "${list[i]}")") ]]
    then
        unset "list[i]"
        break
    fi
done

# compress all remaining files (usually just one)
gzip "${list[@]}"

unset list
unset size

# get array of all compressed logfiles
mapfile -t list <  <(find "$srcpath" -maxdepth 1 -name "$srcname.gz" | sort)
size=${#list[@]}

if [[ $size -gt $count ]]
then
    idx=("${!list[@]}")
    # remove $count most recent files from $list
    for i in "${idx[@]: -$count:$count}"
    do
        unset "list[$i]"
    done

    # delete the remaining old files
    rm -f "${list[@]}"
fi

exit 0

Shellcheck points out this one:

pid="$(pidof $procname)"
if [[ ! $? == 0 ]]
then

We can write that more clearly
if ! pid="$(pidof $procname)"
then

Shortly after that is:

# don't rotate anything if the application is not running
echo "$procname process not running"

That looks like an error message - redirect that to &2.
I'm a little concerned about

# find the active logfile (should be the last one) and delete it from the array

If the daemon didn't get around to opening a new file by the time we reach here, then we'll be out of sync.  That said, it probably doesn't matter - we'll just have one more unrotated file around until we next run, which isn't a big concern.
Take care with commands like this:

gzip "${list[@]}"

It's a good idea to use -- so we can be sure that filenames beginning with - aren't interpreted as options.
Also, commands such as gzip can fail - do we really want to keep going if they do?  (This question isn't so simple to answer, as one of the likely reasons is "no space on device", and in that case we do want to delete some files!)