Genetic algorithm fitness function for scheduling For Job shop scheduling, I built a fitness function:

def makespan_timeCalculation(lot_size, RHS, setup_time, processing_time, sequence, mc_op):
    machine_assigne_time =[[0 for i in range(mc_op[k])] for k in range(len(mc_op))]
    max_c = [0 for i in range(len(lot_size))]  # Equal number of job
    completion_time = [0 for i in range(len(RHS))]

    # Check all indices of RHS (Step 2 + 3 + 6)
    for r in range(len(RHS)):
        procTime = run_time(r, lot_size, RHS, sequence, processing_time) + setup_time[RHS[r][2]]

        if lot_size[RHS[r][0]][RHS[r][1]] > 0:                                    # Lot size not zero (Step 4)
            if (machine_assigne_time[RHS[r][2]][RHS[r][3]] == 0 and RHS[r][2] == sequence[RHS[r][0]][0]):
                # Step 5.1: First assignment to machine m and operation equal first operation in sequence

                completion_time[r] = setup_time[RHS[r][2]] + procTime
                machine_assigne_time[RHS[r][2]][RHS[r][3]] +=1

            elif (machine_assigne_time[RHS[r][2]][RHS[r][3]] == 0 and RHS[r][2] != sequence[RHS[r][0]][0]):
                # Step 5.2: First assignment to machine m and operation over first operation in sequence

                curent_sequence_index = sequence[RHS[r][0]].index(RHS[r][2])                              # Calculate index sequence of current chromosome
                prev_sequence = sequence[RHS[r][0]][curent_sequence_index-1]                              # Return previous sequence operation of current chromosome
                prev_operation_comp_index = [i[0:3] for i in RHS].index([RHS[r][0], RHS[r][1], prev_sequence])      # Return previous chromosome of current job + sublot

                completion_time[r] = max(setup_time[RHS[r][2]], completion_time[prev_operation_comp_index]) + procTime
                machine_assigne_time[RHS[r][2]][RHS[r][3]] +=1
                #print(curent_sequence_index, RHS[r])
            elif (machine_assigne_time[RHS[r][2]][RHS[r][3]] > 0 and RHS[r][2] == sequence[RHS[r][0]][0]):
                # Step 5.3: First operation in sequence and machine m assignment > 1 (dung de xem xet khi co su khac nhau giua setup time ban dau, va setup time doi part)
                prev_op_mc_RHS_index = max([i for i, al in enumerate(RHS[:r]) if RHS[r][2:] == al[2:]])
                completion_time[r] = completion_time[prev_op_mc_RHS_index] + procTime
                machine_assigne_time[RHS[r][2]][RHS[r][3]] +=1

            elif (machine_assigne_time[RHS[r][2]][RHS[r][3]] > 0 and RHS[r][2] != sequence[RHS[r][0]][0]):
                # Step 5.4 Operation over first operation in sequence and machine m assignment > 1

                curent_sequence_index = sequence[RHS[r][0]].index(RHS[r][2])                              # Calculate index sequence of current chromosome
                prev_sequence = sequence[RHS[r][0]][curent_sequence_index-1]                              # Return previous sequence operation of current chromosome
                prev_operation_comp_index = [i[0:3] for i in RHS].index([RHS[r][0], RHS[r][1], prev_sequence])      # Return previous chromosome of current job + sublot
                prev_op_mc_RHS_index = max([i for i, al in enumerate(RHS[:r]) if RHS[r][2:] == al[2:]])
                completion_time[r] = max(completion_time[prev_operation_comp_index], completion_time[prev_op_mc_RHS_index]) + procTime
                machine_assigne_time[RHS[r][2]][RHS[r][3]] +=1

        max_c[RHS[r][0]] = max(max_c[RHS[r][0]], completion_time[r])

    return max(max_c)

The result is fine but speed is very slow. Could you help to have a look and advise on improvement?

I think the biggest boost to readability will come from giving things proper names. You currently have a lot of RHS[r][i] going on, with i = 0, 1, 2, 3. For someone who does not know what these things are (including possibly you in a few months), this makes the code basically unreadable.

I would start by making the iteration look like this:

for r, (a, b, c, d) in enumerate(RHS):
    ...

Where a, b, c, d are actual sensible names (like name, length, or whatever is actually at those positions).

Next, your setup code can be simplified using the fact that e.g. [0] * 3 == [0, 0, 0] (but don't do it with nested loops, see below):

machine_assigne_time = [[0] * op] for op in mc_op]
max_c = [0] * len(lot_size)  # Equal number of job
completion_time = [0] * len(RHS)

And finally, without being able to run your code, the most obvious case for the slow down are your repeated calls to list.index. This is very slow (\$\mathcal{O}(n)\$ in the worst case). I would try to find a more efficient data structure for this (dictionaries?).


Creating an ArrayList of Maps I am making an ArrayList that contains Map values. itemData is my ArrayList that contains itemselected, which is a map. Then, because I want to send this array to my PHP code, I am converting itemData into a JSONArray. Then parsing the array in my PHP script and inserting those individual values into a MySQL database.

private ArrayList<Map<String, String>> itemData = new ArrayList<>();
private Map<String, String> itemselected = new HashMap<>();
JSONArray itemSelectedJson = new JSONArray();

 private void selectedItems() {
        if(invEstSwitch.isChecked())
        {
            billType = textViewEstimate.getText().toString();
        }else{
            billType = textViewInvoice.getText().toString();
        }

        itemselected.put("custInfo",custSelected.toString());
        itemselected.put("invoiceNo", textViewInvNo.getText().toString());
        itemselected.put("barcode", barCode.getText().toString());
        itemselected.put("desc", itemDesc.getText().toString());
        itemselected.put("weight", weightLine.getText().toString());
        itemselected.put("rate", rateAmount.getText().toString());
        itemselected.put("makingAmt", makingAmount.getText().toString());
        itemselected.put("net_rate", netRate.getText().toString());
        itemselected.put("itemTotal", itemtotal.getText().toString());
        itemselected.put("vat", textViewVat.getText().toString());
        itemselected.put("sum_total", textViewSum.getText().toString());
        itemselected.put("bill_type", billType);
        itemselected.put("date", textViewCurrentDate.getText().toString());

        //Add the map to the Array
        itemData.add(index, itemselected);
        itemSelectedJson= new JSONArray(Arrays.asList(itemData));
        index++;
    }

The code is pretty straightforward, but I am interested in any possible improvement. I am sure there is a better way to do it.

Just some convention points:

The indentation is off in the first couple of lines. Is this due to copy-paste? I have to assume so, as the rest of your code seems fine.
The following if statement:

if(invEstSwitch.isChecked())
{
    billType = textViewEstimate.getText().toString();
}else{
    billType = textViewInvoice.getText().toString();
}

is inconsistent. You put a newline before the beginning brace at the if, and not at the else.

It is also bad formatting. There should be a space both left and right of the else, and a space after if:

if (invEstSwitch.isChecked()) {
    billType = textViewEstimate.getText().toString();
} else {
    billType = textViewInvoice.getText().toString();
}

The if statement can be simplified into a ternary expression, as it is assigning the same thing (assuming textViewEstimate and textViewInvoice are the same type):

billType = (invEstSwitch.isChecked() ? textViewEstimate : textViewInvoice)
        .getText().toString();

Try not to use ArrayList. As I said in this answer:

  It is almost always a poor option, as it is pretty much an array with
  helper functions. If you have an ArrayList of 10000 integers, and
  then add another integer, ArrayList will (if its current array it's
  storing the values in is of size 10000) create a new array, move all
  the values of the old array in the new array, then add the integer to
  the end. Sounds inefficient? Certainly does.

  My opinion is to use LinkedList. LinkedList is faster, because it
  works like this:

  Each value in the list is stored in a Node.
  Each Node points to the next Node.
  Adding something to the end is as simple as creating a new Node and linking it to the Node chain.
  Removing and inserting is as simple as changing some links around. The problem would be to find that link.

  Worst-case time complexity comparison:

        LinkedList        ArrayList
Get:       O(n)              O(1)
Add:       O(n)              O(n)
Insert:    O(n)              O(n)

  Best-case time complexity comparison:

        LinkedList        ArrayList
Get:       O(1)              O(1)
Add:       O(1)              O(1)
Insert:    O(1)              O(n) <- assuming not inserting at end of list

  Yes, LinkedList is losing, but in this case, you don't use get
  often. On the other hand, memory is an issue for ArrayList:

        LinkedList        ArrayList
Get:       O(1)              O(1)
Add:       O(1)              O(n)
Insert:    O(1)              O(n)

Your method is fairly inefficient. You create a new JSONArray each time you call the method. Instead, use the given put() method (this also means you don't need an ArrayList):

private Map<String, String> itemselected = new HashMap<>();
JSONArray itemSelectedJson = new JSONArray();

private void selectedItems() {
    billType = (invEstSwitch.isChecked() ? textViewEstimate : textViewInvoice)
            .getText().toString();

    itemselected.put("custInfo",custSelected.toString());
    itemselected.put("invoiceNo", textViewInvNo.getText().toString());
    itemselected.put("barcode", barCode.getText().toString());
    itemselected.put("desc", itemDesc.getText().toString());
    itemselected.put("weight", weightLine.getText().toString());
    itemselected.put("rate", rateAmount.getText().toString());
    itemselected.put("makingAmt", makingAmount.getText().toString());
    itemselected.put("net_rate", netRate.getText().toString());
    itemselected.put("itemTotal", itemtotal.getText().toString());
    itemselected.put("vat", textViewVat.getText().toString());
    itemselected.put("sum_total", textViewSum.getText().toString());
    itemselected.put("bill_type", billType);
    itemselected.put("date", textViewCurrentDate.getText().toString());

    // Add the map to the Array
    itemSelectedJson.put(itemselected);
    index++;
}


Passing file identifiers to all subroutines of fortran code The Fortran program 'MAIN' below calls 3 subroutines.

It opens a file if it is not present, (overwrites if it is already present).
writes something on the file using the file identifier integer and
closes the file.

       program MAIN

2001 FORMAT (A,I10)
   INTEGER FILEIDS(10)
   CHARACTER*64 fname
   FILEIDS(1) = 701
   write(*,2001) 'opnoutfl', FILEIDS(1)

   fname = 'advect1.dat'

   CALL OFNFL (FILEIDS(1), fname)

   CALL BDEXCH (FILEIDS(1))
   CALL CLOOUTFL(FILEIDS(1))
   END

   SUBROUTINE OFNFL (UNITNO, fname)

   LOGICAL EXISTS, OPND

   INTEGER UNITNO
   CHARACTER*64 fname,dirname,path

2001 FORMAT (A,I10)
     write(*,2001) 'opnoutfl', UNITNO
    dirname = 'sediment/'
    path  = trim(dirname) // trim(fname)

   INQUIRE (file=path, EXIST=EXISTS)

  IF (EXISTS) THEN
     OPEN (UNITNO, file=path, status='old')
     close(UNITNO, STATUS='DELETE')
     OPEN (UNITNO, file=path, status='NEW')
     WRITE(UNITNO,2001) 'Alte Dateil ist geloest  ',UNITNO
  ELSE
     OPEN (UNITNO, file=path, status='NEW')
     WRITE(UNITNO, 2001) 'Neue Datei ist geoeffnet',UNITNO
  END IF

    RETURN
    END

  SUBROUTINE CLOOUTFL (UNITNO)

  INTEGER UNITNO

  CLOSE(UNITNO)
  RETURN
  END

   SUBROUTINE BDEXCH (MESSU)

   INTEGER MESSU

2001 FORMAT (A,I10)

   WRITE(MESSU, 2001) 'bdexch schreibt', MESSU+10

    RETURN
    END

My question is, at this moment I am passing file identifier as argument. How can I modify/improve this code so that the file identifier created in first subroutine i.e. OFNFL() be automatically seen to subsequent subroutines which uses/writes on the file? The program where I want to use it creates lots of files and then different subroutines writes on different files, so passing file identifier like this will not look very clean!!

If you want to share access to one or more files that are known beforehand, a possible way is to make each file into a singleton and to pack all of them in a  module. I illustrate the principle below, after rewriting your code in modern fortran. The example incorporates many feature that should be considered for a production code. Of course, the implementation should be adapted to your real code.
module mod_output
  use, intrinsic :: iso_fortran_env, only : ERROR_UNIT
  implicit none
  private

  character(len=*), parameter :: MSG_NEW = 'Neue Datei ist geoeffnet'
  character(len=*), parameter :: MSG_DEL = 'Alte Dateil ist geloest '

  integer, public , parameter :: DIR_ID  = 1
  integer, public , parameter :: FILE_ID = 2

  !.. Singleton
  type, private :: class_output
     character(len=:), allocatable :: Directory
     character(len=:), allocatable :: FileName
     integer                       :: unitID      = 0
     logical                       :: opened      =.FALSE.
   contains
     procedure, public :: set     => class_output_set
     procedure, public :: getUnit => class_output_getUnit
     procedure, public :: open    => class_output_open
     procedure, public :: isSet   => class_output_isSet
     procedure, public :: close   => class_output_close
  end type class_output
  !*** some examples:
  type(class_output), public :: output_1
  type(class_output), public :: output_2
  integer, parameter, public :: SIZE_OUTPUT_LIST=10
  type(class_output), public :: outputList(SIZE_OUTPUT_LIST)

contains

  subroutine class_output_set(self,fieldID,strn)
    implicit none
    class(class_output), intent(inout) :: self
    integer            , intent(in)    :: fieldID
    character(len=*)   , intent(in)    :: strn
    select case ( fieldID )
    case ( DIR_ID  )
       self%Directory = trim(adjustl(strn))
    case ( FILE_ID )
       self%FileName  = trim(adjustl(strn))
    end select
  end subroutine class_output_set

  logical function class_output_isSet(self) result( isSet )
    implicit none
    class(class_output), intent(in) :: self
    isSet = allocated( self%Directory ) .and. allocated( self%FileName )
  end function class_output_isSet

  subroutine class_output_open(self)
    implicit none
    class(class_output), intent(inout) :: self
    character(len=:), allocatable :: path, msg
    character(len=100)            :: iomsg
    logical                       :: exists
    integer                       :: uid, iostat
    if( .not. self%IsSet() )then
       write(ERROR_UNIT) " output file not set"
       error stop
    end if
    call execute_command_line("mkdir -p "//trim(self%Directory))
    path = trim(self%Directory) // trim(self%FileName)
    inquire( file = path, exist = exists )
    if(exists)then
       msg = MSG_DEL
    else
       msg = MSG_NEW
    endif
    open(newunit = uid   , &
         file    = path  , &
         action  ="write", &
         iostat  = iostat, &
         iomsg   = iomsg )
    if( iostat /= 0 )then
       write(ERROR_UNIT,"(a)") iomsg
       error stop
    endif
    write(uid,"(a,x,i0)") msg, uid
    self%unitID = uid
    self%opened = .TRUE.
  end subroutine class_output_open

  subroutine class_output_close(self)
    implicit none
    class(class_output), intent(inout) :: self
    character(len=100) :: iomsg
    integer            :: iostat
    close(self%unitID  , &
         iostat= iostat, &
         iomsg = iomsg )
    if( iostat /= 0 )then
       write(ERROR_UNIT,"(a)") iomsg
       error stop
    endif
  end subroutine class_output_close

  integer function class_output_getUnit(self) result( uid )
    implicit none
    class(class_output), intent(inout) :: self
    if(.not.self%Opened)then
       write(ERROR_UNIT,"(a)") "Attempt to access unit of closed file"
       error stop
    endif
    uid = self%unitID
  end function class_output_getUnit

end module mod_output

Then your main program would look like this
program main
  !.. the other outputs can be initialized and used as well.
  !   If the different files have constant directory and filename
  !   they may be specified in mod_output and initialized collectively
  !   by an init routine at the beginning of the main program
  use mod_output
  implicit none
  integer, parameter :: int_vector(3) = [1,2,3]
  call output_1%Set(  DIR_ID, "sediment/"   )
  call output_1%Set( FILE_ID, "advect1.dat" )
  call output_1%Open()
  CALL bdexch( int_vector, size(int_vector) )
  call output_1%Close()
end program main

subroutine bdexch( ivec, n )
  use mod_output, only : output_1
  implicit none
  integer, intent(in) :: ivec(n), n
  integer :: uid
  uid = output_1%getUnit()
  write(uid,"(a,*(x,i0))") 'bdexch schreibt', ivec + 10
end subroutine bdexch

For an introduction to the simple syntax of modern Fortran, I suggest the following quick modern Fortran tutorial.


JavaScript function that flips brackets direction I have these strings:
)hello(
this has ]some text[
flip }any{ brackets
even with )))]multiple[((( brackets

As you can see, the brackets are all in the wrong direction.
I have a function called flipBracketsDirection() to handle each scenario. Here is an example of input and output of the above strings:
flipBracketsDirection(')hello(');
// should return:  (hello)

flipBracketsDirection('this has ]some text[');
// should return:  this has [some text]

flipBracketsDirection('flip }any{ brackets');
// should return:  flip {any} brackets

flipBracketsDirection('even with )))]multiple[((( brackets');
// should return:  even with ((([multiple]))) brackets

Note: The direction is flipped at all times. So this is fine too:
flipBracketsDirection('flip (it) anyway');
// should return:  flip )it( anyway

Here is my solution.
function flipBracketsDirection(str: string) {
  return str
    // flip () brackets
    .replace(/\(/g, 'tempBracket').replace(/\)/g, '(').replace(/tempBracket/g, ')')

    // flip [] brackets
    .replace(/\[/g, 'tempBracket').replace(/\]/g, '[').replace(/tempBracket/g, ']')

    // flip {} brackets
    .replace(/\{/g, 'tempBracket').replace(/\}/g, '{').replace(/tempBracket/g, '}')
    ;
}

Is this the best way to create this function?

Your function seems to work fine, but you can condense it a bit by search for all bracket types in one regex replace with the following pattern:
/[\(\)\[\]\{\}]/g

and then use the replace function that takes a replace function as argument:
const brs = "()({}{[][";
function flipBracketsDirection(str) {
  return str.replace(/[\(\)\[\]\{\}]/g, br => brs[brs.indexOf(br) + 1]);
}

brs holds all the replaceable brackets with the opening brackets twice, so brs[brs.indexOf(')') + 1] finds '(' as the next char in brs;
You could also let brs be an object like:
const brs =
{
  "(": ')',
  ")": '(',
  "{": '}',
  "}": '{',
  "[": ']',
  "]": '[',
};

and then cquery it as:
function flipBracketsDirection(str) {
  return str.replace(/[\(\)\[\]\{\}]/g, br => brs[br]);
}

Your version actually query the string nine times, where the above only iterates it once.
As an alternative to the 'sophisticated' brs dictionary-solution, you could just create a function with a switch statement:
function swapBracket(br) {
  switch (br) {
    case '(': return ')';
    case ')': return '(';
    case '{': return '}';
    case '}': return '{';
    case '[': return ']';
    case ']': return '[';
  }
}

And call that instead, this way:
function flipBracketsDirection(str: string) {
  return str.replace(/[\(\)\[\]\{\}]/g, swapBracket);
}

flipBracketsDirection('}hello{');
// will return {hello}

Your tests cases could also be simplified, so it is easier to maintain - for instance like:
  let strs = [
    ')hello(',
    'this has ]some text[',
    'flip }any{ brackets',
    'even with )))]multiple[((( brackets'
  ];

  for (let s of strs) {
    let t = flipBracketsDirection(s);
    console.log(t);
    console.log(flipBracketsDirection(t));
    console.log("");
  }

Both the above suggestions should conform to the DRY principle.


Python Pandas Apply with a Lambda Function I have a table in pandas that has two columns, QuarterHourDimID and StartDateDimID ; these columns give me an ID for each date / quarter hour pairing. For instance for January 1st, 2015, at 12:15PM the StartDateDimID would equal 1097 and QuarterHourDimID would equal 26. This is how the data I'm reading is organized.

It's a large table that I'm reading using pyodbc and pandas.read_sql(), ~450M rows and ~60 columns, so performance is an issue.

To parse the QuarterHourDimID and StartDateDimID columns into workable datetime indexes I'm running an apply function on every row to create an additional column datetime.

My code reading the table without the additional parsing is around 800ms; however when I run this apply function it adds around 4s to total run time (anywhere between 5.8-6s a query is expected.) The df that is returned is around ~45K rows and 5 columns (~450days*~100quarter-hour-parts)

I am hoping to more efficiently rewrite what I've written and get any input along the way.

Below is the code I've written thus far:

import pandas as pd
from datetime import datetime, timedelta
import pyodbc

def table(network, demo):
    connection_string = "DRIVER={SQL Server};SERVER=OURSERVER;DATABASE=DB"
    sql = """SELECT [ID],[StartDateDimID],[DemographicGroupDimID],[QuarterHourDimID],[Impression] FROM TABLE_NAME
        WHERE (MarketDimID = 1
        AND RecordTypeDimID = 2
        AND EstimateTypeDimID = 1
        AND DailyOrWeeklyDimID = 1
        AND RecordSequenceCodeDimID = 5
        AND ViewingTypeDimID = 4
        AND NetworkDimID = {}
        AND DemographicGroupDimID = {}
        AND QuarterHourDimID IS NOT NULL)""".format(network, demo)

    with pyodbc.connect(connection_string) as cnxn:
        df = pd.read_sql(sql=sql, con=cnxn, index_col=None)

    def time_map(quarter_hour, date):
        if quarter_hour > 72:
            return date + timedelta(minutes=(quarter_hour % 73)*15)
        return date + timedelta(hours=6, minutes=(quarter_hour-1)*15)

    map_date  = {}

    init_date = datetime(year=2012, month=1, day=1)

    for x in df.StartDateDimID.unique():
        map_date[x] = init_date + timedelta(days=int(x)-1)

    #this is the part of my code that is likely bogging things down
    df['datetime'] = df.apply(lambda row: time_map(int(row['QuarterHourDimID']),
                                                   map_date[row['StartDateDimID']]),
                                                   axis=1)
    if network == 1278:
        df = df.loc[df.groupby('datetime')['Impression'].idxmin()]

    df = df.set_index(['datetime'])

    return df

Style concerns
There is a bunch of constant that you define in the function that doesn't really need to. No need do (re)define your credentials for the DB at each call, for instance, nor the init_date. You should extract these out as the constant they are.
Same thing for time_map, there is nothing in it that make it mandatory to be defined inside table, so move it out too.
You should also try to reduce too long lines and come up with better names: table doesn't convey much.
Prepared SQL
When dealing with SQL, it is often recommended to not build your query string yourself and let the manager do it for you. pandas let you do it using the params parameter of pd.read_sql. You’ll need to adapt the query a bit to change '{}' into ?.
Pandas efficiency
When dealing with pandas, it is often more efficient to perform operations on a whole column at once; and more often than not, going back and forth between the pure Python world and the pandas one will lead to the kind of performances you’re having.
pandas has its own kind of objects to deal with time. Namely pd.Timestamp and pd.Timedelta. Converting your columns to these objects instead of datetime.datetime or datetime.timedelta should help you speed up your computation. pd.to_timedelta can be quite handy for that.
You should also try to reduce the amount of extra computation. Even the tiniest ones will add up. I’m talking about your offset management, why start at 2012-01-01 and add x - 1 days? You can perform '2011-12-31' + x days instead. Same for the minutes: instead of starting at 6 o'clock and adding x - 1 × 15 minutes, why not start at 5:45?
Unfortunately, you’re dealing with strings to convert to timedeltas. You can take care of the conversion using df['QuarterHourDimID'].map(int), for instance; but it would be so much faster if you could extract integers directly out of your database.
Proposed improvements
import pandas as pd
import pyodbc

DB_CREDENTIALS = "DRIVER={SQL Server};SERVER=OURSERVER;DATABASE=DB"
SQL_QUERY = """
SELECT [ID],[StartDateDimID],[DemographicGroupDimID],[QuarterHourDimID],[Impression]
FROM TABLE_NAME
WHERE (MarketDimID = 1
    AND RecordTypeDimID = 2
    AND EstimateTypeDimID = 1
    AND DailyOrWeeklyDimID = 1
    AND RecordSequenceCodeDimID = 5
    AND ViewingTypeDimID = 4
    AND NetworkDimID = ?
    AND DemographicGroupDimID = ?
    AND QuarterHourDimID IS NOT NULL
)"""

INIT_TIMESTAMP = pd.Timestamp('2011-12-31')
QUARTER_TO_SIX = pd.Timedelta(5.75, unit='h')
DAY = pd.Timedelta(1, unit='D')

def table(network, demo):
    with pyodbc.connect(DB_CREDENTIALS) as cnxn:
        df = pd.read_sql(
            sql=SQL_QUERY,
            con=cnxn,
            params=(network, demo),
            index_col=None
        )

    quarters = pd.to_timedelta(df['QuarterHourDimID'].map(int), unit='m') * 15 + QUARTER_TO_SIX
    # This is what I understood of `time_map`, if this isn't quite right, adapt accordingly
    quarters[quarters>=DAY] -= DAY

    df['datetime'] = pd.to_timedelta(df['StartDateDimID'].map(int), unit='D') + quarter_offsets + INIT_TIMESTAMP

    if network == 1278:
        df = df.loc[df.groupby('datetime')['Impression'].idxmin()]

    df = df.set_index(['datetime'])

    return df


Fetching API with requests.Session I wrote a script that parses API on schedule (Tuesday-Saturday), downloading everything for the previous day.

import requests
import pandas as pd
from datetime import date, timedelta

# # This is what I'd normally use, but since there would be no data today,
# # I assign specific date myself
# DATE = (date.today() - timedelta(days=1)).strftime("%Y-%m-%d")
DATE = "2020-10-23"
URL = "https://spending.gov.ua/portal-api/v2/api/transactions/page/"

def fetch(session, params):
    next_page, last_page = 0, 0
    while next_page <= last_page:
        params["page"] = next_page
        data = session.get(URL, params=params).json()
        yield pd.json_normalize(data.get("transactions"))\
                .assign(page=params.get("page"))
        next_page, last_page = next_page+1, data["count"] // data["pageSize"]

def fetch_all():
    with requests.Session() as session:
        params = {"page": 0, "pageSize": 100, "startdate": DATE, "enddate": DATE}
        yield from fetch(session, params)

if __name__ == "__main__":
    data = fetch_all()
    pd.concat(data).to_csv(f"data/{DATE}.csv", index=False)

Here I’m wondering about a couple of things.
Firstly, if I’m using requests.Session correctly.
I read in the documentation that:

The Session object allows you to persist certain parameters across requests. ... So if you’re making several requests to the same host, the underlying TCP connection will be reused, which can result in a significant performance increase.

I'm not sure whether that's the case here as I didn't notice any changes in the performance.
Secondly, if splitting code into two functions instead of one was a good idea.
Here I thought that it would be easier to maintain -- the underlying function fetch doesn't change while fetch_all potentially could. For example, I could feed a range of dates instead of a singe date, changing fetch_all to:
def fetch_all(date_range):
    with requests.Session() as session:
        for date in date_range:
            params = {"page": 0, "pageSize": 100, "startdate": date, "enddate": date}
            yield from fetch(session, params)

Also, the yield and yield from -- could've used .append and returned a list instead. Not sure which approach is better.

Here I’m wondering about a couple of things.
Firstly, if I’m using requests.Session correctly.

Yes, you are. In one of my other reviews, using requests.Session in the same way for iterating over a paginated API almost halved the total execution time.
I did some quick testing by downloading the last 7 pages (pages 1625-1631) for "2020-10-23" and it did marginally better than making requests with requests.get:

requests.get: 23.2 seconds
requests.Session: 17.7 seconds

Secondly, if splitting code into two functions instead of one was a good idea.

I think it's fine to have it split into two functions. That said, I do have some comments about the responsibilities and interface of fetch and how to better take advantage of your usages of yield and yield from down below.

Overall the code looks clean and is easy to read. Here's how I think it can be improved:

I think all the low-level details of how to issue requests to the API should be abstracted away from the caller of fetch. That is, fetch's function signature should look something like this:
def fetch(
    session: requests.Session,
    start_date: date,
    end_date: date,
    starting_page: int = 0,
    page_size: int = 100,
) -> Iterator[pd.DataFrame]:
    pass

So now creating an appropriate params would be fetch's responsibility, not fetch_all's. Notice also that start_date and end_date are of type datetime.date, not str. Similarly, fetch_all should not have to be concerned with what date string serialization format the API accepts; this is fetch's responsibility.

Within fetch, instead of maintaining variables next_page and last_page on each request, I think it would be better to calculate the total number of pages (n) only once with the first request (page k), then use a for loop for pages k+1..n-1:
def to_dataframe(json_data: Dict[str, Any], page: int) -> pd.DataFrame:
    return pd.json_normalize(json_data["transactions"]).assign(page=page)

def fetch(
    session: requests.Session,
    start_date: date,
    end_date: date,
    starting_page: int = 0,
    page_size: int = 100,
) -> Iterator[pd.DataFrame]:
    params = {
        "startdate": start_date.isoformat(),
        "enddate": end_date.isoformat(),
        "page": starting_page,
        "pageSize": page_size,
    }

    data = session.get(URL, params=params).json()
    page_count = math.ceil(data["count"] / data["pageSize"])
    last_page = page_count - 1
    if starting_page > last_page:
        return
    print(f"{starting_page} / {last_page}")
    yield to_dataframe(data, starting_page)

    for page in range(starting_page + 1, page_count):
        params["page"] = page
        data = session.get(URL, params=params).json()
        print(f"{page} / {last_page}")
        yield to_dataframe(data, page)

The tradeoff here is that there's a small duplication of code because the first request is handled a little differently, but now we've delegated responsibility of page number iteration to the for loop.

I recommend adding an event hook to the session object so that it always calls raise_for_status() on the response object. This ensures that all requests made with the session raise requests.HTTPError if the server gives us a 4xx or 5xx response, and prevents us from converting an error response's .json() data into a dataframe:
session.hooks["response"].append(
    lambda r, *args, **kwargs: r.raise_for_status()
)

Currently the program is combining all dataframes in memory before exporting it to a CSV file. To take advantage of fetch_all being an Iterator[pd.DataFrame], I think it would be better to write each dataframe to the CSV immediately, so we don't need to hold it in memory any longer than necessary:
output_path = Path(f"data/{DATE}.csv")
output_path.unlink(missing_ok=True)
data = fetch_all()
for i, dataframe in enumerate(data):
    write_header = True if i == 0 else False
    dataframe.to_csv(
        output_path, header=write_header, index=False, mode="a"
    )

Refactored version:
#!/usr/bin/env python3

import math
from datetime import date, timedelta
from pathlib import Path
from typing import Any, Dict, Iterator

import pandas as pd  # type: ignore
import requests

# # This is what I'd normally use, but since there would be no data today,
# # I assign specific date myself
# DATE = date.today() - timedelta(days=1)
DATE = date.fromisoformat("2020-10-23")
URL = "https://spending.gov.ua/portal-api/v2/api/transactions/page/"

def to_dataframe(json_data: Dict[str, Any], page: int) -> pd.DataFrame:
    return pd.json_normalize(json_data["transactions"]).assign(page=page)

def fetch(
    session: requests.Session,
    start_date: date,
    end_date: date,
    starting_page: int = 0,
    page_size: int = 100,
) -> Iterator[pd.DataFrame]:
    params = {
        "startdate": start_date.isoformat(),
        "enddate": end_date.isoformat(),
        "page": starting_page,
        "pageSize": page_size,
    }

    data = session.get(URL, params=params).json()
    page_count = math.ceil(data["count"] / data["pageSize"])
    last_page = page_count - 1
    if starting_page > last_page:
        return
    print(f"{starting_page} / {last_page}")
    yield to_dataframe(data, starting_page)

    for page in range(starting_page + 1, page_count):
        params["page"] = page
        data = session.get(URL, params=params).json()
        print(f"{page} / {last_page}")
        yield to_dataframe(data, page)

def fetch_all() -> Iterator[pd.DataFrame]:
    with requests.Session() as session:
        session.hooks["response"].append(
            lambda r, *args, **kwargs: r.raise_for_status()
        )
        yield from fetch(session, start_date=DATE, end_date=DATE)

if __name__ == "__main__":
    output_path = Path(f"data/{DATE}.csv")
    output_path.unlink(missing_ok=True)
    data = fetch_all()
    for i, dataframe in enumerate(data):
        write_header = True if i == 0 else False
        dataframe.to_csv(
            output_path, header=write_header, index=False, mode="a"
        )


Remove item from a list, which (of the 2) methods is better? I have a List, and I have to delete one item from the list when I find it. I am using the code below to do that:

Iterator<MyClass> iter = flist.iterator();
while (iter.hasNext()) {
    Long deletedFiId = iter.next().getId();
    if (deletedFiId.equals(fiId)) {
         iter.remove();
         break;
     }
}

One of my co-workers suggested I should hold the object to be removed and remove it once we get out of the for loop, like this:

MyClass deletedFiId = null;
for (MyClass dto: flist) {
  if (dto.getId().equals(fiId)) {
    deletedFiId = dto;
    break;
  }
if (deletedFiId != null)
  fiList.remove(deletedFiId);

Which one of the above is a good-to-have in the code?

The two methods are functionally identical.
The first method is faster to execute.
The first method is clearer for someone who doesn't know the code.

Unless there's an "else" you need (that is, logic to execute if the item can't be found), I can't think of a reason to prefer the second approach.

First approach wins.


Improving canvas pre-rendering I define my canvas as you normally would, but I also include a bunch of different makeshift canvases:

var graphicOne = document.createElement("canvas");
var graphicOneDraw = graphicOne.getContext("2d");

// etc... (eg. graphicTwo)

Then I render all of these canvases, like so:

function preRender() {
  graphicOne.width = 24;
  graphicOne.height = 24;

  graphicOneDraw.beginPath();
  graphicOneDraw.fillStyle = '#0000ff';
  graphicOneDraw.arc(12, 12, 12, 0, Math.PI * 2, true);
  graphicOneDraw.fill();

  // etc... (eg. graphicTwo)
}

In the main loop, my game logic is called before I render it on canvas:

ctx.drawImage(graphicOne, player.x - 12, player.y - 12);

// etc... (eg. graphicTwo) and other things that needs to be rendered

The graphics I need to render is actually more CPU/GPU intensive; this is just a bare bones example so you get the gist of it. But over to the question: Is there a better way to go about this?

JIT content rendering

There is no observable difference in performance (after setting up) between using an image or a canvas to render context. As rendering content can be many times faster than downloading content it is a very good method of increasing performance at startup.

Some warnings.

Clean up state

Using the 2D context requires that its state be maintained. Be sure to clean the state when you no longer have a need as the state can occupy a lot of memory.

eg

ctx.filter = url("#element-id"); // can consume lots of memory and if animated ongoing CPU/GPU resources
ctx.fillStyle = ctx.createPattern(ctx.canvas,"repeat"); // requires copy of source image
ctx.strokeStyle = ctx.createPattern(image,"repeat"); // requires copy of source image
ctx.font = "128px myFancyFont"; // a loaded font

ctx.beginPath(); // lets create a very long path
for(var i = 0; i < 10000; i ++) { ctx.lineTo(Math.random() * 100, Math.random() * 100) }
ctx.stroke();  // You may be done with the path but its still there
               // ready for another stroke or fill

All the above (and there are more) require considerable memory resources. They also prefer these resources to be readily available to the GPU so if possible will occupy GPU RAM (shadowed from RAM)

Running out of free GPU RAM is a the biggest performance killer for the browser's rendering engines as it begins to swap resources in and out of GPU RAM to complete the render (GPU bus thrashing, or GPU RAM thrashing). Do your best to keep usage as low as possible by always releasing unneeded resources.

I would make some changes

function createAndRenderResource(renderFunc, ...args)
    const img = document.createElement("canvas");
    const ctx = img.getContext("2d");
    img.width = 24;
    img.height = 24;
    ctx.save(); // save default low memory state
    renderFunc(ctx, ...args); // add the content
    ctx.restore(); // cleanup releasing any unneeded RAM or connected processes
    return img; // you can not remove the 2D context once made
                // it remains in memory
}

function render(ctx, color) {
    ctx.beginPath();
    ctx.fillStyle = color;
    ctx.arc(12, 12, 12, 0, Math.PI * 2, true);
    ctx.fill();
}

const images = [
     createAndRenderResource(render,"red"),
     createAndRenderResource(render,"green"),
     createAndRenderResource(render,"blue"),
]

Powers of two

The canvas size can be anything, an image can be anything. But the way GPU hold images is optimized for very fast access. Images are always stored in sizes that are powers of 2 2, 4, 8, 16, 32, 64, 128, 256, 512, 1024, 2048, 4096... etc

To know the memory foot print of any resource within the GPU find the nearest power of two above the width or height.

For example a 48 by 12 (576 pixels) image will occupy, nearest width is 64, and nearest height is 16 64 by 16 (1024 pixels) almost double the amount of Ram needed to hold the image in GPU RAM.

Dead pixels.

The code you have given creates a canvas that is 24 by 24 pixels. The memory footprint on the GPU is thus 32 by 32 pixels. 77% per pixel dead (unused) RAM.

You can make things better by using a single canvas and drawing multiple sprites to that. A canvas 128 by 128 can hold 25 sprites that are 24 by 24 and have room for a one pixel boundary between them to stop pixel bleeding due to linear lookups. This reduces the dead pixel rate to ~14%

Note: the same power of two rule applies to Images.

This will only effect your code if you are getting close to the GPU memory limits imposed by the browser. The more dead pixels the quicker you get to that limit for the same resources.

For large numbers of variable sized sprites use a single canvas near a power of two size I use a sprite lookup as a property, attached to the image / canvas being used, that stores the position and size of each sprite.

// a sprite is {x,y,w,h} the position on the sprite sheet.
// sprites is an array of sprites. and spriteIndex is which to render
function drawSprite(img, spriteIndex, x,y){
    const spr = img.sprites[spriteIndex];
    ctx.drawImage(img, spr.x, spr.y, spr.w, spr.h, x, y, spr.w, spr.h);
}

BTW you break the codeReview site rules by not supplying working code, you have given example of what your working code does. It is incomplete and non functioning.


Sort, slice and rebuild new object with array data I'm using the matomo reports api to pull out some data in js.
For some charts, I'm in the need to sort the array data based on inner property value ("val"), further to get only the 3 highest (Hong Kooong, Munich, Tokyo). Finally, this data should be used for the creation of a new key/val object (newObj).

This is what my code looks like what does what it should. I would be interested if it can be optimized using es6 syntax.

    let data = {
      0 : {"label":"New York", "val": 20},
      1 : {"label":"Rio", "val": 10},
      3 : {"label":"Tokyo", "val": 50},
      4 : {"label":"Santiago", "val": 20},
      5 : {"label":"Hong Kooong", "val": 100},
      6 : {"label":"Munich", "val": 90},
    }

    let compare = (a, b)  => {
      let valA = a.val;
      let valB = b.val;
      let c = 0;

      if (valA > valB) {
        c = 1;
      } else if (valA < valB) {
        c = -1;
      }
      return c * -1;
    }

    let dataArray = Object.values(data).sort(compare)
    dataArray = dataArray.slice(0, 3)

    let newLabel = []
    let newValue = []
    Object.keys(dataArray).map(function(key, index) {
      newLabel.push(dataArray[index]['label'])
      newValue.push(dataArray[index]['val'])
    });

    let newObj = {}
    newObj.data = {}
    newObj.data.labels = newLabel
    newObj.data.values = newValue

    console.log(newObj)

Let's start with how I would have done it.

const data = {
  0 : {"label":"New York", "val": 20},
  1 : {"label":"Rio", "val": 10},
  3 : {"label":"Tokyo", "val": 50},
  4 : {"label":"Santiago", "val": 20},
  5 : {"label":"Hong Kooong", "val": 100},
  6 : {"label":"Munich", "val": 90},
}

const result = Object.values(data)
  .sort((a, b) => b.val - a.val)
  .slice(0, 3)
  .reduce((c, v) => {
    c.data.labels.push(v.label)
    c.data.values.push(v.val)
    return c
  }, { data: { labels: [], values: []}})

console.log(result)

Now onto your code:

let dataArray = Object.values(data).sort(compare)
dataArray = dataArray.slice(0, 3)

You were off to a good start here. One minor nitpick is to just chain the operations since you're just reassigning dataArray with sort's results.

let newLabel = []
let newValue = []
Object.keys(dataArray).map(function(key, index) {
  newLabel.push(dataArray[index]['label'])
  newValue.push(dataArray[index]['val'])
});

let newObj = {}
newObj.data = {}
newObj.data.labels = newLabel
newObj.data.values = newValue

This is where it went crazy.

Firstly, you used Object.keys() on dataArray (an array), which is not needed. If you need the array item index, all of the array methods provide the item index as second argument of the callback.

Also, this is a bad use of array.map(). array.map() is used to transform values in one array to another array of values, i.e. a 1:1 transform operation. In this case, you're merely using it for looping, something better suited for array.forEach().

Since you're "reducing"/"aggregating" an array of values into one value (turning an array of values into an object), array.reduce() is the better option.


PHP Going Multiple Directories Back Best Practice I'm writing a php library for managing configurations of an application.

but the problem is anyone who uses my library will store their configuration files in ./config folder of their project root directory.

e.g. var/www/example.com/config/site.php

and my library is installed in a sub directory of the project

i.e  vendor/azi/config/src/Config.php

To Load files from project root i am doing it this way

    if (!defined('PATH_TO_CONFIG_DIR')) {
        define('PATH_TO_CONFIG_DIR', dirname(dirname(dirname(dirname(dirname(__FILE__))))) . "/config/");
    }

is it right
? Or It can be more efficient. Code suggestion will be helpful.

Github : https://github.com/azeemhassni/Config

Instead of nesting the dirname calls, according to the documentation for dirname you could call this as of PHP 7:

dirname(__FILE__, 5)

For supporting lower versions of PHP, you could write a parentDirectory function:

function parentDirectory($path, $levels) {
    for ($i = 0; $i < $levels; $i++) {
        $path = dirname($path);
    }
    return $path;
}

Or just embed it inside your current code, without a specific function:

$configPath = __FILE__;
for ($i = 0; $i < 5; $i++) {
    $configPath = dirname($configPath);
}
define('PATH_TO_CONFIG_DIR', $configPath . "/config/");


Case of Cyclomatic Complexity I'm trying to recreate the method inject (Works like Reduce) from ruby Enumerable Module.
Using Rubocop (A Linter) I got the error:

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

The code is:

module Enumerable
  def my_inject(*args)
    list = to_a if Range
    reduce = args[0] if args[0].is_a?(Integer)
    operator = args[0].is_a?(Symbol) ? args[0] : args[1]

    if operator
      list.each { |item| reduce = reduce ? reduce.send(operator, item) : item }
      return reduce
    end
    list.each { |item| reduce = reduce ? yield(reduce, item) : item }
    reduce
  end
end

I already did my best to reduce the Cyclomatic complexity,
but still have 1 above (7/6), now I stuck, any directions will be appreciate.

The code can be run on reply:
https://repl.it/@ThiagoMiranda2/INJECT

Before I answer your question I should point out that:

list = to_a if Range

is incorrect. Range is a constant and if Range is always true. You probably means if is_a?(Range) which would mean list was undefined in most cases.

As a first attempt at solving this I would take the case where an operator is provided and have the method call itself with a block something like below.

module Enumerable
  def my_inject(*args)
    unless block_given?
      operator = args.delete_at(-1)
      initial  = args.first
      return my_inject(initial) { |accumulator, i| accumulator.send(operator, i) }
    end

    list = is_a?(Range) ? to_a : self

    accumulator = args.first
    list.each { |item| accumulator = accumulator ? yield(accumulator, item) : item
    accumulator
  end

end

Note: I didn't test this or check its complexity.

This code is also incorrect. There is actually a difference between an initial value that is not provided and one of nil. It is also possible for the block to to return nil which would cause the yield block not to be called for that iteration. The real solution is to check args.length not just args.first but that would increase your complexity.

To do this properly and still pass Rubocop's checks for complexity (as well as its other checks) you will need to break this method up into a few smaller methods.


VBA - recursive combination listing based on Excel columns I have rewritten a recursive procedure to list combinations of values in Excel columns in a way, that instead of one cell string output, there is an array output to Excel range (one value per cell, one combination per row). It lists up to 1 000 000 combinations and executes in 45 seconds / 1 000 000 combinations on my PC. Are any improvements which would let me decrease execution time possible?

   Sub ListCombinations()

    Dim arr As Variant, outCnt As Long, vOut() As Variant
    Dim lastRow As Long
    Dim tim As Double: tim = Timer

    arr = ThisWorkbook.Worksheets("assignment optimization").Range("b2:j12")
    ReDim vOut(1000000, UBound(arr, 2))
    outCnt = 1
    Arrangements arr, vbNullString, LBound(arr, 2), outCnt, vOut

    Application.ScreenUpdating = False

    With ThisWorkbook.Worksheets("assignment optimization")
        With Range(.Cells(1, 12), .Cells(outCnt, 11 + UBound(vOut, 2)))
            lastRow = .Columns.Find(what:="*", searchorder:=xlByRows, searchdirection:=xlPrevious).Row
            .Columns.ClearContents
            .Value = vOut
        End With
    End With

    Application.ScreenUpdating = True

    Debug.Print Timer - tim

End Sub

Sub Arrangements(ByRef arr, ByVal s As String, ByVal lInd As Long, ByRef outCnt As Long, ByRef vOut As Variant)

Dim i As Long, arrayIndex1 As Long

    For i = LBound(arr) To UBound(arr)
        If arr(i, lInd) <> False Then 'exclude values "FALSE" from listing
            If lInd = UBound(arr, 2) Then
                For arrayIndex1 = LBound(vOut, 2) To UBound(vOut, 2)
                    vOut(outCnt, arrayIndex1) = Split(Mid$(s, 2) & "," & arr(i, lInd), ",")(arrayIndex1 - 1)
                Next arrayIndex1
                If outCnt = 1000000 Then Exit Sub
                outCnt = outCnt + 1
            Else
                vOut(outCnt, lInd) = arr(i, lInd)
                Arrangements arr, s & "," & arr(i, lInd), lInd + 1, outCnt, vOut
            End If
        End If
    Next i

End Sub

Edit:

I already found first improvement. Instead of splitting the same string on each arrayIndex1 iteration, I do the splitting outside of the loop and assign it to array. Execution time is now 18 seconds.

Dim outVals () as string

...

outVals = Split(Mid$(s, 2) & "," & arr(i, lInd), ",")

For arrayIndex1 = LBound(vOut, 2) To UBound(vOut, 2)
    vOut(outCnt, arrayIndex1) = outVals(arrayIndex1 - 1)
Next arrayIndex1

Edit 2:

Combinations input and output layout:

I continually get Subscript Out of Range at

vOut(outCnt, arrayIndex1) = Split(Mid$(s, 2) & "," & arr(i, lInd), ",")(arrayIndex1 - 1)

ByRef

Sub Arrangements(ByRef arr, ByVal s As String, ByVal lInd As Long, ByRef outCnt As Long, ByRef vOut As Variant)

Arrangements arr, s & "," & arr(i, lInd), lInd + 1, outCnt, vOut

Why are you passing all these arguments ByRef? Especially because you're calling the procedure from within the procedure. By definition the arr passed ByVal will always persist as will the outCnt. The vOut is being passed back as well, so it doesn't need to be ByRef.
Do you see what I mean? If you had this -
Sub main()
    Dim i As Long
    i = 1
    adding i
End Sub
Sub adding(ByVal i As Long)
    If i > 0 Then i = i + 1
    adding i
End Sub

The i would increase every time it's passed back. What ByRef would be used for is making changes to something that isn't passed back.
That being said, why are you calling your procedure from within your procedure anyway?

Sub Arrangements(ByRef arr, ByVal s As String, ByVal lInd As Long, ByRef outCnt As Long, ByRef vOut As Variant)
For
  If
    If lInd = UBound(arr, 2) Then
   Else
       vOut(outCnt, lInd) = arr(i, lInd)
       Arrangements arr, s & "," & arr(i, lInd), lInd + 1, outCnt, vOut
End If

Seems to me if your second If isn't true, maybe you need to go to the next For? Or does it need to re-perform the entire process again?
Maybe I'm having trouble understanding it. I have no idea what lInd is.
Variables
Give your variables meaningful names. This makes following the code easier and it also makes future you happy that you can take a look and know what is happening without tracing the entire procedure.

arr - tell me of what. e.g. arrayOfCombinations()
outCnt = it's a count of what and where is it going out?
vOut - once again, what is this?
s -?
arrayIndex1 - why is this #1? Is there a second one? Try to never put numerical digits in a variable's name
lInd - this is created as a ByVal argument and then sent back to itself as its own value. I - what?!

I'm not being mean, but how would I ever be able to figure out what lInd is if it's never truly defined and has a name that doesn't tell me much. That's very poor logic (of the procedure, not you).
Magic numbers
I see the number 1,000,000 twice. What is it? The number of combinations?
Const TOTAL_COMBINATIONS as Long = 1000000

Or name it whatever it is.


Take a limited number of items from a potentially infinite generator Hypothetically, I have a bunch of generators that never end, like:

function* progression() {
  let n = 0;

  while (true) {
    yield n++;
  }
}

I want to take first N items from that generator. So I made a function take for that:

function* take(n, generator) {
  let i = 0;
  let values = generator();

  for (let v of values) {
    if (++i > n) {
      return;
    }

    yield v;
  }
}

Is there a better way, maybe standard ES6+ API or a more concise syntax to achieve the same thing? If this function could fail under circumstances that I didn't account for, please give me a hint.

I would write it as:

function* take(n, generator) {
  let values = generator();

  let i = 0;
  while (i++ < n) {
    yield values.next().value;
  }
}

this seems to match the intent more closely though its mostly a matter of personal preference.

Update

You can also add the take method as an extension to the generator so you could call it like generator().take(3). See this SO answer if you are interested in that approach.


Removing duplicate words from an unsorted list I was using brute force to remove duplicate words since the lists were really small. But I want a solution that won't become too slow if the input grows.

This function creates a binary tree and inserts all words appearing on the list, then collects the unique words without sorting. Duplicate words are handled during insertion. For the tree, I'm using about the same code from Unbalanced binary search tree.

#include "bst.h"
#include <strings.h>
#include <stdlib.h>

#define LIST_TERMINATOR 1

static size_t i = 0;
static char **final_list;

static void insert(void *word)
{
    final_list[i++] = word;
}

char **unique_words(const char **words)
{
    //Binary tree containing the words
    BST unique;
    bst_init(&unique, (int(*)(const void *, const void *))strcasecmp);

    //Every word will be inserted at most 1 time
    while(*words != NULL){
        if(bst_insert(&unique, (void *)*words) == BST_NO_MEMORY){
            bst_free(&unique);
            return NULL;
        }

        ++words;
    }

    //Array to return
    final_list = malloc(sizeof(char *) * (unique.node_count + LIST_TERMINATOR));
    if(final_list == NULL){
        bst_free(&unique);
        return NULL;
    }

    //Collect words without sorting, so if the list is merged with another
    //and passed again, the tree won't become a linked list
    if(bst_iterate_top_down(&unique, insert) == BST_NO_MEMORY){
        free(final_list);
        bst_free(&unique);
        return NULL;
    }
    final_list[i] = NULL;

    bst_free(&unique);

    //Clear state
    i = 0;

    return final_list;
}

Would sorting the input then removing duplicates be faster?

This question boils down to the number and sequence of operations. If you are building one large list but removing lots of duplicates, use a hash table or sort and uniquify the list.

Sorting and making the list unique is O(n log n) at best. Removing a duplicate is O(n) at worst so removing m duplicates is O(m * n). While in general O(k * n) = O(n), once m exceeds log n the one-time cost of sorting the set pays for itself. And additional lookups remain O(1).

You must evaluate the costs with realistic values for m and n to decide which method is better. Of course, their values will vary over time along with the relative costs of CPU versus RAM versus disk. But for most "probable" values of m and n over time, relying on standard library collection classes--with a sprinkle of tuned variations from Guava and the like--is the hands-down winner.


Game Character class with many skills Im creating a class that has all of my data in and can see it would quickly become messy.

I feel that there must be a way of making it neater rather than making a new method each time

For example. I have a character that out of the huge skill list each with different values for different things, they just have 1h weapon and shield. I declare this by

Character character = new Character();
character.Skills.Add(Skill.OneHanded());
character.Skills.Add(Skill.Shield());

it also makes it fairly easy if they level up and learn a new skill as i can just do

character.Skills.Add(Skill.Polearm());

however currently my Skill class looks like a mess. (note that there will be many more skills (about 30 in total) and each one will have more attributes such as applicable weapons, level requirements etc, but not currently coded.

public class Skill
{
    public string Description { get; set; }
    public bool Advanced{ get; set; }
    public int PointsToLearn{ get; set; }

    public static Skill OneHanded()
    {
        return new Skill
        {
            Description = "Skill needed for the use of one handed weapons",
            Advanced = false,
            PointsToLearn= 1
        };
    }
    public static Skill TwoHanded()
    {
        return new Skill
        {
            Description = "Skill needed for use of two handed weapons.",
            Advanced= false,
            PointsToLearn = 2
        };
    }
    public static Skill Polearm()
    {
        return new Skill
        {
            Description = "Skill needed for use of polearms",
            Advanced = false,
            PointsToLearn = 3
        };
    }
    public static Skill Shield()
    {
        return new Skill
        {
            Description = "Skill needed to use shields",
            Advanced = false,
            PointsToLearn = 1
        };
    }
    public static Skill ResistMagic()
    {
        return new Skill
        {
            Description = "Negates the first spell cast at the character",
            Advanced = true,
            PointsToLearn = 0
        };
    }
}

As requested the Character class is

class Character
    {
        public List<Skill> Skills = new List<Skill>();
        public int Level { get; set; }
        public string Name { get; set; }
        public int Command { get; set; }
        public int Coordination { get; set; }
        public int Reflex { get; set; }

    }

There are many characters (i guess think fire emblem if you have ever played that) each one can be skilled and levelled (and die) independently . The class is mostly to keep a record of all characters that have joined the players mercenary company, with their current skills, attributes, level etc etc.

It will called when ever a new character joins the company and added to a character list and then remain in the list ready to be called with the attributes of the character when needing to know information about them.

is there a way to simply this? I also got advised I could do

public class Skill
{
    public enum Skills{ OneHanded,TwoHanded,Polearm,Shield};
}

which opens up using Skill.Skills."Skill" but I'm not sure how I would set the data against each of the options.

In the way you show that you create each Skill instance, you really have anything than the Description to distinguish them from each other.

I read your Skill class as merely an advanced flag that defines a small set of properties. In the below I further anticipate, that each skill type doesn't change throughout the game. If that is correct, then I would make the Skill class as a "singleton" per Skill type (onehanded, twohanded etc.) and let a static Factory method create/return the right Skill according to a Skill Type enum:

public class Skill
{
  public string Description { get; private set; }
  public bool Advanced { get; private set; }
  public int PointsToLearn { get; private set; }
  public SkillType Type { get; private set; }

  private Skill()
  {
  }

  private static Dictionary<SkillType, Skill> s_skills = new Dictionary<SkillType, Skill>
  {
    { SkillType.OneHanded, new Skill { Type = SkillType.OneHanded, Description = "Skill needed for the use of one handed weapons", Advanced = false, PointsToLearn = 1, } },
    { SkillType.TwoHanded, new Skill { Type = SkillType.TwoHanded, Description = "Skill needed for use of two handed weapons", Advanced = false, PointsToLearn = 2, } },
    { SkillType.Polearm, new Skill { Type = SkillType.Polearm, Description = "Skill needed for use of polearms", Advanced = false, PointsToLearn = 3, } },
    { SkillType.Shield, new Skill { Type = SkillType.Shield, Description = "Skill needed to use shields", Advanced = false, PointsToLearn = 1, } },
    { SkillType.ResistMagic, new Skill { Type = SkillType.ResistMagic, Description = "Negates the first spell cast at the character", Advanced = true, PointsToLearn = 0, } },
  };

  public static Skill Factory(SkillType type)
  {
    return s_skills[type];
  }
}

public enum SkillType
{
  OneHanded,
  TwoHanded,
  Polearm,
  Shield,
  ResistMagic
}

If their role is more advanced than that with methods and state, it IMO calls for at class hierarchy in some way like:

public abstract class Skill
{
  public string Description { get; set; }
  public bool Advanced { get; set; }
  public int PointsToLearn { get; set; }
  public SkillLevel SkillLevel { get; set; }
}

public class OneHanded: Skill
{

}

public class TwoHanded : Skill
{

}
...

which of cause requires a lot more work.

Update

If you want to filter a list of skills by SkillLevel, it can be done like this:

List<Skill> skills = new List<Skill> { new OneHanded(), new TwoHanded(), ... etc. }

foreach (Skill skill in skills.Where(s => s.SkillLevel == SkillLevel.Basic))
{
  Console.WriteLine(skill.GetType().Name);
}

Beside that I would change the Advanced flag to an enum too, because you can then distinguish between more levels like:

enum SkillLevel
{
  Basic,
  Medium,
  Advanced,
}


Show dialog only once. Other calls should bring dialog to front, but otherwise not have a result This code works, as far as I can tell, but there's something that just feels wrong about it. The intended flow is like this:

If no dialog is showing

Show dialog, return result

else

Move dialog into view, return no result

It sounds easy enough, but because there's a possiblity of multiple threads I have to use locking. However, because the ShowDialog() call is blocking, I couldn't just throw a lock around the whole thing, so instead my code is littered with Monitor.Enter and Monitor.Exit calls. It gets even more complicated due to exception handling.

interface IDialogService
{
    bool? ShowAuthenticationDialog();
}

class DialogService : IDialogService
{
    private readonly IServiceProvider _serviceProvider;

    public DialogService(IServiceProvider serviceProvider)
    {
        _serviceProvider = serviceProvider;
    }

    private readonly object _authenticationDialogLock = new object();
    private bool _isAuthenticationDialogShowing;
    private AuthenticationDialog _authenticationDialog;

    public bool? ShowAuthenticationDialog()
    {
        Monitor.Enter(_authenticationDialogLock);
        if (!_isAuthenticationDialogShowing)
        {
            try
            {
                _isAuthenticationDialogShowing = true;
                _authenticationDialog = _serviceProvider.GetService<AuthenticationDialog>();
                Monitor.Exit(_authenticationDialogLock);

                var authenticationDialogResult = _authenticationDialog.ShowDialog();

                Monitor.Enter(_authenticationDialogLock);
                return authenticationDialogResult;
            }
            catch
            {
                return null;
            }
            finally
            {
                _authenticationDialog = null;
                _isAuthenticationDialogShowing = false;

                // Need this check because an exception could come from ShowDialog()
                // where we're not locked.
                if (Monitor.IsEntered(_authenticationDialogLock))
                    Monitor.Exit(_authenticationDialogLock);
            }
        }

        _authenticationDialog.Activate();
        Monitor.Exit(_authenticationDialogLock);
        return null;
    }
}

Can I make this less "messy" somehow? Have I missed any obvious cases where the constant grab-and-release of the lock can lead to problems?

I feel like there could be an edge case where a thread can call Activate() before ShowDialog() has been called or after it has returned, but I honestly can't think of how to protect against that specific case because ShowDialog() is blocking and thus has to release its lock.

Oh, and if anyone's wondering, the GetService<>() method on the IServiceProvider is an extension method that looks like this:

public static class ServiceProviderExtensions
{
    public static T GetService<T>(this IServiceProvider serviceProvider)
    {
        return (T)serviceProvider.GetService(typeof(T));
    }
}

Shout in the dark, really...

public class DialogService : IDialogService
{
    private static readonly ConcurrentQueue<AuthenticationDialog> DialogQueue =
        new ConcurrentQueue<AuthenticationDialog>();
    private static volatile bool _isShowing;

    public DialogService(IServiceProvider serviceProvider)
    {
        DialogQueue.Enqueue(serviceProvider.GetService<AuthenticationDialog>());
    }

    public bool? ShowAuthenticationDialog()
    {
        AuthenticationDialog dialog;
        DialogQueue.TryDequeue(out dialog);

        if (dialog == null)
            return null;

        if (_isShowing)
        {
            dialog.Activate();
            return null;
        }

        _isShowing = true;
        bool? result = null;

        try
        {
            result = dialog.ShowDialog();
        }
        catch
        {}
        finally
        {
            _isShowing = false;
        }

        return result;
    }
}


Determine if an input is an integer, a double or a String I created a method to test whether the specified input is a double, int or String and then pass it to a 3rd party library which has the methods doFoo which accepts either a String, double or int. This is the method that I would like feedback on:

public static void testString(String val) {
    System.out.print("Original '" + val + "'  ");
    String x = val.trim();
    try {
        int i = Integer.parseInt(x);
        System.out.println("It's an integer: " + i);
        doFoo(i);
    } catch (NumberFormatException e) {
        try {
            double d = Double.parseDouble(x);
            System.out.println("It's a double: " + d);
            doFoo(d);
        } catch (NumberFormatException e2) {
            System.out.println("It's a String: " + x);
            doFoo(x);
        }
    }
}

Is this good code? Could it be improved? I don't like the throwing and catching of Exceptions.

Some test code to prove it works:

testString("N/A");
testString("19.");
testString("19.0");
testString("19.4");
testString("  1 ");
testString(" 1");
testString("1 ");
testString("1");
testString("    ");

Results in:

Original 'N/A'  It's a String: N/A
Original '19.'  It's a double: 19.0
Original '19.0'  It's a double: 19.0
Original '19.4'  It's a double: 19.4
Original '  1 '  It's an integer: 1
Original ' 1'  It's an integer: 1
Original '1 '  It's an integer: 1
Original '1'  It's an integer: 1
Original '    '  It's a String:

I don't like the throwing and catching of Exceptions

This can be made much cleaner with the use of a Scanner. It might not be the most performant way, but it's fast and easy to use.

try (Scanner scanner = new Scanner(x)) {
    if (scanner.hasNextInt()) doFoo(scanner.nextInt());
    else if (scanner.hasNextDouble()) doFoo(scanner.nextDouble());
    else doFoo(x);
}

However, if this is going to be called hundreds of thousands of times, the try catch method might be faster, though you should encapsulate those into their own functions. You'd need to profile to be sure which is faster, but I believe it would be this because Scanner.hasNextFoo uses regular expressions:

public static boolean isInteger(String str) {
    try {
        Integer.parse(str);
        return true;
    } catch (NumberFormatException e) {
        return false;
    }
}

Also, your function is doing multiple things: printing/reporting, and parsing/forwarding to doFoo. This is not a good thing. I'd recommend removing those and handling them where it's more appropriate:

public static void testString(String val) {
    String x = val.trim();

    try (Scanner scanner = new Scanner(x)) {
        if (scanner.hasNextInt()) doFoo(scanner.nextInt());
        else if (scanner.hasNextDouble()) doFoo(scanner.nextDouble());
        else doFoo(x);
    }
}

That was much shorter. Now if you wanted the same functionality, it would look like so:

public static void testTestString(String val) {
     System.out.print("Original '" + val + "'  ");
     testString(val);
}

// ...

public static void doFoo(int i) {
    System.out.println("It's an integer: " + i);
    // ...
}

If you want your code to be extremely extensible, there is another way. Notice how the new function I suggested still does multiple things:

It detects the type of the string
It parses the value from the string
It forwards the value on to another function

We can separate these into their own components.

This is only really worth it if you can foresee adding types to be a common feature, but especially if the "another function" you forward to should be selectable by the user (say, if you packaged these functions as member functions of an object):

// Class is the easiest type we can return
private static Class<?> determineType(String val) {
    try (Scanner scanner = new Scanner(val)) {
        if (scanner.hasNextInt()) return Integer.class;
        if (scanner.hasNextDouble()) return Double.class;
        return String.class;
    }
}

private static final Map<Class<?>, Function<String, ?>> parsers = new IdentityHashMap<>();
private static final Map<Class<?>, Consumer<Object>> functionSwitch = new IdentityHashMap<>();

static {
    parsers.put(Integer.class, Integer::parseInt);
    parsers.put(Double.class, Double::parseDouble);
    parsers.put(String.class, Function.identity());

    // Note that, due to limitations in the type system,
    // i is of type Object, so we need to cast it to the appropriate
    // class before forwarding on to the function.
    functionSwitch.put(Integer.class, i -> doFoo((Integer) i));
    functionSwitch.put(Double.class, d -> doFoo((Double) d));
    functionSwitch.put(String.class, str -> doFoo((String) str));
}

public static void testString(String val) {
    val = val.trim(); // This could even be part of the parser's responsibility
    Class<?> stringType = determineType(val);
    Function<String, ?> parser = parsers.get(stringType);
    functionSwitch.get(stringType).accept(parser.apply(val));
}


Web scraper for parsing names and email addresses from Yellowpage I've written a script using python to parse the names and email addresses of different pizza shops in USA. I am very new in writing classes using python so I'm not very sure I didn't do anything wrong with it's design.

However, it serves the purpose scraping the required fields. Firstly, it scrapes all the links of different pizza shops then parses the links to the next page and finally tracking down the links of each pizza shops to it's main page to harvest the documents from there. I would be very happy if I could learn the way I could improve my class crawler.

Here is what I have tried with:

import requests

from lxml import html

class YellowPage:
    main_url = "https://www.yellowpages.com/search?search_terms=pizza&geo_location_terms=Los+Angeles%2C+CA"

    def __init__(self):
        self.links = [self.main_url]
        self.storage = []

    def crawl(self):

        for link in self.links :
            self.get_link(link)

    def get_link(self, link):

        print('Scraping Now: ' + link)
        url = "https://www.yellowpages.com"
        response = requests.get(link)
        tree = html.fromstring(response.text)

        #scraping links of each pizza shop
        for items in tree.xpath("//div[@class='info']"):
            link_page = items.xpath(".//a[@class='business-name'][not(@itemprop='name')]/@href")
            for page in link_page:
                if page and url + page not in self.links:
                    self.links += [url + page]

        #parsing the links to the next page
        next_page = tree.xpath("//div[@class='pagination']//li/a/@href")
        for nepage in next_page:
            if nepage and url + nepage not in self.links:
                self.links += [url + nepage]

        #going to the main page of each pizza shop and harvest the record
        for posts in tree.xpath("//*[@id='main-header']"):
            name = posts.xpath(".//div[@class='sales-info']/h1/text()")[0] if posts.xpath(".//div[@class='sales-info']/h1/text()") else ""
            email = posts.xpath(".//a[@class='email-business']/@href")[0] if posts.xpath(".//a[@class='email-business']/@href") else ""
            records = name, email
            self.storage.append(records)

    def __str__(self):
        return self.storage

crawler = YellowPage()
crawler.crawl()
for item in crawler.storage:
    print(item)

Btw, when I write the above code using function then i find that very speedy.

Here is the summary of the applied improvements:

keeping a visited set of links - \$O(1)\$ lookups
using collections.deque() to keep the queue of the next links to scrape - popping left from it is \$O(1)\$
reusing the same requests.Session() instance, keeping a session instance as an instance variable of your crawler class
using urljoin() to join the base url and the relative url
collecting both pizza shop links and the next page links into a single list and process that list once
removing the loop when extracting a pizza shop profile page information
put the main execution logic to under the if __name__ == '__main__':

Improved code:

from collections import deque
from urllib.parse import urljoin

import requests
from lxml import html

class YellowPage:
    base_url = "https://www.yellowpages.com"
    main_url = "https://www.yellowpages.com/search?search_terms=pizza&geo_location_terms=Los+Angeles%2C+CA"

    def __init__(self):
        self.session = requests.Session()
        self.session.headers = {'User-Agent': 'Mozilla/5.0 (Macintosh; Intel Mac OS X 10_12_5) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/59.0.3071.115 Safari/537.36'}

        self.visited = set([])
        self.links = deque([self.main_url])

        self.storage = []

    def crawl(self):
        while self.links:
            link = self.links.popleft()
            self.scrape(link)

    def scrape(self, link):
        if link in self.visited:
            return

        self.visited.add(link)
        print('Scraping Now: ' + link)

        response = self.session.get(link)
        tree = html.fromstring(response.text)

        # extracting the follow up links
        pizza_shop_links = tree.xpath("//div[@class='info']//a[@class='business-name'][not(@itemprop='name')]/@href")
        next_page_links = tree.xpath("//div[@class='pagination']//li/a/@href")
        links = pizza_shop_links + next_page_links

        for link in links:
            link = urljoin(self.base_url, link)
            if link not in self.visited:
                self.links.append(link)

        # going to the main page of each pizza shop and harvest the record
        is_pizza_shop_page = tree.xpath("//*[@id='main-header']/article")
        if is_pizza_shop_page:
            name = tree.findtext(".//div[@class='sales-info']/h1")
            try:
                email = tree.xpath(".//a[@class='email-business']/@href")[0]
            except IndexError:
                email = ""
            self.storage.append((name, email))

    def __str__(self):
        return self.storage

if __name__ == '__main__':
    crawler = YellowPage()
    crawler.crawl()
    for item in crawler.storage:
        print(item)

(not tested)


Abusive use of long database query? I somehow acquired the idea that I should reduce the number of trips to the database as much as possible and so I use a lot of very long queries.

For example, I had a function that does this:

$shelf = get_shelves($shop_id)
foreach($shelf)
{
    $weight = get_weight($shelf);
}

Everytime a user "view shop" it will list all the shelves, and when I see the loop, I thought I should "optimise" this, so that I only go to the database once. But I ended up with a long query:

$shelf = "SELECT * FROM shelf WHERE shop_id=?";
$weight = "SELECT tray.shelf_id, SUM(egg.weight) AS total_weight FROM tray LEFT JOIN egg WHERE egg.tray_id=tray.tray_id GROUP BY shelf_id";
$query = "SELECT * FROM ($shelf) AS shelf LEFT JOIN ($weight) AS weight ON shelf.shelf_id=weight.shelf_id";

With this, I didn't have to get_weight() anymore.

Is this "optimisation" at all, is it acceptable? Is there a rule of thumb so that programmers and DBA can be happy at the same time?

You are on the right track about trying to reduce trips to the database. I think the rule of thumb would be: whatever gives the best overall performance ;-)

A single query will almost always outperform N queries. Sometimes you may have the choice between two single queries: one that's extremely complex but loads exactly the data you need, and another that's simple but loads more data than you need and you have to filter the result yourself in code. When the database is heavily loaded, the simple query can be faster.

That said, your final $query is not optimal. This would be better:

SELECT col1, col2 FROM shelf
LEFT JOIN (
    SELECT tray.shelf_id, SUM(egg.weight) AS total_weight FROM tray
    LEFT JOIN egg USING (tray_id)
    GROUP BY shelf_id
) AS weight USING (shelf_id)
WHERE shop_id = ?

Of course col1 and col2 are not real columns, I just wanted to emphasize that you should always write the column names explicitly. It's faster to load only what you really need. It also protects you from changes in the column order.

Also notice that I removed one sub-query, which should also make it faster.


Run a query for each value in a column For each ID found in Column A in the XLS file, run a query to
  retrieve and save a customer.

"Customers" table in db.accdb Access file
+----+-----------------+
| ID |    Customer     |
+----+-----------------+
|  1 | Thomas Moody    |
|  2 | Natalie Laguna  |
|  3 | James Robinson  |
+----+-----------------+

.xlsx Excel worksheet
+----------+
| Column A |
+----------+
|        1 |
|        2 |
|        3 |
+----------+

Solution:

Should I use Singleton pattern to persist connection lifetime?
I'm sure it's better to retrieve all customers at once but what's next? How do I retrieve customers that easily?

Option Explicit

Private pDb As ADODB.Connection
Private pDbCmd As ADODB.Command
Private pDbRecordset As ADODB.Recordset

Private Sub Main()

    ' Assuming I've already read values from column into an array
    Dim customerIDs(1 To 3, 1 To 1) As Long
    Let customerIDs(1, 1) = 1
    Let customerIDs(2, 1) = 2
    Let customerIDs(3, 1) = 3

    Dim i As Long
    Dim customerID As Long
    Dim customer As String

    ' For each ID run a query, retrieve and display a customer name
    For i = 1 To UBound(customerIDs, 1)
        Let customerID = customerIDs(i, 1)
        Let customer = GetCustomer(customerID)

        Debug.Print customer
    Next i

    Call CloseConnection

End Sub

Private Function QueryDB(sqlQuery As String) As String

    ' Singleton
    If (pDb Is Nothing) Then
        Call OpenConnection
    End If

    Call ExecuteCmd(sqlQuery)
    Let QueryDB = ReadRecord

End Function

Private Function GetCustomer(ID As Long) As String

    ' Prepare SQL query
    Dim sqlQuery As String
    Let sqlQuery = "SELECT Customer FROM Customers WHERE Customers.ID = " & ID

    ' Return
    Let GetCustomer = QueryDB(sqlQuery)

End Function

Private Sub OpenConnection()

    Set pDb = New ADODB.Connection
    Set pDbCmd = New ADODB.Command
    Set pDbRecordset = New ADODB.Recordset

    Dim dataSource As String
    Let dataSource = ThisWorkbook.Path & "\db.accdb"

    Dim connectionString As String
    Let connectionString = "Provider=Microsoft.ACE.OLEDB.12.0;Data Source=" & dataSource & ";Persist Security Info=False;"

    Call pDb.Open(connectionString)
    Let pDbCmd.ActiveConnection = pDb

End Sub

Private Sub ExecuteCmd(sqlQuery As String)

    pDbCmd.CommandText = sqlQuery
    pDbCmd.CommandType = adCmdText

    Set pDbRecordset = pDbCmd.Execute

End Sub

Private Function ReadRecord() As String

    Do While Not (pDbRecordset.EOF)
        Let ReadRecord = pDbRecordset.Fields("Customer").value
        Call pDbRecordset.MoveNext
    Loop

End Function

Private Sub CloseConnection()

    Call pDb.Close

    Set pDbRecordset = Nothing
    Set pDbCmd = Nothing
    Set pDb = Nothing

End Sub

The solution above shows some very old coding practices that are no longer valid. In addition the code has some level of complexity that is not required - almost like a Rube Goldberg approach!

The use of Option Explicit is to be applauded. Same as the lack of On Error [Resume Next]!

Main

Some broad comments:

The use of "Main" as a function name is not required and hides what
the real intent is.
You use a 2-D array when a single dimension will do.
You juggle values through different variables without any real reason.
Why not show the code you will use to extract the IDs from the Excel file?
You call CloseConnection but you have not opened one.

Refactored code below

Private Sub Main()
    ' Assuming I've already read values from column into an array
    'Dim customerIDs(1 To 3) As Long
    'customerIDs(1) = 1
    'customerIDs(2) = 2
    'customerIDs(3) = 3
    Dim customerIDs as Variant
    customerIDs = Range("NamedRangeHoldingColumnOfIDs") ' this will return an array

    Dim i As Long
    Dim customer As String

    ' For each ID run a query, retrieve and display a customer name
    OpenConnection ' should Open and Close in the same scope
    For i = 1 To UBound(customerIDs)
        If IsNum(customerIDs(i)) then  ' doesn't hurt to add a bit of error checking
            customer = GetCustomer(CLng(customerIDs(i)))
            Debug.Print customer
        End If
    Next i
    CloseConnection
End Sub

Helper functions

You have fractured your helper functions too much, you are probably confusing yourself with your logic.

Your ReadRecord function reads all records in the recordset, which you have set with your GetCustomer, QueryDB and ExecuteCmd routines.
Perhaps breaking them apart would make sense if you were to re-use
them, but your calls (parameters) and returns are not set up to allow
this. Your use of globals would make re-use fraught with danger
(particular in overlapping or older calls, and race conditions)

Refactoring all this into one function makes it cleaner.

Private Function GetCustomer(ID As Long) As String
    Dim sqlQuery As String
    Dim readResult as String
    sqlQuery = "SELECT Customer FROM Customers WHERE Customers.ID = " & ID
    If (pDb Is Nothing) Then
        OpenConnection  ' could consider opening and closing connection only in this scope - could be expensive but safer.
    End If

    ' Return
    pDbCmd.CommandText = sqlQuery  ' Assumes pDbCmd is valid
    pDbCmd.CommandType = adCmdText
    Set pDbRecordset = pDbCmd.Execute
    Do While Not (pDbRecordset.EOF)
        readResult = pDbRecordset.Fields("Customer").value
        pDbRecordset.MoveNext
    Loop
    GetCustomer = readResult
End Function

Connections

Having done this, you rely on globals to manage your connections. All fine if you have a single purpose - but if you are going to make this more complex it will introduce problems.
You can fix this by treating your Open and Close as parameterised routines.

Private Sub CloseConnection(Db as ADODB.Connection, DBRecordSet as ADODB.Recordset, DbCmd As ADODB.Command)
    Db.Close
    Set DbRecordset = Nothing
    Set DbCmd = Nothing
    Set Db = Nothing
End Sub

Similarly for OpenConnection

Private Sub OpenConnection(ByVal DbName, [ByRef] Db as ADODB.Connection, [ByRef] DbCmd As ADODB.Command ) 'As Boolean - perhaps indicate if successful?
    Set Db = New ADODB.Connection
    Set DbCmd = New ADODB.Command
    Dim dataSource As String
    dataSource = ThisWorkbook.Path & DbName '"\db.accdb"
    Dim connectionString As String
    connectionString = "Provider=Microsoft.ACE.OLEDB.12.0;Data Source=" & dataSource & ";Persist Security Info=False;"
    Db.Open(connectionString)
    DbCmd.ActiveConnection = Db
End Sub

Additional thoughts

Having gone through the above process, the key DB element is the DBCmd object. Your Open/Close could simply pass that as a result with all other workings to get there encapsulated within the relevant function.

You don't do any error checking.

What happens if you cannot open the database?
What happens if you cannot open the query?
What happens if you cannot find a relevant record?


Follow-up: Find sum of number of times that each character of source occurs in the target This question is a follow-up of this question. The aim of the questions is to find sum of number of times that each character of source occurs in the target.

In the new code, I've renamed the char array and added the functionality to prevent the following "if a character is duplicated in the source, you will double-count it in the result."

I would like to know if there is still a better solution for it.

It was mentioned in one of the answers of the previous question that I should use HashMap or other data structures for chars more than 100, but I am not sure how and why.

String target = "alex alexander";
String source = "ardx";
Map<Character,Integer> tempMap = new HashMap<>();

char[] tchars = source.toCharArray();

int occur = 0;
int occurrence = 0;

for (int i = 0; i < tchars.length; i++) {
   if(!tempMap.containsKey(tchars[i])){ //if the occurrence of the current char
        String temp = target;           //   has been already calculated
        int index = temp.indexOf(tchars[i]);

        while (index != -1) {
            occur++;
            index = temp.indexOf(tchars[i], index + 1);
        }
        tempMap.put(tchars[i],occur);
        occurrence += occur; //add to the total counter
        occur = 0;           //reset the single counter
   }else{
       occurrence += tempMap.get(tchars[i]);
   }

}
System.out.println(occurrence);

Output

a is added to the map and its associated counter is 3
r is added to the map and its associated counter is 1
d is added to the map and its associated counter is 1
x is added to the map and its associated counter is 2
25

Sorry, but this looks worse. I guess, you've been confused by the different ideas presented in the answers. And I am, confused, too. I don't understand it, so just some minor comments:

//reset the single counter

This is a sign that you've defined the counter in a wrong scope. Define it inside of the loop.

tempMap, temp... everything is temporary, we too. But this doesn't mean that it can't be properly named. Or removed... you need no temp String anymore.

I think I sort of understand it. The tempMap keeps track of the number of occurrences per character. So name it like it....

I don't think it really works. However, why not use a Set to keep track of what chars you've seen already? The logic is then trivial: "already seen -> skip it".

Enough for now.


Truncatable Primes -- Project Euler 37? I just finished Project Euler 37 after a bit of debugging and can't figure out how to make it faster. Whenever I add extra tests that I want to speed the program up, it ends up just slowing it down. My primes test is from here. Please help me optimize!

from primes import test as is_prime
from itertools import product
from timeit import default_timer as timer

def is_trunc(list_num):
    num = ''.join(map(str, list_num)) # turn (1, 2, 3) into 123
    if is_prime(int(num)):
        for k in range(1, len(num)):
            if not is_prime(int(num[k:])) or not is_prime(int(num[:k])):
                return False
        return True
    return False

start = timer()
data = {"count": 0, "length": 2, "sum": 0}

while data["count"] < 11:
    for combo in product([1, 2, 3, 5, 7, 9], repeat = data["length"]):
        if is_trunc(combo):
            data["count"] += 1
            data["sum"] += int(''.join(map(str, list(combo))))
    data["length"] += 1

ans = data["sum"]
elapsed_time = (timer() - start) * 1000 # s --> ms

print "Found %d in %r ms." % (ans, elapsed_time)

There's no need to convert lists to ints every time. Truncation from the right is an integer division by 10. Truncation from the left is modulo certain power of ten; you can keep track of the modulo as the loop progresses.

There's no need to run a very expensive primality every time. Keep track of the primes you already found.

PS: The biggest hint in the problem is that there's only 11 numbers of interest. There must be a way to prove that fact, and the proof is most likely constructive.


Kernighan & Ritchie - detabbing program Task (1.20)

  Write a program detab that replaces tabs in the input with the proper number
  of blanks to space to the next tab stop.

Also, the condition is is not to use pointers and structs etc because they will be considered in next chapters.

I'm new in C, and want to know what in my solution is bad idea and what can be done other way (but without pointers):

#include <stdio.h>
#define TABLINE 4

int count(char stri[]);

void main() {

    printf ("\nTabline is %d\n", TABLINE);

    int i = 0;
    int h;
    char stri[100];
    char c;

    while ((c = getchar()) != '\n' && c != EOF) {
        if (c == '\t')
            for (h=0; h<TABLINE; ++h) {
                stri[i] = ' ';
                ++i;
        } else {
            stri[i] = c;
            ++i;
        }
    }

    printf ("%d\n", count(stri));

    for (i=0; i< count(stri); ++i)
        printf ("%c", stri[i]);
}

int count(char stri[]) {

    int l;

    for (l=0; stri[l]; ++l)
        ;

    return l;
}

And its work:

$ ./detab

Tabline is 4
befortan        aftertab
20
befortan    aftertab

Apart from the fact that a tab doesn't necessarily map to 4 spaces, as was nicely put by @Loki in his comment, there are a few other points that you can improve:

Your main function is not following a standard signature. It should be returning an int.
Your count() function, besides having a vague name (what is it counting exactly?), is reinventing strlen(). Avoid reinventing the wheel, unless you have a good reason to (in this case I'd say yes, since you are learning, but it is also valuable to know the standard functions and how to use them).
Still talking about count(), avoid calling it in the conditional section of a loop, like here:

 for (i=0; i< count(stri); ++i)

Remember that the condition gets tested every iteration of the loop, so this would imply a full scan of stri if the compiler is not smart enough to hoist that function call out the loop. Better would be to call the function once and store its result in a local variable.
Declare variables as close to their first usage as it is possible. For instance, h could be declared inside the body of the for loop:

for (int h = 0; h < TABLINE; ++h)

Reducing scope is important to avoid name collisions and facilitate the understanding of the program states. If you had to remember the state of all variables in a program, you would be doomed. Variables that are scoped inside a { } block can be forgotten as soon at the block ends. There is a discussion here that might also find relevant.
You make two other dangerous assumptions in your program: One is that the input will never exceed one line (you terminate the loop in the first '\n') and that the input will never exceed 100 characters (that's the size of your string buffer). Those assumptions are very unrealistic. More than likely the input would consist of multiple lines in a real world scenario. A 100 chars limit is also quite small. If using a statically allocated buffer, without resorting to malloc and pointers, at least make it a bit bigger and make sure to end the loop when the buffer is full. Something in the lines of (where MAX_CHARS is the size of your char[] buffer):

while (i < MAX_CHARS && (c = getchar()) != '\n' && c != EOF) {
    ...
}

Lastly, there is a confusion with the curly braces in the main loop which I'm not quite sure how the compiler is resolving, but it certainly is broken:

while ((c = getchar()) != '\n' && c != EOF) {
    if (c == '\t')
        for (h=0; h<TABLINE; ++h) {
            stri[i] = ' ';
            ++i;
    } else { // <--- does this belong to the first if ???
        stri[i] = c;
        ++i;
    }
}

As you can see if you look carefully, the } else part is bound to the { in the for loop, the if has no {. I was surprised that this compiled, but it did!

Be very careful with this. My advice is to always add curly braces on all control statements and loops, even for the single line ones.