Serial port communication app This code connects to COM 3 which has a broadband card and sends a command that returns the RSSI value. I use this program to see patterns in signal strength in certain areas.

#include <Windows.h>
#include <iostream>
#include <string>

int main()
{
    HANDLE hSerial = CreateFile("COM3",GENERIC_READ | GENERIC_WRITE,0,0,OPEN_EXISTING,FILE_ATTRIBUTE_NORMAL,0);

    if(hSerial==INVALID_HANDLE_VALUE)
        std::cout << "Insert error message";

    DCB dcbSerialParams = {0};
    dcbSerialParams.DCBlength=sizeof(dcbSerialParams);

    if (!GetCommState(hSerial, &dcbSerialParams))
        std::cout << "Insert error message";

    dcbSerialParams.BaudRate=CBR_9600;
    dcbSerialParams.ByteSize=8;
    dcbSerialParams.StopBits=ONESTOPBIT;
    dcbSerialParams.Parity=NOPARITY;

    if (!SetCommState(hSerial,&dcbSerialParams))
        std::cout << "Insert error message";

    COMMTIMEOUTS timeouts={0};
    timeouts.ReadIntervalTimeout=50;
    timeouts.ReadTotalTimeoutConstant=50;
    timeouts.ReadTotalTimeoutMultiplier=10;
    timeouts.WriteTotalTimeoutConstant=50;
    timeouts.WriteTotalTimeoutMultiplier=10;
    if(!SetCommTimeouts(hSerial, &timeouts))
        std::cout << "Insert error message";

    while(1)
    {
        char szBuff[50+1] = {0};
        char wzBuff[14] = {"AT+CSQ\r"};

        DWORD dZBytesRead = 0;
        DWORD dwBytesRead = 0;

        if(!WriteFile(hSerial, wzBuff, 7, &dZBytesRead, NULL))
            std::cout << "Write error";

        if(!ReadFile(hSerial, szBuff, 50, &dwBytesRead, NULL))
            std::cout << "Read Error";

        std::string subString = std::string(szBuff).substr(8,3);
        std::cout << subString;

        Sleep(500);
    }

    return 0;
}

In all the test that could be an error. Do you really want to continue if there is an error?
Seems counterproductive to generate an error message and continue running. That will just generate more spurious errors.

if(hSerial==INVALID_HANDLE_VALUE)
{
    throw std::runtime_error("Insert error message");
}

Then just wrap your code in a try catch block:

try
{
    ... Code here
}
catch(std::exception const& e)
{
    std::cerr << "Exception: " << e.what() << "\n";
    throw; // re-throw so the OS can provide help if available.
}
catch(...)
{
    std::cerr << "Exception: Unknown ...\n";
    throw;
}


Iterative "counting change" implementation in mit-scheme Here is my iterative implementation of the Counting change example in SICP. Please note that I'm a beginner, which means I only know the basic syntax for the time being.

Requirement:

  How many different ways can we make change of $ 1.00, given
  half-dollars, quarters, dimes, nickels, and pennies? More generally,
  can we write a procedure to compute the number of ways to change any
  given amount of money?

Code:

(define (count-change-iterative amount)
  ;; penny is not in the signiture, bacause it equals (- amount
  ;;                                                     (* half-dollar 50)
  ;;                                                     (* quarter 25)
  ;;                                                     (* dime 10)
  ;;                                                     (* nickeli 5))
  (define (count-iter half-dollar quarter dime nickeli)
    (cond ((> (* half-dollar 50) amount)
           0)
          ((> (+ (* half-dollar 50)
                 (* quarter 25)) amount)
           (count-iter (+ half-dollar 1) 0 0 0))
          ((> (+ (* half-dollar 50)
                 (* quarter 25)
                 (* dime 10)) amount)
           (count-iter half-dollar (+ quarter 1) 0 0))
          ((> (+ (* half-dollar 50)
                 (* quarter 25)
                 (* dime 10)
                 (* nickeli 5)) amount)
           (count-iter half-dollar quarter (+ dime 1) 0))
          (else (+ 1
                   (count-iter half-dollar quarter dime (+ nickeli 1))))))
  (count-iter 0 0 0 0))

If you run (count-change-iterative 100), it would tell you 292.

I think this is the best scheme code I've written by now. How can I improve it?

In SICP the authors note:

  Count-change generates a tree-recursive process with redundancies similar to those in our first implementation of fib. (It will take quite a while for that 292 to be computed.)
  On the other hand, it is not obvious how to design a better algorithm for computing the result, and we leave this problem as a challenge.

And while your solution is more efficient than the solution they provide it still has a lot of redundancies. If you're will to sacrifice some readability you could use a bottom-up algorithm to eliminate these redundancies.

For example:

(define (add-if predicate x addition)
  (if predicate (+ x addition) x))

(define (inc-if predicate x)
  (add-if predicate x 1))

(define (zero-if predicate x)
  (if predicate 0 x))

(define (count-change amount)
  (define (cc-iter value ways-to-change new-ways nickel-ways dime-ways quarter-ways nickels dimes quarters)
    (define (current-coins num-nickels num-dimes num-quarters)
      (and (= nickels num-nickels)
           (= dimes num-dimes)
           (= quarters num-quarters)))
    (if (> value amount)
        ways-to-change
        (cc-iter (+ value 5)
                 (+ ways-to-change new-ways)
                 (+ new-ways (if (= nickels quarters) dime-ways nickel-ways))
                 (add-if (current-coins 0 1 1) nickel-ways quarter-ways)
                 (add-if (current-coins 0 1 0) dime-ways quarter-ways)
                 (inc-if (current-coins 0 1 0) quarter-ways)
                 (zero-if (or (= nickels 1) (= dimes 2)) 1)
                 (zero-if (= dimes 2) (inc-if (= nickels 1) dimes))
                 (zero-if (current-coins 0 2 1) (inc-if (= dimes 2) quarters)))))
  (cc-iter 0 0 1 1 0 1 0 0 0))


Dereference template class Usually, C++ standard library algorithms have two versions, e.g.:

template< class RandomIt >
void sort( RandomIt first, RandomIt last );

template< class RandomIt, class Compare >
void sort( RandomIt first, RandomIt last, Compare comp );

Where one version is default in some sense and the second one is customized.

When I write my own algorithms, I use the following approach:

Let us suppose that I want to write a flexible algorithm that computes the average value. The standard C++ approach would be to write 2 algorithms:

template< class RandomIt >
double average( RandomIt first, RandomIt last );

template< class RandomIt, class TGetValue >
double average( RandomIt first, RandomIt last, TGetValue getValue );

where getValue is something that obtains the value to be averaged from the object pointed to by the iterator.

But what if we have an iterator that loses some information when dereferenced? For example, let us consider std::map<TKey, TValue>::iterator. This iterator results in a reference to a std::pair<TKey, TValue>, but some iterators such as QMap::iterator return a reference to TValue. That is, after we dereference QMap::iterator, we lose the ability to examine the TKey. Here some flexibility is lost:

QMap<int, double> map;
...
double averageOfKeys = average(map.begin(), map.end(), ???);

It is impossible to compute the average of keys using this interface, since the getValue object expects the thing pointed to by the iterator, which has the type of the second template parameter. There is no way we can get the key, which is of type int, from the value, which is of type double.

But the iterator itself can be used to get both the key and the value, using the iterator::key() and iterator::value() member functions. All we need to do is to change the interpretation of the getValue functor to expect an iterator instead of a thing pointed to by the iterator:

QMap<int, double> map;
...
using iterator = QMap<int, double>::const_iterator;
double averageOfKeys = average(map.begin(), map.end(),
                               [](iterator it){return it.key();});

So I made this template class:

template <class T>
class Dereference
{
public:
    typedef decltype(*std::declval<T>())                      TReference;
    typedef typename std::remove_reference<TReference>::type  TValue;
    typedef typename std::remove_cv<TValue>::type             TNonConstValue;

    TReference operator()(const T& t) const {return *t;}
};

To serve as a default argument in my algorithms. So for example for the average example, my algorithms would be:

template< typename RandomIt, typename TGetValue = Dereference<RandomIt> >
double average( RandomIt first,
                RandomIt last,
                TGetValue getValue = Dereference<RandomIt>() );

This increases the flexibility of the algorithm and reduces the need to have two overloads thanks to the default parameter.

Since I am new to programming in general and especially to C++11, I would like to ask:

If this is intuitive and usable.
If this can backfire in the future in some unexpected way.
If this approach can be improved somehow.

That's definitely an interesting approach. However, I would say that this interface is trying to accomplish too much. That is, average should just be responsible for calculating the average value over some range [first, last) and not worry about any indirection. E.g., only provide the signature

template< class InputIterator >
double average( InputIterator first, InputIterator last );

Note that I've changed your RandomIt to an InputIterator since I think this is a better choice.

Any indirection should instead be handled with iterator transformation. I.e., an adapter which wraps the raw QMap::iterator and overrides operator *() to either return the key or the value. Boost has the boost::transform_iterator which does exactly that. Even more concretely, there is also the boost::indirect_iterator which is very similar to your Dereference class.

Again, the motivation is to divide up the responsibility into separate classes so that each such class is as simple as possible. Complex tasks are then accomplished by combining these separate classes.

All that being said, let me review the code that you already have written. Because, like I said, it is still an interesting approach!

Use InputIterator instead of RandomIterator as the template parameter for average. InputIterator is more generic and is all you need to compute the average value of a range. When C++ gets ConceptsLite, this will make even more sense.
average always returns double. What about float ranges? Use a trailing return type together with iterator_traits to get the value type of the iterator instead. E.g.,

template< class InputIterator>
auto average( InputIterator first, InputIterator last )
    -> typename iterator_traits<InputIterator>::value_type;

In C++14 this is even simpler as you can simply drop the trailing return type.
Prefer using over typedef. They accomplish the same thing but using is more powerful.

Besides that, the code looks fine to me. Good job! Please also consider the alternative I wrote in the first part of this post.

Edit: I recently read D4128: Ranges for the Standard Library: Revision 1 by Eric Niebler which proposes to overload all algorithms with Projection callables exactly as you did. Note that the very same proposal also includes Range Transform View which is an analogue to the iterator transforms I mentioned. That is, Projections and Range Transform Views coexist and serve slightly different purposes. You can read the proposal to get all the details.

In short, a future C++ standard may include a feature which is very similar to your suggestion. It may even come sooner than C++17 in the shape of a TS.


Optimize code for sending collected data to a database I'm sending some data over to a database. There is a particular part that requires me to send a bunch of stuff all at once. It seems a little silly to me to write it this way. I feel that I could do this with an array and for loop, but I'm not sure about how that would look. I tried finding implementations elsewhere but I'm not even sure how to phrase what it is I'm looking for.

$sql = "
        INSERT INTO `computers` (`ComputerName`, `usage_0`, `usage_10`, `usage_20`, `usage_30`, `usage_40`, `usage_50`, `usage_60`, `usage_70`, `usage_80`, `usage_90`, `users`, `OrderDate`)
        VALUES (
        '" . mysqli_real_escape_string($cxn, $_POST['ComputerName']) . "',
        '" . mysqli_real_escape_string($cxn, $_POST['usage_0']) . "',
        '" . mysqli_real_escape_string($cxn, $_POST['usage_10']) . "',
        '" . mysqli_real_escape_string($cxn, $_POST['usage_20']) . "',
        '" . mysqli_real_escape_string($cxn, $_POST['usage_30']) . "',
        '" . mysqli_real_escape_string($cxn, $_POST['usage_40']) . "',
        '" . mysqli_real_escape_string($cxn, $_POST['usage_50']) . "',
        '" . mysqli_real_escape_string($cxn, $_POST['usage_60']) . "',
        '" . mysqli_real_escape_string($cxn, $_POST['usage_70']) . "',
        '" . mysqli_real_escape_string($cxn, $_POST['usage_80']) . "',
        '" . mysqli_real_escape_string($cxn, $_POST['usage_90']) . "',
        '" . mysqli_real_escape_string($cxn, $_POST['users']) . "',
        '" . mysqli_real_escape_string($cxn, $_POST['OrderDate']) . "');
";

mysqli_query($cxn, $sql);

Functional approach

For simplicity you can create few helper functions to get rid of code duplication:

getValuesFromPost(array $list): gets list of parameters names and
returns according POST values.
composeFieldListSql(array $list): gets list of fields and returns SQL expression of fields list like 'ComputerName', 'usage_0', 'usage_10'
composeValuesListSql(array $list): gets list of values and returns SQL expression of escaped values list like 'PC', 'value1', 'value2'
composeInsertSql(array $fields, array $values): composes full insert SQL expression from fields and values lists.

Functions composeFieldListSql() and composeValuesListSql() could be declared inside function composeInsertSql() since you don't need to use them elsewhere.

Your code will look like this:

$fields = ['ComputerName', 'usage_0', 'usage_10', 'usage_20', 'usage_30', 'usage_40', 'usage_50', 'usage_60', 'usage_70', 'usage_80', 'usage_90', 'users', 'OrderDate'];
$sql = composeInsertSql($fields, getValuesFromPost($fields));
mysqli_query($cxn, $sql);

Object oriented approach

Basically you could write wrapper class for mysqli extension functions to encapsulate low level interactions with extension and to make interaction with database more abstract. Also, it will be easy to change inner working of wrapper, if for example you will decide to change database type.

Or you can use one of many existent wrappers, which are already covered by tests and have rich functionality.


Haskell function to explode a list I've written the following explode function using break. The intended behavior is, that a list is divided into sublists by a predicate function, i.e. those elements which match the predicate are the separators - no empty lists should be returned.

Here are some examples:

explode id [True, True, False] = [[False]]
explode id [False, True, False] = [[False], [False]]
explode undefined [] = []
explode id [False, False, True, False] = [[False, False], [False]]
explode (\x -> mod x 3 == 0) [0..10] = [[1,2],[4,5],[7,8],[10]]

The signature of this function is:

explode :: (a -> Bool) -> [a] -> [[a]]

Here is the implementation:

explode _ [] = []
explode f xs
    | null zs = [z]
    | null z  = explode f (tail zs)
    | True    = z : explode f (tail zs)
        where (z, zs) = break f xs

Is there a better way to do this, a maybe more haskell-idiomatic one? Also, what is the complexity of such a function? I believe the running time is linear, but what about the space complexity?

I like blufox' solution, but remember you can replace explicit recursion by a fold, too:

explode f xs = filter (not.null) $ foldr go [[]] xs where
  go x (xs:xss) = if f x then []:xs:xss else (x:xs):xss


Efficient use of regular expression and string manipulation The following is my solution to Java vs C++. I think the way I have used the re library is inefficient, and possible erroneous as I am getting tle.

import sys
import re
cpp = re.compile("^[a-z]([a-z_]*[a-z])*$")
java= re.compile("^[a-z][a-zA-Z]*$")
r = re.compile("[A-Z][a-z]*")
lines = [line for line in sys.stdin.read().splitlines() if line!=""]
for line in lines:
    if cpp.match(line) :
        line = line.split("_")
        for i in range(1,len(line)):
            line[i] = line[i].capitalize()
        print "".join(line)
    elif java.match(line):
        namelist = r.findall(line)
        for name in namelist:
             line = line.replace(name , name.replace(name[0],"_"+name[0].lower()))
        print line
    else : print "Error!"

For instance, is there a better way to replace string components inline instead of creating a new string and copying the way I have used :

line = line.replace(name , name.replace(name[0],"_"+name[0].lower()))

or is there a way that is entirely different from my approach?

When using the re module, make sure to use strings marked with a leading 'r' as I do in the code below. This tells the interpreter to not try and interpolate any escaped strings.

By using findall to test for matches too we end up with more succinct code that still does exactly what we want in a clear fashion.

By putting the logic in a function you get the ability to write unit tests or do something other than print the result.

Using an exception for the failure cases makes the code clear and is more Pythonic in style.

Here is how I would have coded it.

import re
import sys

cpp = re.compile(r"(_[a-z]*)")
java= re.compile(r"([A-Z]+[a-z]*)")

class InvalidStyle(Exception):
    pass

def style_transform(input):
    if input[0].isupper():
        raise InvalidStyle(input)

    m = cpp.findall(input)
    if m:
        # import pdb; pdb.set_trace()
        if any(re.match(r"^[A-Z_]$", w) for w in m):
            raise InvalidStyle(input)

        pos = input.find(m[0])

        return input[:pos] + "".join(w.capitalize() for w in m)

    m = java.findall(input)
    if m:
        # import pdb; pdb.set_trace()
        pos = input.find(m[0])
        words = [input[:pos]] + [w.lower() for w in m]
        return "_".join(words)

    if input.lower() == input:
        return input
    else:
        raise InvalidStyle(input)

if __name__ == "__main__":
    if len(sys.argv) == 2:  # allows for debugging via pdb
        fp = open(sys.argv[1])
    else:
        fp = sys.stdin

    for line in fp.readlines():
        line = line.strip()
        if not line:
            continue

        try:
            print style_transform(line)
        except InvalidStyle as e:
            # print e, uncomment to see errors
            print "Error!"


Count frequencies of elements in array I've been writing code to get the frequencies of elements of an array. The code works, but it uses a lot of "old style" C constructs, some of which I feel should be changed to more C++-like features, but I don't know exactly which way would be best. In general, I feel that a lot of the code could be improved to be more elegant.

The first two functions, remdups and freq, are helper functions for getFrequencies, which is "facade" of the API.

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

template <class T> void remdups(T *arr, size_t *n)
{
    for (size_t i = 1; i < *n; ) {
        if (arr[i] == arr[i-1]) {
            memmove(arr + i - 1, arr + i, (*n-i) * sizeof (T));
            --*n;
        } else
            ++i;
    }
}

int *freq(const int *arr, size_t n, size_t *outN)
{
    int *freq = new int[n], cnt;

    for (size_t i = *outN = 0; i < n; ++i) {
        freq[i] = -1;
        cnt = 1;
        for (size_t j = i + 1; j < n; ++j) {
            if (arr[i] == arr[j]) {
                ++cnt;
                freq[j] = 0;
            }
        }
        if (freq[i] != 0) {
            freq[i] = cnt;
            ++*outN;
        }
    }

    // remove duplicate frequency entries
    remdups<int>(freq, outN);
    return freq;
}

std::vector<std::pair<int, size_t>> getFrequencies(std::vector<int> arr)
{
    size_t n = arr.size(), newSize;
    int *freqs = freq(&arr[0], n, &newSize);

    // remove duplicate values from the array
    remdups(&arr[0], &newSize);

    std::vector<std::pair<int, size_t>> ret;

    for (size_t i = 0; i < newSize; ++i)
        ret.push_back(std::pair<int, size_t> { arr[i], freqs[i] });

    delete []freqs;
    return ret;
}

Example driver code:

int main()
{
    std::vector<int> v = { 5, -3, -3, -3, -2, 0, 0 };
    auto freqs = getFrequencies(v);

    printf("%8s %8s\n", "Elem.", "Freq.");
    for (auto it = freqs.cbegin(); it != freqs.cend(); ++it)
        printf("%8d %8zu\n", it->first, it->second);
}

This seems over-complicated.  A simpler solution would use a std::map or std::unordered_map for the counting:

std::unordered_map<int, std::size_t> counts;
for (auto const &element: container) {
     ++counts[element];
}

Instead of being tied to std::vector<int>, we could make the code much more flexible as a template, taking a pair of iterators (or, from C++20, a std::range) with any appropriate element type.  The signature might look something like:

template<typename Iter>
auto getFrequencies(Iter begin, Iter end)
   -> std::unordered_map<decltype(*Iter), std::size_t>;

We might use constraints or SFINAE to choose whether to return an ordered or unordered map, based on whether *Iter is ordered and/or hashable.


Code that prints ASCII bowties to screen given inputs The goal of this program is to take an input from Stdin and output some ASCII bowties to Stdout. The first integer in the input represents the number of test cases. Each test case contains 2 ints and a char. The first int represents the length in rows of the bowties, the second int the number of bowties, and the third the character to make the bowtie out of.

Input:

2
5 2 #
11 1 !

Output:

#      # #      #
##    ## ##    ##
# #[]# # # #[]# #
##    ## ##    ##
#      # #      #

!            !
!!          !!
! !        ! !
!  !      !  !
!   !    !   !
!    ![]!    !
!   !    !   !
!  !      !  !
! !        ! !
!!          !!
!            !

This program calculates the internal and external spaces and concatenates them plus the material for the bowtie together for the first half. The second half of the bowties are achieved by copying the first half (I couldn't figure out the absolute value function).

I'm a self-taught Java programmer so I don't know much about conventions on when to use void versus return methods and how to structure my code.

import java.util.*;

public class Main
{
    public static void main(String[] args)
    {
        Scanner in = new Scanner(System.in);

        int sets, width, bowties, length, intspaces, extspaces, mid, counter;
        char mat;

        sets = in.nextInt();
        for(int i = 1; i <= sets; i++)
        {
            length = in.nextInt();
            bowties = in.nextInt();
            mat = in.next().charAt(0);
            width = length + 3;
            mid = length/2 + 1;
            counter = 2;
            String[] out = new String[length];

            for(int x = 0; x < mid; x++)
            {
                if(x == 0)
                {
                  intspaces = width - 2;
                  String intspace = space(intspaces, length);
                  out[x] = mat + intspace + mat;
                }
                else if(x != (mid-1))
                {
                  intspaces = width - 2 * (x + 1);
                  extspaces = x - 1;
                  String intspace = space(intspaces, length);
                  String extspace = space(extspaces, length);
                  out[x] = mat + extspace + mat + intspace + mat + extspace + mat;
                }
                else
                {
                  extspaces = x - 1;
                  String extspace = space(extspaces, length);
                  out[x] = mat + extspace + mat + "[]" + mat + extspace + mat;
                }
            }

            for(int x = mid; x < length; x++)
            {
              int count = x - counter;
              out[x] = out[count];
              counter += 2;
            }

            if(bowties != 1)
            {
            stringMulti(out, length, width, bowties);
            }

            for(int x = 0; x < length; x++)
            {
              System.out.println(out[x]);
            }
            System.out.println();

        }
    }
    public static String space(int spaces, int length)
    {
        StringBuilder sb = new StringBuilder(length - 2);
        for(int i = 1; i <= spaces; i++)
        {
            sb.append(" ");
        }
        return sb.toString();
    }

    public static void stringMulti(String out[], int length, int width, int sets)
    {
      for(int x = 0; x < length; x++)
      {
        StringBuilder sb = new StringBuilder(width + sets * (width + 1));
        sb.append(out[x]);
        for(int y = 1; y < sets; y++)
        {
       sb.append(" ");
       sb.append(out[x]);
        }
        out[x] = sb.toString();
      }

    }
}

I think the main issue you need to focus first, is to split the input from the processing and maybe even from the output. I'm not sure if you meet the requirements of the task, when you start outputting stuff before completing the input.

If Java was a conscious choice, you might want to think about a more OOP approach. BowtieSet seems a good candidate for a class.

I assume you left away any input validation/user interface by purpose in relation to the task.

In Java you usually define variables not at the beginning of a method, but where you use them. In addition to that, for loops general start with 0 and end <max.

public class Main
{
    public static class BowtieSet
    {
        private final int  length;
        private final int  repeat;
        private final char style;

        public BowtieSet(final int length, final int repeat, final char style)
        {
            this.length = length;
            this.repeat = repeat;
            this.style = style;
        }

        public String getAscii()
        {
            return "TODO:" + length + "-" + repeat + "-" + style;
        }
    }

    public static void main(String[] args)
    {
        Scanner in = new Scanner(System.in);
        final int numberOfBowtieSets = in.nextInt();
        List<BowtieSet> bowties = new ArrayList<>(numberOfBowtieSets);
        for( int i = 0; i < numberOfBowtieSets; i++ )
        {
            int length = in.nextInt();
            int repeat = in.nextInt();
            char style = in.next().charAt(0);
            bowties.add(new BowtieSet(length, repeat, style));
        }

        bowties.forEach(b -> System.out.println(b.getAscii()));
    }
}

If you stay with an procedural style (totally fine for such a task), you could anyway try to split your main method into smaller well names methods.

Nevertheless, after this initial objectifaction or extracting of a method like

public String getAscii(int length, int repeat, char style)

it is far easier to test your code (automatically and manually). You don't need to type stuff anymore and there is nothing on stdout that needs to be read. As a first step you can simply move the content of your outer for loop into such an method (plus some minor adaptations and appending to a StringBuilder instead of printing to system out).

Afterwards it's time to write some unit tests, or at least two for your samples.

public class BowtieSetTest
{
    @Test
    public void sample1()
    {
        assertEquals("...", new BowtieSet(5, 2, '#').getAscii());
    }

    @Test
    public void sample2()
    {
        assertEquals("...", new BowtieSet(11, 1, '!').getAscii());
    }
}

The length parameter in your space() method is "useless". I think in your application you will have no benefit from defining the capacity of the StringBuilder, it just reduces the readability of your code. In case you ever need an other millisecond, come back to this place. Nevertheless, padding strings can also be done with following common and shorter method:

public static String space(int spaces)
{
    if( spaces == 0 ) return "";
    return String.format("%1$" + spaces + "s", "");
}

You can skip the same optimization in your stringMulti method. In addition to that in Java you usually don't pass arrays around the system and modify them. If we make the the method private and keep the issue local, I think we can live with this for the moment. (List as parameter and new list as return values would be more common)

private static void repeatEachLine(String lines[], int repetitions)
{
    for( int x = 0; x < lines.length; x++ )
    {
        StringBuilder sb = new StringBuilder(lines[x]);
        for( int y = 1; y < repetitions; y++ )
        {
            sb.append(" ");
            sb.append(lines[x]);
        }
        lines[x] = sb.toString();
    }
}

It really took me a moment to figure out what the methods stringMulti did. I hope the new method name will help others (or my future self) to be faster at this point.

Let's get back to your main method. You have two for loops, one to the mid and the second for the remaining lines. You already notice that the lower lines are mirrored from the upper lines, so you can also merge the fors and omit your counter

At this point my remaining getAscii method looks like (Disclaimer: I like oneliner, many don't):

public String getAscii(int length, int repeat, char style)
{
    int width = length + 3;
    int mid = length / 2 + 1;
    String[] lines = new String[length];

    for( int x = 0; x < mid; x++ )
    {
        if( x == 0 )
        {
            int intspaces = width - 2;
            String intspace = space(intspaces);
            lines[x] = style + intspace + style;
            lines[length - x - 1] = lines[x];
        }
        else if( x != (mid - 1) )
        {
            int intspaces = width - 2 * (x + 1);
            int extspaces = x - 1;
            String intspace = space(intspaces);
            String extspace = space(extspaces);
            lines[x] = style + extspace + style + intspace + style + extspace + style;
            lines[length - x - 1] = lines[x];
        }
        else
        {
            int extspaces = x - 1;
            String extspace = space(extspaces);
            lines[x] = style + extspace + style + "[]" + style + extspace + style;
        }
    }

    if( repeat != 1 ) repeatEachLine(lines, repeat);
    return Arrays.stream(lines).collect(Collectors.joining("\n"));
}

I think now it is time to extract the content of the for loop to a getAsciiLine(...) method. Add some tests and check if we can simplify the ifs. At least I would inline the int variables. They no longer add any information.

Let me try an other approach here. Instead of building the string character by character, let me take a blank string of the correct length (char[]) and only fill some places with the style character:

void getAsciiLine(final String[] lines, final int currentLine, final int width, final int mid /*...+ style, length if not an object*/)
{
    char[] string = space(width).toCharArray();
    // set left and right border
    string[0] = style;
    string[width - 1] = style;
    // moving to the center
    string[currentLine] = style; //redundant for 0/width-0, but at least no if
    string[width - currentLine - 1] = style;

    final boolean isMiddleLine = currentLine == (mid - 1);
    if( isMiddleLine )
    {
        string[currentLine + 1] = '[';
        string[currentLine + 2] = ']';
    }
    lines[currentLine] = new String(string);
    if( !isMiddleLine ) lines[length - currentLine - 1] = lines[currentLine];
}

Keep in mind that some refactorings are opinion based, so this answer is not the only truth.

BowtieSet:

import java.util.Arrays;
import java.util.stream.Collectors;

public class BowtieSet
{
    private final int  length;
    private final int  repeat;
    private final char style;

    public BowtieSet(final int length, final int repeat, final char style)
    {
        this.length = length;
        this.repeat = repeat;
        this.style = style;
    }

    public String getAscii()
    {
        int width = length + 3;
        int mid = length / 2 + 1;
        String[] lines = new String[length];
        for( int x = 0; x < mid; x++ ) getAsciiLine(lines, x, width, mid);
        if( repeat != 1 ) repeatEachLine(lines, repeat);
        return Arrays.stream(lines).collect(Collectors.joining("\n"));
    }

    private void getAsciiLine(final String[] lines, final int currentLine, final int width, final int mid)
    {
        char[] string = space(width).toCharArray();
        // set left and right border
        string[0] = style;
        string[width - 1] = style;
        // moving to the center
        string[currentLine] = style; //redundant for 0/width-0, but at least no if
        string[width - currentLine - 1] = style;

        final boolean isMiddleLine = currentLine == (mid - 1);
        if( isMiddleLine )
        {
            string[currentLine + 1] = '[';
            string[currentLine + 2] = ']';
        }
        lines[currentLine] = new String(string);
        if( !isMiddleLine ) lines[length - currentLine - 1] = lines[currentLine];
    }

    private static String space(int spaces)
    {
        if( spaces == 0 ) return "";
        return String.format("%1$" + spaces + "s", "");
    }

    private static void repeatEachLine(String lines[], int repetitions)
    {
        for( int x = 0; x < lines.length; x++ )
        {
            StringBuilder sb = new StringBuilder(lines[x]);
            for( int y = 1; y < repetitions; y++ )
            {
                sb.append(" ");
                sb.append(lines[x]);
            }
            lines[x] = sb.toString();
        }
    }
}

Main:

import java.util.ArrayList;
import java.util.List;
import java.util.Scanner;

public class Main
{

    public static void main(String[] args)
    {
        Scanner in = new Scanner(System.in);
        final int numberOfBowtieSets = in.nextInt();
        List<BowtieSet> bowties = new ArrayList<>(numberOfBowtieSets);
        for( int i = 0; i < numberOfBowtieSets; i++ )
        {
            int length = in.nextInt();
            int repeat = in.nextInt();
            char style = in.next().charAt(0);
            bowties.add(new BowtieSet(length, repeat, style));
        }

        bowties.forEach(b -> System.out.println(b.getAscii()));
    }
}

BowtieSetTest:

import static org.junit.Assert.assertEquals;

import org.junit.Test;

public class BowtieSetTest
{
    @Test
    public void sample1()
    {
        assertEquals(//
            "#      # #      #\n" +//
                "##    ## ##    ##\n" +//
                "# #[]# # # #[]# #\n" +//
                "##    ## ##    ##\n" +//
                "#      # #      #",//
            new BowtieSet(5, 2, '#').getAscii());
    }

    @Test
    public void sample2()
    {
        assertEquals(//
            "!            !\n" +//
                "!!          !!\n" +//
                "! !        ! !\n" +//
                "!  !      !  !\n" +//
                "!   !    !   !\n" +//
                "!    ![]!    !\n" +//
                "!   !    !   !\n" +//
                "!  !      !  !\n" +//
                "! !        ! !\n" +//
                "!!          !!\n" +//
                "!            !",//
            new BowtieSet(11, 1, '!').getAscii());
    }
}

The empty comments in the test are only used to trick my IntelliJ IDEA formatter.


Using a Generic Value Object in a DDD application I am writing this question in the context of a DDD application.  I found this code online on a number of blogs etc e.g. here  and here.

public abstract class ValueObject<T>
         where T : ValueObject<T>
     {
        protected abstract IEnumerable<object> GetEqualityComponents();

         public override bool Equals(object obj)
       {
            var valueObject = obj as T;

             if (ReferenceEquals(valueObject, null))
                return false;

            return EqualsCore(valueObject);
       }

         private bool EqualsCore(ValueObject<T> other)
         {
             return GetEqualityComponents().SequenceEqual(other.GetEqualityComponents());
         }

         public override int GetHashCode()
         {
             return GetEqualityComponents()
                 .Aggregate(1, (current, obj) => current * 23 + (obj?.GetHashCode() ?? 0));
         }

         public static bool operator ==(ValueObject<T> a, ValueObject<T> b)
        {
             if (ReferenceEquals(a, null) && ReferenceEquals(b, null))
                 return true;

             if (ReferenceEquals(a, null) || ReferenceEquals(b, null))
                 return false;

             return a.Equals(b);
         }

         public static bool operator !=(ValueObject<T> a, ValueObject<T> b)
         {
             return !(a == b);
         }
     }
 }

Every Value Object in my application inherits from it.  It means the methods like: Equals and GetHashCode are dealt with implicitly.  The main problem I see arises if I want to add a collection e.g. array as a member of a Value Object - in this scenario the generic value object .Equals will not behave as expected as it cannot deal with collections.

Therefore:

Can this class be tweaked to deal with collections?
Are there any other downfalls with this approach?
Is it advisable not to use a generic value class when developing a DDD application?

Using the code in your example along with the following article for reference

Representing a collection as a Value Object

Lets start with refactoring the City value object to use the improved ValueObject

public sealed class City : ValueObject<City> {

    public string Name { get; }
    public bool IsEnabled { get; }

    public City(string name, bool isEnabled) {
        Name = name;
        IsEnabled = isEnabled;
    }

    protected override IEnumerable<object> GetEqualityComponents() {
        yield return Name;
        yield return IsEnabled;
    }
}

Note how GetEqualityComponents was implemented. Reflection could also have been used to enumerate the properties as needed

return this.GetType().GetProperties().Select(propInfo => propInfo.GetValue(this, null);

There is room for improvement above and I will leave that as an exercise.

For collections, the GetEqualityComponents allows for a simplified approach of returning the items used to check equality.

public class CityList : ValueObject<CityList>, IEnumerable<City> {

    private List<City> _cities { get; }

    public CityList(IEnumerable<City> cities) {
        _cities = cities.ToList();
    }

    protected override IEnumerable<object> GetEqualityComponents() {
        return this.OrderBy(c => c.Name);
    }

    public IEnumerator<City> GetEnumerator() {
        return _cities.GetEnumerator();
    }

    IEnumerator IEnumerable.GetEnumerator() {
        return GetEnumerator();
    }
}

Note that since the collection already implements IEnumerable one simple needs to return the desired enumeration that will be used to calculate equality.

GetEqualityComponents() proves to be a very convenient approach and really does improve and simplify how the ValueObject can be implemented. I initially though it to be a premature optimization attempt but having taken some time to review its potential I have changed my initial view of its function.

UPDATE from comments about the potential of using reflection.:

  The use of reflection automates and simplifies a repetitive task with
  a minor performance trade off. Caching the property/field info could
  help a little but I personally have had no issues with it performance
  wise. Also using reflection grabs all while using the yield like in
  the example you get to be selective of which ones to use for equality.
  You can make an extension method to get the best of both worlds and
  have options.

public static class ValueObjectExtensions {
    /// <summary>
    /// Uses reflection to extract all public properties from the target value object
    /// </summary>
    public static IEnumerable<object> GetComponents<T>(this ValueObject<T> target)
        where T : ValueObject<T> {
        return target.GetType().GetProperties().Select(propInfo => propInfo.GetValue(target, null));
    }
}

and used like

protected override IEnumerable<object> GetEqualityComponents() {
   return this.GetComponents(); //<-- calling extension method
}


Merge objects in Kotlin if they are updated I am very new to Kotlin and I am trying to achieve this:

I get a batch of PersonDetails to save or update
In the batch, the same PersonDetails with name is different locale can be present.
The end goal of this method is to merge the batch itself and also merge the existing entries in Database then return the updated data structure for saving into DB.

I am not very happy with the way I have coded, can you please review this code and give your review comments.

fun doProcess(batchToProcess: List<PersonDetail>) : Map<Long, PersonDetail> {
    val idsInBatch = batchToProcess.map { it.lcmId }.toList()
    val personDetailsFromDb = personDetailsRepository.getBatch(idsInBatch)
        .map { it.lcmId to it }
        .toMap()

    val batchesOfPersonDetails = mutableMapOf<Long, PersonDetail>()

    batchToProcess.map { aPerson ->
        var personDetail = batchesOfPersonDetails[aPerson.lcmId]
        if (personDetail == null) {
            personDetail = personDetailsFromDb[aPerson.lcmId]
        }

        val updatedPersonDetail = when {
            personDetail == null -> aPerson.toPersonDetail()

            personDetail.isActive != aPerson.isActive -> {
                personDetail.isActive = personDetail.isActive
                personDetail
            }

            personDetail.localeDetails[aPerson.localeCode] != aPerson.localeName -> {
                personDetail.localeDetails[aPerson.localeCode] = aPerson.localeName
                personDetail
            }

            else -> {
                logger.info { "Duplicate_Event for id ${aPerson.lcmId}" }
                personDetail
            }
        }

        batchesOfPersonDetails[updatedPersonDetail.lcmId] = updatedPersonDetail
    }

    return batchesOfPersonDetails
}

To make your code understandable, you should extract the code for merging a single PersonDetail into a separate function:

fun merge(fresh: PersonDetail, fromDb: PersonDetail): PersonDetail {
}

This function is where the main complexity happens, and it should be easy to write unit tests for this part of the code. And you should definitely write unit tests, after all you're dealing with details of actual people here, so you better get it correct from the beginning.

In this merge function you can have much shorter variable names. Since you are dealing with persons only, you can remove the word person from the variable names, like I already did in the function declaration above. You can remove the word detail as well.


Make querySelector in kotlin-js more convenient and type-safe I'd like you to take a look at those extension functions I made in kotlinjs (version 1.3.61):

inline fun <reified E : Element> Element.queryHtmlSelectorOrNull(selectors: String): E? {
    return this.querySelector(selectors = selectors) as E?
}

inline fun <reified E : Element> Element.queryHtmlSelector(selectors: String): E {
    return this.queryHtmlSelectorOrNull(selectors = selectors)
        ?: error("couldn't find element by selectors '$selectors'")
}

Now let me elaborate. I was using classic javascript querySelector. This signature:

fun querySelector(selectors: String): Element?

Has some downsides/qualities:

It is optional.
When I force cast to for example HTMLDivElement and result is actually null, I get NullPointerException with no additional information/error message. Example:

document.querySelector(".something-non-existent") as HTMLDivElement //throws

I have to explicitly cast my query to specific element I want (example HTMLDivElement? or HTMLDivElement).

My functions fix some of the issues, but create new issues:

You can explicitly select what type you need or compiler can guess it based on variable type.

Example:

document.queryHtmlSelector<HTMLDivElement>(".something")

or

val element: HTMLDivElement = document.queryHtmlSelector(".something")

The downside is that this doesn't work (need to set type explicitly):

val element = document.queryHtmlSelector(".something")

I would be cool somehow to "default" base type to Element if none is specified, but can't think of way to do that.

I'd like something that can pretty much replace original function and just add benefits of stronger typing and nicer error message. Any hints on how to improve this are greatly appreciated.

"Would be cool somehow, to "default" base type to Element if none is specified, but can't think of way to do that." Indeed it is not possible to do that, at least not with the same method. An option could be to have a separate method for that. document.queryHtmlSelectorRaw ? Or wait, actually... That leaves you back to what you were starting with, Element.querySelector.

  I'd like something, that can pretty much replace original function and just add benefits of stronger typing and nicer error message. Any hints on how to improve this are greatly appreciated.

Given those requirements, I'd say that the current methods is about as good as you're gonna get. You have the stronger typing (but you will have to specify it). You have the better error message if it's null.

The only things that I would consider are:

Get rid of reified, I'm not convinced that you need it.
Change method names, queryHtmlSelector can easily be confused with the built in querySelector. May I recommend just the name selector or select?
Improve the error message even further, such as No element found using selector '$selector' on $this.


Rust decimal to roman numeral kata I just wrote this decimal to Roman numeral converter, inspired by Kevlin Henney's talk "Get Kata".
I have been reading the Rust docs little by little and this is one of my first attempts at some code.
I'd like to get feedback on style, general rust idioms and how I could have implemented this algorithm in a more succinct manner.
Code also available here.
use std::env;
use std::io;
use std::io::BufWriter;
use std::io::Write;

static BASE10_NUMERALS: [&str; 7] = ["I", "X", "C", "M", "X̄", "C̄", "M̄"];

static CENTRE_NUMERALS: [&str; 6] = ["V", "L", "D", "V̄", "L̄", "D̄"];

struct Bases {
    base: usize
}

impl Iterator for Bases {
    type Item = usize;

    fn next(&mut self) -> Option<usize> {
        self.base = self.base - 1;
        Some(self.base)
    }
}

fn iter_bases(largest_base: usize) -> Bases {
    Bases { base: largest_base }
}

fn encode((decimal_number, base, ): (char, usize, )) -> String {
    let digit = decimal_number.to_digit(10).unwrap();
    let max_base = CENTRE_NUMERALS.len();

    if base >= max_base {
        BASE10_NUMERALS[BASE10_NUMERALS.len() - 1]
            //This pow function is the main limiter for decimal size
            .repeat((10_u32.pow((base - max_base) as u32) * digit) as usize)
    } else {
        if digit == 9 {
            format!("{}{}"
                    , if base == 3 { "Ī"} else { BASE10_NUMERALS[base] }
                    , BASE10_NUMERALS[base + 1]
            )
        } else if digit >= 5 {
            format!("{}{}", CENTRE_NUMERALS[
                base], BASE10_NUMERALS[base]
                        .repeat((digit - 5) as usize))
        } else if digit == 4 {
            format!("{}{}", BASE10_NUMERALS[base], CENTRE_NUMERALS[base])
        } else {
            // Less than 4
            BASE10_NUMERALS[base].repeat(digit as usize)
        }
    }
}

fn main() {
    let args: Vec<String> = env::args().collect();
    let mut writer = BufWriter::new(io::stdout());
    let input = &args[1];
    for roman_numeral in input
        .chars()
        .zip(iter_bases(input.len()))
        .map(encode) {
        writer.write(roman_numeral.as_bytes())
            .expect("Unable to write to stdout");
    }
}

cargo fmt and cargo clippy
Use cargo fmt to format your code according to the official Rust
Style Guide.
Clippy's suggestions:

collapse else { if .. } into else if ..;

change self.base = self.base - 1 to self.base -= 1;

use write_all instead of write when writing a slice of bytes,
since the latter only makes one writing attempt:

io::Write::write(_vectored) and io::Read::read(_vectored) are
not guaranteed to process the entire buffer. They return how many
bytes were processed, which might be smaller than a given buffer’s
length. If you don’t need to deal with partial-write/read, use
write_all/read_exact instead.  (source)

The interface
Instead of passing in a tuple, it is preferable to pass separate
parameters:
fn encode(decimal_number: char, base: usize) -> String {
    // ...
}

Now, we notice that the interface is fairly curious.  A better
self-explanatory interface might simply be:
fn to_roman(number: u32) -> String {
    // ...
}

which can be coded to encapsulate calls to encode while avoiding
repetitive allocation.
Grouping use declarations
It's common to group use declarations together:
use std::{
    env,
    io::{self, BufWriter, Write},
};

The Bases utility
Bases is not necessary — you can use (0..n).rev() to iterate
from n - 1 to 0.


Input text upload to MySQL DB I am uploading and selecting code from my MySQL DB and I have a feeling some of it could be written in a much shorter way but I don't know how. Can someone tell me if this is clean code or if it's too much?
<?php

error_reporting(E_ALL);
ini_set('display_errors', 1);

include 'config/connect.php';
$con = new mysqli(...$dbCredentials);

if (isset($_POST['add_execise'])) {
    # code...

    $create_exercise = $con->prepare("INSERT INTO exercises (exercise_name, difficulty) VALUES (?, ?)");
    $create_exercise->bind_param("ss", $_POST['exercise_creation'], $_POST['difficulty']);
    $create_exercise->execute();
}

if (isset($_POST['beginner'])) {
    # code...
    $get_exercises = $con->prepare("SELECT exercise_name, difficulty FROM exercises WHERE difficulty = ?");
    $get_exercises->bind_param("s", $_POST['beginner']);
    $get_exercises->execute();
    $get_exercises->store_result();
    $num_of_rows = $get_exercises->num_rows;
    $get_exercises->bind_result($exercise_selected, $difficulty);
}

if (isset($_POST['intermediate'])) {
    # code...
    $get_exercises = $con->prepare("SELECT exercise_name, difficulty FROM exercises WHERE difficulty = ?");
    $get_exercises->bind_param("s", $_POST['intermediate']);
    $get_exercises->execute();
    $get_exercises->store_result();
    $num_of_rows = $get_exercises->num_rows;
    $get_exercises->bind_result($exercise_selected, $difficulty);
}

if (isset($_POST['advanced'])) {
    # code...
    $get_exercises = $con->prepare("SELECT exercise_name, difficulty FROM exercises WHERE difficulty = ?");
    $get_exercises->bind_param("s", $_POST['advanced']);
    $get_exercises->execute();
    $get_exercises->store_result();
    $num_of_rows = $get_exercises->num_rows;
    $get_exercises->bind_result($exercise_selected, $difficulty);
}
?>

<form action="create.php" method="POST">
    <input type="text" name="exercise_creation" placeholder="exercise">
    <input type="text" name="difficulty" placeholder="difficulty">
    <input type="submit" name="add_execise">
</form>

Choose level of difficulty:
 &nbsp;&nbsp;
<form action="create.php" method="POST">

    <button id="beginner" name="beginner" value="beginner">Beginner</button>

    <button id="intermediate" name="intermediate" value="intermediate">Intermediate</button>

    <button id="advanced" name="advanced" value="advanced">Advanced</button>
</form>

<?php

    while ($get_exercises->fetch()) {
        # code...

        echo $exercise_selected . "
";
    }

?>

Can someone tell me if this is clean code...

The code is somewhat easy to read (e.g. it appears to somewhat align with the recommendations of PSR-12. It is also great that parameters are bound to the queries so SQL injection should be less likely to occur.

...or if it's too much?

The mixing of PHP within HTML is not great. It is recommended to have the PHP fetch data and then within the HTML just output any data that might have been retrieved.
There are quite a few duplicate lines in these three blocks:

if (isset($_POST['beginner'])) {
    # code...
    $get_exercises = $con->prepare("SELECT exercise_name, difficulty FROM exercises WHERE difficulty = ?");
    $get_exercises->bind_param("s", $_POST['beginner']);
    $get_exercises->execute();
    $get_exercises->store_result();
    $num_of_rows = $get_exercises->num_rows;
    $get_exercises->bind_result($exercise_selected, $difficulty);
}

if (isset($_POST['intermediate'])) {
    # code...
    $get_exercises = $con->prepare("SELECT exercise_name, difficulty FROM exercises WHERE difficulty = ?");
    $get_exercises->bind_param("s", $_POST['intermediate']);
    $get_exercises->execute();
    $get_exercises->store_result();
    $num_of_rows = $get_exercises->num_rows;
    $get_exercises->bind_result($exercise_selected, $difficulty);
}

if (isset($_POST['advanced'])) {
    # code...
    $get_exercises = $con->prepare("SELECT exercise_name, difficulty FROM exercises WHERE difficulty = ?");
    $get_exercises->bind_param("s", $_POST['advanced']);
    $get_exercises->execute();
    $get_exercises->store_result();
    $num_of_rows = $get_exercises->num_rows;
    $get_exercises->bind_result($exercise_selected, $difficulty);
}

It appears that only two of each eight lines in each block changes - i.e. the if condition and the second parameter passed to $get_exercises->bind_param().
A commonly accepted principle is the Don't Repeat Yourself. principle. The common lines could be abstracted to a function, or even a simple check for common values:
Instead of setting the name attribute uniquely on each button

<button id="beginner" name="beginner" value="beginner">Beginner</button>

Set the name on all three the same - e.g. name=“difficulty”. That can make the query logic much simpler:
//initialize to avoid undefined variables if POST not set
$exercise_selected = $difficulty = null;
if (isset($_POST['difficulty']) {
    $get_exercises = $con->prepare("SELECT exercise_name, difficulty FROM exercises WHERE difficulty = ?");
    $get_exercises->bind_param("s", $_POST['difficulty']);
    $get_exercises->execute();
    $get_exercises->store_result();
    $num_of_rows = $get_exercises->num_rows;
    $get_exercises->bind_result($exercise_selected, $difficulty);
}


Loose string matching for words in postal addresses I'm trying to compare every element in a vector with every other.

From a dataframe of addresses, I extract streets for a zip code. And a vector of these street strings (like "123 main avenue") is the input.

streets <- c ("123 main avenue", "123 main ave", "456 paul st",
              "456 paul street", "1 r parkway", "1 r pkwy")

I want to test for similarity of strings (like the one before with "123 main ave"). I could use string distance on the substrings, but preferred grep.

I want to avoid the nested for loop, but cannot think of anything better. Here's what I am trying to do:

compare = function(postal_code)
{
streets <- as.vector(props[props$Zip == postal_code,]["Street"])
cnt <- lengths(streets)[1][1]

#Assume two streets "123 main ave" and "123 main avenue"

for (i in 1:cnt)
{
    for (j in 1:cnt)
    {

    prop1 = streets[[1]][i]
    prop2 = streets[[1]][j]

    #split "123 main avenue" into parts
    prop1_parts = strsplit(trimws(prop1), ' ')
    prop2_parts = strsplit(trimws(prop2), ' ')
    prop1_parts_count = length(prop1_parts[[1]])
    prop2_parts_count = length(prop2_parts[[1]])

    #only if the parts of the two streets are equal
    if (prop1_parts_count == prop2_parts_count)
        {
            #compare apples to apples: 123 with 123, main with main, and ave with avenue
           for (x in 1:prop1_parts_count)
            {
                part1 = prop1_parts[[1]][x]
                part2 = prop2_parts[[1]][x]

                if (part1 == part2) { matched = matched & TRUE }
                else {
                    #ignore number parts like 123
                    if (any(grep("[[:alpha:]]", part1)) & any(grep("[[:alpha:]]", part2))) {
                    #check if avenue is in ave or ave is in avenue
                    if (any(grep(part1, part2)) | any(grep(part2, part1))) {
                            matched = matched & TRUE
                            print(paste0(prop1, '-', prop2))
                        }
                    }
                    else {
                        matched = matched & FALSE
                        break #to pass to the next string
                    }
                }
            }
        }
    }
 }
}

For a vector with 104 streets, the comparisons should be (104*103)/2; but with this code it is 104**2; and for these 10816 comparisons, the benchmark is:

I compiled the function as well but no improvement in the execution time.

Any elegant/quick way to improve this code is appreciated.

You'd better use another strategy:

Find a table of all usual abbreviations used in postal addresses.
Create a new column normalized_street in your original dataframe by replacing all occurrences of abbreviations in column Street by the corresponding full words.
Perform an exact self-join on your original dataframe, by Zip and normalized_street.

Why I think it's better:

It will allow you to avoid false positives like cat street / catfish street
The exact self-join will be very efficient
Your code will be simpler, cleaner, and entirely vectorized

There are lots of things to say about your current code. I'll just point out a few mistakes so that you can avoid them next time:

matched is not initialized
as.vector(props[props$Zip == postal_code,]["Street"]) can be written in a simpler way: props[props$Zip == postal_code, "Street"]
Since streets is a vector, you get its length using length(streets)
For the same reason, you have to use streets[i] instead of streets[[1]][i]
You should use grepl() instead of any(grep())


Sqlite3 handler for python 3x this is my first time learning sqlite and I'm still wondering how to do it. I ended up with this sort of library code:

database_path = "userfiles\databases"

def connect(database):
    """Open a connection and a cursor to the database file provided."""
    conexion = sqlite3.connect(database_path + database + ".db")
    cursor = conexion.cursor()
    return {"conexion":conexion, "cursor":cursor}

def disconnect(conexion, cursor, savechanges=False):
    """Disconnect from database, closing the cursor first.
    and saving the cursor data with a commit."""
    cursor.close()
    if savechanges:
        conexion.commit()
    conexion.close()

def build_sqlquery(**sqlquery):
    """Create the query that we will send to the sqlite file."""
    if "select" in sqlquery:
        query_built = "SELECT " + str(sqlquery["select"])
        if "fromtable" in sqlquery:
            query_built = query_built + " FROM " + str(sqlquery["fromtable"])
    elif "insert_into" in sqlquery:
        query_built = "INSERT INTO " + str(sqlquery["insert_into"])
        if "values" in sqlquery:
            query_built = query_built + " VALUES " + str(sqlquery["values"])
    elif "delete" in sqlquery:
        query_built = "DELETE FROM " + str(sqlquery["delete"])
    elif "update" in sqlquery:
        query_built = "UPDATE " + str(sqlquery["update"])
        if "set_values" in sqlquery:
            query_built = query_built + " SET " + str(sqlquery["set_values"])
    if "where" in sqlquery:
        query_built = query_built + " WHERE " + str(sqlquery["where"])
    if "limit" in sqlquery:
        query_built = query_built + " LIMIT " + str(sqlquery["limit"])
    return query_built

def findifexists(cursor, pydata=False, **sqlquery):
    """Query the params into a working sqlite query and
    execute the result, including the pydata if found."""
    query_built = build_sqlquery(sqlquery)
    if not pydata:
        cursor.execute(query_built)
    else:
        cursor.execute(query_built, pydata)
    datos_de_registro = cursor.fetchone() is not None
    return datos_de_registro

And then, once that library is created, I executed my queries like this:

conn = connect("food")
exists = findifexists(conn["cursor"], (value1, value2, value3, condition1),
             select="(?,?,?)",
             fromtable="mytable",
             where="Something = ?")
disconnect(conn["conexion"], conn["cursor"])
if exists:
    print("Yes, it exists")
else:
    print("Nope")

Now my concern is that it feels like re-inventing the wheel, where I could just throw an SQLite query directly at the caller without building it up in the build_sqlquerymethod.

What do you think about it? What cons or pros does this have?
Also: If I want to execute multiple queries, It's okay to keep a conection and a cursor open to execute them one after another before closing them? Or should I connect, save something, disconnect, and then connect again and repeat the procces?

Code improvements
Going from top to bottom, here are some improvements I would suggest.

If database_path is a constant, it should be named instead DATABASE_PATH, or some other all-caps name, to make it evident that it shouldn't be changed. You may consider storing this outside of the code in some config file, so if it ever changes you only need to change the config file and not the code.

Since you are using Python 3, consider using type hints. This will make your code easier to read, as well as allow static analysis of your code to make sure the correct types are used. For example, here would be your connect function with type hints. Note the added import from typing, which has common types like List, Dict, and Tuple.
from typing import Dict

def connect(database: str) -> Dict:
    """Open a connection and a cursor to the database file provided."""
    conexion = sqlite3.connect(database_path + database + ".db")
    cursor = conexion.cursor()
    return {"conexion":conexion, "cursor":cursor}

Instead of making up a word like conexion, consider just adding an underscore at the end when you are worried about using a reserved keyword, i.e.:
connection_ = sqlite3.connect(database_path + database + ".db")

I would recommend using the string .format() method instead of +'ing strings together. For example:
connection_ = sqlite3.connect("{0}{1}.db".format(database_path, database))

It makes the intention more clear, and makes sure everything is the right type to avoid type errors. You could also split the string building by itself for easier reading:
connection_string = "{0}{1}.db".format(database_path, database)
connection_ = sqlite3.connect(connection_string)

Design improvements
Having worked with SQL databases quite a bit, I personally think it is a bad idea to dynamically build SQL query strings as you are doing. The main reasons are as follows:

They are error-prone
They have potential security risks
They usually have poor performance

To briefly address each:

Imagine that you (or another programmer) makes a typing error, say typing "Usres" instead of "Users", the SQL DB will return an error stating that the "Usres" object doesn't exist. Then you need to build in error handling in your functions to account for misspelled or otherwise non-existent database objects.

Imagine you had some kind of input form where a user types in some information, for example, their user name; then, you put that into your SQL query string and execute it on the DB. It's all well and good, as long as the user types something that could be valid. But there is a risk for what has become jokingly known as a Bobby Tables problem, or SQL injection.

Take this for example: query_built = query_built + " WHERE " + str(sqlquery["where"])
Say I entered this as user name: Phrancis'; DROP TABLE Users;--
Your executable query string would then look something like this:
"SELECT Id FROM Users WHERE Name = 'Phrancis'; DROP TABLE Users;--'"

Notice that trailing single quote at the very end after --, which is now commented out and ignored, and the single quote I put in the user name is used instead.
And if SQL executed that, then, you better have a recent backup of your users table!

For performance, queries that are executed "ad hoc" (or, basically, improvised) on the database usually have a poorer performance than queries that are stored. The SQL database usually has to recalculate execution plans for ad hoc queries each time, as opposed to re-using the execution plan from the last time(s) a similar query was executed. On busy production databases, this can make a significant impact on the database processing load.

Ultimately, for any production code, database queries should each have their own functions/methods, as in this example:
def check_user_exists(user_name: str) -> bool:
    query = "SELECT id FROM users WHERE name = ?"
    cursor.execute(query, user_name)
    # etc.


Async processes in Java Spring I need to refactor this class having in mind that the logic inside the call methods is not so short and it require spring dependencies to work, the base class I have is:

@Service
public class ConcurrentService{

    @Autowired
    private Service1 service1;

    @Autowired
    private Service2 service2;

    public executeParallelProcess(){
        List<Callable> callableList = new ArrayList<Callable>();
        callableList.add(new Callable<String>(){

            @Override
            public String call() throws Exception {
                return service1.run();
            }
        });

        callableList.add(new Callable<String>(){

            @Override
            public String call() throws Exception {
                return service2.run();
            }
        });

        final List<Future<?>> futureList = new ArrayList<Future<?>>();

        for(Callable callable : callableList){
            futureList.add(taskExecutor.submit(callable));
        }

        final List<Object> resultObjects = new ArrayList<Object>();

        for(int i = 0 ; i < futureList.size() ; i++){
            try {
                resultObjects.add(futureList.get(i).get());
            }catch (InterruptedException ite){
                serviceExceptionThrown = true;
            }
        }

        if(serviceExceptionThrown)
            throw new ServiceException(ConstantUtil.THREAD_EXCEPTION);

        return resultObjects;

    }
}

After notice that I can not use Mockito to test the logic within the call method I use this way.

@Service
public class ConcurrentService{

    @Autowired
    private Service1 service1;

    @Autowired
    private Service2 service2;

    public executeParallelProcess(){

        Future<String> future1 = this.process1();
        Future<String> future2 = this.process2();

        final List<Future<?>> futureList = new ArrayList<Future<?>>();
        futureList.add(cosmosRequestCall);
        futureList.add(vendavoRequestCall);

        final List<Object> resultObjects = new ArrayList<Object>();

        for(int i = 0 ; i < futureList.size() ; i++){
            try {
                resultObjects.add(futureList.get(i).get());
            }catch (InterruptedException ite){
                serviceExceptionThrown = true;
            }
        }

        if(serviceExceptionThrown)
            throw new ServiceException(ConstantUtil.THREAD_EXCEPTION);

        return resultObjects;

    }

    @Async
    public Future<String> process1(){
        return new AsyncResult<String>(service1.run());
    }

    @Async
    public Future<String> process2(){
        return new AsyncResult<String>(service2.run());
    }
}

I need your advise and help here, to know if there is better way to implement this kind of logic, have in mind that could be more than 2 async processes , I am looking for a generic way to run several callable statements in a way that makes me easy the unit testing of the internal process and not having big blocks of code within the call() method.

The question is more about how to design an executor class that can run several callable in different scenarios for different situation not just tied to an specific implementation. I use anonymous classes, inner classes to create. Just imagine something that I can built to run more than the two thread process that are in the example.

To test these things the first thing I usually do is wire in a sameThreadExecutor; if you don't want a dependency you'll have to write your own but it's just an implementation of ExecutorService that calls its callable immediately and returns a completed future.

The aim is to replace the async part of your code with something synchronous. The mocking could be done with Mockito, it's just very unwieldy. You would have to mock the task executor with a doAnswer:

Mockito.when(taskExecutor.call(any())).thenAnswer((invocation) -> ... ugly work goes here ...)

For safety you might want to add timeout parameters to the call() method and handle timeout exceptions (cancel the timed out future).


Binary Tree max sum conditionally Problem : A binary tree is given as an input, each node of binary tree contains one integer value. Find the maximum sum of collection of nodes such that following two conditions are met.

if node's parent node is considered for sum, then node cannot be considered.
if node's either of child is considered for sum, then node cannot be considered.

For example :

For a binary tree

             15
      20            25
   5      15      5

The max sum we can get is considering [20, 25] which is 45.

To implement this I used O(n^2) approach and code is here.

I am trying to optimize it and looking for feedback on improving algorithm, code efficiency and cleanliness

tree is String representation of tree, given in BFS manner. _ is present in place of null element. _ is not given for the deepest level's element's null representation.

the above tree would be represented as 15 20 25 5 15 5 _

static int findMaxSum(String tree) {
    String[] tokens = tree.split(" ");
    int max = 0;
    for (int i = 0; i < tokens.length; i++) {
        if (!isValidToken(tokens[i])) {
            continue;
        }
        Set<Integer> set = new HashSet<>();
        //wildcard entry
        set.add(i);
        int sum = Integer.parseInt(tokens[i]);
        for (int j = 0; j < tokens.length; j++) {
            if (isValidToken(tokens[j])) {
                if(!set.contains(j) && !set.contains(getParentIndex(j))){
                    if (!set.contains(getLeftChildIndex(j)) && !set.contains(getRightChildIndex(j))) {
                        set.add(j);
                        sum += Integer.parseInt(tokens[j]);
                    }
                }
            }
        }
        max = Math.max(max,sum);
    }

    return max;
}

static boolean isValidToken(String token) {
    return !"_".equals(token);
}

static int getLeftChildIndex(int parentIndex) {
    return parentIndex * 2 + 1;
}

static int getRightChildIndex(int parentIndex) {
    return parentIndex * 2 + 2;
}

static int getParentIndex(int childIndex) {
    return (childIndex - 1)/2;
}

Note: this code works but it is not efficient. I am looking for ideas on how to improve it.

I think I understand the problem, which if I am not mistaken means we want to pick a set of nodes from the tree such that no two taken nodes are connected in the tree. If you just think about it in a recursive manner, you either take a node and neither of its children, recursively trying to solve the same problem with the grandchildren. Or you do not take the node, and take the sum of the same problem solved for both children. An implementation could look like this (I left out underscore, using the index to check for the end of the tree):

static int max(final String tree) {
    return max(0, stream(tree.split(" ")).mapToInt(Integer::parseInt).toArray());
}

static int max(final int root, final int... nodes) {
    if (root >= nodes.length) {
        return 0;
    }

    final int maxWithRoot =
            nodes[root]
            + max(getLeftChildIndex (getLeftChildIndex(root)),  nodes)
            + max(getRightChildIndex(getLeftChildIndex(root)),  nodes)
            + max(getLeftChildIndex (getRightChildIndex(root)), nodes)
            + max(getRightChildIndex(getRightChildIndex(root)), nodes);

    final int maxWithoutRoot = max(getLeftChildIndex(root),  nodes) +
                               max(getRightChildIndex(root), nodes);

    return Integer.max(maxWithoutRoot, maxWithRoot);
}

You can now also see that the same calculation will be executed multiple times for the same node. But because the result of the max function only depends on the root index, you can cache the calculations, meaning you calculate the result for each node only once.

In this case, you could simply create an integer array as a cache, using the node index as the cache index.


Web scraping news articles with Beautiful Soup It's been nearly a year I've been working with C++ and now I am diving in Python and its libraries and frameworks. I am currently creating a website for my portfolio that gathers news articles from a wide range of news websites. I am using Django framework for web development and the Beautiful Soup library for web scraping.

This is currently my Django views.py file, where all the web scraping happens:

from django.shortcuts import render

import urllib.request
from bs4 import BeautifulSoup
import re

# More websites will be added below.
URLS = ["https://www.theguardian.com/uk/rss", "http://rss.cnn.com/rss/edition.rss"];

class Brick:
    """
    Class to represent each article in a news website. 1 brick = 1 news article.
    """
    def __init__(self, title="Untitled", link="#", description="No description",
                 img_url="#", creator="No creator", date="No Date"):
        self.title = title;
        self.link = link;
        self.description = description;
        self.img_url = img_url;
        self.creator = creator;
        self.date = date;

def strip_html(raw_html):
    """
    Strips any HTML tags within a string.
    (I know that this is not safe, but I would like to practice regular expressions).
    """
    pattern = re.compile(r"<.*?>");
    return pattern.sub("", raw_html);

def shorten(cleaned_html):
    """
    If the description of an article is too long, let's cut it
    to only 50 characters.
    """
    new_string = "";

    index = 0;
    for c in cleaned_html:
        if index < 100:
            new_string += cleaned_html[index];

        index += 1;

    new_string += "...";
    return new_string;

# Create your views here.
def news_list(request):
    """
    View that will redirect to the home template that shows the full list
    of articles.
    """

    unformatted_xmls = [];

    formatted_xmls = [];

    group_of_items = [];

    for URL in URLS:
        with urllib.request.urlopen(URL) as file_object:
            unformatted_xmls.append(file_object.read());

    for unf_xml in unformatted_xmls:
        formatted_xmls.append(BeautifulSoup(unf_xml, "xml"));

    for form_xml in formatted_xmls:
        group_of_items.append(form_xml.find_all("item"));

    bricks = [];

    for item_group in group_of_items:
        for item in item_group:
            b = Brick();
            b.title = item.find("title").get_text();
            b.link = item.find("link").get_text();
            b.description = shorten(strip_html(item.find("description").get_text())) if item.find("description") is not None else "No description.";
            b.img_url = item.find("media:content").get("url") if item.find("media:content") is not None else "No media.";
            b.creator = item.find("dc:creator").get_text() if item.find("dc:creator") is not None else "No creator.";
            b.date = item.find("dc:date").get_text() if item.find("dc:date") is not None else "No date.";
            bricks.append(b);

    return render(request, "news/list.html", {"bricks": bricks});

As I am new to web scraping and web development in general, would you mind giving me your tips and opinions about this code? Would you recommend any good practices? Are these libraries and frameworks strongly recommended for this purpose?

It seems wasteful to re-crawl the specified websites every time your web page gets hit, particularly if the web pages you're crawling have nothing to do with the request sent to your webpage. This seems like a job that should be run as a cron job (say, every 5 or 30 minutes), stored in the database, then simply fetched from the database when your webpage gets hit.

A great framework for more advanced crawling is scrapy. It very likely might be overkill for this application, but if you're pondering more advanced scraping, it's a good utility for that purpose.

Now to actually review your code.

Your shorten function could be radically simplified using slices:

def shorten(cleaned_html, limit=50):
    """
    If the description of an article is too long, let's cut it
    to only 50 characters.
    """
    return cleaned_html[:limit] + ('...' if len(cleaned_html) > limit else '')

Also, in strip_html, you compile your regex every time, but only use it once. In Python's re module, there are shortcut functions when you're only using a regex once. You could simply do return re.sub(r"<.*?>", "", raw_html)

When doing your crawling, you create several temporary lists, each of which is a simple function that transforms the elements of the previous list, which you do using a list. In Python, there's a very efficient way (both in memory and compute time) to represent this using generator comprehensions. You also have a robust constructor for Brick; instead of creating a generic Brick and then modifying it, just make the exact Brick you want in the first place:

def extract_brick(item):
    return Brick(
        title=item.find("title").get_text(),
        link=item.find("link").get_text(),
        description=shorten(strip_html(item.find("description").get_text())) if item.find("description") is not None else "No description.",
        img_url=item.find("media:content").get("url") if item.find("media:content") is not None else "No media.",
        creator=item.find("dc:creator").get_text() if item.find("dc:creator") is not None else "No creator.",
        date=item.find("dc:date").get_text() if item.find("dc:date") is not None else "No date."
    )

def news_list(request):
    unformatted_xmls = (urllib.request.urlopen(URL) for URL in URLS)
    formatted_xmls = (BeautifulSoup(unf_xml, "xml") for unf_xml in unformatted_xmls)
    group_of_items = (form_xml.find_all("item") for form_xml in formatted_xmls)
    bricks = [extract_brick for item_group in group_of_items for item in item_group]

    return render(request, "new/list.html", {"bricks": bricks})

And lastly, you don't need semi-colons at the end of every line. It's a hard habit to break when transitioning from languages where whitespace doesn't really matter, such as C++, C, or Java, but in Python they're mostly just clutter.


Highlighting and copying spreadsheet rows that match a criterion I use the code (provided below) to check for certain criteria in a row. In this case, if cell F in worksheet "Swivel" contains "After Dispute For SBU" then that row needs to be highlighted in yellow and the text changed to red, then the row needs to be copied to another sheet named "Disputed" that is within the same workbook. I have added some code to remove the highlight and the font color change if the row is removed from the workbook manually and then the code is triggered again. This particular code is run from a menu system that I created (code not included). The additional code is also used to protect the color of the header row.

This updated code is causing erratic behavior of the cursor and is also running a little longer than it was before the changes, even though the amount of data did not change. Is there a way to improve the code? or is this normal behavior that I need to live with?

The code is completing the tasks as expected.

Sub Highlight_Copy_Disputed()

Application.ScreenUpdating = False

    ' This part highlights all rows that are Disputed
    Dim row As Range
    For Each row In ActiveSheet.UsedRange.Rows
        If row.Cells(1, "F").Value = "After Dispute For SBU" Then
            row.Interior.ColorIndex = 6
            row.Font.Color = RGB(255, 0, 0)
    ElseIf row.Cells(1, "F").Value = "Impact Status" Then
            row.Interior.Color = RGB(197, 190, 151)
            row.Font.Color = RGB(0, 0, 0)
    Else
        row.Interior.ColorIndex = xlNone
        row.Font.Color = RGB(0, 0, 0)
        End If
    Next row

' This part clears the Disputed worksheet and copies all disputed rows to the sheet

With ThisWorkbook.Worksheets("Disputed")
Range(.Range("A2"), .UsedRange.Offset(1, 0)).EntireRow.Delete
End With

Dim LR As Long, lr2 As Long, r As Long
LR = Sheets("Swivel").Cells(Rows.Count, "A").End(xlUp).row
lr2 = Sheets("Disputed").Cells(Rows.Count, "A").End(xlUp).row
    For r = LR To 2 Step -1
        If Range("F" & r).Value = "After Dispute For SBU" Then
            Rows(r).Copy Destination:=Sheets("Disputed").Range("A" & lr2 + 1)
            lr2 = Sheets("Disputed").Cells(Rows.Count, "A").End(xlUp).row
        End If

        Range("A2").Select
    Next r
Application.ScreenUpdating = True
    Range("C" & Rows.Count).End(xlUp).Offset(1).Select
End Sub

I don't see you using Option Explicit which is always a good idea. But you are defining your variables - which is good.

Using variable names like Row is generally a bad idea - Row already means something in excel.

The other variables LR, lr2 and r are bad names - what do they do? Why do you need two LR? I'd used SwivelLastRow and DisputedLastRow for the LR variables. The r is just a counter, so why not give it a better name? CurrentRow maybe?

I might also break the two procedures up into different subs, maybe Highlight_Disputed and CopyDisputed.

You're also using ActiveSheet.UsedRange and select which is generally something to be avoided.

The first procedure could be something like this -

   Dim wsSwivel As Worksheet
   Set wsSwivel = Sheets("Swivel")
   Dim TestCell As Range
   Dim LastRow As Long
   LastRow = wsSwivel.Cells(Rows.Count, "F").End(xlUp).row
   Dim TestArea As Range
   Set TestArea = Sheets("Swivel").Range("F1:F" & LastRow)

   Dim AfterDispute As String
   AfterDispute = "After Dispute For SBU"
   Dim ImpactStatus As String
   ImpactStatus = "Impact Status"
   Dim AfterDisputeHighlight As Long
   AfterDisputeHighlight = 6

   For Each TestCell In TestArea
    If TestCell = AfterDispute Then
        TestCell.Rows.Interior.ColorIndex = AfterDisputeHighlight
        TestCell.Rows.Font.Color = RGB(255, 0, 0)
    ElseIf TestCell = ImpactStatus Then
        TestCell.Rows.Interior.Color = RGB(197, 190, 151)
        TestCell.Rows.Font.Color = RGB(0, 0, 0)
    Else:
        TestCell.Rows.Interior.ColorIndex = xlNone
        TestCell.Rows.Font.Color = RGB(0, 0, 0)
    End If
   Next

I'd also find a way to assign the RGB colors to variables so it looks cleaner.

After that you can exit sub or just CopyDisputed. Since CopyDisputed isn't returning a value, it's not a function. And since you already have some needs defined you can put in parameters:

Sub CopyDisputed(ByVal FromSwivel As Long, ByVal DisputedText As String)

And just use what you have -

CopyDisputed LastRow, AfterDispute

This eliminates the LR and one of the For Loops.

Sub CopyDisputed(ByVal FromSwivel As Long, ByVal DisputedText As String)
   Dim wsDisputed As Worksheet
   Set wsDisputed = Sheets("Disputed")
   Dim CopyRow As Long
   Dim DisputedRow As Long
   DisputedRow = 1
   wsDisputed.Range("B:Z").ClearContents

    For CopyRow = FromSwivel To 1 Step -1
        If Cells(CopyRow, "F") = DisputedText Then
           Rows(CopyRow).EntireRow.Copy Destination:=wsDisputed.Rows(DisputedRow)
           'Whoops you don't want to delete, but then why are you step -1?
           'Rows(CopyRow).EntireRow.Delete shift:=xlUp
           DisputedRow = DisputedRow + 1
        End If
    Next

End Sub

I'm assuming I understand your Disputed ws copying to be on the entire row, unless you're trying to keep column A intact?

Breaking it into two procedures makes it easier to gather what each code block is doing.

But, of course, you can avoid this if you do the copying to Disputed when you do the highlighting of the AfterDispute rows. This is something that would increase performance so you don't need to loop through column F twice:

Option Explicit

Sub Highlight_Copy_Disputed()

Application.ScreenUpdating = False

   Dim wsSwivel As Worksheet
   Set wsSwivel = Sheets("Swivel")
   Dim wsDisputed As Worksheet
   Set wsDisputed = Sheets("Disputed")
   Dim TestCell As Range
   Dim LastRow As Long
   LastRow = wsSwivel.Cells(Rows.Count, "F").End(xlUp).row
   Dim DisputedRow As Long
   DisputedRow = 1
   Dim AfterDisputeHighlight As Long
   AfterDisputeHighlight = 6

   Dim TestArea As Range
   Set TestArea = Sheets("Swivel").Range("F1:F" & LastRow)

   Dim AfterDispute As String
   AfterDispute = "After Dispute For SBU"
   Dim ImpactStatus As String
   ImpactStatus = "Impact Status"

   wsDisputed.Range("B:Z").ClearContents

   For Each TestCell In TestArea
    If TestCell = AfterDispute Then
        TestCell.Rows.Interior.ColorIndex = AfterDisputeHighlight
        TestCell.Rows.Font.Color = RGB(255, 0, 0)
        TestCell.EntireRow.Copy Destination:=wsDisputed.Rows(DisputedRow)
        DisputedRow = DisputedRow + 1
    ElseIf TestCell = ImpactStatus Then
        TestCell.Rows.Interior.Color = RGB(197, 190, 151)
        TestCell.Rows.Font.Color = RGB(0, 0, 0)
    Else:
        TestCell.Rows.Interior.ColorIndex = xlNone
        TestCell.Rows.Font.Color = RGB(0, 0, 0)
    End If
   Next
  Application.ScreenUpdating = True
End Sub


Parsing a string to a java LocalTime I have a method that can take in a time string in one of the following formats:

HHmmss - e.g. 073055 = 07:30:55
HHmm - e.g. 0730 = 07:30:00
HHmm'H - e.g. 0730H = 07:30:30
a whitespace or empty string - return null

My current code is as follows:

private final static String timeFormatPattern = "HHmmss";
private final static DateTimeFormatter timeFormatter = DateTimeFormatter.ofPattern(timeFormatPattern);

private static LocalTime getLocalTime(String inputTime)
{
    if (inputTime.trim().equals(""))
    {
        return null;
    }
    else
    {
        char secondsIndicator = inputTime.charAt(4);

        if (secondsIndicator == ('H'))
        {
            // H = 30 seconds
            return LocalTime.parse(inputTime.substring(0, 4) + "30", timeFormatter);
        }
        else if (secondsIndicator == (' '))
        {
            // no H = 00 seconds
            return LocalTime.parse(inputTime.substring(0, 4) + "00", timeFormatter);
        }
        else // specific time provided
        {
            return LocalTime.parse(inputTime,timeFormatter);
        }
    }
}

Profiling shows that this method takes 16,439 milliseconds over 7.6 million invocations; and thus is the method with the highest self time in my program's startup.

I have tried splitting this into two methods (one to format the string, and one that simply contains return LocalTime.parse(formattedTime, timeFormatter);), to see whether it was the LocalTime.parse or my string manipulation that was slow. It turns out that 2/3 of the processing is from LocalTime.parse, and the other 1/3 from the string manipulation. This is with half the inputs being of the 4th type (and so not calling the LocalTime.parse at all); and half being of the 2nd or 3rd type.

Am I missing a more efficient way of getting the LocalTime here?

Since your format is relatively constant I would suggest parsing it yourself into hours and minutes and seconds and use the LocalTime.of method.  It could look something like this:

public static LocalTime getLocalTime(String inputTime){
    if (inputTime.trim().equals(""))
    {
        return null;
    }
    int hour = ((inputTime.charAt(0)-'0')*10)+(inputTime.charAt(1) - '0');
    int minutes = ((inputTime.charAt(2)-'0')*10)+(inputTime.charAt(3)-'0');
    int seconds = 0;
    if(inputTime.length() != 4){
        if(inputTime.charAt(4) == 'H'){
            seconds = 30;
        }else{
            seconds = ((inputTime.charAt(4)-'0')*10)+(inputTime.charAt(5)-'0');
        }
    }
    return LocalTime.of(hour, minutes, seconds);
}


Binary exponentation with modulo I have implemented binary exponentiation. Is this good?

def quad_pow(base, exponent, modul):
    alpha = (bin(exponent).replace('0b', ''))[::-1]
    a = 1
    b = base

    for i in range(0, len(alpha)):
        if int(alpha[i]) == 1:
            a = (a * b) % modul
        b = (b*b) % modul
    return a

The string manipulation is avoidable, by working with bits of the exponent directly:

def quad_pow(base, exponent, modul):
    a = 1
    b = base

    while exponent:
        if exponent & 1:
            a = (a * b) % modul
        b = (b * b) % modul
        exponent >>= 1
    return a


Status updates code design I need help identifying any significant design issues with some code I have generated to track status updates.

I have a big task A which contains multiple small tasks and each of these small tasks updates the status of A in the data store.

This is how I've implemented:
For each type of Task A(A can be of multiple types), I'll create an enum class implementing the interface  StatusTask given below.

And then, I'll call StatusTask.Chain.start(starting enum object, param);

public interface StatusTask<T, V extends Enum<V> & StatusTask<T,V>>
{
    public V execute(T t) throws Exception;

    public void fail(Exception e) throws Exception;

    public static class Chain
    {
        public static <T,V extends Enum<V> & StatusTask<T,V>> void start(V v, T t)
        {
            V n = null;
            try
            {
                n = v.execute(t);
            }
            catch(Exception e)
            {
                v.fail(e);
            }

            if(n == null || n == v)
            {
                return;
            }

            start(n, t);
        }
    }
}

Do you see any significant flaws with this design?
Is there a better design to handle such a scenario?

Example Scenario: Fetching a file and processing it. Following are the task-status:
(0. Initial task. Move to Task 1.)

Send Request and mark status as Sent Request
Successful Response Received and mark status as Response Received and move to task 4
Error Response Received and mark status as Response Received
If file present, then mark status as Processing and execute step 4 else step 5
Mark status as File not available and exit
Parse file. If no error, goto step 7 else step 8
Mark status as File parsing succeeded and do something.....
Mark status as File parsing failed and exit

Single letter variable names are highly discouraged. A variable name should give a reader an idea about what value is stored in it. n, v, and t tells a reader nothing. v and n are the same type, but how would someone who hasn't seen the code before know which means what.

You are checking if n is null. The only way this can happen is if an exception is thrown in execute() or if execute() returnsnull`. In the first case, you can directly return from that catch block. Doing this will more clearly indicate that an exception will stop execution of the chain.

In the second case, that means that null is a valid return value from execute(). However, execute() returns an enum, which is a way to explicitly state all valid values. If null is a valid value, it would just be another state that should be explicitly stated.

The code doesn't have any documentation. While the interface is fairly simple, it does assume that a reader interpreters the signatures properly.

fail() takes an Exception as an argument. But what should the implementer do with this exception. The method can also thrown an exception, does that mean a valid implementation could be the following?

public void fail(Exception ex) throws Exception {
  throw ex;
}

If execute() returns the same enum value that it was passed, the chain of execution is stopped. This is extra logic that would not be clear to someone who hasn't seen the implementation. Since there is no documentation, someone needing to use this code has to look at the code to know that this is the way to stop execution.


CAPTCHA with images of objects This CAPTCHA is not text but basically images of objects that the users has to identify between an array of options. My current code is kind-of slow because the query takes almost .7s to execute and it has to run three times.

I'm looking for a way to improve it and make it faster.

// Loop through to display the pictures and options
for ($i = 0; $i < 2; $i ++) {
    $answers = array();

    // Fill an array with 5 random objects unique objects
    $result = mysqli_query($link, "SELECT MAX(captcha_images.image_code) AS image_code, captcha_objects.name FROM captcha_objects LEFT JOIN captcha_images ON captcha_images.object_id = captcha_objects.id WHERE captcha_images.approved = 1 GROUP BY captcha_objects.name ORDER BY RAND() LIMIT 5") or die(mysqli_error($link));
    while($row = mysqli_fetch_assoc($result)) $answers[] = array("code" => $row['image_code'], "name" => $row['name']);

    // Pick the first one as the correct answer
    $_SESSION['captcha_ans'][] = $answers[0];
    $chosen_objects_id[] = mysqli_get_value("id", "captcha_objects", "name", $answers[0]['name'], false);

    // Shuffle 'em
    shuffle($answers);

    // show picture
    $image = ImageCreateFromString(base64_decode($_SESSION['captcha_ans'][$i]['code']));

    // Generate the output of the image, encode it and include it as a data URI
    ob_start();
    imagejpeg($image);
    $base64 = base64_encode(ob_get_contents());
    ob_end_clean();
    echo '

<select name="answer[]"><option value=""></option>';

    // show answers
    foreach ($answers as $answer)
        echo '<option value="'.$answer['name'].'">'.$answer['name'].'</option>';

    // close options
    echo '</select>
';
    unset($answers);
}

Performance

You didn't post the mysqli_get_value code, but I would assume that it interacts with the database. Is this call really necessary? You don't seem to use the result. But if you do need it somewhere else: couldn't you just retrieve the id in the initial query?

Have you tried storing the path to an image in the database instead of storing the whole image?

As to your main query: ORDER BY RAND() is quite slow. Check out this question for faster solutions to select random rows from a big table. If you google for mysql select random values from table you should find a lot more results as well (for example here).

Also, do captcha_images and captcha_objects really need to be different tables? I don't know your whole db schema, but it seems that it could all go into the same table, thus saving you a join.

Misc

a line length of 323 characters is way too long, it will not fit on any screen. I would aim for 80 (maybe 120).
use curly braces, even for one-line statements (for readability and to prevent bugs).
use spaces around . (I think that it really helps with readability).


Adding N characters to a string I need to prepend N spaces to a string to make it 10 characters in length. I came up with the following method, but since it is going to be called often, I want to ensure it is optimized. Could this be made more efficient?

public static string PrependSpaces(string str)
{
    StringBuilder sb = new StringBuilder();
    sb.Append(str);
    for (int i = 0; i < (10 - str.Length); i++)
    {
        sb.Insert(0, " ");
    }
    return sb.ToString();
}

If you need to prepend a number of characters to a string you might consider String.PadLeft().

For example:

string str = "abc123";
Console.WriteLine(str);
str = str.PadLeft(10);
Console.WriteLine(str);

Will produce:

abc123
    abc123

On the efficiency of your example, StringBuilder.Append() is more efficient than StringBuilder.Insert(), so you might try something like:

public static string PrependSpaces2(string str)
{
    StringBuilder sb = new StringBuilder();
    for (int i = 0; i < (10 - str.Length); i++)
    {
        sb.Append(" ");
    }
    sb.Append(str);
    return sb.ToString();
}

EDIT As svick suggested, Append(char, repeat) is simpler and faster on the order of the native Pad methods:

public static string PrependSpaces3(string str)
    StringBuilder sb = new StringBuilder();
    if((NUM - str.Length) > 0)
    {
        sb.Append(' ', (NUM - str.Length));
    }
    sb.Append(str);
    return sb.ToString();
}

Which when tested with this:

string str = "abc123";
Console.WriteLine("str: " + str);

int iterations = 100000;

string result = string.Empty;
Stopwatch s1 = Stopwatch.StartNew();
for (int i = 0; i < iterations; i++)
{
    result = PrependSpaces(str);
}
s1.Stop();
Console.WriteLine(iterations + " calls to PrependSpaces took: {0:0.00 ms}", s1.Elapsed.TotalMilliseconds);

string result2 = string.Empty;
Stopwatch s2 = Stopwatch.StartNew();
for (int i = 0; i < iterations; i++)
{
    result2 = PrependSpaces2(str);
}
s2.Stop();
Console.WriteLine(iterations + " calls to PrependSpaces2 took: {0:0.00 ms}", s2.Elapsed.TotalMilliseconds);

string result3 = string.Empty;
Stopwatch s3 = Stopwatch.StartNew();
for (int i = 0; i < iterations; i++)
{
    result3 = str.PadLeft(NUM, ' ');
}
s3.Stop();
Console.WriteLine(iterations + " calls to String.PadLeft(" + NUM + ", ' ') took: {0:0.00 ms}", s3.Elapsed.TotalMilliseconds);

string result4 = string.Empty;
Stopwatch s4 = Stopwatch.StartNew();
for (int i = 0; i < iterations; i++)
{
    result4 = PrependSpaces3(str);
}
s4.Stop();
Console.WriteLine(iterations + " calls to PrependSpaces3 took: {0:0.00 ms}", s3.Elapsed.TotalMilliseconds);

returns this (benchmarks for prepending 1000 characters):

str: abc123
100000 calls to PrependSpaces took: 20190.16 ms
100000 calls to PrependSpaces2 took: 1025.87 ms
100000 calls to String.PadLeft(1000, ' ') took: 84.32 ms
100000 calls to PrependSpaces3 took: 84.32 ms

for prepending 10 characters:

100000 calls to PrependSpaces took: 31.02 ms
100000 calls to PrependSpaces2 took: 11.41 ms
100000 calls to String.PadLeft(10, ' ') took: 4.53 ms
100000 calls to PrependSpaces3 took: 4.53 ms