Word VBA Code to Automate Table Captioning I have a Word VBA script that auto-generates a table caption based on user input & a previous heading. Its purpose is to speed up editing & formatting of massive software documents (like the 1000+ page test procedure I'm currently reviewing). At this point, it's slightly quicker than scrolling back up, copying the heading manually, creating a caption, and pasting it in, but it lags a little. So, I'm looking for advice on optimizing & speeding it up.

I should note that this is only for my use (which might be obvious by the less than serious user prompts) and that I'm a tech writer who dabbles in VBA,  rather than someone with a programming background.

Here's the script itself:

Sub TestTableCaption()

Dim strCaption As String
Dim intTableType As Integer
Dim strTableType As String

strTableType = InputBox("Enter 1 for Test Requirements Matrix, 2 for Test Status, or 3 for Test Steps")
intTableType = Val(strTableType)

Select Case intTableType

Case 1
    strTableType = "Test Requirements Matrix"

Case 2
    strTableType = "Test Status"
Case 3
    strTableType = "Test Steps"
Case Else
    strTableType = ""
    MsgBox ("Entry needs to be 1, 2, or 3. Learn to follow directions, then try again.")
    Exit Sub

End Select

strCaption = FindHeading("Heading 3") & " " & strTableType
strCaption = Replace(strCaption, vbCr, "")

strCaption = Replace(strCaption, Chr(150), "")
strCaption = Replace(strCaption, "  ", " ")
'In theory, Gets rid of dash from title. Doesn't actually work.

'MsgBox (strCaption)
Call GenerateCaption(strCaption, "Table")
End Sub

And here's the FindHeading function:

Function FindHeading(strHeadLevel As String) As String
'Gets the heading level as an input and finds the closest heading of that level before the selection.

Dim rngSelection As Range
'the initial selection

Dim rngPrev As Range
'The current paragraph to be evaluated.

Set rngSelection = Selection.Range

Set rngPrev = rngSelection.Previous(wdParagraph, 1)

'MsgBox (rngPrev.Text)

Do While rngPrev.Style <> strHeadLevel

    If ActiveDocument.Range(0, rngPrev.Paragraphs(1).Range.End).Paragraphs.Count > 1 Then

        Set rngPrev = rngPrev.Previous(wdParagraph, 1)
        'MsgBox ("rngPrev Style =  " & rngPrev.Style & vbCr & "Text = " & rngPrev.Text)
    Else

        'MsgBox ("Heading not found")
        Exit Do

    End If

Loop

If rngPrev.Style = strHeadLevel Then
    FindHeading = rngPrev.Paragraphs(1).Range.Text
Else
    FindHeading = "No heading found"
End If

End Function

Clean everything up

Code should be written for other people (including future you) to read and understand. A key aspect of this is using consistent rules and conventions for laying out your code.

Indent logic levels, put comments on the same line as the thing being commented (where feasible), use whitespace liberally and consistently.

Same code, much easier to "see", at a glance, what's going on:

Public Sub TestTableCaption()

    Dim strTableType As String
    strTableType = InputBox("Enter 1 for Test Requirements Matrix, 2 for Test Status, or 3 for Test Steps")

    Dim intTableType As Integer
    intTableType = Val(strTableType)
    Select Case intTableType

        Case 1
            strTableType = "Test Requirements Matrix"
        Case 2
            strTableType = "Test Status"
        Case 3
            strTableType = "Test Steps"
        Case Else
            strTableType = ""
            MsgBox ("Entry needs to be 1, 2, or 3. Learn to follow directions, then try again.")
            Exit Sub

    End Select

    strCaption = FindHeading("Heading 3") & " " & strTableType
    strCaption = Replace(strCaption, vbCr, "")
    strCaption = Replace(strCaption, Chr(150), "")
    strCaption = Replace(strCaption, "  ", " ")
    'In theory, Gets rid of dash from title. Doesn't actually work.

    'MsgBox (strCaption)
    Call GenerateCaption(strCaption, "Table")

End Sub

And now we can immediately see that your sub has 4 distinct sub-sections:

Get user input number
Convert number to the appropriate table style
Find the relevant table heading and clean the text a bit
Call GenerateCaption with the (now cleaned) caption text.

This leads us to the next key idea:

Refactor Mercilessly

Refactoring is the process of taking some set of operations, and abstracting them into their own Sub/Function. This makes your code cleaner, easier to follow, easier to modify and more re-usable.

Let's take this a step at a time.

Step 1: Get the user input string.

What are our requirements? The user needs to indicate which of 3 table types to change by inputting a number from 1 to 3.

First, a Descriptive, Unambiguous, name:

Public Function TypenumFromUser() As Long

Second, get input.

    Dim userInput As String
    userInput = InputBox("Enter 1 for Test Requirements Matrix, 2 for Test Status, or 3 for Test Steps")

Third, validate Input. Input must be a number. And it must be 1, 2 or 3. If the input is invalid, let's prompt the user to try again.

Public Function TypenumFromUser() As Long
    '/ User must input a table type as a number
    '/ Current options are 1, 2 or 3
    '/ If the input is invalid, prompt the user to try again

GetNewInput:
    Dim userInput As String
    userInput = InputBox("Enter 1 for Test Requirements Matrix, 2 for Test Status, or 3 for Test Steps")

    Dim intCheck As Long
    On Error GoTo BadUserInput
        intCheck = CLng(userInput)
    On Error GoTo 0

    Dim isValidInput As Boolean
    isValidInput = False
    If IsWholeNumber(intCheck) Then
        If intCheck >= 1 And intCheck <= 3 Then
            isValidInput = True
        End If
    End If

    If isValidInput Then
        TypenumFromUser= intCheck
    Else
        GoTo BadUserInput
    End If

CleanExit:
    Exit Function

BadUserInput:
    MsgBox "Input must be a whole number from 1 to 3. Please try again."
    GoTo GetNewInput

End Function

Public Function IsWholeNumber(ByVal checkNum As Variant) As Boolean
    '/ First check that input is a number.
    '/ Then round to the nearest whole number.
    '/ If the rounded number is the original number, then it is a whole number

    IsWholeNumber = False
    If IsNumeric(checkNum) And (Math.Round(checkNum, 0) = checkNum) Then
        IsWholeNumber = True
    End If

End Function

Sure, it's a bit verbose, but now you never need to touch this function again. And if your inputs change, you know exactly where to go to change things.

You've also now got an IsWholeNumber function which you can use in other projects down the line. You should build these utility function up over time and before long, you'l have a whole module full of useful little functions that you can import into any new project.

Now, your sub looks like this:

Public Sub TestTableCaption()

    Dim intTableType As String
    intTableType = TypenumFromUser
    Select Case intTableType

        Case 1
            strTableType = "Test Requirements Matrix"
        Case 2
            strTableType = "Test Status"
        Case 3
            strTableType = "Test Steps"
        Case Else
            strTableType = ""
            MsgBox ("Entry needs to be 1, 2, or 3. Learn to follow directions, then try again.")
            Exit Sub

    End Select

    strCaption = FindHeading("Heading 3") & " " & strTableType
    strCaption = Replace(strCaption, vbCr, "")
    strCaption = Replace(strCaption, Chr(150), "")
    strCaption = Replace(strCaption, "  ", " ")
    'In theory, Gets rid of dash from title. Doesn't actually work.

    'MsgBox (strCaption)
    Call GenerateCaption(strCaption, "Table")

End Sub

Step 2: Convert input to a table typestring

This is also begging to be refactored into a function. TypestringFromTypenum.

Public Function TypestringFromTypenum(ByVal typeNum As Long) As String

    Dim typeString As String

    Select Case typeNum

        Case 1
            typeString = "Test Requirements Matrix"
        Case 2
            typeString = "Test Status"
        Case 3
            typeString = "Test Steps"
        Case Else
            GoTo BadInput

    End Select

CleanExit:
    TypestringFromTypenum = typeString
    Exit Function

BadInput:
    MsgBox "Unexpected Input Error. See Debug."
    Debug.Print "The following should be 1, 2 or 3: " & CStr(typeNum)
    Resume CleanExit

End Function

Now, if you have to add another input, all you have to do is add 2 lines here, modify the input validation, and you don't even have to touch the rest of your code.

And now, we should roll this together with TypenumFromUser. Like so:

Public Function TypestringFromUser() As String

    Dim typeNum As Long
    typeNum = TypenumFromUser

    Dim typeString As String
    typeString = TypestringFromTypenum

    TypestringFromUser = typeString

End Function

With TypenumFromUser and TypestringFromTypenum as above.

Now your sub looks like this:

Public Sub TestTableCaption()

    Dim tableTypeString As String
    tableTypeString = TypestringFromUser

    strCaption = FindHeading("Heading 3") & " " & tableTypeString
    strCaption = Replace(strCaption, vbCr, "")
    strCaption = Replace(strCaption, Chr(150), "")
    strCaption = Replace(strCaption, "  ", " ")
    'In theory, Gets rid of dash from title. Doesn't actually work.

    'MsgBox (strCaption)
    Call GenerateCaption(strCaption, "Table")

End Sub

With the rest nicely abstracted away.

Step 3: I'm not very familiar with the Word Object Model, so I'm going to leave FindHeading alone. Though I would rename it to GetTableHeading.

Step 4: Clean heading Text.

This absolutely needs to be its' own function. CleanTableHeading

Public Function CleanTableHeading(ByVal heading As String) As String
    '/ Remove carriage returns
    '/ Remove dashes
    '/ Reduce multi-spaces to single spaces

    Dim cleanString As String

    cleanString = heading
    cleanString = Replace(cleanString, vbCr, "")
    cleanString = Replace(cleanString, "-", "")

    Dim hasMultiSpaces As Boolean
    hasMultiSpaces = (InStr(cleanString, "  ") <> 0)
    Do Until hasMultiSpaces = False
        cleanString = Replace(cleanString, "  ", " ")
        hasMultiSpaces = (InStr(cleanString, "  ") <> 0)
    Loop

    CleanTableHeading = cleanString

End Function

You ever need to add another paramater to your string cleaning? Just come straight here, change it once and you're done.

Your main sub now looks like this:

Public Sub TestTableCaption()

    Dim tableTypeString As String
    tableTypeString = TypestringFromUser

    Dim tableHeading As String
    tableHeading = FindHeading("Heading 3") & " " & tableTypeString

    Dim cleanHeading As String
    cleanHeading = CleanTableHeading(tableHeading)

    GenerateCaption cleanHeading, "Table"

End Sub

Much easier to work with.

Performance

Rule #1 of performance optimisation: Benchmarking.

I guarantee you that 95% of your operations run in a fraction of a second. There will be one sub/function/loop somewhere which is taking up the other 40 seconds. In order to fix it, you must first find it.

Simple Option: Just Debug.Print "[Position]: " & NOW() throughout your code. Like so:

Public Sub TestTableCaption()

    Debug.Print "ExecutionStart: " & Now()

    Dim tableTypeString As String
    tableTypeString = TypestringFromUser
    Debug.Print "Got TypeString From User: " & Now()

    Dim tableHeading As String
    tableHeading = FindHeading("Heading 3") & " " & tableTypeString
    Debug.Print "Found Table Heading: " & Now()

    Dim cleanHeading As String
    cleanHeading = CleanTableHeading(tableHeading)
    Debug.Print "Cleaned Table Heading: " & Now()

    GenerateCaption cleanHeading, "Table"
    Debug.Print "Generated Caption: " & Now()

End Sub

Run that, and you'll get a nice printout in the immediate window of how long each sub/function took to execute.

Once you know where the problem code is, post that as its' own separate performance question here or on Stack Overflow. (I recommend here).

That aside, some very basic VBA performance improvements:

Public Sub MainSub()

    Application.ScreenUpdating = False
    Application.EnableEvents = False
    Application.StatusBar = False

    ...

    Code

    Code

    Code

    ...

    Application.ScreenUpdating = True
    Application.EnableEvents = True
    Application.StatusBar = True

End Sub


Parse and format data on Google Sheets This is a script that I use on a Google sheet for parsing data that has been pasted from various other software and getting it into the correct format and columns on another sheet. Data is pasted into the "CL" sheet where it is parsed and formatted via formulas on the sheet.

The data is then sent to either just the "Work Log" sheet or both the "Work Log" sheet and "Represented" sheet if cell D15 is set to "Yes".

The cells that were changed on sheet "CL" are then deleted and/or changed back to the original values so another entry can be pasted.

Is there a more efficient way to setValue on each of the cells that need to be changed? Or do I need to do them one at a time, as I have them here?

function SendToWorkLogCL() {

  var ss = SpreadsheetApp.getActiveSpreadsheet();
  var sheet1 = ss.getSheetByName("CL");
  var sheet2 = ss.getSheetByName("Work Log");
  var repsheet = ss.getSheetByName("Representing");
  var rep = sheet1.getRange("D15");
  var repvalue = rep.getValue();

  if (repvalue == 'Yes') {
  sheet1.getRange("A2:W2").copyTo(repsheet.getRange(repsheet.getLastRow()+1,1,1,7), {contentsOnly:true});
  sheet1.getRange("A2:W2").copyTo(sheet2.getRange(sheet2.getLastRow()+1,1,1,7), {contentsOnly:true});
  SpreadsheetApp.getActiveSheet().getRange('A3:B23').setValue('');
  SpreadsheetApp.getActiveSheet().getRange('A3').setValue('Ctrl+SHIFT+V');
  SpreadsheetApp.getActiveSheet().getRange('D12').setValue('Chargeback');
  SpreadsheetApp.getActiveSheet().getRange('D13').setValue('Yes');
  SpreadsheetApp.getActiveSheet().getRange('D14:D15').setValue('No');
  SpreadsheetApp.getActiveSheet().getRange('D17').setValue('');
  SpreadsheetApp.getActiveSheet().getRange('D18').setValue('0');
  SpreadsheetApp.getActiveSheet().getRange('D20').setValue('');
  SpreadsheetApp.getActiveSheet().getRange('G12:G15').setValue('');
  SpreadsheetApp.getActiveSheet().getRange('D16').setFormula("=IF(A2=\"\",\"\",VLOOKUP(E18,'Data Validation'!C2:E204,3,FALSE))");

  }else{
  sheet1.getRange("A2:W2").copyTo(sheet2.getRange(sheet2.getLastRow()+1,1,1,7), {contentsOnly:true});
  SpreadsheetApp.getActiveSheet().getRange('A3:B23').setValue('');
  SpreadsheetApp.getActiveSheet().getRange('A3').setValue('Ctrl+SHIFT+V');
  SpreadsheetApp.getActiveSheet().getRange('D12').setValue('Chargeback');
  SpreadsheetApp.getActiveSheet().getRange('D13').setValue('Yes');
  SpreadsheetApp.getActiveSheet().getRange('D14:D15').setValue('No');
  SpreadsheetApp.getActiveSheet().getRange('D17').setValue('');
  SpreadsheetApp.getActiveSheet().getRange('D18').setValue('0');
  SpreadsheetApp.getActiveSheet().getRange('D20').setValue('');
  SpreadsheetApp.getActiveSheet().getRange('D16').setFormula("=IF(A2=\"\",\"\",VLOOKUP(E18,'Data Validation'!C2:E204,3,FALSE))");

 }
 }

In Google Apps Script, Less code doesn't always mean greater efficiency wrt real-time. According to best practices, focus first on reducing service calls.

To save real time, use a single .setValues() with ALL the desired content. "Javascript" operations are very fast compared to calls to Google's services.

You do have redundant code because of the structure of your if / then / else blocks; you can move repeated portions outside of that logic.

function SendToWorkLogCL() {

  var ss = SpreadsheetApp.getActiveSpreadsheet();
  var sheet1 = ss.getSheetByName("CL");
  var sheet2 = ss.getSheetByName("Work Log");
  var repsheet = ss.getSheetByName("Representing");
  var rep = sheet1.getRange("D15");
  var repvalue = rep.getValue();

  var clRange = sheet1.getRange("A2:W2");
  if (repvalue == 'Yes') {
    clRange.copyTo(repsheet.getRange(repsheet.getLastRow() + 1, 1, 1, 7), {
      contentsOnly: true
    });
  }

  // This part does not need to be in an if/then/else, because it's always done.
  clRange.copyTo(sheet2.getRange(sheet2.getLastRow() + 1, 1, 1, 7), {
    contentsOnly: true
  });

  // Only need this block once, instead of two identical copies.
  var sheet = SpreadsheetApp.getActiveSheet();
  var dataRange = sheet.getDataRange();
  var values = dataRange.getValues();

  values[3-1][ColA1ToIndex('A')] = 'Ctrl+SHIFT+V';

  // ...getRange('A3:B23').setValue('') handled in loops
  for (var col=ColA1ToIndex('A'); col <= ColA1ToIndex('B'); col++) {
    for (var row=(3-1); row<=(23-1); row++) {
      values[row][col] = '';
    }
  }
  values[12-1][ColA1ToIndex('D')] = 'Chargeback';
  values[13-1][ColA1ToIndex('D')] = 'Yes';
  values[14-1][ColA1ToIndex('D')] = 'No';
  values[15-1][ColA1ToIndex('D')] = 'No';
  values[17-1][ColA1ToIndex('D')] = '';
  values[18-1][ColA1ToIndex('D')] = '0';
  values[20-1][ColA1ToIndex('D')] = '';
  // Finally, one service call to write ALL values. Fast!
  dataRange.setValues(values);
  // Formulas would have been overwritten by values, so need to be refreshed
  sheet.getRange('D16').setFormula("=IF(A2=\"\",\"\",VLOOKUP(E18,'Data Validation'!C2:E204,3,FALSE))");
}

Helper functions: For clear conversion between A1Notation and 0-based array indices (full implementation in this gist):

/**
 * Return a 0-based array index corresponding to a spreadsheet column
 * label, as in A1 notation.
 *
 * @param {String}    colA1    Column label to be converted.
 *
 * @return {Number}            0-based array index.
 */
function ColA1ToIndex( colA1 ) {
  if (typeof colA1 !== 'string' || colA1.length > 2)
    throw new Error( "Expected column label." );

  var A = "A".charCodeAt(0);

  var number = colA1.charCodeAt(colA1.length-1) - A;
  if (colA1.length == 2) {
    number += 26 * (colA1.charCodeAt(0) - A + 1);
  }
  return number;
}

/**
 * Return a 0-based array index corresponding to a spreadsheet row
 * number, as in A1 notation.
 *
 * @param {Number}    rowA1    Row number to be converted.
 *
 * @return {Number}            0-based array index.
 */
function RowA1ToIndex( rowA1 ) {
  return rowA1 - 1;
}


What are the drawbacks of DRY input validation configuration? I have designed the following input validation implementation with one goal: DRY. I'd like to know whether it is intuitive and what are the possible shortcomings.

$vlad = new \ay\vlad\Vlad();

$test = $vlad->test([ // Test script
    [ // Group of selectors and validators. Each selector is assessed against each validator.
        [ // Selectors
            'foo',
            'bar',
            'baz'
        ],
        [ // Validators/ validation options
            ['fail' => 'break'],
            // Define Validator failure scenario:
            // * 'silent' exclude input from the current validator chain.
            // * 'soft' record an error and progress to the next Validator.
            // * 'hard' (default) record an error and exclude the selector from the rest of the Test.
            // * 'break' record an error and interrupt the Test.
            'required',
            'not_empty',
            ['fail' => 'hard'], // Reset default Validator failure scenario.
            'email',
            ['length', 'max' => 10] // Length Validator with 'max' option.
        ]
    ],
    [
        ['qux', 'quux'],
        [
            // Validate email only if it is non-empty string.
            ['fail' => 'silent'],
            'not_empty',
            ['fail' => 'hard'],
            'email'
        ]
    ],
    [
        ['password'],
        [
            // 'password' selector value must match 'password_confirm' selector
            ['match', 'selector' => 'password_confirm']
        ]
    ]
]);

$result = $test->assess($dummy_input);

The entire library documentation and examples are avaiable at http://anuary.com/vlad.

Well, since you didn't post your lib code, I'm assuming you're looking for an API-review. I hope you're not going to take this the wrong way, but I wouldn't like to use your validator class at all.
If I were you, I'd provide the users with an API that uses constants for the supported validation types, and -like I said in my comment- DRY is all well and good, but DOYOC (Do not Obfuscate Your Own Code) is more important, still.

I suggest you bash out an API that would allow me to do this:

$validator = new Validator();//basic
//or
$validator = new Validator(
    array(
        Validator::DEFAULT_FAIL => VALIDATOR::FAIL_BREAK,
        Validator::DEFAULT_VALUE => VALIDATOR::VALUE_REQUIRED,
    )
);

So pass an array with the default behaviour to the constuctor, and in case no array is passed: provide a setDefaults method + force the constructor to the strictest settings possible by default. Then the user can specify only those settings he wants to loosen up a little. But in general, when it comes to validation: the stricter the better.

Then, add methods to add what you call a selector, I'd call it an entity, which the user can then use to add a validation rule:

$validator->addEntity(
    $name,//name of entity
    Validator::TYPE_EMAIL,//validation type
    array(
        'value' => Validator::VALUE_OPTIONAL //override defaults here
    )
);

This not only makes the resulting code easier to read/maintain, but also makes it more reusable! This, too is DRY! in your case, the user will need to create a new array if he/she wants to validate things differently. I only have to do this:

$emailOptions = array('value'=>Validator::VALUE_OPTIONAL);
if ($config->requireEmail) $emailOptions['value'] = Validator::VALUE_REQUIRED;

And the rest of the code can remain as-is.
While testing, I can quickly comment out an addEntity call, or change one of them without having to go over that huge array again.

In light of this (changing rules on-the-fly): provide methods like alterRule or removeRule, too

If, in time, you get to writing form-building classes, you'll probably want your API to be able to deal with instances of a given class, and distill from it the rules required. One way to prepare your code to do so is by adding type-hints for Traversable objects.
If your API grows, it might also be worth wile considering writing Argument classes, that need to be passed to your API

I'm not going to write a full example of how I'd structure a validator class, as I've only briefly looked at your code, and I'm tired. Just thought I'd expand a little on my comment.


Modifying a random String stored in the clipboard I wrote a simple program, that will make a string 'noisy' if the clipboard contains one.
What disappoints me, is that I should manually check what I got from
getClipboardString — in the Nothing case, we simply return from program, otherwise we modify the string. Is there a better way to do this kind of check?

import Data.Char (toUpper)
import System.Random (randomIO)
import System.Clipboard (setClipboardString, getClipboardString)
import Control.Monad (join)

main :: IO ()
main = do
 join $ fmap (test doNoise) getClipboardString
 where
 test :: (String -> IO ()) -> (Maybe String) -> IO ()
 test _ Nothing = return ()
 test f (Just s) = f s

 doNoise :: String -> IO ()
 doNoise s = do
  capsed <- (sequence $ map randCap s)
  setClipboardString capsed

 randCap :: Char -> IO Char
 randCap x = fmap ($ x) $ fmap choice (randomIO :: IO Bool)

 choice :: Bool -> (Char -> Char)
 choice x = if x then toUpper else id

main :: IO ()
main = traverse_ doNoise =<< getClipboardString where
 doNoise = setClipboardString <=< traverse randCap
 randCap x = bool id toUpper <$> (randomIO :: IO Bool) <*> pure x

Edit: Each change was arrived at through pattern-matching from my experience; compact code is usually easier to work with further. Looking at this again makes me think there ought to be a more lensy solution, one that fuses getting and setting, and indeed:

main = modifyClipboardString =<<
  zipWith (bool id toUpper) <$> getRandoms

We are fortunate to not actually need side effects depending on clipboard contents, but if we did, I would recommend writing a monadic variant of modifyClipboardString.


Standardizes Rows of a Pandas.DataFrame Object Please take a look at this little snippet of code and explicate on whether there are an efficiency enhancements that you'd make for it. It standards a row of a pandas.DataFrame by adding zeros to it so that the total length of the datum is the same as the longest datum in the column

Sample "solubility.csv" file for testing

SMILES,Solubility
[Br-].CCCCCCCCCCCCCCCCCC[N+](C)(C)C,-3.6161271205000003
O=C1Nc2cccc3cccc1c23,-3.2547670983
[Zn++].CC(c1ccccc1)c2cc(C(C)c3ccccc3)c(O)c(c2)C([O-])=O.CC(c4ccccc4)c5cc(C(C)c6ccccc6)c(O)c(c5)C([O-])=O,-3.9244090954
C1OC1CN(CC2CO2)c3ccc(Cc4ccc(cc4)N(CC5CO5)CC6CO6)cc3,-4.6620645831

def generate_standard():
    dataframe = pd.read_csv('solubility.csv', usecols = ['SMILES','Solubility'])
    dataframe['standard'],longest = '',''
    for _ in dataframe['SMILES']:
        if len(str(_)) > len(longest):
            longest = str(_)
        continue
    # index,row in dataframe
    for index,row in dataframe.iterrows():
        # datum from column called 'SMILES'
        smi = row['SMILES']
        # zeros = to difference between longest datum and current datum
        zeros = (0 for x in range(len(longest) - len(str(smi))))
        # makes the zeros into type str
        zeros_as_str = ''.join(str(x) for x in zeros)
        # concatenate the two str
        std = str(smi) + zeros_as_str
        # and place it in a new column called standard at the current index
        dataframe.at[index,'standard'] = std
    return dataframe,longest

Ways to improve and optimize:

function generate_standard is named too general whereas it loads concrete csv file 'solubility.csv' and processes a particular column 'SMILES'.A more maintainable and flexible way is to make the function more decoupled and unified such that it accepts an input csv file name and the crucial column name. The main purpose can be described as "aligning a specific column's values length by the longest value".Let's give a function the appropriate name with the following signature:

def align_col_length(fname, col_name):
    """Align a specified column's values length by the longest value"""
    df = pd.read_csv(fname)
    ...

finding the longest string value in SMILES column.Instead of for loop, since you are dealing with pandas which is powerful enough and allows str accessor on pd.Series objects (points to column values in our case):

  Series and Index are equipped with a set of string processing
  methods that make it easy to operate on each element of the array.
  Perhaps most importantly, these methods exclude missing/NA values
  automatically. These are accessed via the str attribute and
  generally have names matching the equivalent (scalar) built-in string
  methods

Thus, applying a flexible chain df[col_name].str.len() + pd.Series.idxmax we would be able to get the row label/position of the maximum value.Then, just easily get the column's longest value by the row position using:

longest = df.loc[df[col_name].str.len().idxmax(), col_name]

New dataframe['standard'] column with padded values. It only remains to apply a flexible one-liner using pd.Series.str.pad routine (to pad the col_name values up to width of len(longest))

The final optimized function now becomes a more concise and pandas-flavored:

import pandas as pd

def align_col_length(fname, col_name):
    """Align a specified column's values length by the longest value"""
    df = pd.read_csv(fname)
    longest = df.loc[df[col_name].str.len().idxmax(), col_name]
    df['standard'] = df[col_name].str.pad(len(longest), side='right', fillchar='0')

    return df, longest

Testing:

df, longest = align_col_length('solubility.csv', col_name='SMILES')

print('longest SMILE value:', longest)
print('*' * 30)   # just visual separator
print(df)

The output:

longest SMILE value: [Zn++].CC(c1ccccc1)c2cc(C(C)c3ccccc3)c(O)c(c2)C([O-])=O.CC(c4ccccc4)c5cc(C(C)c6ccccc6)c(O)c(c5)C([O-])=O
******************************
                                                                                SMILES  ...                                                                                standard
0  [Br-].CCCCCCCCCCCCCCCCCC[N+](C)(C)C                                                                       ...  [Br-].CCCCCCCCCCCCCCCCCC[N+](C)(C)C000000000000000000000000000000000000000000000000000000000000000000000
1  O=C1Nc2cccc3cccc1c23                                                                                ...  O=C1Nc2cccc3cccc1c23000000000000000000000000000000000000000000000000000000000000000000000000000000000000
2  [Zn++].CC(c1ccccc1)c2cc(C(C)c3ccccc3)c(O)c(c2)C([O-])=O.CC(c4ccccc4)c5cc(C(C)c6ccccc6)c(O)c(c5)C([O-])=O  ...  [Zn++].CC(c1ccccc1)c2cc(C(C)c3ccccc3)c(O)c(c2)C([O-])=O.CC(c4ccccc4)c5cc(C(C)c6ccccc6)c(O)c(c5)C([O-])=O
3  C1OC1CN(CC2CO2)c3ccc(Cc4ccc(cc4)N(CC5CO5)CC6CO6)cc3                                                       ...  C1OC1CN(CC2CO2)c3ccc(Cc4ccc(cc4)N(CC5CO5)CC6CO6)cc300000000000000000000000000000000000000000000000000000

[4 rows x 3 columns]


Am I risking a deadlock in this manner of re-queueing up events? I have a DelayQueue in Java, which multiple threads will read from. They will then perform a task, and call a requeue method which performs some mathematical logic (no odd concurrent access), and then calls queue.put(this). I'm aware that both take and put are blocking methods on the queue, so will this risk any deadlocks?

Here's the processing code (to be run on 8 threads concurrently):

 Task task;
            try {
                task = queue.take();
            } catch (InterruptedException e) {
                logger.error("InterruptedException in futures processor trying to take from queue.");
                continue rLoop;
            }
           task.getCallback().run();
           task.requeue();

task.requeue(), again, makes a blocking call of queue.put() but the queue is unbounded and I'm not expecting any more than 10,000 actual queue elements over application lifetime, and no more than 100 or so at once.

Why are you requeueing the instance instead of completing the operation...... ? Just asking...

If you really have to reque the instance, and if there is no other locking happening around the code you have shown, then there should be no problems with deadlocks.

Once you have taken the instance from the queue, your lock on the queue is relinqished. There is nothing you can do at this point to deadlock against anything else.

When you requeue the task, it is not going to deadlock either because it is not currently locking the queue.

So, you are fine.... technically.

Your code is a little awkard though. Why do you have part of your logic outside the try/catch? It would be neater if you put it inside the try/catch.

Also, your handling of InterruptedException is not great..... The InterruptedException is a real PITA, but you have to just deal with it. The minimum you can/should do is:

Thread.currentThread().interrupt();


discord.js/node : filter().map() best method I'm learning javascript with discord.js and node and I'm always looking for the "best" code.

Is this the "best code" for my map() ? ([role] required, (role) optional)

// Get [role] (@role) args
let roles = msg.mentions.roles.filter(role => (args[2] === `<@&${role.id}>` || args[3] === `<@&${role.id}>`)).map(role => {
  nbArgs++;
  return role.id;
});

// Get text
let txtContent = msg.content.split(/ +/g).splice(nbArgs).join(' ');

Maybe this is better :

let roles = msg.mentions.roles.filter(role => (args[2] === role.toString() || args[3] === role.toString())).map(role => role.id);

And use roles.length instead of nbArgs++ :

// Get text
let txtContent = msg.content.split(/ +/g).splice(roles.length + nbArgs).join(' ');


Performance of Codility MaxCounter challenge solution I'm doing some of the Codility challenges using Ruby. The current challenge is "The MaxCounter," which is described as:

  Calculate the values of counters after applying all alternating operations: increase counter by 1; set value of all counters to current maximum.

See the link above for more details. I managed to get a solution. The performance, however, scored 0, being that some operations timed out. How can I improve my algorithm to perform better?

def solution(n, a)
  counter = (0..n-1).to_a.map{|z| z = 0}
  for value in a
    counter.map!{|x| x = counter.max} unless value <= n
    counter[value-1] += 1 unless value == n+1
  end
  counter
end

Update - my second and third attempts, still fails on performance

2nd

def solution(n, a)
  counter = (0..n-1).to_a.map{|z| z = 0}
  a.each{|x| counter.map!{|c| c = counter.max} unless x <= n; counter[x-1] +=1 unless x==n+1;}
  counter
end

3rd

def solution(n, a)
  counter = (0..n-1).to_a.map{|z| z = 0}
  a.each{|x| counter[x-1] +=1 and next unless x==n+1; counter.map!{|x| x = counter.max};}
  return counter
end

I can't speak to performance, since I don't know what codility considers acceptable, but I'll give it a go.

In terms of reviewing your code, your solutions seem to be getting more and more compact, but that doesn't necessarily help performance. Shorter code does not  equal faster code. Sure, if you can skip code, that's one thing, but just shaving off bytes of source code is unnecessary.

You certainly should not make confusing constructions like unless ... unless or and next unless. Just read that out loud, and it'll sound strange.

And I don't think there's any advantage to using the self-modifying map!. In fact, there's no reason to use map at all.

And please don't add pointless semi-colons. A nice aspect of Ruby is not having to use those darn things everywhere.

You also seem to misinterpreting how map (with or without the !) works. These are all equivalent:

 counter.map! {|x| x = counter.max}
 counter.map! {|x| counter.max}
 counter.map! {counter.max}

In other words, you setting the block parameter x does absolutely nothing useful - you don't even need the block parameter. The map method(s) only use the block's return value.

Moreover, your block gets invoked n times by map!, since n is the counter-array's length. That means that counter.max gets called n times, even though the result is the same each time! If n is large, you're looking at a lot of unnecessary work being done there. Worst case is at that all of the values in a equal n + 1, in which case you'll be calling counter.max a total of a.count * n times. And given that you're using map!, I doubt Ruby even has a chance to cache/memoize or otherwise optimize the result of counter.max, since you keep changing the array in-place.

Besides, Array provides a lot of help here, if you check the docs.

fill does what it says in the name: Fills the array with a given value (or by using a block), which is what you're trying to do with map!
and Array.new accepts a length and a seed value, so you can fill a brand new array right away. Don't muck around with mapping a range; just make an array of the right length that's filled with zeros.

Here's my take

def solution(n, a)
  counters = Array.new(n, 0) # an array of zeros (and make the var name plural, since it's an array)
  limit = n + 1 # let's just calculate this once, since it's constant

  a.each do |v|
    if v == limit
      counters.fill(counters.max) # max gets called once, and the array gets filled
    elsif v > 0 && v < limit
      counters[v-1] += 1 # just increment
    end
  end

  counters
end

It works for the sample input in Codility's example, but as mentioned I haven't gone beyond that.

Update: With a bit of manual work, you can get rid of the call to max entirely by tracking the maximum yourself:

def solution(n, a)
  counters = Array.new(n, 0)
  limit = n + 1
  maximum = 0 # the maximum counter value

  a.each do |v|
    if v == limit
      counters.fill(maximum) # use our known maximum
    elsif v > 0 && v < limit
      counter = (counters[v-1] += 1) # increment a counter and store the result
      maximum = counter if counter > maximum # use the new value as maximum, if it's higher
    end
  end

  counters
end

This will likely be faster than the first approach, especially for larger values of n. It's maybe not quite as Ruby-esque to do things "manually" like this, but it's not terribly complex either.


Date validation in Java I'm learning Java, and would really appreciate any tips on how to improve this code.

It's just a simple date validation class.

import java.util.Map;
import java.util.HashMap;

class DateValidator {
    private int day;
    private int month;
    private int year;

    private enum Months {
        JANUARY(1, 31), FEBRUARY(2, 28), MARCH(3, 31), APRIL(4, 30), MAY(5, 31), JUNE(6, 30),
        JULY(7, 31), AUGUST(8, 31), SEPTEMBER(9, 30), OCTOBER(10, 31), NOVEMBER(11, 30), DECEMBER(12, 31);

        private final int MonthNumber;
        private final int numberOfDaysInMonth;

        private static Map<Integer, Integer> map;

        private Months(int monthMnumber, int numberOfDays) {
            this.MonthNumber = monthMnumber;
            this.numberOfDaysInMonth = numberOfDays;
        }

        static {
            map = new HashMap<Integer, Integer>();

            for (Months m : Months.values()) {
                map.put(m.MonthNumber, m.numberOfDaysInMonth);
            }
        }

        public static Map<Integer, Integer> getMap() {
            return map;
        }

    }

    DateValidator(int d, int m, int y) {
            this.day = d;
            this.month = m;
            this.year = y;
    }

    private static boolean leapYear(int y) {
        return ((y % 4 == 0) && (y % 100 != 0) || (y % 400 == 0));
    }

    public boolean checkDay() {
        if (this.month == 2 && this.leapYear(this.year)) {
            Months.getMap().put(this.month, 29);
        }

        return (this.day <= Months.getMap().get(this.month));
    }
}

class Main {
    public static void main(String[] args) {
        DateValidator d = new DateValidator(31, 10, 90);

        System.out.print(d.checkDay());
    }
}

A few observations in no particular order:

I would consider redesigning the class so that it presents a static method: checkDay(day, month, year). There doesn't seem to be much benefit in requiring callers to create an instance of the class, bound to a particular date. If you do this, ensure you make your constructor private so that the class cannot be instantiated.
If you decide to stick with the current layout, then the fields day, month and year could be declared as final, since you don't adjust them at all within the class.
A minor point, but your constructor may be more readable if you use day, month and year rather than d, m and y.
Because your leapYear method is static, you should call it in a static way:

public boolean checkDay() {
  if (this.month == 2 && leapYear(this.year)) { // <----- here
    Months.getMap().put(this.month, 29);
  }

In general, you use this. more than is necessary. I guess it's a style issue, but generally code will only use this. when it's necessary to distinguish between local variables of the same name and class fields.
In your Months enum, you have a field named MonthNumber, but it should be monthNumber to match normal Java style.
You've created an enum for your month data, however this is used solely to populate a map. The rest of your code just uses the map. As a result, I would suggest you remove the enum and just stick with a map, unless you plan to extend this class in future and use the enum for other purposes. If you stick with using an enum, the map can be declared final.
It would be easier to read your leapYear method if you added some parentheses, rather than relying solely on operator precedence. You don't need the outer parentheses however:

return (y % 4 == 0) && ((y % 100 != 0) || (y % 400 == 0));

In your checkDay() method, I would avoid putting data into your map. Certainly you don't want to pollute your map if you plan to move to a static checkDay() method as discussed above. I would do something like this instead:

public boolean checkDay() {
  int daysInMonth =
      month == 2 && leapYear(year) ? 29 : Months.getMap().get(month);

  return day <= daysInMonth;
}

(Very minor...) Typically one would call your method isLeapYear rather than simply leapYear.


Performance of BFS in Python 3 How can I increase speed performance of this Python 3 programming challenge?

  Find the shortest path from one point to another (last line)

  Example:

4 4
1 2
4 1
2 3
3 1
2 4

  Output:

2

  Explanation:

4--3
| /|
|/ |
1--2

  The shortest path: 2-1-4

import sys
from queue import Queue

def bfs(graph, start, end):
    global distance
    # path var for store current path
    path = ()
    Q = Queue()
    Q.put(start)
    # visited dict, store key - visited vertices, value - path from the start
    # query - to store current queue items
    while Q.empty() is False:
        # take last element from the queue
        u = Q.get()
        path = path + (u,)
        # set it to visited and add current path
        if u == end:
            return distance[end]
        # go trough its children
        for child in graph[u]:
            if distance[child] == 0:
                # add it to queue
                Q.put(child)
                distance[child] = distance[u] + 1
    else:
        return -1

if __name__ == '__main__':
    global distance
    input = sys.stdin.read()
    data = list(map(int, input.split()))
    n, m = data[0:2]
    data = data[2:]
    adj = [[] for _ in range(n)]
    distance = [0 for _ in range(n)]
    edges = list(zip(data[0:(2 * m):2], data[1:(2 * m):2]))
    edges.sort()
    for (a, b) in edges:
        adj[a - 1].append(b - 1)
        adj[b - 1].append(a - 1)
    s, t = data[2 * m] - 1, data[2 * m + 1] - 1
    print(bfs(adj, s, t))

This code must build a graph adjacency list in format [[1][0,2][1], where list index is graph node index, and numbers are connected edges to it.

The input format is next:

3 2
1 2
2 3

where the first two numbers is the number of vertexes (3) and number of edges (2), and the next lines describe the connections through graph vertices.

1. Review

The bfs function relies on a global variable distance. This make it hard to test (you have to remember to set up the distance list before you call bfs) and means that it can't be used from multiple threads. It would be better if bfs constructed all its own data structures.
The path variable is a tuple of all the vertices that have been visited so far. Adding a new element to the path, as in path = path + (u,), requires copying out the old path. This makes the overall runtime \$\Omega(n^2)\$. But path is not actually used for anything, so you could just remove it.
queue.Queue is a thread-safe data structure intended for use by multi-threaded programs. It has to take and release a lock for every operation, so it is overkill to use this in a single-threaded program. It is more than ten times faster to use collections.deque instead.
The code handles the exceptional situation (no path is found) by returning an exceptional value (−1). This is an error-prone design because it would be very easy for the caller to forget to check for the exceptional value. It would be easy to write code like this:

dist = bfs(graph, start, end)
if dist <= goal:
    print("reachable in {} steps or fewer".format(goal))

which would go wrong when dist is -1. It's better to handle an exceptional situation by raising an exception.
The code checks to see whether a vertex is equal to end after removing it from the queue. But it would be better to do this check before adding a vertex to the queue — that way you wouldn't have to wait for the vertex to move all the way up the queue before you spot that the search is done.

This optimization works for this problem because all the edges have the same length and so breadth-first search adds vertices to the queue in order by their distance from the start. This means that as soon as you've found a path to end you know that more searching cannot find a shorter path.
The value distance[u] + 1 is computed again for every child. It would be better to compute it once and remember it.

2. Revised code

from collections import deque

class NotFound(Exception):
    """Exception raised when no path is found."""

def bfs(graph, start, end):
    """Return the shortest distance from start to end in graph, which is
    represented as a mapping from a vertex to a list of adjacent
    vertices. If there is no path from start to end, raise NotFound.
    """
    # Mapping from v to shortest distance from start to v.
    distance = {start: 0}
    queue = deque([start])
    while queue:
        u = queue.popleft()
        d = distance[u] + 1
        for neighbour in graph[u]:
            if neighbour not in distance:
                if neighbour == end:
                    return d
                distance[neighbour] = d
                queue.append(neighbour)
    else:
        raise NotFound()


Bitmask, How to abstract those callbacks in this implementation I've made an POC about a bitmask implementation in order to replace a huge if/else statement.

Those statements are about strings and I have almost 15 /20 string to test. Depends on values the if/else statement was about 1500 lines of code.

After refactoring I was able to have only 20 objects and a map with all my callbacks in a "handler" class.

The code is working well, but I'm sure it can be improved.

<?php
declare(strict_types=1);

namespace App\Api\Application\Service;

abstract class AbstractValue implements BitwiseCallback
{
    protected $flag;
    public static function getFlag($value): BitwiseCallback
    {
        $class = get_called_class();
        $var = new $class();
        $var->bitValue = $value ? $var->flag : 0;
        return $var;
    }
    protected $bitValue;
    public function bitValue(): int
    {
        return $this->bitValue;
    }
}

<?php
declare(strict_types=1);
namespace App\Api\Application\Service;

interface BitwiseCallback
{
    static function getFlag($value): BitwiseCallback;
    public function bitValue(): int;
}

<?php
declare(strict_types=1);
namespace App\Api\Domain\ValueObject;

use App\Api\Application\Service\VarA;
use App\Api\Application\Service\VarB;
use App\Api\Application\Service\VarC;
use App\Api\Application\Service\VarX;

/**
 * Object Calisthenics or how to use lookup map and biwise map to refactor a huge if statement.
 * Class Test
 * @package App\Api\Domain\ValueObject
 */
class Test
{
    public function aMethod($values)
    {
        $a = VarA::getFlag($values[0]);
        $b = VarB::getFlag($values[1]);
        $c = VarC::getFlag($values[2]);

        $x = VarX::getFlag($values[5]);
// and so on

        $flags = $a->bitValue() | $b->bitValue() | $c->bitValue();
        $map = [
            1 => function() use ($a) { $a->doStuff(); },
            3 => function() use ($a, $b) { return $a->doStuff() . $b->doOtherStuff() . $a->toStuffWithB($b);},
            5 => function() use ($a, $c) { return $c->doStuff() . $b->doOtherStuff() . $c->toStuffWithB($b);},

            // and so on
            35 => function() use ($a, $b, $x) { return $x->sendEmailToAdmin($a, $b) ;}
            // and so on
        ];
        $result = $map[$flags];
        return $result();
    }
}

VarA / VarB / VarC/ and so on are like this

<?php
declare(strict_types=1);
namespace App\Api\Application\Service;
class VarA extends AbstractValue
{
    protected $flag = 1;

    public function doStuff()
    {
        return "This is a correct answer ...";
    }

    public function toStuffWithB($b)
    {
        return ' and you earned a bonus point !';
    }
}

<?php
declare(strict_types=1);
namespace App\Api\Application\Service;

use App\Api\Application\Service\VarA;
use App\Api\Application\Service\VarB;

class VarX extends AbstractValue
{
    protected $flag = 32;

    public function sendEmailToAdmin(VarA $a, VarB $b)
    {
// do stuff with varB and varA because VarX has been triggered
        return "We correctly aknowledge your answer, you'll be notify soon by our customer services";
    }
}

I can't show the real code with the proper names but the implementation remains the same. But' the whole thing is about a quiz with a custom baseline to show depends on answers, each stuff to do are very different depends on the bits (store data to a file / send notification to admin / send a mail about an answer given and so on).

So basically my class that handle the callback map is quite huge.

$map = [
            1 => function() use ($a) { $a->doStuff(); },
            3 => function() use ($a, $b) { return $a->doStuff() . $b->doOtherStuff() . $a->toStuffWithB($b);},
            5 => function() use ($c, $b) { return $c->doStuff() . $b->doOtherStuff() . $c->toStuffWithB($b);}
            // and so on
        ];

this part is quite ugly (around 200 lines of code in my code) with a lot of context and $options.

As implied from the comments under OP, it means that VarA is completly unrelated to VarB, they do completly different things.

If that is true, then it is all entirely obscured way of saying that based on some integer you need to do some arbitrary stuff. Arbitrary except that in all cases it returns a string.

Then all you need is encapsulate each callback in an interface:

interface StringGenerator
{
  public function getTheString(): string;
}

Whether each implementation has the right dependencies that arise from bits that are set in the flags which drive which StringGenerator implementation is used is an implementation detail.

class GeneratorABX implements StringGenerator
{
   private $serviceA;
   private $serviceB;
   private $serviceX;

   public function __construct($serviceA, $serviceB, $serviceX)
   {
     $this->serviceA = $serviceA;
     $this->serviceB = $serviceB;
     $this->serviceX = $serviceX;
   }

   public function getTheString(): string
   {
      return $this->serviceA->doStuff() . $this->serviceB->doOtherStuff() . $this->serviceX->toStuffWithB($this->serviceB);
   }
}

Use a DI container to handle the instantiation of all the implementations of StringGenerator.

function setupContainer(): \Psr\Container\ContainerInterface
{
  $container = new ContainerBuilder();
  $container->addService('serviceA', ServiceA::class);
  $container->addService('serviceB', ServiceB::class);
  $container->addService('serviceX', ServiceX::class);
  //...
  $prefix = 'stringGenerator';
  $container->addService($prefix . '1', GeneratorA::class);
  $container->addService($prefix . '3', GeneratorAB::class);
  $container->addService($prefix . '5', GeneratorAC::class);
  $container->addService($prefix . '35', GeneratorABX::class);

  $container->addService('test', Test::class, ['prefix' => $prefix]);
  return $container->buildContainer();
}

To compute flags from array of bools you can use this:

private static function getFlags(bool ...$values): int
{
  $flags = 0;
  $flag = 1;
  foreach ($values as $value) {
    if ($value) {
      $flags |= $flag; // or $flags += $flag;
    }
    $flag = $flag << 1; // or $flags *= 2;
  }
  return $flags;
}

And to get the right generator:

private function getGenerator($flags): StringGenerator
{
  return $this->container->get($this->prefix . $flags);
}

Final result:

class Test
{
  private $container;
  private $prefix;

  public function __construct(\Psr\Container\CotainerInterface $container, string $prefix)
  {
     $this->container = $container;
     $this->prefix = $prefix;
  }

  public function aMethod($values)
  {
     $flags = self::getFlags(...$values);

     return $this->getGenerator()->getTheString();
  }

  // and the private methods mentioned earlier
}

And call it like this:

$container = setupContainer();
$result = $container->get(Test::class)->aMethod($values);

EDIT: Altogether it is a bit analogous to a router. Router is first setup with a bunch of routes. Upon request, the router choses which controller is to be invoked based on the request and the set of routes. Then the appropriate controller is invoked and response is returned to the caller. The Test class is the router. The setup is done via DI container. The StringGenerators are the controllers. The flags are the request. And the resulting string is the response.

EDIT2: To avoid having a lot of StringGenerator implementations, we can instead have one method per each of the generators. Let me show a way how to do that:

class Test
{
  private $map = [
    1 => ServiceA::class,
    2 => ServiceB::class,
    32 => ServiceX::class,
    // ...
  ];

  private $container;

  public function __construct(\Psr\Container\ContainerInterface $container)
  {
    $this->container = $container;
  }

  public function aMethod(array $values): string
  {
    $services = $this->getServices(...$values);
    return $this->getTheString($services);
  }

  private function getServices(bool ...$values): array
  {
    $services = [];
    $flag = 1;
    foreach ($values as $value) {
      if ($value) {
        $services[$flag] = $this->container->get($this->map[$flag]);
      }
      $flag = $flag << 1; // or $flags *= 2;
    }
    return $services;
  }

  private function getTheString(array $services): string
  {
    $flags = \array_sum(\array_keys($services));
    $method = 'generate' . $flags;
    if (!method_exists($this, $method)) {
      throw new Exception('invalid combination of flags ' . $flags);
    }
    return $this->$method(...array_values($services));
  }

  private function generate1(ServiceA $a): string {}
  private function generate2(ServiceB $b): string {}
  private function generate35(ServiceA $a, ServiceB $b, ServiceX $x): string {}
}

function setupContainer(): \Psr\Container\ContainerInterface
{
  $container = new ContainerBuilder();
  $container->addService(ServiceA::class);
  $container->addService(ServiceB::class);
  $container->addService(ServiceX::class);
  //...
  return $container->buildContainer();
}

$container = setupContainer();
$test = new Test($container);
$result = $test->aMethod($values);

Or the same in a more general, more SOLID, although more verbose version:

interface ServiceProviderInterface
{
  public function getServices(bool ...$values): array;
}

class Services implements ServiceProviderInterface
{
  private $map = [
    1 => ServiceA::class,
    2 => ServiceB::class,
    32 => ServiceX::class,
    // ...
  ];

  private $container;

  public function __construct(\Psr\Container\ContainerInterface $container)
  {
    $this->container = $container;
  }

  public function getServices(bool ...$values): array
  {
    $services = [];
    $flag = 1;
    foreach ($values as $value) {
      if ($value) {
        $services[$flag] = $this->container->get($this->map[$flag]);
      }
      $flag = $flag << 1; // or $flags *= 2;
    }
    return $services;
  }
}

interface StringGeneratorInterface
{
  public function getTheString(array $services): string;
}

class Methods
{
  public function generate1(ServiceA $a): string {}
  public function generate2(ServiceB $b): string {}
  public function generate35(ServiceA $a, ServiceB $b, ServiceX $x): string {}
}

class StringGenerator implements StringGeneratorInterface
{
  private $methods;

  public function __construct(object $methods)
  {
    $this->method = $methods;
  }

  public function getTheString(array $services): string
  {
    $flags = \array_sum(\array_keys($services));
    $method = 'generate' . $flags; // could be a method name inflector responsibility
    if (!method_exists($this->methods, $method)) {
      throw new Exception('invalid combination of flags ' . $flags);
    }
    return $this->methods->$method(...array_values($services));
  }
}

class Test
{
  private $services;
  private $generator;

  public function __construct(ServiceProviderInterface $services, StringGeneratorInterface $generator)
  {
     $this->services = $services;
     $this->generator = $generator;
  }

  public function aMethod(array $values): string
  {
    $services = $this->services->getServices(...$values);
    return $this->generator->getTheString($services);
  }
}

function setupContainer(): \Psr\Container\ContainerInterface
{
  $container = new ContainerBuilder();
  $container->addService(ServiceA::class);
  $container->addService(ServiceB::class);
  $container->addService(ServiceX::class);
  //...
  $container->addService(Services::class);
  $container->addService(Methods::class);
  $container->addService(StringGenerator::class, ['methods' => Methods::class]);
  $container->addService(Test::class);
  return $container->buildContainer();
}

$container = setupContainer();
$result = $container->get(Test::class)->aMethod($values);
```


Datagrid not refreshing correctly with Async Ping VB .Net I have an application coded in VB.NET that has a bunch of servers in a DataGridView and does a continuous asynchronous ping.  If all the servers are up it refreshes great, but if one goes down and starts to time out it takes about 5-10 seconds before the program starts responding again.  This program needs to ping all the servers at the same time.
I need help with improving the performance of this code.

Private Sub cmdStartPing_Click(ByVal sender As System.Object, ByVal e As System.EventArgs) Handles cmdStartPing.Click

    Dim rc, myic As Integer
    Dim sname As String
    'Dim preply As PingReply

    pinging = 1
    cmdStopPing.Select()
    rc = DGV1.RowCount - 1

    If rc > -1 Then

        bCancel = False

        Do Until bCancel = True

            For myic = 0 To rc

                Application.DoEvents()

                If bCancel Then
                    bCancel = True
                    DGV1.BackgroundColor = Color.White
                    MsgBox("Pinging Stopped")
                    Exit For

                End If

                Try
                    sname = DGV1.Item(0, myic).Value

                    PingHost(sname)

                    If pingresults = "Success" Then
                        DGV1.Rows(myic).Cells(1).Value = "Success"
                        DGV1.Rows(myic).Cells(2).Value = roundtriptime
                        DGV1.Rows(myic).DefaultCellStyle.BackColor = Color.YellowGreen
                        DGV1.Refresh()
                    Else
                        DGV1.Rows(myic).Cells(1).Value = "No Reply"
                        DGV1.Rows(myic).Cells(2).Value = "Timed Out"
                        DGV1.Rows(myic).Cells(3).Value = currentdt
                        DGV1.Rows(myic).DefaultCellStyle.BackColor = Color.Red
                        DGV1.Refresh()
                    End If

                Catch ex As Exception
                    DGV1.Rows(myic).Cells(1).Value = "No Reply"
                    DGV1.Rows(myic).Cells(2).Value = "Timed Out"
                    DGV1.Rows(myic).Cells(3).Value = currentdt
                    DGV1.Rows(myic).DefaultCellStyle.BackColor = Color.Red
                    DGV1.Refresh()

                End Try

            Next

        Loop

    Else
        MsgBox("Please add at least one host to the datagrid to ping.")
    End If

End Sub

Stop ping:

Private Sub cmdStopPing_Click(ByVal sender As System.Object, ByVal e As System.EventArgs) Handles cmdStopPing.Click
    bCancel = True
    pinging = 0
End Sub

Uses this function to ping:

Imports System.Net.NetworkInformation
Module AsyncPingHost
'Function PingIPAdress(ByVal IPAdress As String)
Function PingHost(ByVal host As String)
    Dim ping As Ping
    Dim preply As PingReply
    ping = New Ping

    Try
        preply = ping.Send(host)
        roundtriptime = preply.RoundtripTime
        If preply.Status = IPStatus.Success Then
            pingresults = "Success"
        Else
            pingresults = "Failed"

        End If
    Catch ex As Exception
        pingresults = ex.Message
    End Try

End Function

I have a similar problem (but I have to use PSLoggedOn.Exe which times out in 10-15 seconds).

To mitigate the problem I use WMI to Ping.

I will then do PSLoggedOn only if the result was a valid IPnumber.

WmiPingStatus is perhaps a little less reliable because it is faster.
But if You don't get an answer you will get an answer the next loop.

    Friend Function WmiPingStatus(computer As String) As String
    Dim searcher As New ManagementObjectSearcher("SELECT ProtocolAddress FROM Win32_Pingstatus WHERE Address = '" & computer & "'")
    For Each wmiObj As ManagementObject In searcher.Get()
        For Each prop As PropertyData In wmiObj.Properties
            If prop.Name.Equals("ProtocolAddress", StringComparison.InvariantCultureIgnoreCase) Then Return wmiObj(prop.Name.ToString).ToString
        Next
    Next
    Return ""
End Function

Your function PingHost is not async but You can do it async if You use :

Await preply = SendAsync(host, Object)

But that is only part of the problem becasue You still have to wait for the answer.
Async is allways good because it will use less resources when Waiting.
And while You wait you could perhaps do for example 5 Task in "Parallell".
But then You still have to wait for the longest query to end.
So perhaps WmiPingStatus could be useful also for You


Reading bytes from packet I have a device I connect to on TCP socket. I send it a message with what I want and it sends back all sorts of useful information:

Header:

+---------------+----------+----------+
| MagicWord     | 2 bytes  | 0xCAFE   |
| Version       | 1 byte   | 0x02     |
| Type          | 1 byte   | Variable |
| UTC offset    | 1 byte   | Variable |
| Reserved      | 1 byte   | Variable |
| PayloadSize   | 2 bytes  | Variable |
+---------------+----------+----------+

Payload:

+---------------+----------+----------+
| StartTime     | 4 bytes  | Variable |
| Duration      | 4 bytes  | Variable |
| Temp          | 2 bytes  | Variable |
| EstimatedDist | 2 bytes  | Variable |
| NumKnocks     | 1 byte   | Variable |
| Reserved      | 1 byte   |          |
+---------------+----------+----------+

Here is my attempt at how I got the variables from the message:

var location:[(String, [UInt8], Int)] = []

//Name, message, location in data

let dict  = [
    ("startTime", [UInt8](), 8),
    ("duration", [UInt8](), 12),
    ("Temp", [UInt8](), 16),
    ("EstimatedDist", [UInt8](), 18),
    ("NumKnocks", [UInt8](), 20),
    ("Reserverd", [UInt8](), 21)]

for item in dict {
    location.append(item)
}

var i = 0
var itemLen = Int()
for item in location {
    if 0...4 ~= i {

        //find length of byte by length subtraction of item itself from item above.
        itemLen = location[i + 1].2 - location[i].2
    }
    let typeRange = NSRange(location: location[i].2, length: itemLen)
    let typeData = data.subdataWithRange(typeRange)
    var arr = Array(UnsafeBufferPointer(start: UnsafePointer<UInt8>(typeData.bytes), count: typeData.length))
    location[i].1 = arr
    i = i + 1
}

0000   fe ca 02 30 00 00 0e 00 4e 5b a2 56 12 19 00 00
0010   22 00 12 19 07 00

In my situation data is NSData from a GCDAsyncSocket response with the header removed.

For testing i've been doing and connecting to port.

echo -e "\xfe\xca\x02\x30\x00\x00\x0e\x00\x4e\x5b\xa2\x56\x12\x19\x00\x00\x22\x00\x12\x19\x07\x00" | nc -kl -c 1975

I can connect, send the packet, get the message back thats all fine. I can get the variables from the packet. However, I don't feel it's that elegant. I want to know what I should be looking at to try to improve. Examples (with explanations) would be great.

One things for certain... an array of tuples which is basically being used as a glorified dictionary is never going to suffice as an acceptable data model.  Moreover, an array of bytes isn't really that much useful than what we start with.

So what's clear, we need an actual data model.  So names and data types may need some twiddling, but here's what I started with:

class Information {
    let startTime: UInt32
    let duration: UInt32
    let temperature: UInt16
    let estimatedDistance: UInt16
    let knockCount: UInt8
    let reserved: UInt8

    // TODO: implement initializers
}

This is the model for our data.  This is the easiest way for the entire rest of our app to access the data.  This will be way easier to use.

All that's left is getting from our data packet (<4e5ba256 12190000 22001219 0700>) to an actual instantiated Information object.

I know that we can get from an NSData object to an array of UInt8s, but I notice, we're going to need some UInt16s and some UInt32s.  So, I extended UInt16 and UInt32 to include some constructors for taking an array of bytes:

extension UInt16 {
    init?(bytes: [UInt8]) {
        if bytes.count != 2 {
            return nil
        }

        var value: UInt16 = 0
        for byte in bytes.reverse() {
            value = value << 8
            value = value | UInt16(byte)
        }
        self = value
    }
}

extension UInt32 {
    init?(bytes: [UInt8]) {
        if bytes.count != 4 {
            return nil
        }

        var value: UInt32 = 0
        for byte in bytes.reverse() {
            value = value << 8
            value = value | UInt32(byte)
        }
        self = value
    }
}

If it's not immediately obvious how this is useful, keep reading.  We'll see in a bit.

Now, what do we need to input and what do we need to get in return?  At the end of the day, we need an Information object, which is this class of six variously sized unsigned integers.  What we start with, however, is some raw data.  So ideally, we'd like to simply do something like this:

let myInformation = Information(data: data)

Right?  That'd be nice, eh?

So the end goal is to have an Information initializer which takes an NSData object.

Well, let's start with the simplest initializer for the most ideal case:

init(startTime: UInt32, duration: UInt32, temperature: UInt16, estimatedDistance: UInt16, knockCount: UInt8, reserved: UInt8) {
    self.startTime = startTime
    self.duration = duration
    self.temperature = temperature
    self.estimatedDistance = estimatedDistance
    self.knockCount = knockCount
    self.reserved = reserved
}

If we get the needed values exactly as we need them, we can just directly assign them as such.  This is just a member-wise initializer.

Let's rephrase that initializer in the form of UInt8 (or [UInt8]) though.  This next one will be marked as a failable convenience initializer.

convenience init?(startTimeBytes: [UInt8], durationBytes: [UInt8], temperatureBytes: [UInt8], estimatedDistanceBytes: [UInt8], knockCount: UInt8, reserved: UInt8) {
    if let
        startTime = UInt32(bytes: startTimeBytes),
        duration = UInt32(bytes: durationBytes),
        temperature = UInt16(bytes: temperatureBytes),
        estimatedDistance = UInt16(bytes: estimatedDistanceBytes) {
            self.init(startTime: startTime, duration: duration, temperature: temperature, estimatedDistance: estimatedDistance, knockCount: knockCount, reserved: reserved)
    } else {
        self.init(fail: true)
    }
}

(Don't worry about self.init(faile: true) right now.  I'll paste that implementation in a second.  For now, just note that it's saving me some boilerplate copy-pasta.)

We've got an if let trying to bind four variables using the UInt32 and UInt16 initializers we extended (which of course return optionals).  So if the passed in arrays are of the wrong size, this initializer will return nil, otherwise, it will return us the Information object we need.

So, what's left?  The next step up is the exact initializer that we want... the one that takes the NSData argument:

convenience init?(data: NSData) {
    if data.length == 14 {
        let allBytes = Array(UnsafeBufferPointer(start: UnsafePointer<UInt8>(data.bytes), count: data.length))
        self.init(startTimeBytes: Array(allBytes[0..<4]), durationBytes: Array(allBytes[4..<8]), temperatureBytes: Array(allBytes[8..<10]), estimatedDistanceBytes: Array(allBytes[10..<12]), knockCount: allBytes[12], reserved: allBytes[13])
    } else {
        self.init(fail: true)
    }
}

We optimize a little bit here by checking the data length (which is also preventing any array index out of bounds exceptions).  And then assuming the data is the right size, we call the previously mentioned convenience initializer.

For the sake of completeness, here's what self.init(fail: true) does:

private init?(fail: Bool = true) {
    self.startTime = 0
    self.duration = 0
    self.temperature = 0
    self.estimatedDistance = 0
    self.knockCount = 0
    self.reserved = 0
    if fail {
        return nil
    }
}

Because of the way Swift initializers work, having this initializer saves us from having to assign every property in every single initializer.  If we know any of our other failable initializers have entered a failure state, we just call this and we're done.

The code, start to finish:

import Foundation

extension UInt16 {
    init?(bytes: [UInt8]) {
        if bytes.count != 2 {
            return nil
        }

        var value: UInt16 = 0
        for byte in bytes.reverse() {
            value = value << 8
            value = value | UInt16(byte)
        }
        self = value
    }
}

extension UInt32 {
    init?(bytes: [UInt8]) {
        if bytes.count != 4 {
            return nil
        }

        var value: UInt32 = 0
        for byte in bytes.reverse() {
            value = value << 8
            value = value | UInt32(byte)
        }
        self = value
    }
}

let bytes: [UInt8] = [0x4e, 0x5b, 0xa2, 0x56, 0x12, 0x19, 0x00, 0x00, 0x22, 0x00, 0x12,0x19, 0x07, 0x00]
let data = NSData(bytes: bytes, length: bytes.count)

class Information {
    let startTime: UInt32
    let duration: UInt32
    let temperature: UInt16
    let estimatedDistance: UInt16
    let knockCount: UInt8
    let reserved: UInt8

    private init?(fail: Bool = true) {
        self.startTime = 0
        self.duration = 0
        self.temperature = 0
        self.estimatedDistance = 0
        self.knockCount = 0
        self.reserved = 0
        if fail {
            return nil
        }
    }

    init(startTime: UInt32, duration: UInt32, temperature: UInt16, estimatedDistance: UInt16, knockCount: UInt8, reserved: UInt8) {
        self.startTime = startTime
        self.duration = duration
        self.temperature = temperature
        self.estimatedDistance = estimatedDistance
        self.knockCount = knockCount
        self.reserved = reserved
    }

    convenience init?(startTimeBytes: [UInt8], durationBytes: [UInt8], temperatureBytes: [UInt8], estimatedDistanceBytes: [UInt8], knockCount: UInt8, reserved: UInt8) {
        if let
            startTime = UInt32(bytes: startTimeBytes),
            duration = UInt32(bytes: durationBytes),
            temperature = UInt16(bytes: temperatureBytes),
            estimatedDistance = UInt16(bytes: estimatedDistanceBytes) {
                self.init(startTime: startTime, duration: duration, temperature: temperature, estimatedDistance: estimatedDistance, knockCount: knockCount, reserved: reserved)
        } else {
            self.init(fail: true)
        }
    }

    convenience init?(data: NSData) {
        if data.length == 14 {
            let allBytes = Array(UnsafeBufferPointer(start: UnsafePointer<UInt8>(data.bytes), count: data.length))
            self.init(startTimeBytes: Array(allBytes[0..<4]), durationBytes: Array(allBytes[4..<8]), temperatureBytes: Array(allBytes[8..<10]), estimatedDistanceBytes: Array(allBytes[10..<12]), knockCount: allBytes[12], reserved: allBytes[13])
        } else {
            self.init(fail: true)
        }
    }
}

func stringFromTimeInterval(interval:NSTimeInterval) -> String {
    let ti = NSInteger(interval)

    let ms = Int((interval % 1) * 1000)

    let seconds = ti % 60
    let minutes = (ti / 60) % 60
    let hours = (ti / 3600)

    return NSString(format: "%0.2d:%0.2d:%0.2d.%0.3d",hours,minutes,seconds,ms) as String
}

let i = Information(data: data)!
let timestamp = NSDate(timeIntervalSince1970: NSTimeInterval(i.startTime))
let duration = stringFromTimeInterval(NSTimeInterval(i.duration))
let temperature = i.temperature
let estimatedDistance = i.estimatedDistance
let knockCount = i.knockCount
let reserved = i.reserved

And this is the result I get for the given data:


Gulp task to increment a major, minor, or patch level of the version number of the last build Working codepen: http://codepen.io/leongaban/pen/oxwLzE?editors=1010

The function below takes in user entered text through the cli: V=patch gulp build

It then checks for the last version of the build folder, then iterates the appropriate version number.

Hoping to condense all the if/else into more functional code.

lastVersion starts as an empty string var lastVersion = '';

The ver value that is passed into this function from the user will be 1 of these commands:

V=major gulp build
V=minor gulp build
V=patch gulp build

So ver will be major, minor or patch...

function generateNextVersion(ver) {
    var major, minor, patch;
    var versionArray = lastVersion.split('.');

    major = parseInt(versionArray[0]);
    minor = parseInt(versionArray[1]);
    patch = parseInt(versionArray[2]);

    if (ver === 'major') {
        major = parseInt(versionArray[0]) + 1;
    }
    else if (ver === 'minor') {
        minor = parseInt(versionArray[1]) + 1;
    }
    else if (ver === 'patch') {
        patch = parseInt(versionArray[2]) + 1;
    }

    version = major + '.' + minor + '.' + patch;

    if (ver === '' || ver === undefined) {
        version = '0.0.0';
    }

    gutil.log(gutil.colors.blue.bold('##################################################'));
    gutil.log(gutil.colors.blue.bold('         Building Dashboard version '+version));
    gutil.log(gutil.colors.green.bold('~~           All change is detectable           ~~'));
    gutil.log(gutil.colors.blue.bold('##################################################'));
}

The first thing that I see is that:

    major = parseInt(versionArray[0]) + 1;

Is a bit redundant. You already say:

major = parseInt(versionArray[0]);

Just do:

major += 1;

(This is similar for all the major, minor and patch statements)

You can also use a switch-case instead of if statements so instead of:

if (ver === 'major') {
    major = parseInt(versionArray[0]) + 1;
}
else if (ver === 'minor') {
    minor = parseInt(versionArray[1]) + 1;
}
else if (ver === 'patch') {
    patch = parseInt(versionArray[2]) + 1;
}

You have:

switch (ver) {
    case 'major':
        major += 1;
        break;
    case 'minor':
        minor += 1;
        break;
    case 'patch':
        patch += 1;
        break;
}


Find all substrings that contain k specific characters in a string Given a string, return the count of the number of substrings that contains characters A, B and C. Two substrings are considered different if the starting, ending or both positions differ.

Example - s = "ABBCZBAC", there are 13 substrings which are "ABBC", "ABBCZ", "ABBCZB", "ABBCZBA", "ABBCZBAC", "BBCZBA", "BBCZBAC", "BCZBA", "BCZBAC", "CZBA", "CZBAC", "ZBAC", "BAC".

My code in Java is giving timeout error for large inputs. my code is

public static long find(String s)
{
    for(int i=0;i<s.length()-2;i++)
    {
        int j=i+3;
        while(j<=s.length() && !(s.substring(i,j).contains("A") && s.substring(i,j).contains("B") && s.substring(i,j).contains("C"))
            j++;
        count += s.length()-j+1;
    }
    return count;
}

I tried to improve this code by finding indexes directly and storing them but still, I was getting timeout errors for large inputs. Can anyone give me a better approach to solve this question may be in O(n)?

The trick of these things is to:

create a model, and then
create a mathematical formula that uses the model.

If you look at the example string .A.B.C.A. then you can may first notice that it doesn't matter a bit if you substitute any other character than ABC for the dot. So we don't care a single iota what characters are at those locations.

Now we see a substring xA.B.Cyyy where A.B.C is just the shortest string in which all the characters are present. Any amount of characters x and y can be present, going from the A.B.C part to the outside. In other words, from this you can construct (#x + 1) * (#y + 1) = (1 + 1) * (3 + 1) = 8 separate substrings that contain this A.B.C. This set of substrings of course also includes xA.B.C.Ay, i.e. all longer substrings that contain the ABC.

Now lets look for substrings that are not covered: -axB.C.Ay. Here - and a are not actually part of the substring, it's just to indicate that we have to skip these characters to find a new ones. If we include the first A then of course we've already counted that substring before. So now we calculate that we need to add (#x + 1) * #y = (1 + 1) * (1 + 1) = 4 separate substrings. After that we cannot find a shorter alphabet (-a-bxC.Ay doesn't contain it). So there we are, we have 8 + 4 = 12 substrings that contain the alphabet.

In words: we have to continue looking for the next offset in the string, try to find the shortest alphabet, and stop if there isn't one. We need to keep track where the start of the last substring was that we found, to avoid duplicates. So there is your model and set of calculations.

As you can see, there is just one starting offset of the substring that increases to the right, and then we need to find the end offset within the string. In other words, the complexity is somewhere at O(N * 1/2N) ~ O(N). Easy-peasy for any modern computer.

However, the algorithm doesn't perform any contains! Heck, it doesn't even create the substrings that it just needs to count.

Another longer example for more visually oriented persons (where we count the pipe and dot characters):

ABBCZBAC
|  |.... -> 1 * 5 = 5
 BBCZBAC
 |    |. -> 1 * 2 = 2
 BBCZBAC
  |   |. -> 1 * 2 = 2
  BCZBAC
   |  |. -> 1 * 2 = 2
    ZBAC
    .| | -> 2 * 1 = 2
                   ==+
                   13

I'll post my code (made and thought of from scratch) iff you show you can do the same; post code and the answer for the following string (it should take the computer no time at all to calculate it).

CBZBBZABBBBAABZZZAZZBACBZCACCZCZABBACCAZCABCBBZBAZAACBZCABZAZCZZ

Probably better to create a new question for it. I don't even want to touch the current example.

Happy programming.

No time at all means:

AACZCBZACBZCBZZBCZBBBAABBCAACBCZZCCZABCCCZBAAACCBCBAZAZZBCAAABBCZBCZZZZZCZAZBZZZBCZCBBBCABZZBCCBBBBBCCCBAAZACAZZBCZBAZCBBACACCCZBBZAZAZBCZCZZCCCACCBAZAZBACAAAZCAZACBABZCZZBZACAZZZZACAZBCCABZCCCBAZZBZBAZZCCZCBBBBBBBCAABZCBCAACACCZBCABAZZAZZCZBZCACACZBZCZZBZZCCZZZZBBCCCCZBCCBZABZBAABZZZACZAABBZCABCZCABZAZCZCAZBBBACBCZACZAAZZACBZCCBZBZACCCBACAZACZAACAZAZBCCZBBZCZCZBACACAZBAZZAACCZZBZZZCAZZAAAABBBAZABBBBZAZZBACBBZAACBAZCCBBZCAZZAZAACAAZAZZBCBZCCCZCZCCZZAZABCBZBBZBBBBZZAAZZCABAAABBBBCZCCCCCABBZCBCACBZAZCBZBBAZZZCBCZZZAAAAAZCBABZCCCAZZAAAZAABABZBBAAZZACZAACABZZCBZABZZCACAAACAABAZBABBZBBBCAZAACACZAACAZCBCCBZCCCACBAZBBBZBBCZAAZCZCZBCABCZCBZABAZBAZZBCCZBBABCCCABZZZCBCBAZCBBBCAZZCBBAZBCCZBCCCABZZBZAZBBABACCBABZZCCBCAZBZZACBAZBCCAZZBAACZACZBZZBBBCAAZBBBABBAZBBZABCCBBBZBZZZAZABBZBACBCZAZBCZACBZBBZZBZABAZZBZCAABZBZBBZCCCAZABZCAZBBBZCZBBBZZBCBCCZAZZZABCBZZZCAABBBZCZZCBZZCZZZZACZBZCBCBBBZCCBZZBBCCZZCCCBZAAZZCZBBCBBCBZCAZCZZBBCBBZCZBAZBAAZCZABBCCZAACBBCBZACAZCCCBABBCZZZBZACZCZACZABCCZCCBBBBBCBZACCZZCAABZBCACCAAZZZCZBCAAZBZAZBZCAZBZCCCCZAZBBCCBBCZZCCAAACCZAABCZZAABBZBZZZAAAZABAACBCCBBCZCCBZCABAAABBAAABCBACBZZABZZBAZABCBZZBABBCACABBAZCBZBAZZZZZBACBZCCBCZZBAAZBBAABZCCZCACZAZZBACAZBABCZBBBAAZZCAZBBCCAZBCBCAAAZBZCCCZZZCAZCBBBBACCAZBCBBCCCBBCABZCCAZACZBABZBBCCCAZAACAAACZZAAACABCAAABAAABACAACCCZABZZZZZCBAABCCAAACACAAZCZCBZBZZZCCCBAZBAZZZCZBCCBZCCACCABCAAAABZCZABCBABZCZACCCACCCZZCCBCZBZZAABBZABCZBCZZZZCACBCBAZZBCAZABACZZABABZBZZCBCACAZACAZZZBBBCCABABZBCAZZCBACACABCAZCCACZBBCCAACCBBCBABZZAZCAABAZAACZZBCCBAACCCCAACCBAZBAZCAABBZZBAZZZBBZZBZAABCCBCBZZCZZABCCZBZCACCZAABCAZBBCZACCZBZBBCZCZCAZZBAAZZZAABBAZZZBZCCZCABZBZCZCCAZAZCBAACCAZZCZBBCCACAABBBZZBZZZAACBZCAAZCBZZAZZBBCCZBBAAZAAZZZZBCBCBCBBACCCCZCACZAACZCCAAZAABCZCCZABCZBBCAAACCBCCABZACBACAAAZCBBACAACCCZACCABBACCBABZZBCACCZZACBBAZZCBABBZBAACCCAZAZAZZCCCCCCZCAACAABBZABCZAAZCCBZZZAZCBCAZACCAZBBCZZAZZCABCAZCBZAZABCBZACZCZAAZZZBZZZZZAZZCBZBBZZABABABAAAAZZZACZAZZZZZBBACZCZBZBAAZAAZCCCAZCZZCABCCABBZZBCACCCBBCCCAZCBABBZCBBBAZCBCZBCAZZCBZZAZBBCZABACCZACBABZCBCBZBAACCZBZAZACBBZZZZBBBBZZZAZBAABCCCZCCAZBCAABCAAZACCBZBCCZZABCABABBBBBBCZCBZACBCAABBBCCCZAABCBZBCAZAZZABZAZZCZZAZBAAAAACZZACZCACCZABACAZAACZCCAZABAACBZCABCCAZACCBBZBBZBZZCCZBAABZBBBBCAZAZZACZABABBZCZZZBZBBCBBAZZZBABAAZZBABZBCBAAZCACAACCZAABZZZBBBZACZCCCZABBBCCCBZACZCCZZAZCBABCCACZABCAZZCBZZCBBBCAZZABBABCZZBCABZBAAAABCCAAZABBZZZZZAABBBAZBCACABZACCCBBBZABZBAZAZBAAZAZCAZAZZBBZBCCCCZCZCBAZBCABZCZAACZAZACBABAAZBCBBACACBZCAZCCBZBZBZABCCZBCZCAZAAZCACCCBCBZZZCZBCBCAZBAAZCACAABZBCBBZZBBZBCABZABCCBCAZBAZCBAAZBZAZZZBCZCCBCCACAZBZZCCAZCACAZZCABCBCZCABBCZBBCZCBCBZACBZAACCBZACBZACCBCBZACACBCAZZZZCCZBCZACCACZACZCBCABCCABZAZCABCCCAACZCBZABZZCABBZBCCAABABAZBCZBBABCAZZCBABABZZACBZBBCBBCBZCBCZBCCZZCBABACABBAAABCCCACCABAABBBZZCCCZABZCZZBABCCCBCBZAZBABCZBZZZCABCZBZCBACCBAABABAZBAZACZBAACBAZZBABCZCBZCCAAZACACZCAAACBCBBABACACCCAZBZCCACBBZCCACAZBAZCACACZCZBCZBBBAAZBAZZZACCCCCCCAAZAZCAAAZBZZZCZCZBCBZZCZAACACCBAAABZZZBZZZACBZAABABCBAACAZCCZBZZCZACABCAZZABBCCAAAABAACABAZZACZZBZBAZZCACACBBCACAZCZZBZCACCZACCCABZCAZABCZZACAZACZAABBCZAZZABZCZCCZCZCZCAACACCCAZCCBZZCAZBACCZCBBZAABBAAAZACAAACBCCZAACBZBBBCACZZCZZCCCBZCBAAZCCCCACCZCZCZBZBZZCZCCBBZCAABBAABAACAZZBCACZZABBAZZBZZCBBAACBZAAABZAABBAZCZBZABBBBBCZBBCABABBBZZZABBAAABCCCZBCAAAZABZCBBACCAAZAABZCBCZBCABABCBCZCZCAACACCZZCCCBZZBCABAABABZBACAZBZBABBZCABZBBCACBBZCZABBZCZAZZZABZCZZBAACZZCZACBZBCZCCABCAAZBACCZCAACZBBACZCZCCBCBABBCZZBABZBBACACZZAACCCCCCZCBACCACZZZBACZZZBZCACBBCZCBBCAZBCACBCBBBCAACBZACAAZAACBCCZZZABCBCCAZBBBBCZBBACZCAZACZBBABABZCCZBCCACBAABBZAAACZAABZZCAZAABBBCABCCCACACCBABZCCZBBBAZCCAZZBBZZAZCABBZCCZAACZAZZCBBZABCACBZBCBCZZZBBACBBCAZABAACAZBBBAZBCBCBAACBAZBAAZZBZACBZBACZBZCCABZZCBAZZBCZBCCCZACAZAZCABAACAABBZZBAAACABZAAZBCACBACABACBCBZABCCACBBZBBZBAZCCBCCCABZZCBCZZAAZCCCCAZZACCZZCBZCZZZCAZZABZAZZACCABZABCCAAAAAABZBCCZCBBZCCZAZAACACBBBBCAZABZCBCZABZCBAAACCZBBBCBZCAAAZCZAACBAZZZZABCZCABAABZZAACCCAZZZCZAACZACCCZBACCBBBBZACZBBAZZABZACAZBAZCBCACACAAACBCBAZCCBZBBZAABAZZCCCCBACBZBCCBACZABZBZBZACABBBZBCAZZBCBBCZABCZBBBZZCCAAZBBAZCAZBAAZAAZZBBCCZABCZAAZBCACCCBABCZBCACZAABBBACCZBBABBCZACAZZAZCBZCCAACBZCBABZAZBCAZBCBCZBZBBABZZCBZAAZ

gives

8364211

instantly. Feel free to check that answer.

In under 2 minutes, I can have an answer for a 1 billion sized character string:

499999994166624150 substrings
116292 ms


Find a list of objects in an array with JavaScript Having an array of objects, such as numbers, what would be the most optimal (Memory and CPU efficiency) way if finding a sub group of objects?

As an example:

demoArray = [1,2,3,4,5,6,7]

Finding [3,4,5] would return 2, while looking for 60 would return -1. The function must allow for wrapping, so finding [6,7,1,2] would return 5

I have a current working solution, but I'd like to know if it could be optimized in any way.

var arr = [
  1,
  5,2,6,8,2,
  3,4,3,10,9,
  1,5,7,10,3,
  5,6,2,3,8,
  9,1]
var idx = -1
var group = []
var groupSize = 0

function findIndexOfGroup(g){
  group = g
  groupSize = g.length
  var beginIndex = -2

  while(beginIndex === -2){
    beginIndex = get()
  }

  return beginIndex
}

function get(){
    idx = arr.indexOf(group[0], idx+1);

    if(idx === -1 || groupSize === 1){
      return idx;
    }
    var prevIdx = idx

    for(var i = 1; i < groupSize; i++){
      idx++

      if(arr[getIdx(idx)] !== group[i]){
        idx = prevIdx
        break
      }

      if(i === groupSize - 1){
        return idx - groupSize + 1
      }
    }
    return -2
}

function getIdx(idx){
  if(idx >= arr.length){
    return idx - arr.length
  }
  return idx
}

console.log(findIndexOfGroup([4,3,10])) // Normal
console.log(findIndexOfGroup([9,1,1,5])) // Wrapping

You could first get the index of the first element of the search array.

Then loop while index !== -1 and check all elements in the search array with the elements in the base array. For checking outside of the range of the array use the reminder operator %.

Return index if all elements are equal.

If not get a new index with indexOf and an incremented start value.

Array#every breaks the iteration if the callback returns false.

function find(search, array) {
    var index = array.indexOf(search[0]);

    while (index !== -1) {
        if (search.every(function (a, i) { return a === array[(index + i) % array.length]; })) {
            return index;
        }
        index = array.indexOf(search[0], index + 1);
    }
    return -1;
}

console.log(find([3, 4, 5], [1, 2, 3, 4, 5, 6, 7]));          //  2
console.log(find([6, 7, 1, 2], [1, 2, 3, 4, 5, 6, 7]));       //  5
console.log(find([60], [1, 2, 3, 4, 5, 6, 7]));               // -1
console.log(find([3, 4, 5], [1, 2, 3, 4, 6, 7, 3, 4, 5, 9])); //  6
.as-console-wrapper { max-height: 100% !important; top: 0; }


Print Integers in a Pyramid I need to write a method that will print the integers from [1, maxValue] in a pyramid where the nth row contains n numbers like so (for printChart(24)):

1
2 3
4 5 6
7 8 9 10
11 12 13 14 15
16 17 18 19 20 21
22 23 24

I am wondering if there is a way to optimize my code for functionality, simplicity, or computational complexity (although I doubt the latter, because I think this runs at \$O(n)\$ time).

It has one helper function, which returns a boolean value representing whether or not a given integer, n, is a perfect square.

public boolean isPerfectSquare(int n){
    int root = (int) Math.sqrt(n);
    return root * root == n;
}

public void printChart(int maxValue) {
    int number = 1;
    while (number <= maxValue) {

        System.out.print(number + " ");

        /* A number n can be determined to be triangular iff
         8n+1 is a perfect square;  if n is a triangular number,
         print a line after it*/
        if(isPerfectSquare(8*number+1)) { System.out.println(); }

        number++;
    }
}

You're doing things quite the hard way. You don't need to call sqrt, or determine perfect squares at all. Simply track y, and as soon as the row's x == y, make a newline, increment y and set x = 0.

The following examples show different results from what you've described:

import java.io.StringWriter;
import java.lang.Math;

class Pyramid {
    interface ChartMethod {
        abstract void run(int n);
    }

    static boolean isPerfectSquare(int n){
        int root = (int) Math.sqrt(n);
        return root * root == n;
    }

    static void chart_squares(int maxValue) {
        int number = 1;
        while (number <= maxValue) {
            System.out.print(number + " ");

            /* A number n can be determined to be triangular iff
             8n+1 is a perfect square;  if n is a triangular number,
             print a line after it*/
            if (isPerfectSquare(8*number + 1))
                System.out.println();
            number++;
        }

        System.out.println();
    }

    static void chart_coords(int maxValue) {
        int number = 1, y = 0, x = 0;
        while (number <= maxValue) {
            System.out.print(number + " ");

            if (x == y) {
                x = 0;
                y++;
                System.out.println();
            }
            else
                x++;

            number++;
        }
        System.out.println();
    }

    static void chart_coordloop(int maxValue) {
        int number = 1;
        for (int y = 0;; y++) {
            for (int x = 0; x <= y; x++) {
                System.out.print(number + " ");
                if (number++ >= maxValue) {
                    System.out.println();
                    return;
                }
            }
            System.out.println();
        }
    }

    static void chart_incr(int maxValue) {
        int increment = 2, next = 1;
        for (int number = 1; number <= maxValue; number++) {
            System.out.print(number + " ");
            if (number == next) {
                next += increment;
                increment++;
                System.out.println();
            }
        }
        System.out.println();
    }

    static void chart_buf(int maxValue) {
        StringWriter sw = new StringWriter();
        int increment = 2, next = 1;
        for (int number = 1; number <= maxValue; number++) {
            sw.write(number + " ");
            if (number == next) {
                next += increment;
                increment++;
                sw.write('\n');
            }
        }
        System.out.println(sw.toString());
    }

    static final ChartMethod[] methods = new ChartMethod[] {
        Pyramid::chart_squares,
        Pyramid::chart_coords,
        Pyramid::chart_coordloop,
        Pyramid::chart_incr,
        Pyramid::chart_buf
    };

    static void test(ChartMethod method) {
        method.run(7);
    }

    static void time(ChartMethod method) {
        long start = System.nanoTime();
        final int N = 200000;
        method.run(N);
        long dur = System.nanoTime() - start;

        System.err.println(String.format("%.3f us", dur / 1e3 / N));
    }

    public static void main(String[] args) {
        for (ChartMethod method: methods)
            time(method);
    }
}

This shows:

1.381 us
1.196 us
1.157 us
1.140 us
0.234 us

The most important thing is to buffer the output before writing.


CheckChanged event readability I have a CheckChanged method that I would like to improve as it takes a lot of space in my program and really hurts readability.

The method prompts the user for input "Comments" and that is stored. However the below code is being used up to 70 times so any way to optimize would be great.

  private void checkBox45_CheckedChanged(object sender, EventArgs e)
    {
        if (checkBox45.Checked == true)
        {
            // Create an instance of the dialog
            frmInputBox input = new frmInputBox();
            // Show the dialog modally, testing the result.
            // If the user cancelled, skip past this block.
            if (input.ShowDialog() == DialogResult.OK)
            {
                // The user clicked OK or pressed Return Key
                // so display their input in this form.
                hash.Add("5", input.txtInput.Text);
                problems = problems + "5. Check Auger Motor: " + input.txtInput.Text + Environment.NewLine;
                this.txtProblems2.Text = problems;
                txtProblems2.Visible = true;
            }

            // Check to see if the dialog is still hanging around
            // and, if so, get rid of it.
            if (input != null)
            {
                input.Dispose();
            }
        }
    }

  private void checkBox47_CheckedChanged(object sender, EventArgs e)
    {
        if (checkBox47.Checked == true)
        {
            // Create an instance of the dialog
            frmInputBox input = new frmInputBox();
            // Show the dialog modally, testing the result.
            // If the user cancelled, skip past this block.
            if (input.ShowDialog() == DialogResult.OK)
            {
                // The user clicked OK or pressed Return Key
                // so display their input in this form.
                hash.Add("26", input.txtInput.Text);
                problems = problems + "24. Service Access : " + input.txtInput.Text + Environment.NewLine;
                this.txtProblems5.Text = problems;
                txtProblems5.Visible = true;
            }

            // Check to see if the dialog is still hanging around
            // and, if so, get rid of it.
            if (input != null)
            {
                input.Dispose();
            }
        }
    }

Before thinking I didn't read well, I didn't want to rewrite everything: the final edit is at the bottom of the answer, but please read everything to understand it all. :)

If the code inside the CheckedChanged events for all the checkboxes is exactly the same, you should extract that code and place it in a method. Then, call this method from the events:

private void ProcessInput(CheckBox cb)
{
    if (cb.Checked)
    {
        var input = new frmInputBox();

        if (input.ShowDialog() == DialogResult.OK)
        {
            hash.Add("5", input.txtInput.Text);
            problems = problems + "5. Check Auger Motor: " + input.txtInput.Text + Environment.NewLine;
            this.txtProblems2.Text = problems;
            txtProblems2.Visible = true;
        }

        if (input != null)
        {
            input.Dispose();
        }
    }
}

//Example usage:
private void checkBox45_CheckedChanged(object sender, EventArgs e)
{
    ProcessInput(checkBox45);
}

private void checkBox1000_CheckedChanged(object sender, EventArgs e)
{
    ProcessInput(checkBox1000);
}

Another way of achieving the same is creating one event and assign this event to the CheckedChanged event of any checkbox that requires this:

private void AnyCheckBoxChanged(object sender, EventArgs e)
{
    var cb = (CheckBox)sender;

    if (cb.Checked)
    {
        var input = new frmInputBox();

        if (input.ShowDialog() == DialogResult.OK)
        {
            hash.Add("5", input.txtInput.Text);
            problems = problems + "5. Check Auger Motor: " + input.txtInput.Text + Environment.NewLine;
            this.txtProblems2.Text = problems;
            txtProblems2.Visible = true;
        }

        if (input != null)
        {
            input.Dispose();
        }
    }
}

//Assigning the events:
checkBox45_CheckedChanged += AnyCheckBoxChanged;
checkBox1000_CheckedChanged += AnyCheckBoxChanged;

Other notes:

The var keyword:

From the C# Programming Guide:

  The var keyword can also be useful when the specific type of the variable is tedious to type on the keyboard, or is obvious, or does not add to the readability of the code.

So lines like:

frmInputBox input = new frmInputBox();

would become:

var input = new frmInputBox();

Naming conventions:

Please follow the Capitalization Conventions for class names. The name of the class frmInputBox would become FormInputBox for example.

String manipulation:

This is about this line:

problems = problems + "5. Check Auger Motor: " + input.txtInput.Text + Environment.NewLine;

Concatenating literal strings with variables should be done using the String.Format method:

var result = String.Format("5. Check Auger Motor: {0}", input.txtInput.Text);

If this were in a loop and building large string, you'd be wasting memory. Use a StringBuilder instead (which has a method to append a string using a format, cfr. previous point):

var builder = new StringBuilder();
builder.AppendFormat("5. Check Auger Motor: {0}", input.txtInput.Text);
var result = builder.ToString();

Disposing of the form:

Since an instance of Form is disposable, I suggest using the using statement. No need to check whether or not it can be disposed or not, at the end of the statement it's automatically disposed:

using (var input = new FormInputBox())
{
    if (input.ShowDialog() == DialogResult.OK)
    {
        hash.Add("5", input.txtInput.Text);
        problems += String.Format("5. Check Auger Motor: {0}{1}", input.txtInput.Text, Environment.NewLine);
        txtProblems2.Text = problems;
        txtProblems2.Visible = true;
    }
}

Edit:

Since the code is not exactly the same, use the first way of solving the problem: by extracting the code into a separate method. Following code implements this:

private void ProcessInput(string hashKey, string problem, TextBox tb)
{
    using (var input = new frmInputBox())
    {
        if (input.ShowDialog() == DialogResult.OK)
        {
            hash.Add(hashKey, input.txtInput.Text);
            problems += String.Format({0}: {1}{2}", problem input.txtInput.Text, Environment.NewLine);
            tb.Text = problems;
            tb.Visible = true;
        }
    }
}

//Calling of the method:
private void checkBox45_CheckedChanged(object sender, EventArgs e)
{
    if(checkBox45.Checked)
        ProcessInput("5", "5. Check Auger Motor", txtProblems2);
}

private void checkBox47_CheckedChanged(object sender, EventArgs e)
{
    if(checkBox47.Checked)
        ProcessInput("26", "24. Service Access", txtProblems5);
}

Edit2:

I re-read your updated code and noticed you set the text of a different textbox too, this is now implemented in my code example above.