Store object in memory or de-reference it for performance I have a GUI that contains a Table Object (for displaying columns of data) and a Table Model.  The Table Object contains the Table Model.

Is it better to store the Table Model as a object in the User interface or de-reference it as needed?  Is there any performance gain keeping it separate?  If there is, is it a good design to implement this?

For example:

public GUI {
    private Table table;
    private TableModel model;

    public UI() {
        this.table = new Table();
        this.model = table.getModel();
        }
        public void actionPerformed() {
           // Performance improvement
           // without having to de-reference it from table
           model.setRowLimit();
        }
    // OR
    public void actionPerformed() {
        // Slower but is it better OO Design
        table.getModel().setRowLimit();
    }
}

The GUI has 10 public methods exposed where the model could be updated or changed.

There will be no difference in performance whatsoever between the two approaches.

I would suggest you don't break the responsibility principle and leave the handling of the tablemodel to the Table class.

One remark though: if there are additional computations done in getTableModel() aside from a plain setter, you will bypass these if your create the local variable. This may or may not be something you desire.


Matching a checkbox caption to a cell's value There are 30 checkboxes that need to have their caption compared to a cell value.

The basic code is repeated 30 times for 30 different check boxes

The only Lines that Change per CheckBox are the Following:

The First Line - CheckBox3.Value (will increase by 1 for every checkbox)
Fourth Line from Bottom - Cells(eRow, 24).Value = Controls("CheckBox" & 3).Caption (Needs to match Checkbox Selected (3, 4, 5 etc.))
Third Line from Bottom - Cells(eRow, 25).Value = TextBox11.Value (will increase by 1 for every checkbox)

The basic block is:

If CheckBox3.Value = True Then
    eRow = Sheet3.Cells(Rows.Count, 6).End(xlUp).Offset(1, 0).Row
    Cells(eRow, 7).Value = ComboBox1.Value
    If CheckBox29.Value = True Then
        Cells(eRow, 8).Value = Controls("CheckBox" & 29).Caption
            If CheckBox1.Value = True Then
                Cells(eRow, 14).Value = "Yes"
            Else
                Cells(eRow, 14).Value = "No"
            End If
        Cells(eRow, 15).Value = ComboBox6.Value
        Cells(eRow, 16).Value = ComboBox7.Value
            If CheckBox2.Value = True Then
                Cells(eRow, 17).Value = "Yes"
            Else
                Cells(eRow, 17).Value = "No"
            End If
        Cells(eRow, 18).Value = ComboBox8.Value
        Cells(eRow, 19).Value = TextBox5.Value
        Cells(eRow, 20).Value = TextBox6.Value
        Cells(eRow, 21).Value = TextBox7.Value
        Cells(eRow, 22).Value = TextBox8.Value
        Cells(eRow, 26).Value = TextBox9.Value
        Cells(eRow, 27).Value = TextBox10.Value
    Else
        Cells(eRow, 8).Value = "Sales Call"
        Cells(eRow, 15).Value = "-"
        Cells(eRow, 16).Value = "-"
        Cells(eRow, 18).Value = "-"
        Cells(eRow, 19).Value = "-"
        Cells(eRow, 20).Value = "-"
        Cells(eRow, 21).Value = "-"
        Cells(eRow, 22).Value = "-"
        Cells(eRow, 26).Value = "-"
        Cells(eRow, 27).Value = "-"
        Cells(eRow, 14).Value = "No"
        Cells(eRow, 17).Value = "No"
    End If
    Cells(eRow, 9).Value = ComboBox3.Value
    Cells(eRow, 10).Value = TextBox2.Value
    Cells(eRow, 11).Value = TextBox3.Value
    Cells(eRow, 12).Value = TextBox4.Value
    Cells(eRow, 13).Value = ComboBox4.Value
    Cells(eRow, 23).Value = ComboBox5.Value
    Cells(eRow, 24).Value = Controls("CheckBox" & 3).Caption
    Cells(eRow, 25).Value = TextBox11.Value
    Cells(eRow, 6).Value = TextBox1.Value
End If

The above code is repeated another 29 times with those three lines changed to match the corresponding checkbox.

Addendum

I have the Linked Spread Sheet - Follow Up Spread Sheet

Basically I created a userform for my sales reps to fill out with there sales calls. we have approximately 30 different product lines so I made it where they could enter one call and if they talked about more than one of our lines they could just check they box and add a comment. so in theory if they talked about all the products we offer the one call would have 30 new lines in the spreadsheet.

The macro works great and does what it needs to but the problem I am running into was with 32 bit Excel versions they would get an error saying file is to large so I broke the code into 2 which isn't very efficient.

I am trying to use loops and arrays but since I am teaching myself how to code I am running into issues.

Anywhere there is repetition, there is a function screaming to be born. What is not clear from your code is whether all the same cells are changed (i.e. eRow is the same in all 30 cases) but I will assume this is the case.

Sub HandleCheckBox(SomeText as String, OtherText as String) 'no need to return anything
    Dim tempBool as Boolean ' lousy name, but this is just to shorten code
    Dim eRow as Range
    Set eRow = Sheet3.Cells(Rows.Count, 6).End(xlUp).EntireRow.Columns.Offset(1,0)
    tempBool = CheckBox29.Value
    eRow(6) = TextBox1.Value
    eRow(7) = ComboBox1.Value
    eRow(8) = IIf(tempBool, CheckBox29.Value, "Sales Call")
    eRow(9) = ComboBox3.Value
    eRow(10) = TextBox2.Value
    eRow(11) = TextBox3.Value
    eRow(12) = TextBox4.Value
    eRow(13) = ComboBox4.Value
    eRow(14) = IIf(tempBool And CheckBox1.Value, "Yes", "No")
    eRow(15) = IIf(tempBool, ComboBox6.Value, "-")
    eRow(16) = IIf(tempBool, ComboBox7.Value, "-")
    eRow(17) = IIf(tempBool And CheckBox2.Value, "Yes", "No")
    eRow(18) = IIf(tempBool, ComboBox8.Value, "-")
    eRow(19) = IIf(tempBool, TextBox5.Value, "-")
    eRow(20) = IIf(tempBool, TextBox6.Value, "-")
    eRow(21) = IIf(tempBool, TextBox7.Value, "-")
    eRow(22) = IIf(tempBool, TextBox8.Value, "-")
    eRow(23) = ComboBox5.Value
    eRow(24) = SomeText
    eRow(25) = OtherText
    eRow(26) = IIf(tempBool, TextBox9.Value, "-")
    eRow(27) = IIf(tempBool, TextBox10.Value, "-")
End Sub

Now all that is left is to call it.

You could do this in a series of calls

'[... Your main sub ...]
If Checkbox3.Value then HandleCheckBox CheckBox3.Caption, TextBox11.Caption
If Checkbox4.Value then HandleCheckBox CheckBox4.Caption, TextBox12.Caption
'[... Rest of code ...]

Or perhaps in a series of handlers

Private Sub CheckBox3_Change()
    If Checkbox3.Value then HandleCheckBox CheckBox3.Caption, TextBox11.Caption
End Sub

Or perhaps in a loop

For j = 1 to 30
   cbNumber = j + 2
   tbNumber = j + 10
   If Controls("Checkbox" & cbNumber).Value then HandleCheckBox Controls("Checkbox" & cbNumber).Caption, Controls("TextBox" & tbNumber).Caption
Next j


Bash script that counts the characters, frequency, and percentage of letters in Spanish sonnets This script will be used to analyze a lot of old Spanish sonnets. I have tested it thoroughly and all seems to be working perfectly. However, I want to make sure I am not missing some detail because I certainly want accurate results. For the math part it does not round up if the last decimal place is above five. I asked the person I am writing it for, and he said that did not matter for his purposes.

My operating system is Debian 9. I am glad to receive any feedback or comments.

Usage:

script sonnet.txt

Sample text:

  Sancho Panza es aquéste, en cuerpo chico, pero grande en valor,
  ¡milagro extraño! Escudero el más simple y sin engaño que
  tuvo el mundo, os juro y certifico.

Output:

Carácteres          Frecuencia          Porcentaje
i                   6                   04.87%
e                   17                  13.82%
a                   10                  08.13%
o                   14                  11.38%
u                   7                   05.69%

b                   0                   0
c                   7                   05.69%
d                   3                   02.43%
f                   1                   00.81%
g                   3                   02.43%
h                   2                   01.62%
j                   1                   00.81%
k                   0                   0
l                   5                   04.06%
m                   4                   03.25%
n                   8                   06.50%
p                   4                   03.25%
q                   2                   01.62%
r                   9                   07.31%
s                   8                   06.50%
t                   4                   03.25%
v                   2                   01.62%
w                   0                   0
x                   1                   00.81%
y                   2                   01.62%
z                   1                   00.81%
ñ                   2                   01.62%
Carácteres: 27  Total: 123  Total: 100%

Script:

#!/bin/bash

# Check for argument.
if [ -z "$1" ]; then
    echo "Script needs an argument."
    exit 1
fi

# Create output file name.
fname=${1%.*}.tsv

# Clear if output file already exists.
echo -n > "$fname"

# Get all alpha characters and cut new line characters.
chars=$(grep -io '[a-z]' "$1" | tr -d '\n')

# Count total alpha characters.
chars_total=$(echo -n "$chars" | wc -m)

# Get all consonants except for ñ.
consonants=$(echo {a..z} | sed 's/[ieaou] //g')

# Set long tab length.
tabs 20

# Echo header.
echo -e "Carácteres\tFrecuencia\tPorcentaje" | tee -a "$fname"

# Get letter name. Count all instances of letters including accented letters. Calculate the percent for each letter. Echo data to table.
for ltr in 'i|í' 'e|é' 'a|á' 'o|ó' 'u|ú|ü' $consonants ñ; do
    ltr_name="${ltr:0:1}"
    ltr_count=$(echo -n "$chars" | grep -Eoi "$ltr" | tr -d '\n' | wc -m)
    ltr_percent=$(echo "scale=4; $ltr_count/$chars_total" | bc | tr -d \. | sed 's/..$/.&%/')
    echo -e "$ltr_name\t$ltr_count\t$ltr_percent"
    if [ "$ltr_name" == "u" ]; then echo | tee -a "$fname"; fi
done | tee -a "$fname"

# Echo footer.
echo -e "Carácteres: 27\tTotal: $chars_total\tTotal: 100%" | tee -a "$fname"

The right tool for the job

Nice script! But Bash is not well-suited for this kind of computation.
Bash is good as glue code,
connecting different applications, bridging the gap,
doing simple stuff.
The task at hand would have been better written in proper programming language, for example Python.

Usability

When the first argument is missing or empty,
the script exits with an error message: "Script needs an argument.".
What kind of argument? It would be better if it gave a hint, for example:

if [ $# = 0 ]; then
    echo "Usage: $0 file"
    exit 1
fi

This gives a hint that the script requires a file argument.

I also changed the condition from -z "$1" to $# = 0.
The former means "first argument is missing or empty",
the latter means "script has no arguments".
This is probably closer to your intention.

Creating empty files

A simple way to create an empty file:

> "$fname"

Don't use flags of echo

The flags of echo are not portable, for example echo -n.
Try to avoid as much as possible.
You can use printf instead.
On the other hand, echo without any flags is fine.

Counting characters

In your input I don't see multibyte characters.
If you don't need to support such characters,
then you could use native Bash syntax ${#chars} to get the length of $chars,
no need for wc -m.

Writing to "$fname"

The script passes some text to | tee -a "$fname" at multiple places:

header
loop output

inside the loop, blank line after the letter u -> this one looks like a bug

footer

Writing | tee ... multiple times is tedious and error-prone.
You could group commands to simplify:

{
    echo header ...
    for ...
    echo footer ...
} | tee -a "$fname"

Probably you can drop the -a from tee to truncate the file,
and that will also allow you to remove the manual truncation at the beginning of the script.


What is the more effective way to get array of data out of xml for d3 chart? I am writing a small application that would interactively allow user for xml file manipulation with d3 interactive charts.

My xml file has the following hierarchy :

<?xml version="1.0" encoding="UTF-8" ?>
<testcase>

  <measurement>
    <type>M8015</type>
    <interval>15</interval>
  </measurement>
  <measurement>
    <type>M8016</type>
    <interval>15</interval>
  </measurement>
  <measurement>
    <type>M8020</type>
    <interval>15</interval>
  </measurement>
   ...
</testcase>

What would be the most effective way to get the data out of xml into 2 arrays (one for x axis type, and one for y axis interval).

I have tried the following but don't know if that's a good approach.

$(function() {
  var $mydata = new Array(1);
  $.get("./testcase.xml", function(xml) {

      var $chart = d3.select("body").append("div")
          .attr("class", "chart")
          .attr("id", "chart");

      $(xml).find('measurement').each(function(){
        var $meas = $(this);
        var $type = $meas.find('type').text();
        var $interval = $meas.find('interval').text();

        $mydata.push({'type': $type, 'interval': $interval});
    });
});

Any suggestions if that's the correct way of handling this problem ?

I suggest you use JSON when manipulating the data because it's easier and less code compared to the same XML counterpart code - and much easier.

I can think of two ways you can do this:

Convert your files to JSON, if you can.
If you can't convert to JSON files, stay with what you are doing, and read the XML but convert them to JS objects before handing them over to the chart. There are XML-to-JSON libraries out there which you can use.

A similar data structure in JSON would look like

{
  "testcase" : {
    "measurement" : [
      {
        "type" : "M8015",
        "interval" : 15
      },{
        "type" : "M8016",
        "interval" : 15
      },{
        "type" : "M8020",
        "interval" : 15
      }
    ]
  }
}

And it's a matter of plucking out the data:

var myData = data.testcase.measurement;


Authentication and session creation My session controller has a method for creating new user session. According to Rubocop's output there is 'Assignment Branch Condition' metric is too high [15.17/15].

    def create
      agent = Agent.find_by(login: params[:session][:login])
      if agent && agent.authenticate(params[:session][:password])
        log_in agent
        redirect_to dashboard_url
      else
        flash.now[:danger] = 'Invalid login or password'
        render 'new'
      end
    end

log_in method:

    def log_in(agent)
      session[:agent_id] = agent.id
    end

I've extracted method for processing unsuccessful logins to reduce ABC size:

  def create
    agent = Agent.find_by(login: params[:session][:login])
    if agent && agent.authenticate(params[:session][:password])
      log_in agent
      redirect_to dashboard_url
    else
      unsuccessful_login 'Invalid login or password'
    end
  end

  private
    def unsuccessful_login(message)
      flash.now[:danger] = message
      render 'new'
    end

Is it appropriate to make new method I'm going to use only once in this case?
Is there any guideline when it's suitable to extract method?

It is not technically wrong. In OOP languages you can encounter this sort of thing (a private method apparently only used once) especially in loops. Or if you want a specific part of the algorithm to not be overrideable, etc. Generally spoken however, I see no reason to do it in your example. The code was clear and now has way more clutter.
Whenever you repeat yourself in a class, whenever you use copy paste from another class, is most oftentimes an indication that you could abstract the code to a set of smaller functions / objects.
With regards to the tool reporting a high ABC rank: those tools do not IMHO replace common sense. You should never sacrifice readability and conciseness for the sake of a better score. This is a case where your code quality tool might have too rigid settings, or perhaps there is more code in the inspected scope that is better suited or in more need of refactoring.


Converting to Base 26 in One Based mode I needed to create a simple method that converts numbers in Base 26 (string) using, as expected, letters from a to z.

But that's not all, this Base 26 needs to be One Based. Which means that there is no representation for zero absolute value and there is a representation for 26 absolute value. Let me show a example:

In Base 26 Zero Based the number 18252 will be represented like:

$$
(18252)_{10} = (BBAA)_{26}
$$

where B == 1 and A == 0. But as an One Based I need to get:

$$
(18252)_{10} = (ZYZ)_{26}
$$

where Y == 25 and Z == 26.

So I came up with this snippet:

var array = new List<int>();

var div = myNumber;
var getOne = 0;

while (div > 26)
{
    var value = div % 26;

    value -= getOne;

    if (value <= 0)
    {
        value += 26;
        getOne = 1;
    }
    else
    {
        getOne = 0;
    }

    array.Add(value);

    div = div / 26;
}

div -= getOne;

if (div > 0) array.Add(div);

var letters = array.Select(s => (char)('A' + s - 1)).Reverse().ToArray();

return new string(letters);

Now I wonder if there is a better way write this.

You could slightly simplify the code as follows:

Reduce the number of variables.
Use the LinkedList<T> (and the AddFirst method) instead of the List<T> to eliminate reversion of array.
Alternatively you can use the List<T>.Insert method to achieve the same result.

The code:

private static string ToBase26(int myNumber)
{
    var array = new LinkedList<int>();

    while (myNumber > 26)
    {
        int value = myNumber % 26;
        if (value == 0)
        {
            myNumber = myNumber / 26 - 1;
            array.AddFirst(26);
        }
        else
        {
            myNumber /= 26;
            array.AddFirst(value);
        }
    }

    if (myNumber > 0)
    {
        array.AddFirst(myNumber);
    }
    return new string(array.Select(s => (char)('A' + s - 1)).ToArray());
}


Rewriting SQL IN statement with long list of parameters These IDs seem like they should be a table as, there are several queries that use the same list of IDs.  The following needs to be refactored.

SELECT SUM(CASE WHEN s.TypeID IN(1,2,3,4,7,8,10) THEN 1 ELSE 0 END) FROM Sales s

The problem is a subquery can't be used in an aggregate function.  I'd like to write it as follows, but it causes an error:

SELECT SUM(CASE WHEN s.TypeID IN(SELECT ID FROM SaleTypes WHERE SaleType.Desc = 'US') THEN 1 ELSE 0 END) FROM Sales s

Inside your SUM, each record has a value of either 1 or 0. The value is 1 if some condition is true for that record, and 0 if false. In other words, you're really counting the records where the condition is true.

The condition appears to be, Sales.TypeID should be one of 1,2,3,4,7,8,10. And if I understood it correctly, these correspond to the ids of the SaleType records where SaleType.Desc = 'US'.

It sounds like you want to join the Sales and SaleTypes tables,
where Sales.TypeID match SaleTypes.ID,
and SaleType.Desc = 'US'.
So this query:

SELECT count(*)
FROM Sales s
JOIN SaleTypes st
ON s.TypeID = st.ID
WHERE SaleTypes.Desc = 'US'


Checking for a cache hit Please review and recommend improvements.  It looks horrible to me, difficult to read, too lengthy, and just plain old inelegant.  But I don't see the potential improvements.

private boolean useCachedReportBean(BeanReport cachedReportBean, String strRequestedReportID)
    throws ExceptionInvalidReportRequest
{
    /*  There may or may not be a cached report
     * bean, and their may or may not be a requested
     * report ID.  If both exist, and they match, use
     * the cached report bean.  If only one of
     * the two exist, use it.   Finally if neither exist,
     * throw an Exception.
     */
    if (strRequestedReportID == null || "".equals(strRequestedReportID)) {
        if (cachedReportBean == null) {
            throw new ExceptionInvalidReportRequest(
                    "The request ID is invalid and no cached report exists.");
        } else {
            /* There was no requested ID but there is a cached bean.
             * Returning true causes the cache to be used. */
            return true;
        }
    }
    /* If we are here, we know that there is a requested ID.
     * If there is no cached bean, use the request id. */
    if (cachedReportBean == null) {
        return false;
    }
    /* There is a requested ID and cached bean.  If they match, use the cached bean. */
    if (strRequestedReportID.equalsIgnoreCase(cachedReportBean.getConfigBean().getReportID())) {
        return true;
    } else {
        return false;
    }
}

Rather than nesting if-else blocks it sometimes helps to cut the problem into independent slices and deal with them one by one, starting with the fairly exceptional ones:

private boolean useCachedReportBeanRefact(BeanReport cachedReportBean,
        String strRequestedReportID) throws ExceptionInvalidReportRequest {
    // validate input arguments
    if (cachedReportBean == null
            && StringUtils.isBlank(strRequestedReportID)) {
        throw new ExceptionInvalidReportRequest(
                "The request ID is invalid and no cached report exists.");
    }

    // corner case #1
    if (cachedReportBean == null) {
        return false;
    }

    // corner case #2
    if (StringUtils.isBlank(strRequestedReportID)) {
        return true;
    }

    // after all corner cases are dealt with, just go ahead and compare the id
    return strRequestedReportID.equalsIgnoreCase(cachedReportBean
            .getConfigBean().getReportID());
}

I think the real problem is that this function does too much and is poorly named (it doesn't really use the cached report, it only checks if... a bean, if not null, has a config object whose ID is same as the other input parameter... on the condition that this other parameter is not blank either... I think?). It's not immediately obvious where it would be used and what the returned boolean would mean, it doesn't seem to translate to any real-life concept.

Making ExceptionInvalidReportRequest a checked exception feels like a bad idea, too (difficult to say without knowing the context); wouldn't unchecked IllegalArgumentException do?

Working code including unit tests: http://pastebin.com/pCXQXLee


Prime number calculator in Python 3.4.1 I have only been coding for a short time and I have written these two functions and am looking to optimise them. If anyone could point out things that could be done better it would be much appreciated.
Please Note:
This code has been reviewed/improved using the suggestions below, take a look:
# Python 3.4.1
# Function to check if a number is prime

# Takes integer, returns the factors
def findFactors(num):
    # Create variable to hold the factors and add 1 and itself (all numbers have these factors)
    factors = [1, num]

    # For each possible factor
    for i in range(2, int(num/4) + 3):
        # Check that it is a factor and that the factor and its corresponding factor are not already in the list
        if num % i == 0 and i not in factors and int(num/i) not in factors:
            # Add it and its corresponding factor to the list
            factors.append(i)
            factors.append(int(num/i))

    return factors

# Takes an integer, returns true or false
def isPrime(number):
    number = int(number)

    # Check if the only factors are 1 and itself and it is greater than 1
    if len(findFactors(number)) == 2 and number > 1:
        return True

    return False

Style

Python has a style guide called PEP 8 which is definitly worth reading and and worth following if you do not have good reasons not to. In you case, your function name for instance is not compliant to PEP8. You'll find tools online to check your code compliancy to PEP8 in a automated way if you want to.

Docstrings

There are Python docstring conventions you can find in PEP 257. Instead of

# Takes an integer, returns true or false
def isPrime(number):

You could write :

def isPrime(number):
    """Return whether an integer is prime."""

Please note: the imperative tone, the triple-quote strings under the def line, the ending period.

Do less

If your function isPrime takes an integer as a parameter, you do not need number = int(number).

Also, you can rewrite :

# Check if the only factors are 1 and itself and it is greater than 1
if len(findFactors(number)) == 2 and number > 1:
    return True

return False

as

# Check if the only factors are 1 and itself and it is greater than 1
return len(findFactors(number)) == 2 and number > 1

Different algorithm

You do not really need to find the list of all factors to know if the number is prime. You can stop as soon as a factor is found.


Forth to the past - qsort To make a long story short, I must revive my Forth. It is rusty; last time I did anything serious in Forth was 30 years ago (man, I am old). As an exercise, I translated an STL-like implementation of qsort. It works (if you want to test it, install gforth). It performs well comparing to SDCC-compiled native C (I don't have Keil license). I know where my bottlenecks are.

I am mostly interested in how readable the code is; how much did I sin against Forth spirit; if there are modern Forth coding conventions, how does this code fare.

-1 cells constant -cell
: cell- -cell + ;

: xchg ( a0 a1 -- ) 2dup @ >r @ swap ! r> swap ! ;

: unguarded_linear_insert ( last val -- )
    >r
    begin cell- dup @ dup r@ > while over cell+ ! repeat
    drop r> swap cell+ !
;

: unguarded_insertion_sort ( first last -- )
    >r
    begin dup r@ <> while dup dup @ unguarded_linear_insert cell+ repeat
    drop rdrop
;

: linear_insert ( first last val -- )
    >r over @ r@ swap <
    if
        begin 2dup <> while cell- dup @ over cell+ ! repeat
        drop r> swap !
    else
        r> unguarded_linear_insert drop
    then
;

: insertion_sort ( first last -- )
    2dup <>
    if
        >r
        dup begin cell+ dup r@ <> while 2dup dup @ linear_insert repeat
        rdrop
    then
    2drop
;

: unguarded_partition ( l f p -- cut )
    >r swap cell-
    begin dup @ r@ > while cell- repeat swap
    begin dup @ r@ < while cell+ repeat swap
    begin 2dup < while
        2dup xchg cell- swap cell+ swap
        begin dup @ r@ > while cell- repeat swap
        begin dup @ r@ < while cell+ repeat swap
    repeat
    swap drop begin dup @ r@ < while cell+ repeat
    rdrop
;

: median_of_3 ( n0 n1 n2 -- n )
    >r 2dup > if swap then
    r> 2dup > if swap then drop
       2dup < if swap then drop
;

: pivot ( l f len -- l f p )
    2 / -cell and over +          ( l f m )
    @ >r over cell- @ over @ r>
    median_of_3
;

: quicksort_loop ( l f t -- )
    begin dup >r -rot 2dup - dup r@ > while
        pivot
        >r 2dup r> unguarded_partition
        >r 2dup + r@ swap r> 2 * < if
            rot over r> recurse swap else
            dup -rot swap r> recurse then
        rot
    repeat
    rdrop 2drop 2drop
;

: quicksort ( l f )
    2dup 2 cells quicksort_loop
    dup 2 cells + dup -rot insertion_sort
    swap unguarded_insertion_sort
;

\ Testing and benchmarking

include random.fs
0 random

: shuffle ( n a -- )
    >r dup
    begin ?dup while 1- 2dup cells r@ + swap rnd swap mod cells r@ + xchg repeat
    rdrop 2drop
;

: fill-i ( n a -- )
    >r
    begin ?dup while 1- dup dup cells r@ + ! repeat
    rdrop
;

: fill-ir ( n a )
    begin 2dup ! cell+ swap 1- ?dup while swap repeat drop
;

1024 1024 * constant total-size
variable data total-size cells allocate throw data !
total-size data @ fill-i
total-size data @ shuffle
variable ssrt total-size cells allocate throw ssrt !

: exetime  utime 2>r quicksort execute  utime 2r> d- ." dtime " . cr ;

variable size 16 size !
variable logs  4 logs !

: run ( fp )
    >r
    begin total-size size @ >= while
        data @ ssrt @ size @ cells move
        ssrt @ size @ cells + ssrt @
        size @ . cr
        .s cr
        r@ hex. cr
        r@ exetime
        size @ . size @ m*/
        size @ 2 * size !
    repeat
    rdrop
;

' quicksort run
bye

I see some things that could be improved with this code.

Fix exetime

The exetime word is defined like this:

: exetime  utime 2>r quicksort execute  utime 2r> d- ." dtime " . cr ;

There are a number of problems with that.  First, the formatting is not very good.  I'd prefer to see a stack comment, for example.  Second, this should probably take a parameter so that any word can be timed (this appears to be the intent from run).  Third, that last . should be d. to display the entire delta time rather than just half of it.

: exetime  ( fp -- )
   utime 2>r  execute  utime 2r> d- ." dtime " d. cr
;

Use smaller words

Again using exetime as an example, there are actually two things done by the word as defined.  The first is that it calculates a delta time, and the second is that it prints that time.  I'd make those separate words.

Follow Forth convention

It's common to use prefixes to simplify code.  For example, everywhere ssrt is used, it's followed by @.  For that reason, I'd define and use word like these:

: @ssrt ssrt @ ;
: @size size @ ;
: !size size ! ;

Define and use common idioms

A number of cases in the code there is a sequence like this: size @ . but there is a common idiom for that and it's built in to many Forth implementations.  If it's not, it's easy to define:

: ?  ( a -- )   @ . ;

It would be used like this:  size ?.  Alternatively, one could use the prefix idiom which is associated with a particular value:

: ?size  ( -- )   size @ . ;

Eliminate unused variables

The logs variable is defined but never used.  This needlessly clutters the code.

Simplify control structure

Instead of using begin .. while .. repeat, it's often the case that one can use begin .. until instead and simplify the code.  Using the refactored smaller words as suggested above, here's what run looks like now:

: run ( fp )
    >r
    begin
        copy-array
        array-limits r@ exetime
        ." size " ?size ." dtime " d. cr
    size2x total-size > until
    rdrop
;

These are the refactored words:

: exetime  ( fp -- ) utime 2>r execute utime 2r> d- ;
: copy-array ( -- ) data @ ssrt @ size @ cells move ;
: array-limits ( -- alo ahi ) ssrt @ dup size @ cells + swap ;
: @size ( -- n ) size @ ;
: ?size ( -- ) @size . ;
: size2x ( -- n ) size @ 2 * dup size ! ;

Make sure comments don't lie

The shuffle word starts like this:

: shuffle ( n a -- )

However, that's not correct.  It should instead be:

: shuffle ( n1 n2 a -- )

Use refactoring to improve speed

The code currently contains this:

: median_of_3 ( n0 n1 n2 -- n )
    >r 2dup > if swap then
    r> 2dup > if swap then drop
       2dup < if swap then drop
;

However, in addition to being somewhat opaque, it's not as fast as it could be.  Refactoring into smaller chunks improves both readability and speed:

\ arrange top two stack values to assure n0 <= n1
: lohi ( n0 n1 -- n0 n1 ) 2dup > if swap then ;
\ arrange top three stack values to assure n0 <= n1 <= n2
: 3sort ( n0 n1 n2 -- n0 n1 n2 ) lohi >r lohi r> lohi ;
\ extract median value from top 3 items on stack
: median_of_3 ( n0 n1 n2 -- n1 ) 3sort drop nip ;

Be aware of non-standard extensions

The code uses the non-standard extensions:

 rdrop -rot utime

It is not hard to write replacements for the first two if needed:

: rdrop r> drop ;
: -rot rot rot ;

Since utime is only used in the test code, perhaps it's not as critical.  There is no standard replacement.


Retrieving Assembly without DLL Here is my code to retrieve a class from a different solution (both solutions are in the same folder) using an XML file:

var xml = XElement.Load("settings.xml");
var query = xml.Elements();
     foreach (var XMLProtocol in query)
     {
         string NameSpace = XMLProtocol.Element("NameSpace").Value;
         string ClassName = XMLProtocol.Element("ClassName").Value;
         string FrequencyType = XMLProtocol.Element("FrequencyType").Value;
         string FrequencyValue = XMLProtocol.Element("FrequencyValue").Value;
         string path = Path.GetDirectoryName(Path.GetDirectoryName(System.IO.Directory.GetCurrentDirectory()));
         System.Reflection.Assembly assembly = Assembly.LoadFrom(path + "../../" + NameSpace + "\\bin\\Debug\\" + NameSpace + ".dll");
         System.Type type = assembly.GetType(NameSpace + '.' + ClassName);
         Protocol protocol = new Protocol((IProtocol)Activator.CreateInstance(type), Convert.ToInt64(int.Parse(FrequencyValue) * GetFrequencyTypeMultiplier(FrequencyType)));
         this.Add(protocol);
     }
Execute();

This is the XML:

<Protocols>
  <Protocol>
    <NameSpace>ClassTest</NameSpace>
    <ClassName>Class1</ClassName>
    <FrequencyType>Minute</FrequencyType>
    <FrequencyValue>20</FrequencyValue>
  </Protocol>
</Protocols>

Is there a way to retrieve the assembly without having to "hardcode" the path to the DLL? What is the best practice for that scenario?

Although this feels very "examplish", here are my thoughts:

Personally, I would rethink how you handle paths currently. As it stands, the directory structure must be very specific: you are provided with some "root" folder, and in that folder you have subfolders, each named after the namespace of the library within it, then a bin folder, then a Debug folder, so an example would be:

C:\Projects\Project1\bin\Debug\Project1.dll
C:\Projects\Project2\bin\Debug\Project2.dll
C:\Projects\ClassTest\bin\Debug\ClassTest.dll
et. al.

If you just wanted to drop a bunch of files in one folder, it's currently not possible.

Why don't you, instead, add the path as a line in the XML file?

<Protocols>
  <Protocol>
    <NameSpace>ClassTest</NameSpace>
    <ClassName>Class1</ClassName>
    <FrequencyType>Minute</FrequencyType>
    <FrequencyValue>20</FrequencyValue>
    <FolderPath>../../ClassTest/bin/Debug/</FolderPath>
  </Protocol>
</Protocols>

The nicety of this is that you can now put your DLL's anywhere. You can use relative, absolute and virtual paths, and you can make them much more dynamic.

C:\Projects\Project1\bin\Debug\Project1.dll
C:\Projects\Project2\bin\Release\Project2.dll
C:\Dean Martin\ClassTest.dll
et. al.

This also means, that if you wish to use the release version of Project1, but the debug version of Project2, you can! Just include the appropriate path name!

This is generally the best-practice for this situation, as the "location" of a DLL file is a configuration aspect, rather than a programme aspect. Only in certain situations (with things like dynamic plugins) would you use a hardcoded (absolute, virtual, or relative: any of the three) path to get to the DLL's. And that would typically be something like Plugins\.

On a separate topic: I would recommend you use the @ prefix for your path strings, to be able to omit escapes:

System.Reflection.Assembly assembly = Assembly.LoadFrom(path + "../../" + NameSpace + @"\bin\Debug\" + NameSpace + ".dll");

This tends to make it more readable. Of course, if you move the file paths to the XML, it won't be a problem. :)


Making a web service call based on a type of request I have my code which makes a web service call based on a type of request:

public class Client
{
    IRequest request;

    public Client(string requestType)
    {
        request = new EnrolmentRequest();
        if (requestType == "Enrol")
        {
            request.DoEnrolment();
        }
        else if (requestType == "ReEnrol")
        {
            request.DoReEnrolment();
        }
        else if (requestType == "DeleteEnrolment")
        {
            request.DeleteEnrolment();
        }
        else if (requestType == "UpdateEnrolment")
        {
            request.UpdateEnrolment();
        }
    }

}

So as per open close principle, I can subclass like:

Class EnrolmentRequest:IRequest
{
    CallService();
}
Class ReEnrolmentRequest:IRequest
{
    CallService();
}
Class UpdateEnrolmentRequest:IRequest
{
    CallService();
}

Now my client class will look something like this:

public class Client
{
    public Client(string requestType)
    {
        IRequest request;

        if (requestType == "Enrol")
        {
            request = new EnrolmentRequest();
            request.CallService();
        }
        else if (requestType == "ReEnrol")
        {
            request = new REnrolmentRequest();
            request.CallService();
        }
        else if (requestType == "DeleteEnrolment")
        {
            request = new UpdateEnrolmentRequest();
            request.CallService();
        }
        else if (requestType == "UpdateEnrolment")
        {
            request = new UpdateEnrolmentRequest();
            request.CallService();
        }
    }

}

Now, I still have to use if and else, and will have to change my code if there are any new request type. So, it's definitely, not closed to modification.

Am I missing anything with respect to SOLID?

It would be better to give the Irequest interface an additional "Name" property that returns the value of requestType. Than you can iterate over all Irequest implementations and get the first with the corresponding name ;).

Example:

interface IRequest
{
    void CallService();
    string Name { get; }
}
class EnrolmentRequest : IRequest
{
    public void CallService() { /* impl. */ }
    public string Name => "Enrol";

}
class ReEnrolmentRequest : IRequest
{
    public void CallService() { /* impl. */ }
    public string Name => "ReEnrol";
}

...

public class Client
{
    private static IRequest[] requestHandlers = new IRequest[]
    {
        new EnrolmentRequest(),
        new ReEnrolmentRequest(),
        ...
    }

    public Client(string requestType)
    {
        IRequest request = requestHandlers.SingleOrDefault(x => x.Name == requestType);
        if (request == null) { /* Handle null case */ }

        request.CallService();
    }
}

That code fulfills the open/closed principle because you can add new request handlers without modified existing logic.


Delete all occurrences of an item in linked list This code takes an item as an argument and deletes all occurrences of the item in the linked list. It works well with my testing. Is there anything that I am missing? Can this code be improved further?

void
LinkedList::DeleteAllOccurences(int key) {
   Node *temp = head;
   Node *prev = head;
   while(temp!=NULL) {
      if(temp->item == key){
         if(temp == head) {
            head = temp->next;
            delete temp;
            temp = head;
         } else {
            prev->next = temp->next;
            delete temp;
            temp = prev->next;
         }
      } else {
         prev = temp;
         temp = temp->next;
      }
   }
   return;
}

Yes, your code can be improved by not handling the head as a special case. Yes, that means using a double-pointer, but the structure gets much more straight-forward and shorter:

void LinkedList::DeleteAllOccurences(int key) {
    Node** p = &head;
    while(*p)
        if((*p)->item == key) {
            Node* tmp = (*p)->next;
            delete *p;
            *p = tmp;
        } else
            p = &(*p)->next;
}


Next bigger number with the same digits I have to create a function that takes a positive integer number and returns the next bigger number formed by the same digits: 59853 -> 83559

Please let me know if there are any better solutions. I would like feedback how to improve this.

def next_bigger(num):
    numlist = [int(i) for i in str(num)]

    index_of_replace_num = -1
    i = len(numlist) - 1
    while i > 0:
        if numlist[i] > numlist[i-1]:
            index_of_replace_num = i - 1
            break
        i -= 1

    if index_of_replace_num == -1:
        return -1
    else:
        firstlist = numlist[:index_of_replace_num]
        replaced_num = numlist[index_of_replace_num]
        secondlist = numlist[index_of_replace_num+1:]
        new_replaced_num = 9
        i = 0
        delindex = 0
        while i < len(secondlist):
            if secondlist[i] > replaced_num and secondlist[i] < new_replaced_num:
                new_replaced_num = secondlist[i]
                delindex = i
            i += 1

        secondlist.pop(delindex)
        secondlist.append(replaced_num)
        firstlist.append(new_replaced_num)
        output = firstlist + sorted(secondlist)
        return int(''.join(str(x) for x in output))

No need to convert to int

Since for all digits '0'...'9', comparison works consistently as you would expect from integers, no need to convert to integers.
So instead of this:

numlist = [int(i) for i in str(num)]

You can simplify to:

numlist = list(str(num))

(I would also rename numlist to digits.)

Use range(...)

Instead of manually decrementing loop indexes,
for example with  i -= 1,
it's more idiomatic to use a range(...).
For example, instead of:

i = len(numlist) - 1
while i > 0:
    if numlist[i] > numlist[i-1]:
        index_of_replace_num = i - 1
        break
    i -= 1

I recommend this writing style:

for i in range(len(numlist) - 1, 0, -1):
    if numlist[i] > numlist[i-1]:
        index_of_replace_num = i - 1
        break

Use early returns to reduce nesting

Instead of this:

if index_of_replace_num == -1:
    return -1
else:
    # many many lines of code ...

It becomes a bit more readable if you drop the else statement:

if index_of_replace_num == -1:
    return -1

# many many lines of code ...

Decompose to smaller functions

Instead of putting all the logic inside the next_bigger function,
it would be good to decompose to smaller logical steps,
which can be extracted into their own functions.
Decomposing to many smaller functions increases modularity of your programs,
makes testing easier,
and often leads to reduced duplication.

Alternative implementation

Consider this alternative implementation,
with the main method decomposed to smaller logical steps that are unit tested.
You can run these doctests with the command:

python -mdoctest next_bigger.py

Empty output means all tests pass (or that there are no tests).

In addition to doctests, it's of course recommended to write proper docstrings too, which I omitted for brevity.

def swap_first_with_higher(digits):
    """
    >>> swap_first_with_higher(list('59853'))
    ['8', '9', '5', '5', '3']

    """
    for pos in range(len(digits) - 1, 0, -1):
        if digits[0] < digits[pos]:
            digits[0], digits[pos] = digits[pos], digits[0]
            break
    return digits

def reversed_tail(digits):
    """
    >>> reversed_tail(list('89553'))
    ['8', '3', '5', '5', '9']

    """
    return [digits[0]] + digits[1:][::-1]

def next_biggest(num):
    """
    >>> next_biggest(59853)
    83559

    >>> next_biggest(111)
    -1

    >>> next_biggest(11211)
    12111

    """
    digits = list(str(num))
    for pos in range(len(digits) - 1, 0, -1):
        if digits[pos-1] < digits[pos]:
            left = digits[:pos-1]
            right = reversed_tail(swap_first_with_higher(digits[pos-1:]))
            return int(''.join(left + right))

    return -1


Run-length encoding using C Recently I was asked in an interview to convert the string say "aabbbccccddddd" to "a2b3c4d5". i.e we have to avoid repeating same character and just adding the repeat count. Here 'a' is repeated twice in the input and so we have to write it as 'a2' in the output. Also i have to write a function to reverse the format (i.e from the string "a2b3c4d5" to "aabbbccccddddd") back to the original one. I was free to use either C or C++ language. I have tried the below code. But the interviewer seems to be not very happy with this. He asked me to try some smart way than this.

In the below code, I used formatstring() to eliminate repeated chars by just adding the repeated count and used reverseformatstring() to convert back to the original string.

formatstring()

void formatstring(char* target, const char* source)
{
    int charRepeatCount = 1; bool isFirstChar = true;
    while (*source != '\0')
    {
        if (isFirstChar)
        {
            //Whatever be the first character, we have to add it to the target
            isFirstChar = false;
            *target = *source;
            source++; target++;
        }else
        {
            if (*source == *(source-1))
            {
                //Comparing the current char with previous one and incrementing repeat count
                charRepeatCount++;
                source++;
            }else
            {
                if (charRepeatCount > 1)
                {
                    //Converting repeat count to string and appending it to the target
                    char repeatStr[10];
                    _snprintf(repeatStr, 10, "%i", charRepeatCount);
                    int repeatCount = strlen(repeatStr);
                    for (int i = 0; i < repeatCount; i++)
                    {
                        *target = repeatStr[i];
                        target++;
                    }
                    charRepeatCount = 1; //Reseting repeat count
                }
                *target = *source;
                source++; target++;
            }
        }
    }
    if (charRepeatCount > 1)
    {
        //Converting repeat count to string and appending it to the target
        char repeatStr[10];
        _snprintf(repeatStr, 10, "%i", charRepeatCount);
        int repeatCount = strlen(repeatStr);
        for (int i = 0; i < repeatCount; i++)
        {
            *target = repeatStr[i];
            target++;
        }
    }
    *target = '\0';
}

reverseformatstring()

void reverseformatstring(char* target, const char* source)
{
    int charRepeatCount = 0; bool isFirstChar = true;
    while (*source != '\0')
    {
        if (isFirstChar)
        {
            //Whatever be the first character, we have to add it to the target
            isFirstChar = false;
            *target = *source;
            source++; target++;
        }else
        {
            if (isalpha(*source))
            {
                //If current char is alphabet, we can simply add it to the target
                *target = *source;
                target++; source++;
            }
            else
            {
                while (isdigit(*source)) //Getting repeat count of previous character
                {
                    int currentDigit = (*source) - '0';
                    charRepeatCount = (charRepeatCount == 0) ? currentDigit : (charRepeatCount * 10 + currentDigit);
                    source++;
                }
                //Decrement repeat count as we have already written first unique char to the target
                charRepeatCount--;
                while (charRepeatCount > 0) //Repeating the last char for this count
                {
                    *target = *(target - 1);
                    target++; charRepeatCount--;
                }
            }
        }
    }
    *target = '\0';
}

I didn't find any issues with the above one. Is there any better way of doing the same?

Did you ask any questions before you started?
It seems like there are a couple of holes in the design that need firming up.

Is there a maximum run length?  (ie is single digit lengths enough)
Does the input string only contain alpha? (ie no numbers).
Do you encode single character runs? (ababab => a1b1a1b1a1b1 or ababab)

General comments
The algorithm you are using is too complex.
Some of the answers on SO show you how to simplify the algorithm so I will not repeat that here (they are all good).
Code comments
Don't put two variables on a line:
int charRepeatCount = 0; bool isFirstChar = true;

Any function that begins with an underscore is a not a good idea.
_snprintf(repeatStr, 10, "%i", charRepeatCount);

The underscore means that it is implementation defined. If there are no alternatives fine. But always prefer standard functions to non-standard implementations hacks (especially if they were done by MS). In this specific case the standard one is better designed and make usage easier.
Also why are you printing to a character array then copying to the destination array?
 _snprintf(repeatStr, 10, "%i", charRepeatCount);
 int repeatCount = strlen(repeatStr);
 for (int i = 0; i < repeatCount; i++)
 {
     *target = repeatStr[i];
     target++;
 }

You can print directly to the destination:
 sprintf(target, "%i", charRepeatCount);  // notice not snprintf because I
                                          // I don't know the size of the
                                          // target. That is comming up.
                                          // You should definately use the
                                          // snprintf() version if you can.

You have sections of repeated code (the bit above).
If you are repeating code you should break it into a separate function.
Interface design:
void formatstring(char* target, const char* source)

I have no idea how much space I am allowed to use in target. Either allow the function to allocate its own space (and return the pointer) or get a maximum size passed into the function. If you pass in a size the return value is usually the amount of space you would have needed to succeed. This allows the user to do pre-flighting to work out the size required.
int size = LokiFormatString(source, NULL, 0);
char* tareget = (char*) malloc(size + 1);
int size = LokiFormatString(source, target, size);


Recursive trim() function to handle arrays and objects I have come up with a recursive function which applies trim() recursively to all string members of an array/object.

I don't have any specific quandaries about my code so I am receptive to all feedback.

My goal is maximum compatibility for all PHP versions: past, present, and future.

<?php
/**
 * trim_r
 *
 * Recursively trim an array's or object's string values
 * Preserves all non-string values
 *
 * @access  public
 * @param   mixed
 * @param   mixed
 * @return  mixed
 */
function &trim_r( &$o, $character_mask = null )
{
    // Only apply trim() to strings
    if( is_string( $o ) )
    {
        // Respect the $character_mask; cannot pass null as 2nd parameter for some HHVM versions
        $o = ( $character_mask === null ? trim( $o ) : trim( $o, $character_mask ) );
    }
    elseif( is_array( $o ) || is_object( $o ) )
    {
        // Loop this array/object and apply trim_r() to its members
        foreach( $o as &$v )
        {
            trim_r( $v );
        }
    }

    // Supply this just in case the invoker wishes to receive result as a reference
    return $o;
}

Avoid if-else nesting to keep things flat. Return inside the first if is enough to do so.

I added is_iterable() so this function can handle more types than just array

/**
 * trim_r
 *
 * Recursively trim an array's or object's string values
 * Preserves all non-string values
 *
 * @access  public
 * @param   mixed
 * @param   mixed
 * @return  mixed
 */
function &trim_r( &$o, $character_mask = null )
{

    // Only apply trim() to strings
    if( is_string( $o ) )
    {
        // Respect the $character_mask; cannot pass null as 2nd parameter for some HHVM versions
        $o = trim( $o, $character_mask ?? " \t\n\r\0\x0B" );
        return $o;
    }

    if( is_iterable($o) || is_object( $o ) )
    {
        // Loop this array/object and apply trim_r() to its members
        foreach( $o as &$v )
        {
            trim_r( $v );
        }

    }

    // Supply this just in case the invoker wishes to receive result as a reference
    return $o;
}


How to speed up window leveling (brightness, contrast)? I have to apply window leveling on Canvas Medical Image (CR).  I have to get a pixel array from C# Component.lutArr, which is a lookup table array coming as a C# component and which I use in JavaScript. On click of the button, I have to call the function ApplyWLCust().  In this function, all process done getting result perfect. For window leveling, use bit shifting, which takes around 585ms. I have to reduce this time.  What should I do?

var imagewc = document.getElementById("text1").value;
                var imageww = document.getElementById("text2").value;
pixelArr = [11,11,11,11,11,11,11,11,11,11,11,11,11,11,11,11,11,11,11,11,11,11,11,11,11,11,11,11..]
        if (pixelArr != null)
                {
                    for (var i = 0, n = pixelArr.length; i < n; i++)
                    {
                        pix = pixelArr[i];
                        pix = ApplyWL(pix, imagewc, imageww);
                        pix = lutArr[pix];

                    // var red = (pix & 0x00ff0000) >> 16;
                    v = i * 4;
                    imagedata[v] = pix & 0xff;
                    imagedata[v + 1] = (pix >>> 8) & 0xff;
                    imagedata[v + 2] = (pix >>> 16) & 0xff;
                    imagedata[v + 3] = 255;
                }
                //  offscreenCtx.imageSmoothingEnabled = true;
                offscreenCtx.putImageData(g, 0, 0);
            }
function ApplyWL(value, _valwindowCenter, _valwindowWidth)
        {
            Recalculate(_valwindowCenter, _valwindowWidth);
            if (value <= _windowStart)
                value = _minimumOutputValue;
            else if (value > _windowEnd)
                value = _maximumOutputValue;
            else
            {
                value = Math.round((((value - _windowCenterMin05) / _windowWidthMin1) + 0.5) * 255.0);
            }
            return value;
        }

        var _minimumOutputValue = 0;
        var _maximumOutputValue = 255;
        function Recalculate(_valwindowCenter, _valwindowWidth)
        {
            if (!_valid)
            {
                _windowCenterMin05 = _valwindowCenter - 0.5;
                _windowWidthMin1 = _valwindowWidth - 1;
                _windowWidthDiv2 = _windowWidthMin1 / 2;
                _windowStart = (_windowCenterMin05 - _windowWidthDiv2);
                _windowEnd = (_windowCenterMin05 + _windowWidthDiv2);
                _valid = true;
            }
        }

Width<input id="text1" type="text" />
    center<input id="text2" type="text" />
    <input id="Button1" type="button" value="Apply" onclick="ApplyWLCust();" />
    </br> time required<input id="text3" type="text" />
    <canvas style="height: 500px; width: 500px; display1: none; background-color: black;
                border: 1px solid red;" id="offscreenCanvas">

I think we need more code,

Where are imagewc, imageww, and lutArr defined ? Those are all 3 unfortunate variable names by the way

Regardless, I think a large speed up should be gained from calling Recalculate once before the loop on pixelArr since imagewc and imageww never change in that loop.

Furthermore, since value is cut of between 0 and 255, you should pre-calculate the array of values.

var lookup= [] , i;
for( var i = _minimumOutputValue  ; i < _maximumOutputValue+1; i++ )
    lookup[i] = Math.round((((i- _windowCenterMin05) / _windowWidthMin1) + 0.5) * 255.0);

then your function would be:

function ApplyWL(value, _valwindowCenter, _valwindowWidth)
{
   if (value <= _windowStart)
     return lookup[_minimumOutputValue  ];
   if (value > _windowEnd)
      return lookup[_maximumOutputValue];
   return lookup[value]
}

I might be wrong about the ranges of lookup, your table might have to be larger, but I am sure you realize the value of a lookup table at this point.

EDIT:

I wrote some code to look at it, some notes:
* I moved the variables to the top, please make sure that all variables are declared with var
* Only constructors should start with an uppercase letter

var imagewc = document.getElementById("text1").value,
    imageww = document.getElementById("text2").value,
    _minimumOutputValue = 0,
    _maximumOutputValue = 255,
    _valid,
    cacheWL = [],
    pixelArr = [11,11,11,11,11,11,11,11,11,11,11,11,11,11,11,11,11,11,11,11,11];

if (pixelArr != null)
{
  //Move this out of the for loop
  recalculate(_valwindowCenter, _valwindowWidth);
  //Then set the cache which needs the results of `recalculate`
  cacheWL();
  for (var i = 0, n = pixelArr.length,v = 0,i < n; i++)
  {
    pix = pixelArr[i];
    pix = ApplyWL(pix, imagewc, imageww);
    pix = lutArr[pix];

    imagedata[v] = pix & 0xff;
    imagedata[v + 1] = (pix >>> 8) & 0xff;
    imagedata[v + 2] = (pix >>> 16) & 0xff;
    imagedata[v + 3] = 255;
    //Instead of doing a multiplication, you can keep adding 4
    v += 4;
  }
  offscreenCtx.putImageData(g, 0, 0);
}

function setCacheWL()
{
  for( var i = _minimumOutputValue ; i <= _maximumOutputValue ; i++ )
    cacheWL[i] = Math.round((((i - _windowCenterMin05) / _windowWidthMin1) + 0.5) * 255.0);
}

function ApplyWL(value, _valwindowCenter, _valwindowWidth)
{
   if (value <= _windowStart)
     return cacheWL[_minimumOutputValue];
   if (value > _windowEnd)
      return cacheWL[_maximumOutputValue];
   return cacheWL[value]
}

function recalculate(_valwindowCenter, _valwindowWidth)
{
  if (!_valid)
  {
    _windowCenterMin05 = _valwindowCenter - 0.5;
    _windowWidthMin1 = _valwindowWidth - 1;
    _windowWidthDiv2 = _windowWidthMin1 / 2;
    _windowStart = (_windowCenterMin05 - _windowWidthDiv2);
    _windowEnd = (_windowCenterMin05 + _windowWidthDiv2);
    _valid = true;
  }
}


Extracting matched items from an object which has properties of arrays I have a data object like that:

const data = {
  "id": ["1", "2", "3"],
  "name": ["foo", "bar", "baz"],
  "age": [20, 30, 40 ],
  "location": ["pluto", "mars", "jupiter"],
};

Those properties are matched in the array order. I need to create an array which holds all the items like:

[
  { id: '1', name: 'foo', age: 20, location: 'pluto' },
  { id: '2', name: 'bar', age: 30, location: 'mars' },
  { id: '3', name: 'baz', age: 40, location: 'jupiter' }
]

I tried many things including reduce but can't get a decent way of doing this. Here is how I'm creating this result:

let item = {};
let items = [];
const length = Object.values( data)[0].length;

for( i = 0; i < length; i++) {
  Object.entries( data ).forEach( ( [key,value] ) =>
      item = { ...item, [key]: value[i] }
  );

  items = [ ...items, item ];
}

But, the way that I am getting the length, using a for loop really bothers me.

Try this code:

function transformObject(data) {
  const values = Object.values(data);
  const keys = Object.keys(data);
  const transposed = values[0].map((col, i) => values.map(row => row[i]));
  return transposed.map(itemArr => keys.reduce((acc, key, i) => ({...acc, [key]: itemArr[i]}), {}));
}

The idea is to transpose the values 2D array and then map it using reduce for each row. The transposed array will look like:

[
  ["1", "foo", 20, "pluto"],
  ["2", "bar", 30, "mars"],
  ...
]

This piece of code keys.reduce((acc, key, i) => ({...acc, [key]: itemArr[i]}), {}) is simply zips two arrays: keys and the row from transposed array.

function transformObject(data) {
  const values = Object.values(data);
  const keys = Object.keys(data);
  const transposed = values[0].map((col, i) => values.map(row => row[i]));
  return transposed.map(itemArr => keys.reduce((acc, key, i) => ({...acc, [key]: itemArr[i]}), {}));
}

console.log(transformObject({
  "id": ["1", "2", "3"],
  "name": ["foo", "bar", "baz"],
  "age": [20, 30, 40 ],
  "location": ["pluto", "mars", "jupiter"],
}));


Add Field To PDF with Options for Multiple Pages This is related to here and here. These two methods are used to programatically add a text field to a PDF document. The field can be of any user determined size and have any user determined title, and will be on the pages specified by an enum value.

private void AddFieldToPdf(Field field,
    iTextSharp.text.pdf.PdfStamper pdfStamper, int numberOfPages)
{
    var parentField = iTextSharp.text.pdf.PdfFormField.CreateTextField(
        pdfStamper.Writer, false, false, 0);
    parentField.FieldName = field.Title;
    switch (field.Pages)
    {
        case Pages.All:
            for (var pageNumber = PdfFirstPageNumber;
                pageNumber <= numberOfPages; pageNumber++)
            {
                AddFieldToPage(field, pageNumber, pdfStamper,
                    parentField);
            }
            break;
        case Pages.Odd:
            for (var pageNumber = PdfFirstPageNumber;
                pageNumber <= numberOfPages; pageNumber+=2)
            {
                AddFieldToPage(field, pageNumber, pdfStamper,
                    parentField);
            }
            break;
        case Pages.Even:
            for (var pageNumber = PdfFirstPageNumber + 1;
                pageNumber <= numberOfPages; pageNumber += 2)
            {
                AddFieldToPage(field, pageNumber, pdfStamper,
                    parentField);
            }
            break;
        case Pages.First:
            AddFieldToPage(field, PdfFirstPageNumber, pdfStamper,
                parentField);
            break;
        case Pages.Last:
            AddFieldToPage(field, numberOfPages, pdfStamper,
                parentField);
            break;
    }
    pdfStamper.AddAnnotation(parentField, 1);
}

private void AddFieldToPage(Field field, int pageNumber,
    iTextSharp.text.pdf.PdfStamper pdfStamper,
    iTextSharp.text.pdf.PdfFormField parentField)
{
    var textField = new iTextSharp.text.pdf.TextField(
        pdfStamper.Writer, new iTextSharp.text.Rectangle(
            field.TopLeftX, field.TopLeftY, field.BottomRightX,
            field.BottomRightY), null);
    var childField = textField.GetTextField();
    parentField.AddKid(childField);
    childField.PlaceInPage = pageNumber;
}

The code on your case statements is not that different.
You could consider to group all cases that add many fields and those who only add one.

Another thing that is worth mentioning is that unless the option is
Pages.Last you always start with PdfFirstPageNumber or that +1.

Let's give a shot to reimplement the method based on this:

private void AddFieldToPdf(Field field,
    iTextSharp.text.pdf.PdfStamper pdfStamper, int numberOfPages)
{
    var parentField = iTextSharp.text.pdf.PdfFormField.CreateTextField(
        pdfStamper.Writer, false, false, 0);
    parentField.FieldName = field.Title;
    int pageNumber = field.Pages == Pages.Last ? numberOfPages : PdfFirstPageNumber;
    switch (field.Pages)
    {
        case Pages.All:
        case Pages.Odd:
        case Pages.Even:
            int increment = field.Pages == Pages.All ? 1 : 2;
            if(field.Pages == Pages.Even){
                pageNumber += 1;
            }
            for (;pageNumber <= numberOfPages; pageNumber += increment)
            {
                AddFieldToPage(field, pageNumber, pdfStamper,
                    parentField);
            }
            break;
        case Pages.First:
        case Pages.Last:
            AddFieldToPage(field, pageNumber, pdfStamper,
                parentField);
            break;
    }
    pdfStamper.AddAnnotation(parentField, 1);
}

EDIT Continuing improvements we can now see that we have two major branches, so maybe an if fits better:

 private void AddFieldToPdf(Field field,
    iTextSharp.text.pdf.PdfStamper pdfStamper, int numberOfPages)
{
    var parentField = iTextSharp.text.pdf.PdfFormField.CreateTextField(
        pdfStamper.Writer, false, false, 0);
    parentField.FieldName = field.Title;
    int pageNumber = field.Pages == Pages.Last ? numberOfPages : PdfFirstPageNumber;

    if(field.Pages == Pages.First || field.Pages  == Pages.Last){
        AddFieldToPage(field, pageNumber, pdfStamper, parentField);
    }else{
        int increment = field.Pages == Pages.All ? 1 : 2;
        if(field.Pages == Pages.Even){
            pageNumber += 1;
        }
        for (;pageNumber <= numberOfPages; pageNumber += increment)
        {
            AddFieldToPage(field, pageNumber, pdfStamper, parentField);
        }
    }
    pdfStamper.AddAnnotation(parentField, 1);
}

There is actually a way to transform that switch statement into a Linq code.

Now I am not saying this code is more clear than yours, that may depend on who is maintaining it.
But if you like Linqand would to give it a shot it works as follows:

var query = Enum.Range(PdfFirstPageNumber, numberOfPages);
if(field.Pages == Pages.Odd || field.Pages == Pages.Even){
    int parity = field.Pages == Pages.Odd ? 1 : 0;
    query = query.Where(p => p%2 == parity);
}else if(field.Pages == Pages.First){
    query = query.Take(1);
}else if(field.Pages == Pages.Last){
    query = query.Reverse().Take(1);
}
foreach(var page in query){
    AddFieldToPage(field, page, pdfStamper, parentField);
}


Find the binary gap of a number N For example, number 9 has binary representation 1001 and contains a binary gap of length 2. The number 529 has binary representation 1000010001 and contains two binary gaps: one of length 4 and one of length 3. The number 20 has binary representation 10100 and contains one binary gap of length 1. The number 15 has binary representation 1111 and has no binary gaps.

I'd like to know how this would be performance wise, and if there is a better way to do it.

public int solution(int N) {
    int binaryGap = 0;
    String binaryString = Integer.toBinaryString(N);
    char[] characters = binaryString.toCharArray();
    int j = 0;
    Character c;
    for (int i = 0; i < characters.length; i++) {
         c = characters[i];
        if (c.equals('0')) {
            j += 1;
        }

        if (c.equals('1')) {
            if (j > binaryGap ){
                binaryGap = j;
            }
            j = 0;
        }

    }
    System.out.println(binaryGap);
    return binaryGap;

}

Your current code allocates several objects on the heap even though it doesn't need to. A string, a character array and several character objects.

Puzzles like this can often be solved not by inspecting each bit individually but by processing them in parallel. An alternative approach is:

To find the binary gap of n:

Discard all trailing zeros.
As long as n does not consist of 1s only:

Combine n with n shifted to the right by one place.

The number of repetitions is the length of the largest gap.

Taking 1000010001000 as an example:

1000010001   after 0 steps
1100011001   after 1 step
1110011101   after 2 steps
1111011111   after 3 steps
1111111111   after 4 steps

It took 4 steps, therefore the binary gap is 4.

In Java, this code becomes:

public static int binaryGap(int n) {
    n >>>= Integer.numberOfTrailingZeros(n);
    int steps = 0;
    while ((n & (n + 1)) != 0) {
        n |= n >>> 1;
        steps++;
    }
    return steps;
}

As said in the above description, this code runs in \$\mathcal O(\text{gap})\$, which is a bit better than \$\mathcal O(\text{bits})\$.

It might be worth making it run in \$\mathcal O(\log_2 \text{gap})\$ by first taking 16 steps at once, then 8, then 4, then 2, then 1. If that's possible at all. For 32 bits it's entirely possible to compare all results of the optimized version against the simple code shown above. But then I guess the code will become much more complicated.


Install, configure, and maintain content for Ubuntu webserver I made a script for my Ubuntu server(s) running on Google Cloud Platform. The idea behind it is I can create a new instance and paste this script in Google Cloud so it runs it on the new instance and every time it reboots. The script works, so no problem there.

The idea is that it installs all the required software to run my website the first time it boots up and then clones my git repository to /var/www/html/. Every other time it boots it only updates the software and then pulls the changes from the git repository.

My questions:

From what I understand, it's not very wise to put your git password in the startup script but I know no other way to automate this fully. Can this be done in another way while keeping it fully automated?
Cloning the git repository gave an error about the folder not being empty so I just deleted it upfront. It works now but I am not sure if that's the way to go ...

Here is my startup script:

file="/var/www/check.txt"

if [ -e $file ]
then
sudo apt-get update
sudo git -C /var/www/html pull https://username:password@bitbucket.org/username/repository.git
else
sudo apt-get update
sudo apt-get install apache2 libapache2-mod-php5 php5-mcrypt php5-mysql git -y
sudo cat <<EOF > /etc/apache2/mods-enabled/dir.conf
<IfModule mod_dir.c>
        DirectoryIndex index.php index.cgi index.pl index.html index.xhtml index.htm
</IfModule>

# vim: syntax=apache ts=4 sw=4 sts=4 sr noet
EOF
sudo rm -rf /var/www/html
sudo git clone https://username:password@bitbucket.org/username/repository.git /var/www/html/ /var/www/html/
sudo cat <<EOF > /var/www/check.txt
aanwezig!
EOF
fi

So, this might not be quite in line with your 100% automation approach, but the best way to provide a shell script access to a git repository is to use ssh keys to allow access to your git repository. Of course, this means you either need to log on and set up that key and grant access it to Git  (goodbye 100% automation) or securely copy a private key that already has access to the repository to your server. You'll probably want to keep that key someplace that requires a password to access it, so... chicken and the egg scenario. Unless your cloud provider gives you the ability to create a template that already has that key on it's file system when it's created. If you can do that, then you definitely want to go the ssh route.

Your clone would look something like this.

git clone ssh://username@bitbucket.org/username/repository.git

Okay, let's review your code now.

You're using sudo very liberally. It would be better to set file permissions appropriately. You can run into weirdness when using sudo along with git. You can end up with a repository on the file system owned by root and.. it can get messy.
There's no error handling what so ever. If any given step of your script fails, it will continue on trying to execute the next line.
If you needed to change the location of the repository, you'd need to change the /var/www/html string in 3 different places. If your repository changes locations, it would change in 2. Use variables appropriately.


Minimizing string length checker in C# + format(ish) function I'm taking on a project from a previous programmer. My goal is to make it a bit smaller and hopefully better. Originally the code has a bunch of length checks and Booleans as shown below.

C#

string[] splitStrArray = SNString.Split('-');
string part1 = splitStrArray[0];
string part2 = splitStrArray[1];
string part3 = splitStrArray[2];

bool part1ok = false;
bool part2ok = false;
bool part3ok = false;

if (part1.Length == 3)
{
    part1ok = true;
}

if (part2.Length == 8)
{
    part2ok = true;
}

if (part3.Length == 3)
{
    part3ok = true;
}

if(part1ok && part2ok && part3ok)
{
    return true;
}
else
{
    return false;
}

Is it a good practice to keep that or should I change it to my following idea?

string[] splitStrArray = SNString.Split('-');
string part1 = splitStrArray[0];
string part2 = splitStrArray[1];
string part3 = splitStrArray[2];

if(part1.Length == 3 && part2.Length == 8 && part3.Length == 3)
{
    return true;
}
else
{
    return false;
}

Basically a barcode is scanned, and a SN is passed to the application. The goal is to make sure the SN scanned is of a format XXX-XXXXXXXX-XXX. Sometimes there are occurrences of SNs that are formatted XXXXXXXXXXXXX or XXX-XXXXXXXXXX not complete or missing a specific dash.

Is it a good practice to keep that

No

  or should I change it to my idea

Yours is better but it could be better.

The if..else could just be simplified like

return part1.Length == 3 && part2.Length == 8 && part3.Length == 3;


A helper class for creating icons from a PNG image and colour in Swift I have made a helper struct in Swift that creates circular icons from a PNG image (a white icon on transparent background) and a background UIColor. These variables are user-definable in my app, and the created icons are used in notifications and Spotlight, for example.

The PNG images are named iconA.png, iconB.png etc. and are all a standard 100-points square. The default image is a fully transparent one called iconEmpty.png. I wanted to provide a CGSize argument so that I could use this method for other purposes as well, should the need arise.

It works as expected, but I am wondering how it might be improved or made more efficient, as I am not very experienced with drawing contexts.

import UIKit

struct IconMaker {

    func makeIcon(with color: UIColor, size: CGSize, icon: UIImage = UIImage(named: "iconEmpty.png")!) -> UIImage {

        // Make solid color background
        let rect = CGRect(x: 0, y: 0, width: size.width, height: size.height)
        UIGraphicsBeginImageContextWithOptions(size, false, UIScreen.main.scale)
        color.setFill()
        UIRectFill(rect)
        let backgroundImage: UIImage = UIGraphicsGetImageFromCurrentImageContext()!
        UIGraphicsEndImageContext()

        // Add other image (which has transparency)
        UIGraphicsBeginImageContextWithOptions(size, false, UIScreen.main.scale)
        backgroundImage.draw(in: rect, blendMode: .normal, alpha: 1)

        icon.draw(in: rect, blendMode: .normal, alpha: 1)

        let newImage: UIImage = UIGraphicsGetImageFromCurrentImageContext()!
        UIGraphicsEndImageContext()

        // Circular mask
        let cornerRadius = newImage.size.height/2
        UIGraphicsBeginImageContextWithOptions(size, false, UIScreen.main.scale)
        let bounds = CGRect(origin: CGPoint.zero, size: size)
        UIBezierPath(roundedRect: bounds, cornerRadius: cornerRadius).addClip()
        newImage.draw(in: bounds)
        let finalImage = UIGraphicsGetImageFromCurrentImageContext()
        UIGraphicsEndImageContext()

        return finalImage!
    }
}

let rect = CGRect(x: 0, y: 0, width: size.width, height: size.height)

can be simplified to

let rect = CGRect(origin: .zero, size: size)

The explicit type annotation in

let backgroundImage: UIImage = UIGraphicsGetImageFromCurrentImageContext()!

is not needed. In

UIGraphicsBeginImageContextWithOptions(size, false, UIScreen.main.scale)

one can pass 0.0 as the last argument, that is also interpreted
as the scale factor of the device’s main screen.

let cornerRadius = newImage.size.height/2
UIBezierPath(roundedRect: bounds, cornerRadius: cornerRadius).addClip()

can be simplified to

UIBezierPath(ovalIn: bounds).addClip()

(and that would work even for a non-quadratic frame).

Instead of creating and drawing into a  bitmap-based graphics context
three times, with two intermediate images, it suffices to use a single graphics context, and

Add a round clipping path,
fill with the given background color,
draw the given image,

all with the same context.

Instead of the fully transparent default icon "iconEmpty.png" you
could make that parameter optional, and draw the icon only if an icon
was provided by the caller.

Finally, the method does not access any properties of the IconMaker
type. You can make it static, so that it can be called without
creating an IconMaker instance:

let newIcon = IconMaker.makeIcon(...)

Putting it all together, the method could look like this:

static func makeIcon(with color: UIColor, size: CGSize, icon: UIImage? = nil) -> UIImage {

    // Create graphics context:
    let rect = CGRect(origin: .zero, size: size)
    UIGraphicsBeginImageContextWithOptions(size, false, 0.0)

    // Circular mask
    UIBezierPath(ovalIn: rect).addClip()

    // Make solid color background:
    color.setFill()
    UIRectFill(rect)

    if let icon = icon {
        // Add other image (which has transparency)
        icon.draw(in: rect, blendMode: .normal, alpha: 1.0)
    }

    // Create new image and release context:
    let newImage = UIGraphicsGetImageFromCurrentImageContext()!
    UIGraphicsEndImageContext()
    return newImage
}

Another option is to use the defer statement for releasing
the graphics context:

static func makeIcon(with color: UIColor, size: CGSize, icon: UIImage? = nil) -> UIImage {

    // Create graphics context:
    let rect = CGRect(origin: .zero, size: size)
    UIGraphicsBeginImageContextWithOptions(size, false, 0.0)
    defer { UIGraphicsEndImageContext() }

    // ...

    return UIGraphicsGetImageFromCurrentImageContext()!
}

The block passed to the defer statement is executed just before
the function returns. Here it allows to get rid of the newImage
variable. Generally it is a good approach to ensure that all
acquired resources are released properly, no matter how program
control leaves the function.


Mongoose: find() and count() query Background

I have a query in Mongoose that finds a set of objects, and then returns these said objects together with the total number of them:

var itemsPerPage = 4, pageNum = 1;

Survey.find({})
    .limit(itemsPerPage)
    .skip(itemsPerPage * pageNum)
    .then(docs => {
        if (docs.length === 0)
            console.log("There are no results matching your query.");
        else
            DocModel.count({})
                .then(number => {
                    console.log(JSON.stringify({number, docs}));
                });
        })
        .catch(console.log);

To achieve this I do:

find query with params
use limit and skip to obtain results from correct page
if docs is not empty, start a count query
return information

Problem

The problem here, since I need pagination in the query, is that I have to make 2 separate requests to the DB (find and count) which can potential be very heavy.

To optimize this, I would like to avoid the count query, but I don't know how.

Another issue is the nested promises anti pattern, but that is not the issue I would like to focus on right now.

Question

I am open to any suggestions on improving this code you may have!

Solution

After reviewing many answers, I believe I have two options:

Make two queries, like I am doing now,
Use a pagination plugin for mongoose

My decision was the second one and so I am now using mongoose-paginate.

Hope it helps.


Three clickable navigation elements, where only one should be active I have three clickable
 elements in the nav bar of a static page I'm making as a practice project. I wrote code so when I click one, it lights up.

Since the page is static, it doesn't actually load another page, so it's just the button lighting up. When I click a different button, I want it to light up, and the previous one to turn off. I do this by adding and removing a class that alters the CSS of the element, however, I can't figure out a way of doing this more concisely than what I've already written, which adds the new class and removes any class for the other two elements. Surely there must be a more efficient way of doing this. I thought about a switch statement but I'm not quite sure how to use those yet.

function activeAbout(){
            var about = document.getElementById('about');
            about.setAttribute('class', 'playing');

            document.getElementById('poop').setAttribute('class', '');
            document.getElementById('dgech').setAttribute('class', '');
            }

        function activePoop(){
            var poop = document.getElementById('poop');
            poop.setAttribute('class', 'playing');

            document.getElementById('about').setAttribute('class', '');
            document.getElementById('dgech').setAttribute('class', '');
        }

        function activeDgech(){
            var dgech = document.getElementById('dgech');
            dgech.setAttribute('class', 'playing');

            document.getElementById('about').setAttribute('class', '');
            document.getElementById('poop').setAttribute('class', '');
        }

About

Poop

DGech

How can I refactor this more efficiently?

Edit: Added CSS code.

        #about{
            grid-column: 1;
            grid-row: ;
            justify-self: end;
            align-self: ;
            color: white;
            font-size: 60px;
            margin-top: 10px;

        }

        #poop{
            grid-column: 2;
            justify-self: center;
            color: white;
            font-size: 60px;
            margin-top: 10px;
        }

        #dgech{
            grid-column: 3;
            color: white;
            font-size: 60px;
            margin-top: 10px;
        }

        .playing{
            border-color: #ffc600;
            text-shadow: 0 0 0.5rem #ffc600;
        }

One way to trim down your code would be to make all three links trigger the same function. Inside of this function, you'll want to strip the class from each of the elements, and then assign the playing class to the element that was clicked on. This can be done by passing in the target element as a function parameter.

Also note that .attr() is a jQuery method; you're probably looking for the property className instead.

This can be seen in the following:

function active(target) {
  var elements = document.getElementsByTagName('h1');
  for (var i = 0; i < elements.length; i++) {
    document.getElementsByTagName('h1')[i].className = "";
  }
  target.className = "playing";
}

About

Poop

DGech

Hope this helps! :)


Simple packet parsing command pattern I'm currently creating a small server back-end. My goal is to write an (efficient) packet parser in C++17 using the command pattern. The packets are given by:

type: value in {0, 1, 255}
size: value in {0, 1, 65535}
data: array[size]

Each packet is processed by a packet_operator. The packet_registry is a packet_operator which simply calls the operator for the given packet type. I currently see no need for multiple, pipelined parsers for a single packet type. The code sample is given below and does compile without warnings.

packets.hpp

#pragma once

#include <array>
#include <memory>

// Test class without proper implementation.
class packet {

public:
    uint32_t m_type = 0;

    uint32_t type() const {
        return m_type;
    }

};

template<class ... ArgTypes>
class packet_operator {

public:
    virtual void operator()(const packet &, ArgTypes... args) = 0;

};

template<class ... ArgTypes>
class packet_registry : public packet_operator<ArgTypes...> {

public:
    using op_ptr = std::shared_ptr<packet_operator<ArgTypes...>>;

    explicit packet_registry(op_ptr op) {
        // Initialize registry with default processor
        for (size_t i = 0; i < m_operators.size(); i++)
            m_operators[i] = op;
    }

    void operator()(const packet &packet, ArgTypes... args) override {
        // Call processor for packet type
        (*m_operators[packet.type()])(packet, args...);
    }

    op_ptr &operator[](uint32_t i) {
        return m_operators[i];
    }

private:
    std::array<op_ptr, 256> m_operators;  // Type will always be in [0, 255]

};

main.cpp

#include <iostream>
#include <memory>

#include "packets.hpp"

class processor : public packet_operator<> {

    int m_id;

public:
    explicit processor(int id) : m_id(id) {
        // Empty
    }

    void operator()(const packet &packet) override {
        std::cout << "Processor " << m_id << ". Type: " << packet.type() << std::endl;
    }

};

void init(packet_registry<> &registry) {
    registry[1] = std::shared_ptr<packet_operator<>>(new processor(1));
    registry[2] = std::shared_ptr<packet_operator<>>(new processor(2));
}

int main() {
    // Create registry with default processor
    auto registry = packet_registry<>(
            std::shared_ptr<packet_operator<>>(new processor(0)));

    // Initialize packet processors
    init(registry);

    // Test packets with type 0, 1, 2 and 3
    packet p0, p1, p2, p3;
    p0.m_type = 0;
    p1.m_type = 1;
    p2.m_type = 2;
    p3.m_type = 3;

    registry(p0);
    registry(p1);
    registry(p2);
    registry(p3);

    return 0;
}

My main concern is my use of functors and shared pointers. Did I use those correctly? Additionally, my linter advised me to add the explicit keyword before the constructors. I've researched what it does, but do not really understand why those types of constructors would always need to be explicit.

I'd also like to know if there are any alternatives to my init method. I'd have to register the processors for every packet type here. I'd prefer some sort of self-registering processor. For example, in Java I could annotate all packet processors using a custom annotation. Those will then be added to the right registry using reflection. Maybe this is achievable through some sort of meta-programming?

Lastly, a more more general question: are there any alternative designs, given that I cannot change the protocol?

The keyword explicit should be used with constructors taking a single argument in classes when automatic instantiation is not a universally valid operation (details and examples here).

The code review part

The packet_registry can just be an array of std::function (as you seem to be OK with paying for the virtual dispatch overhead) — std::array<std::function< packet const&, ArgTypes...>, 256>
processor-s, given they are created just once, are better be created by value. Then you will be able to have a very hackable syntax — registry[x] = [...](auto&& packet, auto&& arg) { ... actual code ...};
if for some reason you do need shared pointers, prefer make_shared over direct usage of the new operator — x = std::make_shared<type>(args...).


Correct use of delegate pattern I would much appreciate your advice on this design I am going to implement. I am relatively new to object programming and I am not so sure about the delegate pattern.

I need to provide the implementation of a class that implements several interfaces of different nature. As the class would be huge I am thinking on using the delegate pattern to implement the code for each interface in a separate class. Simplifying the code a bit it goes like this:

The interfaces I have:

public interface DoorControlIF {
    public void openDoor(int id);
    //some more door operations
}

public interface DoorStatusListenerIF {
    public void onDoorOpened(int id);
}

public interface DoorAlarmListenerIF {
    public void onDoorAlarm(int id, String alarmType);
}

public interface DoorConnectionIF {
    public void connectToDoor(int id);
    public void disconnectDoor(int id);
}

public interface DoorNotifierIF {
    public void addDoorStatusListener(DoorStatusListenerIF listener);
    public void addDoorAlarmListener(DoorAlarmListenerIF listener);
}

Then the Manager class would be like:

public class DoorManager implements DoorControlIF,  DoorConnectionIF,  DoorNotifierIF{

SocketController socketController;
DoorController doorController;
DoorNotifier doorNotifier;

public void init() {
    socketController = new SocketController();
    doorController = new DoorController();
    doorController.setSocketController(socketController);
    doorNotifier = new DoorNotifier();
    doorNotifier.setSocketController(socketController);
}

public void connectToDoor(int id) { socketController.open(id); }
public void disconnectDoor(int id) { socketController.disconnect(id); }
public void openDoor(int id) { doorController.openDoor(id); }
public void addDoorStatusListener(DoorStatusListenerIF listener) { doorNotifier.addDoorStatusListener(listener); }
public void addAlarmStatusListener(DoorAlarmListenerIF listener) { doorNotifier.addDoorAlarmListener(listener); }

Then we would have all the delegate classes implement each one of  them a specific interface:

public SocketController implements DoorConnectionIF{}
public DoorController implements DoorControlIerF{}
public DoorNotifier implements DoorNotifierIF{}

Particularly I am not so sure if it is right or not having each delegate class implementing again the same interface as the the Manager class. Then there's this forwarding of listeners that are added to the Manager but inside my class I add to my specific notifier. How about passing the SocketController to each of the delegates?

I think your initial analysis into classes and interfaces is not quite right. I go by the mantra "if you can kick it, then it should be a class instance" (apologies to the purists out there). You can definitely kick a door, so there should be a class Door which will have methods for open(), close() and lock(), plus listeners for onOpen(), onClose() and onAlarm(). A constructor like Door(int id) sounds about right, allowing you to give the door a primary key.

The manager class should maintain a list of doors, and do the work required to:

route incoming commands from the socket interface to the appropriate door instance
send outgoing events from doors to the socket interface.

The Door class should not know that the socket implementation exists. Your DoorConnectionIF interface is likely superfluous.

But it is good to see someone understanding why God invented listeners. Many experienced programmers misunderstand what listeners are designed for, and end up with tangled circular references between classes.