Identifying arrays with the same mean I came across a problem where given a array of integer arrays of different lengths [[1,2,3],[4,1,1], [9,2,1]] you need to return an array of arrays, each array containing indices of the arrays (of the input array) such that the corresponding arrays have the same mean: [[0,1],[2]] This seems relatively simple to solve using Python:
def groupByMean(a):
    d,e=[],[]
    for i,j in enumerate(a):
        if sum(j)/len(j)not in e:
            e+=[sum(j)/len(j)]
            d+=[[i]]
        else:
            d[e.index(sum(j)/len(j))]+=[i]
    return d

However, when trying to solve this in Java this was my approach: using a hashmap, map each new mean to a list of the corresponding indices. Then iterate the hashmap, to get the arraylists and convert them to int[] arrays and construct the 2d array ...
Is there a simpler approach to solve this using Java?
This is my java code - looking for a different way to solve this:
public static void main(String[] args) {
    int[][] arr = { { 1, 2, 3 }, { 2, 3, 4 }, { 2, 4, 0 } };
    for (int[] nums : groupBySum(arr)) {
        for (int n : nums) {
            System.out.print(n + " ");
        }
        System.out.println();
    }
}

public static int[][] groupByMean(int[][] arr) {
    Map<Double, List<Integer>> map = new HashMap<>();
    int i = 0;
    for (int[] nums : arr) {
        double average = getAverage(nums);
        if (!map.containsKey(average)) {
            List<Integer> indices = new ArrayList<>();
            indices.add(i);
            map.put(average, indices);
        } else {
            map.get(average).add(i);
        }
        i++;
    }
    int[][] result = new int[map.size()][];
    int row = 0;
    for (List<Integer> indices : map.values()) {
        result[row] = new int[indices.size()];
        for (int k = 0; k < indices.size(); k++) {
            result[row][k] = indices.get(k);
        }
        row++;
    }
    return result;
}

public static double getAverage(int[] arr) {
    int sum = 0;
    for (int num : arr) {
        sum += num;
    }
    return ((double) sum) / arr.length;
}

Nice implementation. Few suggestions to make the method groupByMean more compact using Java Streams:

Calculate the average:
public static double getAverage(int[] arr) {
  int sum = 0;
  for (int num : arr) {
      sum += num;
  }
  return ((double) sum) / arr.length;
}

To:
public static double getAverage(int[] arr) {
  return Arrays.stream(nums).average().getAsDouble();
}

Group by average:
if (!map.containsKey(average)) {
    List<Integer> indices = new ArrayList<>();
    indices.add(i);
    map.put(average, indices);
} else {
    map.get(average).add(i);
}

To:
map.computeIfAbsent(average, v -> new ArrayList<>()).add(i);

Convert list to array:
for (int k = 0; k < indices.size(); k++) {
    result[row][k] = indices.get(k);
}

To:
result[row] = indices.stream().mapToInt(index->index).toArray();

Convert map's values to a matrix:
int[][] result = new int[map.size()][];
int row = 0;
for (List<Integer> indices : map.values()) {
    result[row] = new int[indices.size()];
    for (int k = 0; k < indices.size(); k++) {
        result[row][k] = indices.get(k);
    }
    row++;
}
return result;

To:
return map.values().stream()
          .map(v -> v.stream().mapToInt(index->index).toArray())
          .toArray(int[][]::new);

Time/Space Complexity
In terms of complexity there are no relevant differences between your solution and the one using Streams. The time complexity is still \$O(n*m)\$ where \$n\$ is the number of arrays and \$m\$ is the size of the longest array. Basically, for each array we need to calculate the average.
To check which approach is faster you need to benchmark the solutions.
Final code
public static void main(String[] args) {
    int[][] arr = {{ 1, 2, 3 }, { 2, 3, 4 }, { 2, 4, 0 }};
    Arrays.stream(groupByMean(arr)).map(Arrays::toString)
        .forEach(System.out::println);
}

public static int[][] groupByMean(int[][] arr) {
    Map<Double, List<Integer>> map = new HashMap<>();
    for (int i=0 ; i<arr.length; i++) {
        double average = Arrays.stream(arr[i]).average().getAsDouble();
        map.computeIfAbsent(average, v -> new ArrayList<>()).add(i);
    }
    return map.values().stream()
      .map(v -> v.stream().mapToInt(index->index).toArray())
      .toArray(int[][]::new);
}


Python script to demonstrate Fermat's Last Theorem, with option to restart I am a beginner coder learning Python. This is a simple exercise from the thinkpython book. I would like to know if this circular referencing to restart the program the best way to do it. Also if you have other comments as to how to improve the code, please feel free to suggest.

def program_restart():
    print("Would you like to try again? ")
    prompt = input("type 'Y' to continue and 'N' to exit:\n")
    if prompt =="Y" or prompt == "y":
        user_input()
    return

def check_fermat(a,b,c,n):
    if a**n+b**n == c**n:
        print("Holy Smokes, Fermat was wrong!")
        print("---------------------------")
    else:
        print("No, that does not work!")
        print("---------------------------")
    program_restart()

def user_input():
    print("Let's see if Fermet was right. He claims that a^n+b^n=c^n cannot be True!")
    while True:
        try:
            a = int(input('Give a positive number bigger than 2 for "a":\n'))
        except ValueError:
            print ("Not a string, a number please.")
            continue

        if a > 2:
            a = int(a)
            break

    while True:
        try:
            b = int(input('Give a positive number bigger than 2 for "b":\n'))
        except ValueError:
            print ("Not a string, a number please.")
            continue

        if b > 2:
            b = int(b)
            break

    while True:
        try:
            c = int(input('Give a positive number bigger than 2 for "c":\n'))
        except ValueError:
            print ("Not a string, a number please.")
            continue

        if c > 2:
            c = int(c)
            break

    while True:
        try:
            n = int(input('Give a positive number for the exponent "n":\n'))
        except ValueError:
            print ("Not a string, a number please.")
            continue

        if n > 2:
            n = int(n)
            break

    check_fermat(a,b,c,n)

user_input()

Mathematics

Fermat's Last Theorem states:

  No three positive integers a, b, and c satisfy the equation an + bn = cn for any integer value of n greater than 2.

That's not what your code does.  Why are you requiring that a, b, and c be greater than 2, when they only need to be greater than 0?  Why does your prompt for n only say Give a positive number for the exponent "n" when it should say Give an integer greater than 2 for the exponent "n"?

And who is "Fermet"?

Flow control

Function calls should not be used as gotos.  Functions should accept parameters and return a result.  Your calls to user_input() and program_restart(), on the other hand, expect the execution to never return to the caller.  To see the problem with that, say "yes" a few times to retry the calculation, then hit CtrlC to abort.  You'll see the stack trace, which indicates that the ever-deepening stack is keeping track of all the calls, in anticipation for the opportunity to return control to the caller.  Since control never returns, the stack grows deeper and would eventually overflow.

Suggested solution

def prompt_yesno(prompt):
    return input(prompt) in 'Yy'

def prompt_int(prompt, min=None):
    while True:
        try:
            ans = int(input(prompt))
            if min is None or min <= ans:
                return ans
        except ValueError:
            print("Input must be an integer.")

def is_fermat_counterexample(a, b, c, n):
    return n > 2 and a**n + b**n == c**n

def fermat_demo():
    print("Let's see if Fermat was right.")
    print("He claims that a^n + b^n = c^n cannot be true for any n > 2.")
    a = prompt_int('Give a positive integer for "a": ', 1)
    b = prompt_int('Give a positive integer for "b": ', 1)
    c = prompt_int('Give a positive integer for "c": ', 1)
    n = prompt_int('Give an integer bigger than 2 for exponent "n": ', 3)
    print("Holy smokes, Fermat was wrong!"
          if is_fermat_counterexample(a, b, c, n) else
          "No, that does not work!")

while True:
    fermat_demo()
    if not prompt_yesno("Would you like to try again?\n"
                        "Type 'Y' to continue and 'N' to exit: "):
        break

A more interesting demonstration

It would be a more satisfying demonstration if the computer asked the user for a, b, and n, then solved for c, showing that c is not an integer.


Haskell monads: sum of primes They say you cannot get out of the monad, but I need to use the result (at least for assertions / unit tests). But print how does it do it?

Otherwise, I worked very hard for the next code with my new limited Haskell abilities (especially when talking about monads), so it needs a review.

--import Control.Monad.ST
--import Control.Monad
import qualified Data.Vector as Vec
import qualified Data.Vector.Mutable as MutableVec

assert :: Monad a => Bool -> a ()
assert False = error "assertion failed!"
assert _     = return ()

get_primes_limit :: Int -> Int
get_primes_limit limit = 1 + floor (sqrt $ fromIntegral limit)

update_sieve4_step_M 2 limit arr = do
    mapM_ (\i -> MutableVec.write arr i 0) [4, 6 .. limit-1]
update_sieve4_step_M prime limit arr = do
    mapM_ (\i -> MutableVec.write arr i 0) [3*prime, 5*prime .. limit-1]

update_sieve4_step limit arrIn = do
    arr <- Vec.unsafeThaw arrIn
    MutableVec.write arr 1 0
    update_sieve4_step_M 2 limit arr
    mapM_ (\p -> update_sieve4_step_M p limit arr) [3, 5 .. (get_primes_limit limit)]
    Vec.unsafeFreeze arr

update_sieve4 limit = update_sieve4_step limit (Vec.generate limit (id))

--TODO: cannot get this out of the monad (not m Int); print does it somehow
primes_sum4_1 limit = do
    sieve <- update_sieve4 limit
    return (Vec.sum sieve)

--primes_sum4 :: Control.Monad.Primitive.PrimMonad m => Int -> m Int
primes_sum4 limit = do
    result <- primes_sum4_1 limit
    --print result
    return result

primes_sum4_validation = do
    result <- primes_sum4_1 9
    --assert (17 == result)
    print "Nothing to validate!"

validate = do
    --assert (17 == primes_sum4 9)
    --assert (17 == primes_sum4 10)
    --assert (17 == primes_sum4 11)
    --assert (28 == primes_sum4 12)
    --assert (100 == primes_sum4 29)
    --assert (129 == primes_sum4 30)
    --assert (1060 == primes_sum4 100)
    --assert (76127 == primes_sum4 1000)
    --assert (5736396 == primes_sum4 10000)
    --assert (32405717 == primes_sum4 25000)
    --assert (454396537 == primes_sum4 100000)

    print "Nothing to validate!"

main = do
    primes_sum4 2000000

In Haskell, unsafe can mean various things, but it almost always means that you have to have a very solid understanding of what the unsafe thing is about before you use it if you want to have any hope of staying out of trouble.
unsafeFreeze and unsafeThaw in particular are low-level functions exposed to allow programmers to dig into the internal guts of vectors to build new abstractions offering a safe interface. For example, you might use a pure function to build a new vector, then use unsafeThaw to produce a mutable vector. You'd then have a new function hiding away the unsafety and exposing a plain old "make a new vector like this within ST/IO/PrimMonad". Or you might create a mutable vector, do a bunch of mutation, and then unsafeFreeze it to make a pure one. You'd then have a new function hiding away the unsafety and exposing a plain old "make an immutable vector like this".
What you've written is something you should never see: an action that takes what, to the outside world, is a pure value, and actually changes it. Such a "function" breaks all the usual rules of Haskell; it's as strange as something that changes every 3 in the world into a 4.
What you should do instead, if you want to keep mutating the same vector, is put that whole thing within the monad. Make the vector, do all your mutations (holding on to whatever you need to return) and then produce a result at the end.
Side note: the general rule about unsafe also applies to any function or constructor imported from a module named GHC.*, *.Internal or *.Private, as well as certain functions whose names begin with unchecked or end in Descending or Ascending. The primitive member of the PrimMonad class is also deeply unsafe.

Update
You should read The Genuine Sieve of Eratosthenes by Melissa O'Neill, which shows how to lazily produce a list of primes in an efficient manner. It's a very accessible and interesting article.
I've pasted my own version of a fairly simple vector-based sieve below, with extensive comments. To run the testing code you will need the arithmoi and hspec packages. You can install those (and vector) in a Cabal sandbox if you wish.
module Primes where

import Control.Monad.ST
import qualified Data.Vector.Unboxed as Vec
import qualified Data.Vector.Unboxed.Mutable as MutableVec
import Control.Exception (assert)

-- For testing
import Test.Hspec
import Test.QuickCheck
import qualified Math.NumberTheory.Primes.Sieve as MNPS

-- Approximate square root of an Int. Note that this may not work properly
-- when the Int is too large to be represented precisely as a Double.
intSqrt :: Int -> Int
intSqrt n = ceiling (sqrt $ fromIntegral n)

-- We make a vector indicating which *odd* numbers are prime. An odd number n
-- is prime if (primalityTable n) ! getIndex n is True.
primalityTable :: Int -> Vec.Vector Bool
primalityTable 0 = Vec.empty
primalityTable 1 = Vec.singleton False
primalityTable 2 = Vec.fromList [False, False]
primalityTable upto = runST (do
  let limit = let root = intSqrt upto in root - fromEnum (even root)

  -- Make a mutable vector and fill it with True
  arr <- MutableVec.replicate (getIndex (upto - fromEnum (even upto)) + 1) True

  -- 1 is not prime
  assert (MutableVec.length arr > 1) $ MutableVec.write arr (getIndex 1) False

  -- For each element of the vector up to the limit, check if the element has
  -- been crossed off. If not, use it to cross off other elements.
  flip mapM_ [0 .. getIndex limit] $ \i -> do
    -- Does the index i represent a prime?
    isPrime <- assert (i < MutableVec.length arr) $ MutableVec.read arr i
    if isPrime
      then crossOff i arr
      else return ()
  Vec.freeze arr)

-- Since we're skipping all the even numbers, we use these functions to convert
-- numbers and the vector indices they correspond to. That way, we don't have
-- to try to keep track of the conversions in our poor heads, except where we
-- want to.  Since we're using Control.Exception.assert, we'll get useful
-- information about where in the source the assertion failed, and the
-- assertion will go away when we compile with optimization enabled (unless we
-- use -fno-ignore-asserts).
getIndex :: Int -> Int
getIndex n = assert (odd n) $ n `quot` 2

getValue :: Int -> Int
getValue n = n + n + 1

-- Cross off odd multiples of the prime represented by the index passed in. How
-- do we calculate this efficiently? We are working with the prime p = getValue
-- i = 2 * i + 1. We first cross off the value p*p. Instead of monkeying around
-- with getValue and getIndex in the inner loop, we will use thte fact that
-- moving up in value by 2*p corresponds to moving up in index by p.
crossOff :: Int -> MutableVec.MVector s Bool -> ST s ()
crossOff i arr = mapM_
     (\q -> assert (q < MutableVec.length arr) MutableVec.write arr q False)
     [startingIndex, startingIndex+p .. MutableVec.length arr - 1]
  where p = getValue i
        startingIndex = getIndex (p*p)

-- Note that we can reasonably expect the intermediate vector here to be fused away by
-- the fancy compiler rewrite rules in the vector library.
primesSum :: Int -> Int
primesSum uptoNotEqual
  | uptoNotEqual <= 2 = 0
  | otherwise = 2 + Vec.sum (Vec.imap (\i isPrime -> if isPrime then getValue i else 0)
                                      (primalityTable (uptoNotEqual-1)))

primesSumMNPS :: Int -> Int
primesSumMNPS upto = sum . takeWhile (< upto) . map fromInteger $ MNPS.primes

staticTests = [(0,0),(1,0),(2,0),(3,2),(4,5),(5,5),(6,10),(9,17),(10,17),(11,17),
               (12,28),(29,100),(30,129),
               (100,1060),(1000,76127),(10000,5736396),
               (25000,32405717), (100000,454396537),
               (2000000,142913828922)]

testOnList summer lst =
    flip mapM_ lst $ \(n,s) ->
           it ("works for "++show n) $ summer n `shouldBe` s

main :: IO ()
main = do
  hspec $ do
  -- Since we're using primesMNPS to check primesSum,
  -- we want to be sure it passes the static tests.
  describe "Primes.primesMNPS" $
    testOnList primesSumMNPS staticTests
  describe "Primes.primesSum" $ do
    testOnList primesSum staticTests
    it "works just like primesSumMNPS" $
      property $ \n -> let upto = abs (n `rem` 30000000)
                       in primesSum upto == primesSumMNPS upto


Batch file to copy specific folders and to delete the folder which is not copied This is the batch file I have created which copy the specific folders which I want. I use the specific server folder name of which I want to copy. This batch file will also delete the folder which you don't want to copy. My batch file will ask for the user input for the source path as well the destination path and to name the folder. If the user does not provides the source and destination path and folder name it will take the default value. I have also added a command which will start the specific service which is stopped. The concept of starting the service is totally related to server where we use to down the server. I also wanted my code to ask whether to delete those logs.

Please help me regarding this script and please suggest any improvements.

@echo off
:: variables
echo This script takes the backup of file SwiftALM Important folders
set /P SOURCEPATHPATH=Enter SOURCEPATH example:D:\backup\test\swift (default SOURCEPATH path=D:\backup\test\swift):
 if "%SOURCEPATH%"=="" (
set SOURCEPATH=D:\backup\test\swift
) else (
 set SOURCEPATH=%SOURCEPATH%
)
set /P DESTPATH=Enter DESTPATH (default DESTPATH path=D:\backup4):
if "%DESTPATH%"=="" (
set DESTPATH=D:\backup4
) else (
 set DESTPATH=%DESTPATH%
)
set /P folder=Enter folder name:
echo default folder name=%Date:~-7,2%-%Date:~-10,2%-%Date:~-4,4%
if "%folder%"=="" (
set folder=%Date:~-7,2%-%Date:~-10,2%-%Date:~-4,4%
) else (
 set folder=%folder%
)
echo folder=%folder%
mkdir %DESTPATH%\%folder%
set xcopy=xcopy /E/V/Q/F/H/I
echo echo conf folder will be copied
%xcopy% %SOURCEPATH%\conf %DESTPATH%\%folder%\conf
echo conf folder is copied
echo lib folder will be copied
%xcopy% %SOURCEPATH%\lib %DESTPATH%\%folder%\lib
echo lib folder is copied
echo deploy folder will be copied
%xcopy% %SOURCEPATH%\deploy %DESTPATH%\%folder%\deploy
echo deploy folder is copied
echo deployers folder will be copied
%xcopy% %SOURCEPATH%\deployers %DESTPATH%\%folder%\deployers
echo deplyers folder is copied
echo files will be copy press enter to proceed
pause
echo Deleting folders after copy
rmdir /s %SOURCEPATH%\tmp
rmdir /s %SOURCEPATH%\data
rmdir /s %SOURCEPATH%\work
echo folders have been removed
pause
echo To Start the task scheduler
sc start "Schedule"
pause

It seems you're not taking the advices from a previous reviews,
which gave very good points:

Always use SETLOCAL, unless you really really really want variables to affect the environment of the calling process
It's good to use @ECHO even if you set ECHO OFF at the top, in case you might want to temporarily disable the ECHO OFF for debugging purposes
It's good to add some blank lines to separate related blocks and improve readability

Now on to this new code.

The path D:\backup\test\swift appears twice in this statement:

set /P SOURCEPATHPATH=Enter SOURCEPATH example:D:\backup\test\swift (default SOURCEPATH path=D:\backup\test\swift):

It's good to avoid repeating yourself, because you might want to change the value later, and then you'll have to change every occurrence, and risk the mistake of forgetting to change one of them.
Extract duplicated strings into a variable, for example:

set DEFAULT_SOURCEPATH=D:\backup\test\swift
set /P SOURCEPATH=Enter SOURCEPATH example:%DEFAULT_SOURCEPATH% (default SOURCEPATH path=%DEFAULT_SOURCEPATH%):

Btw in the original code SOURCEPATHPATH looks like a typo (the ...PATHPATH at the end).

All the else blocks here look utterly pointless:

if "%SOURCEPATH%"=="" (
    ...
) else (
    set SOURCEPATH=%SOURCEPATH%
)
if "%DESTPATH%"=="" (
    ...
) else (
    set DESTPATH=%DESTPATH%
)
if "%folder%"=="" (
    ...
) else (
    set folder=%folder%
)

In all these examples, you set a variable to itself. That does nothing at all.
It would be better to completely omit these else statements,
and the code will be shorter.

A blank line between the treatment of SOURCEPATH, DESTPATH, folder would be nice for visual separation and readability.

Finally, I don't have a Windows with me now,
but I think this would work and be a better way of implementing those if conditions:

if not defined SOURCEPATH (
    ...
)

if not defined DESTPATH (
    ...
)

if not defined folder (
    ...
)


OpenCV calculate center of mass for a single contour I was recently going through some OpenCV code that finds the center of mass in a contour and realized there is potential room for optimisation.

What I noticed looking back through my code is when I calculate the mass I'm using contours.size() which makes me believe that I'm calculating the moments and center of mass for all of my contours.

In my cases this is not needed as I have a function that returns the largest contour that I have found getMaxAreaContourId(contours, imageSize); which returns an int that I can use like contours[largestContour] and largestContour would be set from the result of getMaxAreaContourId

So if I'm not mistaken would I be able to just find the moment and mass from a single contour and completely remove two for loops?

Because Ultimately I end up doing this:

int CenterOfContourX = mc[largestContour].x;
 int CenterOfContourY = mc[largestContour].y;

Really the end goal is to end up with two integers that represent the center of mass of my contour.

Code to optimise:

// Get the moments
mu.reserve(contours.size());
for( int i = 0; i < contours.size(); i++ )
{ mu[i] = moments( contours[i], false ); }

// Get the mass centers:
std::vector<cv::Point2f> mc( contours.size() );
for( int i = 0; i < contours.size(); i++ )
{ mc[i] = cv::Point2f( mu[i].m10/mu[i].m00 , mu[i].m01/mu[i].m00 ); }

It seem like you can definitely merge the two loops together.

Also you should avoid iterating using i, instead, use iterators, or the range-based for loop (since C++11).

// if you needed this vector
mu.reserve(contours.size());

// Get the mass centers:
std::vector<cv::Point2f> mc( contours.size() );
for( auto& c : contours )
{
  auto m = moments(c, false);
  mu.push_back(m); // only if you need the mu vector
  mc.push_back(cv::Point2f( m.m10/m.m00 , m.m01/m.m00 ));
}

And this can even be simplified to a call to std::transfer with std::back_inserter:

std::transform(
  std::begin(contours),
  std::end(contours),
  std::back_inserter(mc),
  [](const contour& c) -> cv::Point2f {
    auto m = moments(c, false);
    return cv::Point2f( m.m10/m.m00 , m.m01/m.m00 )
  }
);

And of course if you only need to know moments of one countour, there is no need to count it for all, but since you haven't included that part of the code, it's hard to tell anything about it.


CSV reader in Windows Forms I have a csv parsing method in my Windows App that reads headers and information from the subsequent lines. I am not sure if I am doing in an efficient way. Please comment.

public List<(string IPN, string FPTech)> GetIPNFPTechFromCSV(string filePath)
    {
        List<(string, string)> IPNFPTechPairs = new List<(string, string)>();
        using (StreamReader csvfileReader = new StreamReader(filePath))
        {
            List<string> headings = new List<string>();
            if (HasHeaderRow)
            {
                headings = LoadFieldNamesFromHeaderRow(csvfileReader.ReadLine());
                if (headings.Count != 2)
                    return null;
                else
                {
                    int IPNPos = headings.IndexOf("PartNumber");
                    int FPPos = headings.IndexOf("FPTech");
                    if (IPNPos != -1 && FPPos != -1)
                    {
                        string line;
                        while ((line = csvfileReader.ReadLine()) != null)
                        {
                            var IPNFPTechPair = (IPN: line.Split(Delimiter)[IPNPos], FPTech: line.Split(Delimiter)[FPPos]);
                            IPNFPTechPairs.Add(IPNFPTechPair);
                        }
                    }
                    else
                        return null;
                }
            }
            else
                return null;
        }
        return IPNFPTechPairs;
    }

In the caller I am doing

var InputPairs = inputcsvfile.GetIPNFPTechFromCSV(txt_filePath.Text);

            if (InputPairs == null)
            {
                MessageBox.Show("Please make sure input file is in valid format with PartNumber,FPTech Headers", "Error", MessageBoxButtons.OK, MessageBoxIcon.Error);
                return;
            }

To me, it looks like I am returning nulls from too many paths in my csv parsing method. Please let me know how can I improve this code. I am also not sure how do we efficiently log or report eceptions though out the Windows App. Any ideas are welcome please.

Sample Input CSV File

FPTech,PartNumber
TK0,H19873-001
TK1,H19873-001
TK2,H19872-001
TK1,H19874-001
TK0,H19872-001

The input .csv should be in this format and the FPTech and PartNumber Headers can interchange and sometime smissing. When missing, any of these I want to report to user.

if (HasHeaderRow)
        {

It seems strange, that you know in advance, if the file has headers or not? If that is right, why then call the method in the first place if HasHeaderRow is false - when you do nothing in that case?

  public List<(string IPN, string FPTech)> GetIPNFPTechFromCSV(string filePath)

I would return IEnumerable<..> instead and then use yield return to return each data pair (see below).
I would define a struct or class instead of using a named tuple. IMO named tuples are only useful very locally, because they can be harder to maintain, if their definition changes and you distribute them across the application (see below).

  var IPNFPTechPair = (IPN: line.Split(Delimiter)[IPNPos], FPTech: line.Split(Delimiter)[FPPos]);

Here you split the same string twice. It would be better to do:

string[] cells = line.Split(Delimiter);
var IPNFPTechPair = (IPN: cells[IPNPos], FPTech: cells[FPPos]);

You should handle all the heading stuff in a separate method in order to make the design and responsibility more clear.

All in all I would do something like:

public IEnumerable<IpnItem> Read(string filePath)
{
  using (StreamReader reader = new StreamReader(filePath))
  {
    (int ipnColumn, int fptechColumn) = ReadHeaders(reader);

    string line;
    while ((line = reader.ReadLine()) != null)
    {
      string[] cells = line.Split(Delimiter);
      yield return new IpnItem(cells[ipnColumn], cells[fptechColumn]);
    }
  }
}

private (int ipnColumn, int fptechColumn) ReadHeaders(StreamReader reader)
{
  string[] headers = reader.ReadLine().Split(Delimiter);
  if (headers.Length != 2) throw new Exception("Number of column headers should be 2");
  (int ipnColumn, int fptechColumn) columnIndices = (Array.IndexOf(headers, "PartNumber"), Array.IndexOf(headers, "FPTech"));
  if (columnIndices.ipnColumn == -1 || columnIndices.fptechColumn == -1) throw new Exception("Invalid or missing column names");
  return columnIndices;
}

Where IpnItem is declared as:

  public class IpnItem
  {
    public IpnItem(string ipn, string fpTech)
    {
      IPN = ipn;
      FPTech = fpTech;
    }

    public string IPN { get; private set; }
    public string FPTech { get; private set; }
  }


Scrape a jobs board and keep persistent record I'm trying to improve my pretty basic coding skills, and I've written this program to scrape a jobs board (I'd ultimately add several) and put the data in a db file with SQLITE3. The code works, but I can't help feeling that there are some simple things I might have done more gracefully.

One error I know about is that I don't have a primary key for my DB. I'm going to add this! In the meantime, I've just been querying the DB with something like
SELECT DISTINCT Title, Company FROM jobs WHERE Date BETWEEN datetime('now', '-3 days') AND datetime('now', 'localtime'); which works nicely.

Thanks in advance for any feedback!

import csv
import datetime
import urllib.request
import sqlite3
from sqlite3 import Error
from bs4 import BeautifulSoup
from dateutil import parser

"""
Scrapes STA jobs board, adds new jobs into a database
"""

def update_db():
    try:
        conn = sqlite3.connect('jobs.db')
        c = conn.cursor()

        # call the scraping functions
        soup = scrape_secret()
        jobs = clean_jobs(soup)
        result = organise(jobs)
        final = data_cleanser(result)
        export(final)

        # after exporting to csv (just in case) we delete the title row and convert nested lists to tuples
        del final[0]
        new_result = [tuple(l) for l in final]
        # only necessary once
        # c.execute('''CREATE TABLE jobs (Title, Company, Location, Type, Date Posted)''')

        c.executemany("INSERT INTO jobs VALUES (?,?,?,?,?)", new_result)
        conn.commit()
        conn.close()

    except Error as e:
        print(e)

# function to remove multiple occurrences of one term ('new')
def remove_value_from_list(the_list, val):
    return [value for value in the_list if value != val]

def length_enforcer(the_list, length):
    return [value for value in the_list if len(value) == length]

# hit the website and scrape the first page
def scrape_secret():
    url = "https://jobs.secrettelaviv.com/"
    req = urllib.request.Request(url, headers={'User-Agent': 'Mozilla/5.0'})
    page = urllib.request.urlopen(req)
    return BeautifulSoup(page, "html.parser")

def clean_jobs(soup):
    # jobs are in 'spans'
    all_spans = soup.find_all("span")
    jobs = []
    for span in all_spans:
        jobs.append(span.get_text().strip())
    # remove extraneous elements
    jobs.remove('')
    jobs.remove('Subscribe to our EVENTS Newsletter')
    jobs.remove('Join our facebook GROUP')
    jobs = remove_value_from_list(jobs, '')
    return remove_value_from_list(jobs, 'new')

def organise(jobs):
    # make list of lists
    result = [["Title", "Company", "Location", "Duplicate", "Type", "Date Posted"]]
    new_list = []
    for job in jobs:
        if len(new_list) == 6:
            a = list(new_list)
            result.append(a)
            new_list = [job]
        else:
            new_list.append(job)
    result.append(new_list)
    return length_enforcer(result, 6)

def data_cleanser(result):
    for i in result:
        del i[3]
        try:
            i[4] = parser.parse(i[4])
        except ValueError:
            pass
    return result

def export(result):
    csvfile = "secret_today" + datetime.datetime.today().strftime('%m-%d') + ".csv"
    with open(csvfile, "w") as output:
        writer = csv.writer(output, lineterminator='\n')
        writer.writerows(result)

if __name__ == '__main__':
    update_db()

Code Quality and Other Improvements

I would use a cursor and a connection as context managers to avoid having to explicitly close them and let Python safely close them if an error happens:

with sqlite3.connect('jobs.db') as connection:
    with connection.cursor() as cursor:

making the query defined as a multi-line string could benefit readability:

cursor.executemany("""
    INSERT INTO
        jobs
    VALUES (?, ?, ?, ?, ?)""", new_result)

watch for PEP8 code style violations - for instance, organize imports in a proper way

Performance

We can definitely do better performance-wise:

switch from lxml to html.parser (requires lxml to be installed):

BeautifulSoup(page, "lxml")

since you need only span elements, let BeautifulSoup parse only them:

from bs4 import BeautifulSoup, SoupStrainer

parse_only = SoupStrainer('span')
BeautifulSoup(page, "lxml", parse_only=parse_only)


A library written in CoffeeScript So, I wrote this small library for fast DOM building with API tailored for CoffeeScript

I feel like the code overall can be made better/smaller/faster.

I am also not particularly happy about a few functions, namely:

  # Parses out css classes and id from string like:
  #   p#warning.big.yellow # => p   # attr {"id": "warning", "class": ['big', 'yellow']}
  #   #container           # => div # attr {"id": "container"}
  # @returns node name (e.g. "span")
  dotHashRe = /[.#]/
  parseElem = (elem, attr) ->
    return elem unless dotHashRe.test elem
    attr['class'] ||= []
    attr['class'] = [attr['class']] if typeof attr['class'] == 'string'
    elem     = "div#{elem}" if dotHashRe.test(elem.charAt(0))
    pieces   = elem.split(dotHashRe)
    elemType = pieces.shift()
    pos      = elemType.length
    classes  = attr['class']
    for piece in pieces
      if elem.charAt(pos) == '#'
        attr['id'] = piece
      else
        classes.push(piece)
      pos += piece.length + 1
    delete attr['class'] unless attr['class'].length
    elemType

It is fast, but it looks like the code can be made a lot nicer

The library (~100 LOC with comments): https://github.com/glebm/DOMBrew/blob/master/dombrew.js.coffee

Looking forward to the suggestions! :)

A bunch of random observations.  I'd rethink the way you approach "classes".  I'd start with something like

classes = attr['class'] || []
classes = [classes] if typeof classes == 'string'

And then write or delete it back at the end.

Creating hashStartRe

hashStartRe = /^[.#]/
elemType = if hashStartRe.test elem then "div" else pieces.shift()

Will probably be faster than the current rewriting and calling elem.charAt(0).  It's definitely easier to read.  However, it causes problems with "pos".  (Solveable problems, but you do need to be careful.)

At this point, the big question is whether you want to allow multiple ids or ids that aren't in the front position.  If you restrict your attention to a#id.class1.class2 and don't process a.class1#id1#id2, you can simplify the for loop as well.

A final performance point, since regular expressions are so fast on modern browsers, you may want to consider using the regular expression /(.#)([^.#)+)/ to do your parsing.  But you'd need to sit down and do some proper analysis on what was faster.  My gut tells me it will be.


Can this parser (concerning about infinite hierarchy of operators) be implemented purely in ReadPrec? In Haskell, I tried to implement a parser for expressions containing hyperoperations, and finally succeeded. Valid expressions shall contain:

Parentheses.

Nonnegative integers.

Addition represented by +. It has precedence of 6, and its associativity shall be exploited.

Multiplication represented by *. It has precedence of 7, and its associativity shall be exploited.

Exponentiation represented by ^. It has precedence of 8.

Tetration represented by ^^. It has precedence of 9.

Pentation represented by ^^^. It has precedence of 10.

ad infinitum.

I was struggling for months to implement this with ReadPrec. It was truly a "Eureka" moment when I found that I have a workaround:
import Control.Monad
import Text.ParserCombinators.ReadPrec
import Text.Read
import Text.Read.Lex

hyperOp :: Int -> Integer -> Integer -> Integer
hyperOp 0 _ y = y + 1
hyperOp 1 x y = x + y
hyperOp 2 x y = x * y
hyperOp 3 x y = x ^ y
hyperOp n x 0 = 1
hyperOp n x y = hyperOp (n-1) x (hyperOp n x (y-1))

parseHyperOp :: Int -> ReadPrec Integer
parseHyperOp opn = parens . choice $
    lift readDecP
    : prec 6 (do
        m <- step (parseHyperOp opn)
        Symbol "+" <- lexP
        n <- parseHyperOp opn
        return (m + n))
    : prec 7 (do
        m <- step (parseHyperOp opn)
        Symbol "*" <- lexP
        n <- parseHyperOp opn
        return (m * n))
    : fmap (\c -> prec (7 + c) $ do
        m <- step (parseHyperOp opn)
        Symbol op <- lexP
        guard (replicate c '^' == op)
        n <- step (parseHyperOp opn)
        return (hyperOp (2 + c) m n)
        ) [1 .. opn]

highestOp :: String -> Int
highestOp "" = 0
highestOp ('^':str) = let
    (str1, str2) = span ('^'==) str
    in max (1 + length str1) (highestOp str2)
highestOp (_  :str) = highestOp str

parseOp :: Prec -> ReadS Integer
parseOp c str = readPrec_to_S (parseHyperOp (highestOp str)) c str

The trick is to find the highest operation within the input string. That makes the choice finite.
Still, I think it's quite ugly to use readPrec_to_S. Can this be implemented without using readPrec_to_S and co?

A simple rule of thumb is that you should never use readS_to_Prec and readS_to_P. Consider that once you've converted a parser to a ReadS to run it, there is no way back. (One handwavy explanation is that those functions are really dirty hacks, which is also why gather is undefined when they are used.)
Under those constraints, the only place where you can find out the maximal level opn of hyperoperators is at the very top level where you actually apply your parser to a string; that is the one location where you can scan the string to compute opn. That means that all your parsers need to be parameterized by opn in order to pass it down to parseHyperOp.
parseOp :: Int -> P.ReadP Integer
parseOp opn = readPrec_to_P (parseHyperOp opn) 0

-- Top-level runner for parsers parameterized by opn
runP :: (Int -> P.ReadP a) -> ReadS a
runP parser str =
  let opn = highestOp str in
  P.readP_to_S (parser opn) str

example = runP parseOp "(1 + 2) ^ 3 + 2 ^ 2"

Having an infinite number of precedence levels makes top-down parsing (the popular approach using parser combinators) rather problematic, since there isn't really a "top" to start from. Consider giving up that feature (by making all operators beyond a certain level non-associative at the same level), or switching to a bottom-up parsing strategy.


Linked list arithmetic I am practicing for interviews and I tried solving the problem on my own and got a solution. I  was wondering how can I improve the performance and memory on this program?

The problem performs addition on linked list nodes. The nodes are 1s->10s->100s->.... The goal of this program is to perform addition on two of these linked lists. The output should be a linked list of the same format.

Example:

  1 -> 4 -> 3
+ 1 -> 5 -> 9 -> 2

Linked list output:

2 -> 9 -> 2 -> 3

Here is my code:

public class prob2_5 {
    /*
     * You have two numbers represented by a linked list, where each node contains a single digit.
     * The digits are stored in reverse order, such that the 1's digit is at the head of the list.
     * Write a function that adds the two numbers and returns the sum as a linked list
     *
     */

    public static LinkedListNode addLists(LinkedListNode l1, LinkedListNode l2) {
        int l1sum = 0;
        int l2sum = 0;
        int multiplier = 1;

        while(l1 != null) {
            l1sum = l1sum + l1.value*multiplier;
            multiplier = multiplier*10;
            l1 = l1.next;
        }
        multiplier = 1;
        while(l2 != null) {
            l2sum = l2sum + l2.value*multiplier;
            multiplier = multiplier*10;
            l2 = l2.next;
        }

        int total = l2sum + l1sum;

        char[] totalnum = (total + "").toCharArray();
        int len = totalnum.length;
        LinkedListNode tail = new LinkedListNode(totalnum[len - 1] - '0');
        LinkedListNode newNode;
        LinkedListNode prevNode = tail;
        System.out.print(totalnum);

        for (int i = len - 2; i >= 0; i--) {
            newNode = new LinkedListNode(totalnum[i] - '0');
            prevNode.next = newNode;
            prevNode = newNode;
        }
        return tail;
    }
}

Here is my linked list class:

public class LinkedListNode {
    LinkedListNode next;
    int value;
    public LinkedListNode(int value) {
        this.next = null;
        this.value = value;
    }
}

I believe you are missing the point of the exercise - instead of 'decoding' the input numbers, and then 're-encoding' the output number - you are supposed to iteratively get to the answer by going over the nodes of both lists:

public static LinkedListNode addLists(LinkedListNode l1, LinkedListNode l2) {
  return addLists(0, l1, l2);
}

private static LinkedListNode addLists(int carryOver, LinkedListNode l1, LinkedListNode l2) {
  // stop conditions
  if (l1 == null && l2 == null && carryOver == 0) {
    return null;
  }
  if (l1 == null) {
    l1 = new LinkedListNode(0);
  }
  if (l2 == null) {
    l2 = new LinkedListNode(0);
  }

  // iteration
  int addedValue = l1.value + l2.value + carryOver;
  carryOver = 0;

  if (addedValue >= 10) {
    addedValue -= 10;
    carryOver = 1;
  }

  l3 = new LinkedListNode(addedValue);

  // recursion
  l3.next = addLists(carryOver, l1.next, l2.next);

  return l3;
}


Check if a string is a permutation of a palindrome using Python This is a solution to exercise 1.4 from Cracking the Coding Interview written using Python 3.

Given a string, write a function to check if it is a permutation of a palindrome.

  Example: 'Tact Coa'

  Output: True (permutations: "taco cat", "atco cta", etc.)

I wanted to get feedback on making my code clearer and more pythonic.  I believe my code has time complexity \$O(n)\$ and space complexity \$ O(n)\$ is that correct?  If so, is there any way to improve on either the space or time complexity?

from collections import Counter

def is_palindrome_permutation(data: str) -> bool:
    """Given a string, check if it is a permutation of a palindrome."""
    data = data.lower().replace(' ', '')
    num_odd = sum(1 for char, freq in Counter(data).items() if char != ' ' and freq % 2 == 1)
# Check for two types of palindromes , 1) Odd Length (e.g. abc cba) 2) Even Length (e.g. abc d cba)
    if num_odd == 1 and len(data) % 2 == 1 or num_odd == 0 and len(data) % 2 == 0:
        return True
    else:
        return False

Test

  datum = 'Tac4t co@A @4'

  print(is_palindrome_permutation(datum))

Result

  True

Test

  datum = 'Tac4t co@A @4s'

  print(is_palindrome_permutation(datum))

Result

  False

Some suggestions:

You strip out spaces, then on the next line check if each character is a space.  The second part is redundant.
The only possible values for freq % 2 are 0 and 1, so num_odd = sum(1 for char, freq in Counter(data).items() if char != ' ' and freq % 2 == 1) can be simplified as num_odd = sum(freq % 2 for Counter(data).values())
If num_odd is 0, then there must be an even number of elements. If num_odd is 1, then there must be an odd number of elements.  So really all you care about is if there is more than 1 odd count.
You can return the results of expressions directly.  So something like return x < y is simpler than using an if test and manually returning True or False depending on the outcome.
I would put the replace first since that reduces the number of characters that lower has to operate over (although this is probably a silly negligible micro optimization).

So the code can be simplified to:

from collections import Counter

def is_palindrome_permutation(data: str) -> bool:
    """Given a string, check if it is a permutation of a palindrome."""
    data = data.replace(' ', '').lower()
    return sum(freq%2 for freq in Counter(data).values()) < 2

One more thing you could do is put the Counter on the first line.  I think this hurts readability a bit, but does mean that your modified data can be garbage collected right away.  There is no point doing this with .values(), though, since this is just a view into the Counter object, so the Counter object has to be kept in memory.

from collections import Counter

def is_palindrome_permutation(data: str) -> bool:
    """Given a string, check if it is a permutation of a palindrome."""
    data = Counter(data.replace(' ', '').lower())
    return sum(freq%2 for freq in data.values()) < 2


fetch() with status code checking and re-routing In examples for fetch() I don't see many example for status code checking.

(1) I'm expecting to catch 403 unauthorised responses and push the login path to React router to render the login page. Haven't seen this elsewhere, but seems to work alright.

(2) Is it alright to nest the second .then the way I did or it should it somehow be outside the first .then?

loadData() {
    fetch(new UrlAssembler().template(this.props.endpoint).query(queryObject.get()).toString())
        .then(response => {
            if (response.status !== 200) {
                this.props.history.push( APP_LOGIN_PATH )
            } else {
                response.json().then(data =>
                    this.setState({
                        data: data,
                        loaded: true,
                        isModalActive: false,
                        selectedElement: null,
                        selectedId: null,
                    }))
                }
        })
}

I don't see many example for status code checking

I think this way of thinking is a remnant of jQuery.ajax where anything not 2xx rejects the promise. On the other hand, fetch makes a request, and returns an object that represents the response. It's up to you and your app to decide what's a "correct" response from a "wrong" response.

Coming from an Angular background, the task of capturing 403s and doing redirects is the job of an HTTP interceptor service and not your component. This way, your component is not aware of this logic (it simply makes requests) and the behavior is applied globally on all your API calls. But since React doesn't have the same service structure, we'll make do with wrapping functions.

So ideally, what your component should only ever contain is:

someWrappedVersionOfFetch(...)
  .then(response => response.json())
  .then(data => {
    this.setState({
      data: data,
      loaded: true,
      isModalActive: false,
      selectedElement: null,
      selectedId: null,
    }))
  })

Then the wrapped fetch would be like:

export const someWrappedVersionOfFetch = (...args) => {
  return fetch(...args)
    .then(response => {

      if (response.status !== 200) {
        // Update your history and reject the promise
        history.pushState( APP_LOGIN_PATH )
        return Promise.reject(response)
      } else {
        // Just pass response through.
        return Promise.resolve(response)
      }
    })
}

The above answers your first question. For your second question, you could do it like the second snippet. You put the conditional on the first then, have it conditionally return a resolved or rejected promise. Then just chain the one with setState after it.


fs to fs-extra using node js I wrote a code by using var fs= require("fs").promises;

to read all my files in directories and sub-directories

var fs= require("fs").promises;

async function checkFileLoc(folderPath, depth) {
  depth -= 1;
  let files = await fs.readdir(folderPath);
  files = await Promise.all(
    files.map(async (file) => {
      const filePath = path.join(folderPath, file);
      const stats = await fs.stat(filePath);
      if (stats.isDirectory() && depth > 0) {
        return checkFileLoc(filePath, depth);
      } else if (stats.isFile()) return filePath;
      else return null;
    })
  );
  return files
    .reduce((all, folderContents) => all.concat(folderContents), [])
    .filter((e) => e != null);
}

Then I install fs-extra package in my project and replace var fs=require("fs");to var fse=require("fs-extra"); and made changes in my code like this

var fse=require("fs-extra");

async function checkFileLoc(folderPath, depth) {
  depth -= 1;
  let files = await fse.readdir(folderPath);
  files = await Promise.all(
    files.map(async (file) => {
      const filePath = path.join(folderPath, file);
      const stats = await fse.stat(filePath);
      if (stats.isDirectory() && depth > 0) {
        return checkFileLoc(filePath, depth);
      } else if (stats.isFile()) return filePath;
      else return null;
    })
  );
  return files
    .reduce((all, folderContents) => all.concat(folderContents), [])
    .filter((e) => e != null);
}

when I was using fs i was getting the desire output and someone told me fs-extra is advance then fs and all you have to do is replace fs with fse
my code not working properly any solution where i have made mistake ?

I tried to figure it out what changes i have to made if I am using var fse= require("fs-extra");
so my code will look like this

var fse=require("fs-extra");

async function checkFileLoc(folderPath, depth) {
  depth -= 1;
  let files = fse.readdirSync(folderPath);
  files = await Promise.all(
    files.map(async (file) => {
      const filePath = path.join(folderPath, file);
      const stats = fse.statSync(filePath);
      if (stats.isDirectory() && depth > 0) {
        return checkFileLoc(filePath, depth);
      } else if (stats.isFile()) return filePath;
      else return null;
    })
  );
  return files
    .reduce((all, folderContents) => all.concat(folderContents), [])
    .filter((e) => e != null);
}

I replaced my

var fs=require("fs"); to var fse=require("fs-extra");
let files = await fs.readdir(folderPath); with let files = fs.readdirSync(folderPath);
const stats = await fs.stat(filePath); with const stats = fs.statSync(filePath);

now i am getting my desire output


If-statement consolidation I often face the problem of stacked if-statements that look repetitive, but usually can't find an easy way to simplify it.

    if (keyState[BOTTOM]){
        dig_direction = BOTTOM;
        startDig(SIZE / 2 - x % SIZE, DIG_DEPTH);
    } else if (keyState[RIGHT] && isTouching[RIGHT]){
        dig_direction = RIGHT;
        startDig(DIG_DEPTH, 0);
    } else if (keyState[LEFT] && isTouching[LEFT]){
        dig_direction = LEFT;
        startDig(-DIG_DEPTH, 0);
    }

You want Replace Conditional with Polymorphism : "Move each leg of the conditional to an overriding method in a subclass. Make the original method abstract."

Your conditional from the example could then look like this :

digPosition.startDig()

digPosition being an instance of an interface DigPosition which has three implementations : BottomDigPosition, RightDigPosition and LeftDigPosition.


Design of thread pool reuse with Executors.newFixedThreadPool in Java In my application I have code which should run every hour on a new items and execute some expensive operation with multithreading:

int i = 1;

//noinspection InfiniteLoopStatement
while (true) {

    try {
        Logger.printIterationNum("Text analysis", "beg", i);

        Document query = new Document("fetchStatus", new Document("$lte", fetchStatusParam));
        ArrayList<Document> unfetchedEvents = dbC_Events.find(query).projection(
                fields(include("_id"), include("link"), include("title"), include("summary"))
        ).into(new ArrayList<>());

        // get full text and images of the events
        this.getFullTextImg(dbC_Events, unfetchedEvents);

        Logger.printIterationNum("Text analysis", "end", i++);

        // make a pause between executions
        Pauser.rest(Pauser.Units.SECONDS, PAUSE_TIME_OUT);
    } catch (Exception e) {
        i = 1;
        e.printStackTrace();
    }
}

private void getFullTextImg(MongoCollection<Document> dbC_Events, ArrayList<Document> unfetchedEvents) {

    int cpuCoresNum = SYS_MAX_NUM_CPU_CORES;
    int itemsPerCore = unfetchedEvents.size() / cpuCoresNum;

    // prevent infinite thread generation if num of unfetched events is less than number of CPU cores
    if (itemsPerCore == 0) {
        itemsPerCore = unfetchedEvents.size();
    }

    // update status of queried items
    DBAgent.updateItemsStatus(dbC_Events, unfetchedEvents, FetchStatus.IN_PROCESS_FETCH.getID());

    int itNum = 1;
    int upperIndex = 0;

    ExecutorService service = Executors.newFixedThreadPool(cpuCoresNum);

    for (int i = 0; i < unfetchedEvents.size(); i += itemsPerCore) {

        int indexFrom = i;
        upperIndex += itemsPerCore;
        int toIndex = upperIndex;

        if (((unfetchedEvents.size() - toIndex) <= itemsPerCore) && (itNum == cpuCoresNum)) {
            toIndex = unfetchedEvents.size();
            i = unfetchedEvents.size();
        }

        int indexTo = toIndex;
        int treadID = itNum++;

        service.execute(() -> {
            try {
                Thread.currentThread().setName("im_evFullTextImg_#" + String.format("%03d", treadID));
                this.fetcher(dbC_Events, new ArrayList<>(unfetchedEvents.subList(indexFrom, indexTo)));
            } catch (Exception e) {
                e.printStackTrace();
            }
        });
    }

    // wait until all threads will be finished
    service.shutdown();
    try {
        service.awaitTermination(Long.MAX_VALUE, TimeUnit.MINUTES);
    } catch (InterruptedException e) {
        e.printStackTrace();
    }
}

My questions:

Is it a good idea to initialize the Executors.newFixedThreadPool(cpuCoresNum) each time I execute the getFullTextImg() method, or is it better to initialize it once and pass it as a parameter each time I call getFullTextImg()?
If I initialize Executors.newFixedThreadPool(cpuCoresNum) out of getFullTextImg() and pass it as a paramete,  will it cause some issues since I use service.shutdown();, which prevents accepting new tasks to the thread pool?
If service.shutdown(); is a problematic to reuse the thread pool, how can I redesign it in order to still wait until all threads in a poll will be finished, and only after to continue the code flow?
Can the Executors.newFixedThreadPool(cpuCoresNum) reuse lead to memory leak issues? Is there some important thing I have to do (e.g. clean the pool) with the thread pool in order to reuse it?

Answers:

It is better to reuse the ExecutorService.  Destroying threads and then recreating this is expensive.  (The only scenario where destroying and recreating a pool might be advisable is when the pool is going to be unused for a long time, and physical memory usage is a problem.  But even then, some flavors of executor are able of shrinking the thread pool size when there is no demand.)
Well yes, that would be a problem.  But that really means that you should not call shutdown on an executor if you still may need to submit new tasks in it.
You can't reuse an executor after calling shutdown on it.
There should be no memory leaks ... apart from the fact that the idle threads themselves are using memory (for thread stacks), and will continue to use it until the executor is shut down.  For a fixed sized / bounded sized pool, that memory usage by threads is bounded.  Most people would not call a bounded memory usage a "leak".

In fact, not reusing an executor is likely to lead to a (real) memory leaks= if you somehow neglect to call shutdown on the executor.


ServiceLocator for easy and central access to replaceable parts For a bigger project of mine, I needed a simple and extensible solution for easy access to replaceable Parts.

This example will show the usage:

Define an Interface, describing the replaceable part Eg: IReportSender
Define the concrete EMailReportSender and TelegramReportSender
Both will be implementing the IReportSender
On Startup (or anywhere else in the Application) add it to the ServiceLocator:

ServiceLocator.getServiceLocator().addService(IReportSender.class, new EMailReportSender());

Anywhere else in the application get the Service and use it:

IReportSender reportSender = ServiceLocator.getServiceLocator().getService(IReportSender.class);
reportSender.SendReport([...]);

And now the Code:

import java.util.HashMap;

public class ServiceLocator {

    private static ServiceLocator instance;
    private HashMap<Class<?>, Object> services;

    /**
     * Creates a new instance of the {@link ServiceLocator}
     */
    private ServiceLocator() {
        services = new HashMap<Class<?>, Object>();
    }

    /**
     * Gets the {@link ServiceLocator}
     * @return the current {@link ServiceLocator}
     */
    public static ServiceLocator getServiceLocator() {
        if (instance == null) {
            instance = new ServiceLocator();
        }

        return instance;
    }

    /**
     * Adds a service
     * @param clazz class of the defining interface
     * @param service instance of the actual implementation
     * @return true, if the service was added, otherwise false
     */
    public boolean addService(Class<?> clazz, Object service) {
        if (services.containsKey(clazz) || !clazz.isAssignableFrom(service.getClass())) {
            return false;
        }

        services.put(clazz, service);

        return true;
    }

    /**
     * Gets a service
     * @param clazz class of the defining interface
     * @param <T> defining interface
     * @return instance of the actual implementation
     */
    public <T> T getService(Class<T> clazz) {
        if (!services.containsKey(clazz)) {
            return null;
        }

        Object service = services.get(clazz);
        return clazz.cast(service);
    }

    /**
     * Removes a service
     * @param clazz class of the defining interface
     * @return true, if the service was removed, otherwise false
     */
    public boolean removeService(Class<?> clazz) {
        if (!services.containsKey(clazz)) {
            return false;
        }

        services.remove(clazz);

        return true;
    }

    /**
     * Removes all services
     */
    public void removeAllServices() {
        services.clear();
    }

}

You have some clean looking code there, but there's a huge problem with Service Locators. They're hidden dependencies. If I reference your class from elsewhere, I'm going to be really confused about the exceptions I'm going to get when I try to use it.

  Where is this null EmailService exception coming from?! WTF???

It's much better to use proper constructor injection of dependencies. That makes it crystal clear what is needed to make your class work.


Building a profile image with a fly-out menu in the header Background

I am more formally trained in backend/server-side coding. Over my career I have often found myself thrown into front-end coding. I have spent the last year and a half learning JavaScript/CSS3 on an "on Demand" basis. I feel like I am getting the hang of it, doing fancier things to meet UX requirements like animations and being able to layout a page without relying too much on a framework like Bootstrap.

Code

In this sample, I have put together an animated flyout menu for a profile image in a header. The idea is, you click on your profile image, and a menu animates to opacity 1, click it again, and the menu animates to opacity 0. For the sake of simplicity, the profile image is simply a cyan circle shaped div . If you would be so kind as to point out any flaws, or better practices I can adopt to make this code more readable/reliable/maintainable, it would be appreciated.

To see a working demo, I have built it in codepen: http://codepen.io/anon/pen/dNxeRN

HTML

    My Logo And Brand

Account settings

My Profile

Logout

CSS

*{
  margin: 0;
  padding: 0;
  font-family: helvetica, arial, sans-serif;
}

.header {
  padding-top: 5px;
  width: 1000px;
  height: 60px;
  border-bottom: 1px solid darkgrey;
  position: relative;
}

a.logo {
  display: inline-block;
  font-size: 25px;
  margin-top: 14px;
  margin-left: 10px;
  text-decoration:none;
  color: black;
}

.profile-image {
  width: 50px;
  height: 50px;
  background-color: cyan;
  border: 2px solid grey;
  border-radius: 50px;
  cursor: pointer;
  float:right;
  margin-right: 50px;
}

.fly-over{
  position: absolute;
  display: none;
  width: 300px;
  border-radius: 5px;
  box-shadow: 1px 1px 10px 1px rgba(0,0,0,.3);
  text-align: left;
  right: 36px;
  top: 80px;
}

.fly-over ul,
.fly-over ul li {
  margin: 0px;
  padding: 0px;
}

.fly-over ul li {
  list-style-type: none;
  padding-left: 20px;
  padding-top: 10px;
  padding-bottom: 10px;
  border-bottom: 1px solid lightgrey;
  cursor: pointer;
  transition: color, background-color 333ms;
}

.fly-over ul li:hover{
  background-color: #888888;
  color: white;
}

.fly-over:after{
  content: " ";
  display:block;
  position: absolute;
  width: 0px;
  height: 0px;
    border-left: 10px solid transparent;
    border-right: 10px solid transparent;
    border-bottom: 10px solid grey;
  bottom: 100%;
  left: 250px;
}

.fly-over.show{
  display: block;
}

.animation-fade-in,
.animation-fade-out{
  display: block;
  animation-name: fadeIn;
  animation-duration: 333ms;
  animation-timing-functon: ease-out;
  animation-fill-mode: forwards;
  animation-direction: normal;
}

.animation-fade-out{
  animation-direction: reverse;
}

@keyframes fadeIn{
  0%{
    opacity: 0;
  }
  100%{
    opacity: 1;
  }
}

JS

var el = document.getElementsByClassName("profile-image")[0];
var menuState = {
  visible: false
};
el.addEventListener('click', function(){
  var flyOverEl = document.getElementsByClassName("fly-over")[0];
  function onFadeInAnimationEnd(){
    this.classList.add('show');
    this.classList.remove('animation-fade-in');
    menuState.visible = true;
    this.removeEventListener('animationend', onFadeInAnimationEnd);
  }
  function onFadeOutAnimationEnd(){
    this.classList.remove('animation-fade-out');
    menuState.visible = false;
    this.removeEventListener('animationend', onFadeOutAnimationEnd);
  }
  if(menuState.visible){
    flyOverEl.classList.remove("show");
    flyOverEl.addEventListener("animationend", onFadeOutAnimationEnd)
    flyOverEl.classList.add("animation-fade-out");
  }
  else{
    flyOverEl.addEventListener("animationend", onFadeInAnimationEnd)
    flyOverEl.classList.add("animation-fade-in");
  }
})

I've reviewed your code and I can make the following remarks:

CSS rules

I dont find your css particularly bad its just you need to think how the rules can fit each other logically with the css class names you assign to. Lets go in depth:

.header {
  width: 1000px;

Why did you hardcode 1000 px here? If you don't know the width sizes of the screen you want to target its better if you give it full width like width: 100% and if you want some minimum widths so things will not collapse you can add a rule like min-width: 700px

.animation-fade-out{
  animation-duration: 333ms;

.fly-over ul li {
  transition: color, background-color 333ms;

I would certainty reduce the animation duration of those 2 to be no more than 0.2s.If you notice the transitions are too fancy and they don't look appealing to the eye. Use a smaller duration to make them smoother.

.fly-over{
  display: none;
  width: 300px;
  border-radius: 5px;
  box-shadow: 1px 1px 10px 1px rgba(0,0,0,.3);
  text-align: left;

  position: absolute;
  right: 36px;
  top: 80px;
}

My main concern on this class is the last 3 rules  about positioning. Why did you choose to mix positions with decorations in the same class? It makes it more difficult to reuse its a one off. It may not be bad for small apps but for larger ones this really counts.

If I were to took they fly-over class and use it on another element I would have to override the positioning with a new class which would add too much bloat. A better solution is to make this class a little more generic and name it like fly-over-menu and add another class a little more specific named profile-menu with the positioning rules. That way I can reuse the fly-over-menu in other places.

background-color: #888888;
color: white;

I don't like mixing color code names with hex values. Pick one preferred representation and stick with it. That way the code will be more consistent.

Now lets focus on the Javascript:

Global state pollution

var menuState = {
  visible: false
};

Why did you choose to save state here? As you will read bellow the state actually is saved in the element classlist. If the element has a class show its visible otherwise its hidden provided that it has display:none by default.

And one other thing What are you saying here is hey I want to save this object to the global namespace and add one property visible to it. You will literally have window.menuState = {visible: false}; Why do you want to save something so specific in a global object?

You really don't want to do that. What if you had 10 of those menus? You had to change the representation of the state object every time. And if you wanted more variations of it like poping-menu or something you would have to add more objects. State management in Javascript is still behind so try to be a little bit more resourceful with what you have.

DOM manipulation

el.addEventListener('click', function(){
  var flyOverEl = document.getElementsByClassName("fly-over")[0];
  function onFadeInAnimationEnd(){
    this.classList.add('show');
    this.classList.remove('animation-fade-in');
    menuState.visible = true;
    this.removeEventListener('animationend', onFadeInAnimationEnd);
  }

  function onFadeOutAnimationEnd(){
    this.classList.remove('animation-fade-out');
    menuState.visible = false;
    this.removeEventListener('animationend', onFadeOutAnimationEnd);
  }

 if(menuState.visible){
    flyOverEl.classList.remove("show");
    flyOverEl.addEventListener("animationend", onFadeOutAnimationEnd)
    flyOverEl.classList.add("animation-fade-out");
 }
 else{
    flyOverEl.addEventListener("animationend", onFadeInAnimationEnd)
    flyOverEl.classList.add("animation-fade-in");
 }

})

Oh boy. I don't mind using plain javascript for simple things but did you notice while typing this code that you are repeating yourself? I did just by reading it. What you are really doing here is this. You just toggling class names of the fly-over element depending if the element is visible or not. You also hook the animationend events that are getting fired once the animation is completed to just clean up the event listeners.

I don't think its too bad but to be honest I believe its too low level. You want something more abstract and simplified when dealing with DOM events and callbacks. so to have time to work on more interesting things. If were you I would use jQuery to handle all of this staff don't get me wrong but also I think its safer.Its more of a personal preference this one so you can ignore it.

However check out this line:

var flyOverEl = document.getElementsByClassName("fly-over")[0];

You are lucky here that you have only one element with that class in the document. If you were to add more than one you would see flying ponies.

Also note that the query selector for the fly-over is the only specific thing here and its better if its passed as a parameter using bind or wrap the function in a wrapper and apply the callback with the arguments. That way you can pass whatever class you like in the event callBack. I will leave that as an exercise.

In general I think you are working it well its just some peculiarities of the front-end development you need to understand better.


Posting accounting data to a report I have some buttons on an Excel report. 2 in particular are for posting all Budget numbers to the report and posting the Actual numbers. I currently have 2 separate subs, one assigned to each button.

Public Sub PostAllActual()
  Dim sheet As Worksheet
  Dim rFind As Range

  For Each sheet In ActiveWorkbook.Worksheets                       'Cycle through all of the sheets
    If sheet.Name <> "Index" Then                                   'Skip the Index sheet
      'Search for the Index row in Col A
      Set rFind = sheet.Cells.Find("Index", [H2], xlValues, xlWhole, xlByColumns, xlNext)
      If rFind Is Nothing Then                                      'Did we find the Index row?
        'Not Found - ORatio sheet
      Else                                                          'Found
        PostNumbers sheet, 37, "I"                                  'Procedure to copy numbers to the YTD
      End If
    End If
  Next sheet                                                        'Move to the next sheet
  Beep                                                              'Auditory notification of completed process
End Sub

Public Sub PostAllBudgets()
  Dim sheet As Worksheet
  Dim rFind As Range

  For Each sheet In ActiveWorkbook.Worksheets                       'Cycle through all of the sheets
    If sheet.Name <> "Index" Then                                   'Skip the Index Sheet
      'Search for the Index row in Col A
      Set rFind = sheet.Cells.Find("Index", [H2], xlValues, xlWhole, xlByColumns, xlNext)
      If rFind Is Nothing Then                                      'Did we find the Index row?
        'Not Found - ORatio sheet
      Else                                                          'Found
        PostNumbers sheet, 21, "H"                                  'Procedure to copy the numbers to the YTD
      End If
    End If
  Next sheet                                                        'Move to the next sheet
  Beep                                                              'Auditory notification of completed process
End Sub

Do I really save anything by reducing these to simple sub calls on the button click, and passing in the 2 parameters like this?

Public Sub PostAllBudgets()
  PostAllNumbers 21, "H"
End Sub

Public Sub PostAllActual()
  PostAllNumbers 37, "I"
End Sub

Public Sub PostAllNumbers(ByVal iRow as Integer, ByVal sCol as String)
  Dim sheet As Worksheet
  Dim rFind As Range

  For Each sheet In ActiveWorkbook.Worksheets                       'Cycle through all of the sheets
    If sheet.Name <> "Index" Then                                   'Skip the Index Sheet
      'Search for the Index row in Col A
      Set rFind = sheet.Cells.Find("Index", [H2], xlValues, xlWhole, xlByColumns, xlNext)
      If rFind Is Nothing Then                                      'Did we find the Index row?
        'Not Found - ORatio sheet
      Else                                                          'Found
        PostNumbers sheet, iRow, sCol                                  'Procedure to copy the numbers to the YTD
      End If
    End If
  Next sheet                                                        'Move to the next sheet
  Beep                                                              'Auditory notification of completed process
End Sub

Your indentation is consistent, but non-standard. Someone else inheriting that code might not be bothered with lining up all the end-of-line comments in the same column, and if their editor sets indent spaces to 4 instead of 2 they're likely going to turn it into a bit of a mess.
Unless they're using Smart Indenter or Rubberduck to automate the indentation. If you are not using a tool to achieve that indentation, then you're wasting your time lining up all these comments.
But are they good comments worth keeping around?

Comments are supposed to make plain what the code does not tell us already.

Good code seldom needs comments.

Good comments say why, not what.

https://codereview.stackexchange.com/a/90113/23788

From what I can see, every single comment is merely rephrasing the code and brings absolutely nothing to the table. If I were maintaining that code, I'd wipe them all out. All of them.
There are a number of other issues with the code:

Set rFind = sheet.Cells.Find("Index", [H2], xlValues, xlWhole, xlByColumns, xlNext)

[H2] is evaluated off the active worksheet, which may not be the sheet that's being iterated. That makes the .Find call hard to parse, and makes me wonder if it works as it should. In 15 years I've never ever needed to use a bracketed expression anywhere. I'd replace it with sheet.Range("H2") to be safe.
I like that your Public members are explicitly public - because by default they're implicitly so. I also like that you're passing your parameters ByVal - especially since the default in VBA is an implicit ByRef.
iRow As Integer is potentially problematic: an Integer is a 16-bit signed integer type with a maximum value of 32,767... and an Excel worksheet can have many times more rows than that. Regardless of how many rows you think you need or can possibly deal with, you should always use Long instead of Integer, at least when referring to sheet rows.
I like how you used sheet as an identifier for a Worksheet object. I'm not sure what the r is doing in rRange, or what i does in iRow, or the s in sCol. These prefixes are an incarnation of the bad kind of Hungarian Notation and should be killed with fire.

iRow As Integer could be destinationRowIndex As Long if that's its purpose (not clear from the posted code). Notice iRow As Long defeats the Hungarian prefix, and lRow As Long uses a lowercase l which can easily be confused for an 1 in the VBE's default font... which is uselessly confusing, since legal identifiers can't start with a digit in VBA.
sCol As String could be destinationColumnHeading As String again if that's its purpose. Note that the distinction between Index and Heading reveals an implementation detail of your PostNumbers procedure; you're definitely using them to concatenate some Range address. Why not give it a single Range parameter then?
rFind As Range could be findResult As Range. At least it wasn't an oRange... although, oranges are tasty ;-)

Note, these are suggestions - I'm not saying destinationColumnHeading is the ideal name to use here. What I am saying, is that an identifier should convey its purpose, not its type. The IDE tells you what the type is already.

I agree with your refactoring. What you've done here is called extracting a method, and it's a very good way to eliminate redundancies in code. Redundant code is bad for several reasons; the DRY (Don't Repeat Yourself) principle exists to remind us that it's always better to have one single place to make a change, than having multiple places to fix in order to make the same change - and run the risk of forgetting one and introducing bugs, as Victor's answer points out.
IMO, macros (procedures attached to some worksheet button) should be tiny little things, so I like what I'm seeing with these two one-liners that call a lower-abstraction procedure.


Get rid of nested ifs and else I have a contact model, on change I set the state of the contact object, how can I make the below function more readable?
The address field is different because it's an object: ['address', 'city', 'state', 'zip']
  const onFieldChange = (
    key,
    index = 0,
    isMulti = false,
    isAddress = false
  ) => event => {
    const value = event.target.value
    const updatedContact = {...contact}
    if (!isMulti) {
      updatedContact[key] = value
      if (
        key === 'type' &&
        value === 'Individual' &&
        contact.entityFormationDate
      ) {
        delete updatedContact.entityFormationDate
      }
    } else {
      const keyInternal = isAddress ? key : 'value'
      key = isAddress ? 'addresses' : key
      updatedContact[key][index][keyInternal] = value
    }
    setContact(updatedContact)
  }

Code readability is a subjective quantity, however it is still considered as on-topic here at Code-Review.
General readability points
Note that code in isolation is must rely on presumptions. The scope, naming, and missing defines may force some of the following points to change if known.

Use simplest statements possible. For example the statement if (!isMulti) { can be simplified to if (isMulti) { and swapping the code blocks.

Avoid breaking lines unless they extend past the right of screen.
Some people use 80 columns (a little last century in my book (word warp is standard in IDE's these days)), 120 or even 160 will be readable in all but the smallest of devices.

Avoid repetition. You test isAddress twice within the listener. This can be done once outside the returned function. See rewrite.
This may not be considered a readability issue, but part of good code readability is clearly showing intent.
To read the two lines testing isAddress I immediately questioned why in the event? This is already known? I had to check as your intent was muddied.

The function onFieldChange is not an event listener, it returns a listener. The the prefixed on obfuscates its usage.
The name would be better as just createFieldChangeEvent, fieldChangeListener, or fieldChange depending on the functions scope.

Semicolons go a long way to improve readability, especially for coders that are swapping between C like languages.

The string literals should be constants, however without the full scope and usage of these strings it is hard to define where and how to declare. So I leave them as is (apart from changing to double quotes)

Rewrite
Two rewrites to contrast the subjective nature of readability.
General rewrite
Using the above points the code can be rewritten as
const fieldChangeListener = (key,  index = 0, isMulti = false, isAddress = false) => {
  const [field, internalKey] = isAddress ? [key, "addresses"] : ["value", key];
  const isTypeKey = key === "type";
  return event => {
    const value = event.target.value;
    const updated = {...contact};
    if (isMulti) {
      updated[field][index][internalKey] = value;
    } else {
      updated[key] = value;
      const isTypeIndividual = value === "Individual" && isTypeKey;
      if (isTypeIndividual && contact.entityFormationDate) {
        delete updated.entityFormationDate;
      }
    }
    setContact(updated);
  }
}

Is it more readable. We that depends on the readers level of experience and skill.
Compact rewrite
Personally I would have compacted the code further as its the total quantity (lines of code) that make code navigation the crucial readability factor. Code is seldom read in isolation.
const fieldChange = (name,  idx = 0, isMulti = false, isAddress = false) => {
  const [internal, fieldName] = isAddress ? ["addresses", name] : [name, "value"];
  const isType = name === "type";
  return event => {
    const [updated, value] = [{...contact}, event.target.value];
    if (isMulti) {
      updated[fieldName][idx][internal] = value;
    } else {
      const canDelete = (updated[name] = value) === "Individual" && isType;
      canDelete && contact.entityFormationDate && (delete updated.entityFormationDate);
    }
    setContact(updated);
  }
}

Defined readability
Code readability is very dependent on code standards used in projects. Standards must be defined in writing.
This is a document that all project coders should be thoroughly versed in and defines the styles, naming, layout, language features, etc... used and not to use in each project.
It can be argued that a comprehensive code standards document makes it impossible to write unreadable code.
To start a project without a standards document has already failed the first rule of readability. "Subjectivity is unreliable".
Example standards (as un-categorized points) I used for "Compact rewrite"

Fields to have names not keys.
index is always written idx.
Short circuited end clauses must be grouped.
Single line expression with multiple assignments must group each assignment.
Use semicolons.
Prefer declarations using destructuring assignments over multi-line declarations.


Battleship coordinate picker I present to you... my battleship location picker! This will be part of a larger battleship program. The coordinates that are important are finalcoords.

from operator import itemgetter
import getch
import skilstak.colors as c #colors
BOARDLENGTH = 10
BOARDHEIGHT = 10
board = [['_'] * BOARDLENGTH for _ in range(BOARDHEIGHT)]
class Ship():
    def __init__(self, size, symbol):
        self.size = size
        self.symbol = symbol
        self.coords = [(x,0) for x in range(size)]
        self.previous_coords = [(None,None) for _ in range(size)]
    def shift(self,direction):

        x_values = list(map(itemgetter(0), self.coords))
        y_values = list(map(itemgetter(1), self.coords))
        if direction == 'R' and 0 not in x_values:
            change = (-1,0)
        elif direction == 'D' and BOARDHEIGHT-1 not in y_values:
            change = (0,1)
        elif direction == 'U' and 0 not in y_values:
            change = (0,-1)
        elif direction == 'L' and BOARDLENGTH-1 not in x_values:
            change = (1,0)
        else:
            change = (0,0)
        for i, coord in enumerate(self.coords):
            self.previous_coords[i] = self.coords[i]
            self.coords[i] = (coord[0] + change[0], coord[1] + change[1])

    def rotate(self):
        a, b = self.coords[0][0], self.coords[0][1]
        x, y = self.coords[-1][0], self.coords[-1][1]
        newx, newy = -(y-b)+a, (x-a)+b
        if newy >= 0 and newx >= 0 and newy <= BOARDHEIGHT-1 and newx <= BOARDLENGTH-1:
            for i, coord in enumerate(self.coords):
                self.previous_coords[i] = self.coords[i]
                self.coords[i] = (-(self.coords[i][1]-b)+a, (self.coords[i][0]-a)+b)

    def show(self, board, hasprevious=True):
        if hasprevious:
            for pcoord in self.previous_coords:
                board[pcoord[1]][pcoord[0]] = '_'
        for coord in self.coords:
            board[coord[1]][coord[0]] = self.symbol
        display_board = '\n'.join(' '.join(line) for line in board)
        print(c.clear + display_board) #c.clear clears the screen
        return board
def hide(board,coords):
    for coord in coords:
        board[coord[1]][coord[0]] = '_'
    return board

def choose_locations(board):
    ships = [Ship(5,'A'),Ship(4,'B'),Ship(3,'c'),Ship(3,'C'),Ship(2,'D')]
    finalcoords = []
    for ship in ships:
        board = ship.show(board,hasprevious=False)
        while True:
            key = getch.getch()
            if key == 'w':
                ship.shift('U')
                board = ship.show(board)
            elif key == 's':
                ship.shift('D')
                board = ship.show(board)
            elif key == 'a':
                ship.shift('R')
                board = ship.show(board)
            elif key == 'd':
                ship.shift('L')
                board = ship.show(board)
            elif key == 'r':
                ship.rotate()
                board = ship.show(board)
            elif key == 'q':
                exit()
            elif key == '\r':
                intersection = list(set(ship.coords).intersection(finalcoords))
                if intersection == []:
                    finalcoords += ship.coords
                    board = hide(board,ship.coords)
                    break
                else:
                    print('''You already have a ship or part of a ship there.Your current coordinates are {}
You have placed a ship on these coords, {}.
You overlapped on these coords: {}'''.format(ship.coords,finalcoords,intersection))
            else:
                print('press w,a,s or d to move, and r to rotate. Press q to quit.')
    return finalcoords
if __name__ == '__main__':
    coords = choose_locations(board)

Here are some observations:

no meaningful comments or docstrings (they are not always needed, but imagine there is that "violent psychopath, who knows where you live" will be reading the code)
PEP8 violations and making things Pythonic:

blank line between class methods, two blank lines between top-level functions and classes (source)
correct import organization
the inline comments formatting
variable naming, if defining a constant, there has to be an underscore between words:

BOARDLENGTH -> BOARD_LENGTH
BOARDHEIGHT -> BOARD_HEIGHT

a space between arguments after a comma:

ship.show(board, hasprevious=False)
            HERE^

no need for extra () after the class definition
if intersection == []: can be rewritten as if not intersection:
simplify the if condition inside the rotate() method, replace:

if newy >= 0 and newx >= 0 and newy <= BOARDHEIGHT-1 and newx <= BOARDLENGTH-1:

with:

if 0 <= newx <= BOARDLENGTH - 1 and 0 <= newy <= BOARDHEIGHT - 1:

other improvements:

looks like you don't need to make the size an instance variable - you only need it in the constructor
what if instead of multiple repetitive if/else blocks inside the choose_locations(), you would create a mapping between action keys and the shift directions, e.g.:

ACTION_KEYS = {
    'w': 'U',
    's': 'D',
    'a': 'R',
    'd': 'L'
}

Then, we can improve the part determining the shift direction:

key = getch.getch()
if key in ACTION_KEYS:
    ship.shift(ACTION_KEYS[key])
    board = ship.show(board)


Simple name-drawer using Python and Tkinter I am a beginner playing around with Python and Tkinter. I wrote this program to draw random names given the students in my classes. I tried to follow PEP 8, but I was wondering if there are any stylistic problems here and also if there is any way to condense the code without affecting the working of the program (as I've finally gotten it to work like I want it to.)

from random import *
from Tkinter import *

class App:

    def __init__(self, master):
        self.count_s7=0
        self.count_s8=0
        self.count_m10=0
        self.m=[]
        self.frame = Frame(master, height=500, width=500)
        self.frame.grid()
        self.Results = Message(self.frame, text="", width=400, font="Courier, 32")
        self.Results.grid(row=1, column=1)
        self.b1 = Button (self.frame, text = "Get Random Name: Science 7", width=50,
                          command = self.s7_Names, bg="#6ddd58")
        self.b1.grid(row=2, column=1)
        self.b2 = Button (self.frame, text = "Get Random Name: Science 8",width=50,
                       command = self.s8_Names, bg="#75c0d2")
        self.b2.grid(row=3, column=1)
        self.b3 = Button (self.frame, text = "Get Random Name: Math 10",width=50,
                          command = self.m10_Names, bg="#ffb9ec")
        self.b3.grid(row=4, column=1)
        self.Quit = Button (self.frame, text = "Quit", command=self.frame.quit)
        self.Quit.grid (row=5, column=1)

    def get_names(self, roster, i):
        words = roster[i]
        self.Results.config(text=words)

    def draw_names(self, roster, count):
        if count == 0:
            self.mixed = sample(roster, len(roster))
            self.get_names(self.mixed, count)
            count +=1

        elif 0 < count < len(roster):
            self.get_names(roster, count)
            count +=1

        else:
            count = 0
            self.mixed = sample(roster, len(roster))
            self.get_names(self.mixed, self.count)
            self.count_s7 +=1

    def s7_Names(self):
        s7 = ['Name1', 'Name2', 'Name3', 'Name4', 'Name5', 'Name6','Name7',
             'Name 8', 'Name9','Name10', 'Name11','Name12','Naome13']
        self.draw_names(s7, self.count_s7)

    def s8_Names(self):
        s8 = ['Kid1','Kid2','Kid3','Kid4','Kid5','Kid6','Another kid']
        self.draw_names(s8, self.count_s8)

    def m10_Names(self):
        m10 = ['Student1', 'Student2', 'Student3', 'Student4', 'Student5'
              'Student6', 'Student7', 'Student8', 'Student9']
        self.draw_names(m10, self.count_m10)

root=Tk()
root.title("Random Names")
app = App(root)
root.mainloop()
root.destroy()

from random import *
from Tkinter import *

Wildcard imports are against PEP 8.  I realize that from Tkinter import * is the usual form expressed in the documentation, but it is still not recommended by PEP 8.

I see s7, s8, and m10 used in many places.  I know what they are only because I can see what the texts of your buttons are.  Ideally, every line of code should be clear without any context.  I would use names that are more clear.  Also, self.m is not a very descriptive name.  I would suggest that you use a better one, but I see that it isn't used anywhere except its definition.  Therefore, it should be taken out completely.

Your equals signs are a little strange.  I give you two rules from PEP 8:

  Always surround these binary operators with a single space on either side: assignment (=), augmented assignment (+=,-= etc), comparisons (==,<,>,!=,<>,<=,>=,in,not in,is,is not), Booleans (and,or,not).

and

  Don't use spaces around the = sign when used to indicate a keyword argument or a default parameter value.

You seem to have those two rules switched up.  You have, for example, self.count_s7=0, but self.b1 = Button(..., text = ...).  Mind you, I mention = specifically, but you also break the rule with +=.

Your capitalization is a little messed up.  From PEP 8 on Method Names and Instance Variables:

  Use the function naming rules: lowercase with words separated by underscores as necessary to improve readability.

You break that rule with self.Results and self.Quit.

get_names doesn't seem to be a fitting name.  It doesn't return anything; it sets the text of self.Results.  Perhaps set_names would be more appropriate?

I don't like that you define the class names within the methods.  What you could do, actually, is make your __init__ method take a dictionary as an argument with the class names for keys and the student names for values.  Then, for each key-value pair, define a button and a random color and give command as just a single method.  It's fairly hard to explain without code, so I'll give an example:

def __init__(self, master, students):
    ...
    i = 2
    for class_name, student_names in students.iteritems():
        students[class_name] = [student_names, 0]
        button = Button(self.frame,
            text='Get Random Name: {}'.format(class_name), width=50,
            command=lambda name=class_name: self.draw_from_class(name),
            bg='#' + hex(randint(16777215))[2:] # 16777215 is FFFFFF
        )
        button.grid(row=i, column=1)
        i += 1

def draw_from_class(self, name):
    class_info = self.students[name]
    self.draw_names(*class_info)

You could then do:

app = App(root, {
    'Science 7': ['Name1', 'Name2', ...],
    'Science 8': ['Kid1', 'Kid2', ...],
    'Math 10': ['Student1', 'Student2', ...],
})

That way, it is very easy to add more classes.

It is generally a good idea to use if __name__ == '__main__' for your module-level code if you don't want it to run when this file is imported.


Best way to replace a beginning and end character in Perl using Regular Expression? I'm wondering if there is a simplier regex I could use in my code to remove the beginning and ending char in a line. Maybe combine some regex's? In this instance, it's a comma at the beginning and ending of a line. My output should be the fields seperated into CSV format.

#!/usr/local/bin/perl

use strict;
use warnings;

parse_DPCRS();

sub parse_DPCRS {
    open ( FILEIN, 'txt_files/AKR_DPCRS.txt' );
    open ( FILEOUT, '>txt_files/AKR_DPCRS.csv' );
    while (<FILEIN>) {
        next if /^(\s)*$/;          #skip blank lines
        next if /^\>/;              #skip command line that start with >
        next if /^\s+POINT\sCODE/;  #skip header
        next if /^\s+NODE\sNAME/;   #skip header
        next if /^\s+\=+/;          #skip header
        next if /^\s+CCS\sDPCRS/;   #skip pageination footer
        chomp;                      #removing trailing newline character
        s/\s+/,/g;                  #replace white space with a comma
        s/^,//;                     #replace beginning comma with empty
        s/,$//;                     #replace ending comma with empty
        my (
            $nodeName, $pointCodeDec ) =  split( "," );
        print FILEOUT ($nodeName . "," . $pointCodeDec . "\n");
        #print "$_\n";
    }
};
close (FILEOUT);
close (FILEIN);
exit;

Here's a slice of the text file I'm parsing

>DISP CCS DPCRS ALL 0

                 POINT CODE  POINT CODE  TYPE OF  ROUTESET  NOTIFY   NODE
   NODE NAME      DECIMAL        HEX      ROUTE    MASTER    SCCP  LOCATION
   ===========  ===========  ==========  =======  ========  ====== ========
   PBVJPRCO01T  1-1-1          010101    FULL PC      119     NO    NON-ADJ
   ROCHNYXA06T  1-6-1          010601    FULL PC       58     NO    NON-ADJ
   NYCNNYDRW17  1-6-2          010602    FULL PC       58     NO    NON-ADJ
   SYRCNYSW01T  1-6-3          010603    FULL PC       22     NO    NON-ADJ
   SYRCNYSWDS0  1-6-15         01060F    FULL PC       58     NO    NON-ADJ
   ROCHNYFEDS0  1-6-17         010611    FULL PC       58     NO    NON-ADJ
   NYCMNYHD01T  1-9-11         01090B    FULL PC       22     NO    NON-ADJ
   NWRKNJ1001T  1-9-14         01090E    FULL PC       22     NO    NON-ADJ
   BSTNMABL01T  1-9-16         010910    FULL PC       22     NO    NON-ADJ

Here is a single regex that removes , (comma) at the beginig or at the end of a string:

$str =~ s/^,+|,+$//g;

and here is a benchmark that compares this regex with a double one:

use Benchmark qw(:all);

my $str = q/,a,b,c,d,/;
my $count = -3;
cmpthese($count, {
        'two regex' => sub {
            $str =~ s/^,+//;
            $str =~ s/,+$//;
        },
        'one regex' => sub {
            $str =~ s/^,+|,+$//g;
        },
    });

The result:

               Rate one regex two regex
one regex  597559/s        --      -58%
two regex 1410348/s      136%        --

We can see that two regex are really faster than one regex that combines the two.


Tic-Tac-Toe in Python3 I am new to Python and would like to get helpful tips and suggestions on this code. Would be awesome if you provide info if anything is fundamentally incorrect, improve the algorithm, and any room for further improvements.

class Game:
  def __init__(self):
    self.board = ["0", "1", "2", "3", "4", "5", "6", "7", "8"]
    self.ai = "X"
    self.human = "O"

  def start_game(self):
    print(" %s | %s | %s \n=========\n %s | %s | %s \n=========\n %s | %s | %s \n" % \
        (self.board[0], self.board[1], self.board[2],
             self.board[3], self.board[4], self.board[5],
             self.board[6], self.board[7], self.board[8]))
    print("Enter number between [0-8]:")
    while not self.game_over(self.board) and not self.draw(self.board):
      self.get_human_position()
      if not self.game_over(self.board) and not self.draw(self.board):
        self.eval_board()

      print(" %s | %s | %s \n=========\n %s | %s | %s \n=========\n %s | %s | %s \n" % \
          (self.board[0], self.board[1], self.board[2],
               self.board[3], self.board[4], self.board[5],
               self.board[6], self.board[7], self.board[8]))

    print("Game over") # have to think of way to print Draw when it's a draw

  def get_human_position(self):
    spot = None
    while spot is None:
      spot = int(input())
      if self.board[spot] != "X" and self.board[spot] != "O":
        self.board[spot] = self.human
      else:
        spot = None

  def eval_board(self):
    spot = None
    while spot is None:
      if self.board[4] == "4":
        spot = 4
        self.board[spot] = self.ai
      else:
        spot = self.get_optimal_move(self.board, self.ai)
        if self.board[spot] != "X" and self.board[spot] != "O":
          self.board[spot] = self.ai
        else:
          spot = None

  def get_optimal_move(self, board, next_player, depth = 0, highest_score = {}):
    available_spaces = [s for s in board if s != "X" and s != "O"]
    optimal_move = None

    for available in available_spaces:
      board[int(available)] = self.ai
      if self.game_over(board):
        optimal_move = int(available)
        board[int(available)] = available
        return optimal_move
      else:
        board[int(available)] = self.human
        if self.game_over(board):
          optimal_move = int(available)
          board[int(available)] = available
          return optimal_move
        else:
          board[int(available)] = available

    if optimal_move:
      return optimal_move
    else:
      return int(available_spaces[0])

  def three_in_row(self, *args):
    return args[0] == args[1] == args[2] == "X" or \
        args[0] == args[1] == args[2] == "O"

  def game_over(self, b):
    return self.three_in_row(b[0], b[1], b[2]) == 1 or \
        self.three_in_row(b[3], b[4], b[5]) == 1 or \
        self.three_in_row(b[6], b[7], b[8]) == 1 or \
        self.three_in_row(b[0], b[3], b[6]) == 1 or \
        self.three_in_row(b[1], b[4], b[7]) == 1 or \
        self.three_in_row(b[2], b[5], b[8]) == 1 or \
        self.three_in_row(b[0], b[4], b[8]) == 1 or \
        self.three_in_row(b[2], b[4], b[6]) == 1

  def draw(self, b):
    return len([s for s in b if s == "X" or s == "O"]) == 9

if __name__ == '__main__':
  game = Game()
  game.start_game()

Currently I do want to fix the Draw portion where I would print Draw instead of game-over when it is a draw.

Design

I'm guessing you're coming from some sort of Object-Oriented Programming Language (OOPL), like Java. Python is an interpreted language, and while treating it like an OOPL isn't bad in many large scale scenarios, something small like this doesn't need that design. If this is for some kind of scholastic exercise or you're just training OOPL design in Python, you can stick to it, but personally I'd avoid it here.

Printing & Strings

Pasting out each array index isn't necessary. Instead of using the modulo operator to format strings, you can either use .format() or f-strings. For the board prompt I'd personally use .format(), like so:

print(' {} | {} | {} \n===+===+===\n {} | {} | {} \n===+===+===\n {} | {} | {} \n'.format(*list(n for n in self.board)))

You use variadic assignment in three_in_row, so I'm assuming you know the four use cases of the asterisk.

I notice you use double quotes a lot in your main code but not in the actual main function, so maybe that was pregenerated by an editor like VSCode? Regardless, you can use single quotes more throughout your code.

Game & Logic

None checks and usage seem to be quite frequent throughout this program. Again, this indicates to me some beginner OOP ideology. We all start somewhere, but as it is in good OOP design to not explicitly use null where possible, the same can apply to None here.

In your eval_board function, you prioritize the fourth, or central, spot for the AI.  Why not just check this outside of the loop, and then start it if it fails? This function appears to be used exclusively by the AI as well, so why isn't it named something like ai_move or ai_eval to reflect this? Other functions could benefit from this naming as well. Also, using while loops and checking for a None spot seems like a bad substitution for Boolean functions and checks in my mind.

Conveniently, I've also put up a Tic Tac Toe question, which is here: Back to Basics - Tic Tac Toe

Any comments I'd have on optimizing your game logic would just be incorporating some of the techniques used in my implementation. Granted, I don't have an "AI," but the imagination shouldn't need to flex over the available possibilities.