Hints to make Sudoku solver more Pythonic As you will see, I am not very familiar with Python and NumPy but want to learn it.

The following code is a very basic Sudoku solver which works fine for simple examples. Although it runs, I still have the feeling it is not very Pythonic.

from numpy import *
import sys

def set(col,row,wert):
    # Set the value in field and adjust the possibility tensor
    pos[col,row,:] = 0 # the field itself
    pos[:,row,wert] = 0 # the col
    pos[col,:,wert] = 0 # the row

    # The 3x3 block
    col_start = floor(col / 3) * 3
    row_start = floor(row / 3) * 3
    pos[col_start:col_start + 3,row_start:row_start + 3,wert] = 0

    # Write down the value
    feld[col,row] = wert + 1

def read(name):
    # Read in the stuff and create field
    fid = open('in','r')
    for col in range(9):
        line = fid.readline()

        for row in range(9):
            if line[row] is not '0':
                set(row,col,int(line[row]) - 1)
    fid.closed

def write(name):
    # Write the output into a file
    fid = open("out","w")

    for col in range(0,9):
        for row in range(0,9):
            fid.write(str(feld[row,col]))
        fid.write('\n')
    fid.closed

def check():
    # Check the solution for errors
    print("%i Missing" % (81 - count_nonzero(feld)))
    neq = lambda x: count_nonzero(unique(x)) != count_nonzero(x)

    for x in range(9):
        if neq(feld[:,x]):
            print("Error in row %i" % (x))

        if neq(feld[x,:]):
            print("Error in col %i" % (x))

        col_start = floor(x / 3) * 3
        row_start = (x % 3) * 3
        if neq(feld[col_start:col_start + 3,row_start:row_start + 3]):
            print("Error in block %i / %i" % (col_start/3,row_start/3))

def only_pos_left():
    # This point is the only one that can be x
    for x in range(9):
            for y in range(9):
                if sum(pos[x,:,y]) == 1:
                    set(x,nonzero(pos[x,:,y])[0][0],y)

                if sum(pos[:,x,y]) == 1:
                    set(nonzero(pos[:,x,y])[0][0],x,y)

                col_start = floor(x / 3) * 3
                row_start = (x % 3) * 3

                if sum(pos[col_start:col_start + 3,row_start:row_start + 3,y]) == 1:
                    index = nonzero(pos[col_start:col_start + 3,row_start:row_start + 3,y]);
                    set(col_start + index[0][0],row_start + index[1][0],y)

def only_pos_for_it():
    # This point can only be x
    for x in range(9):
        for y in range(9):
            if sum(pos[x,y,:]) == 1:
                set(x,y,nonzero(pos[x,y,:])[0][0])

# main method
if __name__ == "__main__":
    # Init some variables
    feld = zeros((9,9),dtype=uint8) # field with the values were sure about
    pos = ones((9,9,9),dtype=bool_) # values which are still possible
    old_pos = 0

    read(sys.argv[0])

    # Let it run
    while any(old_pos != pos):
        old_pos = copy(pos)
        only_pos_left()
        only_pos_for_it()

    write(sys.argv[1])
    check()

Some comments on it:

I know that from NumPy import * is bad practise but its just so much faster to write.
Am I using the lambdas in a correct manner? I want it to be flexible to add more advanced solving algorithms later on.
Please tell me just anything you would change to make better use of the language (speed and readability wise).

I know that in terms of data structures this could be optimized to use something like this

class point:
    def __init__(self)
        self.val = uint8
        self.pos = zeros(9,dtype=bool_)

but then I would lose all the nice indexing that NumPy offers.

I noticed a few suspicious problems with your coding style.  There's plenty to talk about even without analyzing the sudoku solver portion of your program.

Your functions are actually more like procedures that act on global variables.  For example, your read() doesn't return anything, and your write() doesn't take a parameter for the matrix to be written.  Although set() takes several parameters, the object it mutates is not one of the parameters — it operates on the global pos instead.  Similarly, check(), only_pos_left(), and only_pos_for_it() take no parameters at all, which is a bad sign.

Even when read() and write() take a parameter called name (you probably meant "filename"), they ignore the parameter altogether!  The filenames 'in' and 'out' are hard-coded.

Your read() and write() each has a file descriptor leak.  fid.closed does not close the file handle.  You probably meant fid.close().  But really, you should almost always prefer to open files using a with block instead, so you would never need to worry about having to close them again.

Your read() and write() counterintuitively transpose rows ↔ columns.  I suppose those transpositions cancel each other out, and therefore have no effect.  However, it makes it hard for others to think about or discuss your code.

read() tries to do more than just reading the file.  The function should do one thing only: return a 2-dimensional matrix.  Converting characters to numbers could also be acceptable.  However, constructing the data structure you want to use to represent the sudoku puzzle should be a separate function.  The fact that read() also subtracts one from each value makes it more disconcerting.  Perhaps you do want to have constraints in the 0 to 8 range, but such a transformation definitely doesn't fall under "reading".

To summarize some of the suggestions above…

def read(filename):
    """ Return the contents of the file as a 2-D array. """
    with open(filename) as f:
        return [map(int, row.rstrip()) for row in f.readlines()]


Time Limit Exceeded for build_max_heap This problem is from HackerEarth:

  The Monk learned about priority queues recently and asked his teacher for an interesting problem. So his teacher came up with a simple problem. He now has an integer array A. For each index i, he wants to find the product of the largest, second largest and the third largest integer in the range [1,i].

  Note: Two numbers can be the same value-wise but they should be distinct index-wise.

  Input:

  The first line contains an integer N, denoting the number of elements in the array A.
  The next line contains N space separated integers, each denoting the ith integer of the array A.

  Output:

  Print the answer for each index in each line. If there is no second largest or third largest number in the array A upto that index, then print "-1", without the quotes.

  Constraints:

  1 <= N <= 100000
  0 <= A[i] <= 1000000

  Sample Input

5
1 2 3 4 5

  Sample Output

-1
-1
6
24
60

  Explanation

  There are 5 integers 1,2,3,4 and 5.

  For the first two indexes, since the number of elements is less than 3, so -1 is printed.
  For the third index, the top 3 numbers are 3,2 and 1 whose product is 6.
  For the fourth index, the top 3 numbers are 4,3, and 2 whose product is 24.
  For the fifth index, the top 3 numbers are 5,4 and 3 whose product is 60.

  Time Limit: 1.0 sec(s) for each input file.

  Memory Limit: 256 MB

  Source Limit: 1024 KB

  Marking Scheme: Marks are awarded if any testcase passes.

  Allowed Languages: C, CPP, CLOJURE, CSHARP, GO, HASKELL, JAVA, JAVASCRIPT, JAVASCRIPT_NODE, LISP, OBJECTIVEC, PASCAL, PERL, PHP, PYTHON, RUBY, R, RUST, SCALA

Here is my solution:

#include <iostream>
#include <limits>

#define lld long long
int n, N = 0;
using namespace std;
void max_heapify(int arr[], int n, int i){
    int largest = i;
    int left = 2 * i;
    int right = 2*i + 1;
    if(left <= n && arr[left] > arr[largest]){
        largest = left;
    }
    if(right <= n && arr[right] > arr[largest]){
        largest = right;
    }
    if(largest != i){
        swap(arr[i], arr[largest]);
        max_heapify(arr, n, largest);
    }
}
void build_max_heap(int arr[], int n){
    for(int i = n/2; i >=1; i--){
        max_heapify(arr,n,i);
    }
}

int extract_max(int arr[], int in){

    if(in < 1){
        throw "underflow";
    }

    int max = arr[1];
    arr[1] = arr[in--];
    max_heapify(arr, in, 1);
    return max;
}

int main(){
    cin >> n;
    int in;
    int *arr = new int[n+100];
    int _1st,_2nd,_3rd;
    for (int i = 1; i <= n; ++i) {
        cin >> arr[i];
        build_max_heap(arr, i);

        lld ans;

        if (i <= 2) {
            cout << "-1" << endl;
        }
        else {
            _1st = extract_max(arr, n);
            _2nd = extract_max(arr, n);
            _3rd = extract_max(arr, n);

            ans = _1st * _2nd * _3rd;
            printf("%lld\n", ans);
            int p = i;
            arr[p] = _1st; p--;
            arr[p] = _2nd; p--;
            arr[p] = _3rd;

        }
    }

    return 0;
}

Here I'm calling the build_max_heap for every input, because the problem demand to produce an output after each input so, I think that's causing the problem, but as far as I know, Heapify takes O(log n) time and build_max_heap takes linear time, so it shouldn't exceed more that one second for the 4th input which is here around 1000000 input.

First of all, it's probably best to realize that a heap isn't particularly useful for this problem as it's specified. It can be used, but it doesn't really contribute much (if anything at all).

If you do use a heap, you want to take the specific properties of a max heap into account. In particular, a heap is arranged so each node is at least as large as any of its descendant nodes. As such, the first node is always the largest. The second largest is one of the second and third. In a typical case, the other of the second and third will be the third largest (and depending on how we insert items into the heap, we can enforce that being the case, which we probably want to do here). This allows us to find the three largest items in the heap without actually removing them from the heap at all.

As to why we don't need to use a heap, and why it doesn't really contribute much: we really only care about the three largest items seen at any given point. That being the case, we can start with the first three. They're clearly the largest seen to that point, so their product is the first (meaningful) output. Then we want to find the smallest of those three. Compare that to the fourth item, and if it's larger, copy it over the smallest of the first three, then print the product of the first three. Repeat that (find smallest, check against next item, get new value if larger, print out product) until we reach the end of the input.

This requires something like four comparisons (and possibly one assignment) per iteration, regardless of the number of inputs. A quick test shows it running in ~400 ms on my machine

At least in theory, this can be further optimized by observing that the smallest of the first three can only change when we add a new (larger) item to the first three. When we read a number and it's smaller than the smallest of the first three, we can print the previous product, throw away the new number, and repeat.

At least in a quick test on my machine, the latter doesn't seem to make a noticeable difference though. This is probably because the actual computation is already fast enough to keep up with the I/O, so speeding the computation up even more makes little (or no) difference. It's also possible, however that with only three numbers to look at, it's just as fast to scan them as it is to decide whether to scan them or not.


divide a range of numbers into groups of 10, 100, 1000, 10000 and so on Given a start and end of a range as string, for example:

String start = "1230000";
String end   = "1285999";

Generate the shortest list possible with prefixes that include the whole range,
so that for any number from the given range there is a list elemnet which can be used as a prefix. For above example the list would contain:

 123,124,125,126,127,1280,1281,1282,1283,1284,1285

Explanation

The length of the strings may vary (6 - 12) but start.length() and end.length() are always equal.
The above range can be manually divided into the sub ranges:

    //1230000 - 1239999
    //1240000 - 1249999
    //1250000 - 1259999
    //1260000 - 1269999
    //1270000 - 1279999
    //1280000 - 1289999 this range is not completly included so make the ranges smaller with a factor 10
    //1280000 - 1280999
    //1281000 - 1281999
    //1282000 - 1282999
    //1283000 - 1283999
    //1284000 - 1284999
    //1285000 - 1285999

By storing the common substring of each start and end number of the subranges one can easliy validiate that any given number from the range starts with 123,124,125,126,127,1280,1281,1282,1283,1284 or 1285.

Here is a very naive but working solution to print the prefixes:

public static void main(String[] args) {
    String start = "1230000";
    String end   = "1285999";

    while(start.charAt(start.length()-1) == '0' && end.charAt(start.length()-1) == '9'){
        start = start.substring(0,start.length()-1);
        end = end.substring(0,end.length()-1);
    }

    long st = Long.parseLong(start);
    long en = Long.parseLong(end);
    for(long i = st; i <= en; ){
         if(i % 1000 == 0 && i + 999 <= en){
            System.out.println(i/1000);
            i = i + 1000;
        }
        else if(i % 100 == 0 && i + 99 <= en){
            System.out.println(i/100);
            i = i + 100;
        }
        else if(i % 10 == 0 && i + 9 <= en){
            System.out.println(i/10);
            i = i + 10;
        }
        else {
            System.out.println(i);
            i = i + 1;
        }
    }
}

For some not so nice ranges like the above I have to extend my for loop:

public static void main(String[] args) {
    String start = "1279995";
    String end   = "1285999";

    while(start.charAt(start.length()-1) == '0' && end.charAt(start.length()-1) == '9'){
        start = start.substring(0,start.length()-1);
        end = end.substring(0,end.length()-1);
    }

    long st = Long.parseLong(start);
    long en = Long.parseLong(end);
    for(long i = st; i <= en; ){
        if(i % 1000000 == 0 && i + 999999 <= en){
            System.out.println(i/1000000);
            i = i + 1000000;
        }
        else if(i % 100000 == 0 && i + 99999 <= en){
            System.out.println(i/100000);
            i = i + 100000;
        }
        else if(i % 10000 == 0 && i + 9999 <= en){
            System.out.println(i/10000);
            i = i + 10000;
        }
        else if(i % 1000 == 0 && i + 999 <= en){
            System.out.println(i/1000);
            i = i + 1000;
        }
        else if(i % 100 == 0 && i + 99 <= en){
            System.out.println(i/100);
            i = i + 100;
        }
        else if(i % 10 == 0 && i + 9 <= en){
            System.out.println(i/10);
            i = i + 10;
        }
        else {
            System.out.println(i);
            i = i + 1;
        }
    }
}

Which I found not very nice. Any ideas how to optimise my approach or do some one have another approach (I am open to everything, Regex, streams, recursion .. )?

Even the “extended version” does not handle the cases where start and end have more than 6 trailing zeros in common. Instead of hard-coding those ranges and a long if/else if/... statement you can determine the largest fitting range dynamically in a loop:

for (long i = st; i <= en; ) {
    // Determine `range` as largest power of 10 with `i % range == 0`
    // and `i + range - 1 <= en`:
    int range = 1;
    while (range <= i && i % range == 0 && i + range - 1 <= en) {
        range *= 10;
    }
    range /= 10;

    System.out.println(i / range);
    i += range;
}

Removing common zeros/nines is more efficiently done in integer arithmetic instead of string manipulation:

while (st % 10 == 0 && en % 10 == 9) {
    st /= 10;
    en /= 10;
}

I would move the computation from main() into a separate function and separate it from the I/O. That increases the clarity of the program and allows to add test cases more easily. In a comment you said that you need the result as List<String>:

public static List<String> computePrefixes(long start, long end) {
    List<String> prefixes = new ArrayList<String>();

    while (start % 10 == 0 && end % 10 == 9) {
        start /= 10;
        end /= 10;
    }

    for (long i = start; i <= end; ) {
        // Determine `range` as largest power of 10 with `i % range == 0`
        // and `i + range - 1 <= en`:
        int range = 1;
        while (range <= i && i % range == 0 && i + range - 1 <= end) {
            range *= 10;
        }
        range /= 10;

        prefixes.add(String.valueOf(i / range));
        i += range;
    }

    return prefixes;
}

public static void main(String[] args) {
    String start = "1230000";
    String end   = "1285999";

    long st = Long.parseLong(start);
    long en = Long.parseLong(end);
    List<String> prefixes = computePrefixes(st, en);
    System.out.println(prefixes);
}

You may also want to check the validity of the parameters (1 <= start <= end).

Finally note that there is still a problem with numbers close to Long.MAX_VALUE because the calculations can overflow. If that is an issue then the range calculations and comparisons have to be done more carefully:

public static List<String> computePrefixes(long start, long end) {
    List<String> prefixes = new ArrayList<String>();

    while (start % 10 == 0 && end % 10 == 9) {
        start /= 10;
        end /= 10;
    }

    for (long i = start; ; ) {
        // Determine `range` as largest power of 10 with `i % range == 0`
        // and `i + range - 1 <= end`:
        int range = 1;
        while (range <= Long.MAX_VALUE / 10 && range <= i && i % range == 0 && range - 1 <= end - i) {
            range *= 10;
        }
        range /= 10;

        prefixes.add(String.valueOf(i / range));
        if (i == end) {
            break;
        }
        i += range;
    }

    return prefixes;
}


Word Pattern challenge in LeetCode I solved this problem from LeetCode:

  Given a pattern and a string str, find if str follows the same
  pattern.

  Here follow means a full match, such that there is a bijection between
  a letter in pattern and a non-empty word in str

  Examples:

  pattern = "abba", str = "dog cat cat dog" should return true.

  pattern = "abba", str = "dog cat cat fish" should return false.

  pattern = "aaaa", str = "dog cat cat dog" should return false.

  pattern = "abba", str = "dog dog dog dog" should return false.

  Notes:

  You may assume pattern contains only lowercase letters, and str
  contains lowercase letters separated by a single space.

I was able to solve this problem by using a Dictionary to keep track of the relation between the pattern and the str:

public class Solution {
public bool WordPattern(string pattern, string str)
    {
        var result = str.Split(' ').ToList();
        var mapPattern = new Dictionary<char, string>();
        if (result.Count != pattern.Count())
        {
            return false;
        }
        string matchstr;
        int index = 0;
        foreach (var c in pattern)
        {
            if (mapPattern.TryGetValue(pattern[index], out matchstr))
            {
                if (matchstr != result[index])
                {
                    return false;
                }
            }
            else
            {
                if(mapPattern.ContainsValue(result[index]))
                {
                    return false;
                }
                mapPattern.Add(c, result[index]);
            }
            ++index;
        }
        return true;
    }
}

The approach I have currently looks up the Dictionary twice to see if it already exists as a key or as a value in the Dictionary, which I believe would be the reason for slowing this by a significant amount.

Can you determine if I can do away with looking up twice? This solution only runs better than around 36% of the other submissions for this current problem. So, I believe there must be a better and faster way.

Overall, this is a nice, clean solution, congratulations!

Performance

  Can you suggest If I can do away with looking up twice. Currently this solution only runs better that around 36% of the other submissions for this current problem. So, I believe there must be a better and faster way.

"Looking up twice" is not the appropriate term. You do one lookup by key, and another lookup by value. These are very different operations, and lumping them together as "two lookups" hides important details. Imagine a program that does one lookup in an array and one lookup with a Google search on the internet, and the author would wonder if slowness might be caused by doing "two lookups". It would hide a crucial detail that one of the "lookups" is clearly much slower than the other.

The lookup in the dictionary by key is very fast, an \$O(1)\$ operation.

The lookup by value in a typical dictionary (or hash map) implementation is much slower, \$O(n)\$, because dictionaries are indexed by key, not by value. They are designed for fast lookups by key, not by value.

To make the lookup by value faster, you can add a set for values.

Scope

It's good to limit the scope of variables to the minimum needed.

matchstr should be declared inside the loop, as it is not needed outside
mapPattern should be initialized after the early return, as it might not be needed


Efficiency of grouping a List of case classes by more than one field I have the following input:

case class Client(key: String, time: Option[Instant], bool: Boolean)
val l = List (
  Client("87658763", Some(Instant.EPOCH), false),
  Client("87658769", Some(Instant.EPOCH), false),
  Client("87658769", Some(Instant.EPOCH), true)
)

For context, this data is the result of concatenated Lists from two API calls - one to get all Clients, and one to specifically get all Clients where bool is true.

I want the info to be distinct on the key and time. Also, where there are multiple Clients with the same key and time, if at least one of the Clients contains bool = true, I want to return a single client with bool = true otherwise return a single client with bool = false ... ie I want the output of above to be:

List (
  Client(87658763,Some(1970-01-01T00:00:00Z),false),
  Client(87658769,Some(1970-01-01T00:00:00Z),true)
)

What I have so far is the following code:

import java.time.Instant

case class Client(key: String, time: Option[Instant], bool: Boolean)

val l = List (
  Client("87658763", Some(Instant.EPOCH), false),
  Client("87658769", Some(Instant.EPOCH), false),
  Client("87658769", Some(Instant.EPOCH), true)
)

l.groupBy(client => (client.key, client.time)).map {
  case (_, v) if v.exists(_.bool) => v.head.copy(bool = true)
  case (_, v) => v.head
}.toList

I am curious about efficiency and code style - this is the only way to solve this problem that I could come up with, but I wonder if there is a more efficient way to do this (I don't know much about what the Scala compiler does under the hood with things like groupBy and copy), or a more elegant way to do this.

I've not had to make elements distinct by more than one field before and this call will be made hundreds of times in my application (I am writing this as part of a Play! app) so need it to be as efficient as possible.

Your code is fine

There's nothing wrong with the way you've implemented it. It's readable, and will have reasonable O(n) performance.

Alternative implementation

You've mentioned l is created by concatenation of two lists. As far as I understood one of the lists contains only clients with bool=false and one only clients with bool=true. Some clients will appear on both lists. Example:

val l1 = List(
  Client("87658763", Some(Instant.EPOCH), false),
  Client("87658769", Some(Instant.EPOCH), false),
)

val l2 = List(
  Client("87658769", Some(Instant.EPOCH), true)
)

If this is correct, then an alternative way to implement you function is:

val m1 = l1.map(c => (c.key, c.time) -> c).toMap
val m2 = l2.map(c => (c.key, c.time) -> c).toMap

(m1 ++ m2).values.toList

Or for better (most likely) performance you can use the breakOut trick which will do the mapping and List to Map transformation in single step.

val m1: Map[(String, Option[Instant]), Client] = l1.map(c => (c.key, c.time) -> c)(scala.collection.breakOut)
val m2: Map[(String, Option[Instant]), Client] = l2.map(c => (c.key, c.time) -> c)(scala.collection.breakOut)

(m1 ++ m2).values.toList

Whether this solution is more elegant than your's is subjective. I leave it up to you to decide which one you like more. Whether it's faster should be determined by measurement under production-like load.

First measure & profile

First of all, if you're doing these operations once per the pair of mentioned API calls, then it's likely that the performance will get dominated by the API calls, not this code. If you're doing it more often, then you could remember the results. Before optimising the code I recommend you to measure the performance and determine where is the bottleneck.

Performance factors

If you come to a conclusion that performance of this code is indeed critical, then most likely the problem will be one of these:

allocation of garbage - groupBy, map, toList each allocate a new data structure.  No, the Scala compiler does not optimise this in any way. While allocation is a cheap process, all the garbage will have to be collected, and Garbage Collection can be expensive in some scenarios.
passes over the data set - groupBy, map, toList each is an O(n) operation iterating over the data. Additional these operations may slow down if the data won't easily fit in CPUs cache.

Which one will be dominant, will depend on Heap and GC configuration, the size of l, other workloads in the JVM and other factors. Both can be improved by doing as small number of passes over the data as possible.

Less passes over the data

These are the hints I can give on reducing the number of passes over the data, in this, and other similar situations:

Check If you can avoid some collection conversions. Maybe the Iterable[Client] returned by my is ok, and you don't have to convert it toList.
Use breakOut.
Try doing more in single step. foldLeft is a powerful operation which lets you combine many operations on a collection into one (at the expense of readability).
Use mutable structures under the hood. Keep immutables in the API.


Splitting date and time I asked a question on Stack OverFlow and I got an answer to my question but then I found out the program is crashing because I have 250k data points on my Excel sheet. I was wondering what I can do to make it so it won’t crash. I'm hoping I can understand what I can do to improve the code.

Sub CompareTime()

Dim ws As Worksheet

Dim lastRow As Long
Dim Count As Long
Dim test As Double

Set ws = ActiveSheet

'Find last data point
With ws
    .Columns(2).Insert
    lastRow = .Cells(.Rows.Count, "A").End(xlUp).Row

    For Count = 5 To lastRow
        'split date
        test = .Cells(Count, 1).Value2
        .Cells(Count, 1).Value2 = Int(test)
        .Cells(Count, 1).NumberFormat = "m/d/yyyy"
        .Cells(Count, 1).Offset(0, 1).Value2 = test - Int(test)
        .Cells(Count, 1).Offset(0, 1).NumberFormat = "hh:mm AM/PM"

    Next Count
End With

End Sub

Turning off Application.ScreenUpdating and Application.Calculation will greatly increase the speed.  But reading the data into an Array, modifying it and writing the Array back to the Worksheet is the real key to speed.

Recommended viewing: Excel VBA Introduction Part 20.2 - Application Events, Excel VBA Introduction Part 25 - Arrays

Sub CompareTime()
    'Optimize Setting for faster data writing
    With Application
        .ScreenUpdating = False
        .Calculation = xlCalculationManual
        .EnableEvents = False
    End With

    Dim Target As Range, ws As Worksheet
    Dim x As Long
    Dim results As Variant

    Set ws = ActiveSheet

    'Find last data point
    With ws
        .Columns("B:C").Insert
        .Columns("B:B").NumberFormat = "mm/dd/yy;@"
        .Columns("C:C").NumberFormat = "h:mm:ss;@"

        Set Target = .Range("A5", .Cells(.Rows.Count, "A").End(xlUp)).Resize(, 3)
        results = Target.Value2

        For x = 1 To UBound(results)
            results(x, 2) = Int(results(x, 1))
            results(x, 3) = results(x, 1) - results(x, 2)
        Next

        Target.Value2 = results
    End With

    With Application
        .Calculation = xlCalculationAutomatic
        .EnableEvents = True
        .ScreenUpdating = True
    End With

End Sub

  Set Target = .Range("A5", .Cells(.Rows.Count, "A").End(xlUp)).Resize(, 3)

Code Explaination

.Cells(.Rows.Count, "A").End(xlUp)

Target the last non-empty cell in Column A

.Range("A5", .Cells(.Rows.Count, "A").End(xlUp))
= Target Range from A5 to last non-emtpy cell in Column A
.Resize(, 3)
= Resize the Target range to include 2 more Column to the right same number of rows
results = Target.Value2

Create an array of values from the Target range.
.Value 2 is more effiecent than .Value because it ignores formatting

Target.Value2 = results

Assign the modified values from the results Array back to the Target range.


refactor python strategy pattern to use abstract base class I came across this strategy pattern implementation
https://github.com/jtortorelli/head-first-design-patterns-python/blob/master/src/python/chapter_1/adventure_game.py
class Character:
    def __init__(self):
        self.weapon_behavior = None

    def set_weapon(self, weapon_behavior):
        self.weapon_behavior = weapon_behavior

    def fight(self):
        self.weapon_behavior.use_weapon()

class Queen(Character):
    def __init__(self):
        super().__init__()
        self.weapon_behavior = KnifeBehavior()

class King(Character):
    def __init__(self):
        super().__init__()
        self.weapon_behavior = BowAndArrowBehavior()

class Troll(Character):
    def __init__(self):
        super().__init__()
        self.weapon_behavior = AxeBehavior()

class Knight(Character):
    def __init__(self):
        super().__init__()
        self.weapon_behavior = SwordBehavior()

class WeaponBehavior:
    def use_weapon(self):
        raise NotImplementedError

class KnifeBehavior(WeaponBehavior):
    def use_weapon(self):
        print("Stabby stab stab")

class BowAndArrowBehavior(WeaponBehavior):
    def use_weapon(self):
        print("Thwing!")

class AxeBehavior(WeaponBehavior):
    def use_weapon(self):
        print("Whack!")

class SwordBehavior(WeaponBehavior):
    def use_weapon(self):
        print("Thrust!")

knight = Knight()
king = King()
queen = Queen()
troll = Troll()
knight.fight()
king.fight()
queen.fight()
troll.fight()

Would it be correct to refactor it the following way, using an ABC?
from abc import ABC, abstractmethod

class Character:
    def __init__(self):
        self.weapon_behavior = None

    def set_weapon(self, weapon_behavior):
        self.weapon_behavior = weapon_behavior

    def fight(self):
        self.weapon_behavior.use_weapon()

class Queen(Character):
    def __init__(self):
        super().__init__()
        self.weapon_behavior = KnifeBehavior()

class King(Character):
    def __init__(self):
        super().__init__()
        self.weapon_behavior = BowAndArrowBehavior()

class Troll(Character):
    def __init__(self):
        super().__init__()
        self.weapon_behavior = AxeBehavior()

class Knight(Character):
    def __init__(self):
        super().__init__()
        self.weapon_behavior = SwordBehavior()

class WeaponBehavior(ABC):
    @abstractmethod
    def use_weapon(self, message):
        print(message)

class KnifeBehavior(WeaponBehavior):

    def use_weapon(self):
        super().use_weapon("Stabby stab stab")

class BowAndArrowBehavior(WeaponBehavior):
    def use_weapon(self):
        super().use_weapon("Thwing!")

class AxeBehavior(WeaponBehavior):
    def use_weapon(self):
        super().use_weapon("Whack!")

class SwordBehavior(WeaponBehavior):
    def use_weapon(self):
        super().use_weapon("Thrust!")

knight = Knight()
king = King()
queen = Queen()
troll = Troll()
knight.fight()
king.fight()
queen.fight()
troll.fight()

Only the Character and WeaponBehaviour classes are actually useful and neither of them needs to inherit anything. The rest can be just factory functions, because only the constructors differ. If a class constructor does anything except assign arguments to properties, it is probably wrong.
Strategy pattern is based on composition rather then inheritance.
class Character:
    def __init__(self, weapon):
        self.weapon = weapon

    def set_weapon(self, weapon):
        self.weapon = weapon

    def fight(self):
        self.weapon.use()

def Queen():
    return Character(Knife())

def King():
    return Character(Bow())

class Weapon:
    def __init__(self, message):
       self.message = message
    def use(self):
       print(self.message)

def Knife():
    return Weapon("Stabby stab stab")

def Bow():
    return Weapon("Thwing!")

king = King()
queen = Queen()
king.fight()
queen.fight()
queen.set_weapon(Bow())
queen.fight()

Notice that I have removed the behavior part of the names as it seemed a bit useless.
I have also renamed use_weapon() to just use() because it is called on a weapon variable, and so it seemed redundant.
Also notice, that unless I used the set_weapon() method on a constructed Character instance (ie. to switch weapon in middle of battle), the Character class would be useless, because everything could have been done with the weapons alone. Of course I know this is just a pattern demonstration code, but I wanted to point out anyway..
As a bonus, here is something (not only) for the king :) Also notice how again composition is prefered over inheritance to provide flexibility.
class DoubleWeapon:
    def __init__(self, left, right):
        self.left = left
        self.right = right
    def use(self):
        self.left.use()
        self.right.use()

king.set_weapon(DoubleWeapon(Knife(), Sword()))
king.fight()
```


Deduplicate << operator implementation on inheritance I have the following working code:
person.h
#ifndef PERSON_H
#define PERSON_H

#include <iostream>
#include <string>

class Person {
private:
    std::string firstName;
    std::string middleName;
    std::string lastName;

public:
    Person() = default;
    Person (std::string pFirstName, std::string pMiddleName, std::string pLastName);
    Person (std::string pFirstName, std::string pLastName);

    std::string getFirstName();
    void setFirstName(std::string pFirstName);
    std::string getLastName() const;
    void setLastName(std::string pLastName);
    std::string toString() const;

    friend std::ostream& operator<<(std::ostream& target, Person const & person);
};

#endif

person.cpp
#ifndef __person__
#define __person__

#include <iostream>
#include <string>

#include "person.h"

using std::string;
using std::ostream;

Person::Person (string pFirstName, string pMiddleName, string pLastName)
: firstName(pFirstName), middleName(pMiddleName), lastName(pLastName)
{
}

Person::Person (string pFirstName, string pLastName)
: firstName(pFirstName), middleName(""), lastName(pLastName)
{
}

string Person::getFirstName ()
{
    return firstName;
}

void Person::setFirstName (string pFirstName)
{
    firstName = pFirstName;
}

string Person::getLastName () const
{
    return lastName;
}

void Person::setLastName (string pLastName)
{
    lastName = pLastName;
}

string Person::toString () const
{
    if (middleName == "")
        return firstName + " " + lastName;

    return firstName + " " + middleName + " " + lastName;
}

ostream& operator<< (ostream& target, Person const & source)
{
    target << source.toString();
    return target;
}

#endif

tweeter.h
#ifndef TWEETER_H
#define TWEETER_H

#include <string>

#include "person.h"

class Tweeter
: public Person
{
private:
    static int gid;
    int id;
    std::string twitterHandle;

public:
    Tweeter(std::string pFirstName, std::string pMiddleName, std::string pLastName, std::string pTwitterHandle);
    Tweeter(std::string pFirstName, std::string pLastName, std::string pTwitterHandle);
    std::string toString() const;
    friend std::ostream& operator<< (std::ostream& target, Tweeter const & source);
};

#endif

tweeter.cpp
#ifndef __tweeter__
#define __tweeter__

#include <iostream>

#include "tweeter.h"

using std::ostream;
using std::string;
using std::to_string;

int Tweeter::gid = 0;

Tweeter::Tweeter(string pFirstName, string pMiddleName, string pLastName, string pTwitterHandle)
: Person(pFirstName, pMiddleName, pLastName), id(gid), twitterHandle(pTwitterHandle)
{
    gid++;
}

Tweeter::Tweeter(string pFirstName, string pLastName, string pTwitterHandle)
: Tweeter(pFirstName, "", pLastName, pTwitterHandle)
{
}

string Tweeter::toString() const
{
    return "[" + to_string(id) + "] " + Person::toString() + " (" + twitterHandle + ")";
}

ostream& operator<< (ostream& target, Tweeter const & source)
{
    target << source.toString();
    return target;
}

#endif

As you can see, I have a nearly duplicate implementation for the << operator in the subclass Tweeter. I had to do that, since, if I just use the << operator implementation inherited from Person, C++ would silently cast the Tweeter instance as Person as per the operator function's signature and call Person::tostring() instead of Tweeter::toString().
Is it possible to just have one implementation of the << operator that can be inherited without override and thusly code duplication?

Yes use a virtual print() function.
PS. I would also make the toString use the virtual print function to make it more effecient.
class Person
{
    // OTHER STUFF

    virtual void print(std::ostream& target = std::cout) const
    {
        target << firstName << " ";
        if (middleName != "") {
            target << middleName << " ";
        }
        target << lastName;
    }
    std::string toString() const
    {
        std::stringstream  stream;
        this->print(str);
        return stream.str();
    }

    friend std::ostream& operator<<(std::ostream& target, Person const& person)
    {
        person.print(target);
        return target;
    }
};

class Tweeter: public Person
{
    // OTHER STUFF

    // Don't need this any more: => std::string toString() const;
    // Don't need this any more: => friend std::ostream& operator<< (std::ostream& target, Tweeter const & source);

    virtual void print(std::ostream& target = std::cout) const override
    {
         target << "[" << to_string(id) << "] ";
         Person::print(target);
         target <<  " (" << twitterHandle << ")";
    }
};


Using functions to separate code for Fahrenheit to celsius conversion I'm relatively new to programming, and I've been wondering about a problem such as this. How can I think about decomposing the code into its constituent functions? I went the simplest route I could, just putting the formula in its own function. I believe this makes the code more readable, but there's also a part of me that wants to put the while loop into a function as well.

Is there a good rule of thumb for separating my code into functions?

#include <stdio.h>

int fahr_to_celsius(int f);

/* print Farhenheit-Celsius table
   for fahr = 0, 20, ..., 300 *;
   use a function for conversion */
int main(void)
{
  int fahr, celsius;
  int lower, upper, step;

  lower = 0; /* lower limit of the temperature table */
  upper = 300; /* upper limit */
  step = 20; /* step size */

  fahr = lower;
  while (fahr <= upper) {
    celsius = fahr_to_celsius(fahr);
    printf("%d\t%d\n", fahr, celsius);
    fahr = fahr + step;
    }
}

int fahr_to_celsius(int fahr)
{
  int celsius = 5 * (fahr-32) / 9;
  return celsius;
}

In a more complex program I would probably extract the loop, but I think this is actually pretty good as is. Your main routine reads very well and describes precisely what this program does. You separated the concerns of performing the conversion and displaying the results and your conversion method performs a single task. I agree that extracting it was a wise choice.

You'll often hear the phrase

  A method should do one thing, and do it well.

You nailed the first part, but I'm not so sure about the second. I guess it depends on how accurate you need it to be.

  int celsius = 5 * (fahr-32) / 9;

This is very likely to be a non-integer number before you cast it. C casts from floating point to integer via truncation. This means that you're always "rounding" the result down. I doubt that's what you intended. Use the round() method instead for a more accurate result. Or, even better, return a double.

One other thing, don't abbreviate variable names. I understand why you used fahr instead of fahrenheit, but it's a bad habit to get into.


Bank ATM program in Python For homework, I have to code a program in Python that effectively simulates a bank ATM.

print('Welcome to Northen Frock Bank ATM')
restart=('Y')
chances = 3
balance = 67.14
while chances >= 0:
    pin = int(input('Please Enter You 4 Digit Pin: '))
    if pin == (1234):
        print('You entered you pin Correctly\n')
        while restart not in ('n','NO','no','N'):
            print('Please Press 1 For Your Balance\n')
            print('Please Press 2 To Make a Withdrawl\n')
            print('Please Press 3 To Pay in\n')
            print('Please Press 4 To Return Card\n')
            option = int(input('What Would you like to choose?'))
            if option == 1:
                print('Your Balance is Â£',balance,'\n')
                restart = input('Would You you like to go back? ')
                if restart in ('n','NO','no','N'):
                    print('Thank You')
                    break
            elif option == 2:
                option2 = ('y')
                withdrawl = float(input('How Much Would you like to
withdraw? \nÂ£10/Â£20/Â£40/Â£60/Â£80/Â£100 for other enter 1: '))
                if withdrawl in [10, 20, 40, 60, 80, 100]:
                    balance = balance - withdrawl
                    print ('\nYour Balance is now Â£',balance)
                    restart = input('Would You you like to go back? ')
                    if restart in ('n','NO','no','N'):
                        print('Thank You')
                        break
                elif withdrawl != [10, 20, 40, 60, 80, 100]:
                    print('Invalid Amount, Please Re-try\n')
                    restart = ('y')
                elif withdrawl == 1:
                    withdrawl = float(input('Please Enter Desired amount:'))

            elif option == 3:
                Pay_in = float(input('How Much Would You Like To Pay In? '))
                balance = balance + Pay_in
                print ('\nYour Balance is now Â£',balance)
                restart = input('Would You you like to go back? ')
                if restart in ('n','NO','no','N'):
                    print('Thank You')
                    break
            elif option == 4:
                print('Please wait whilst your card is Returned...\n')
                print('Thank you for you service')
                break
            else:
                print('Please Enter a correct number. \n')
                restart = ('y')
    elif pin != ('1234'):
        print('Incorrect Password')
        chances = chances - 1
        if chances == 0:
            print('\nNo more tries')
            break

This program basically does what it is designed to do, but I want to learn how
to use the def() and do it this way.

The best way to do this would be to take each of your sections, and give them each a function of their own. Also, when it's possible, separate user input functions from purely logic to improve readability!

For example, it could start looking like this:

def verify_pin(pin):
    if pin == '1234':
        return True
    else:
        return False

def log_in():
    tries = 0
    while tries < 4:
        pin = input('Please Enter Your 4 Digit Pin: ')
        if verify_pin(pin):
            print("Pin accepted!")
            return True
        else:
            print("Invalid pin")
            tries += 1
    print("To many incorrect tries. Could not log in")
    return False

def start_menu():
    print("Welcome to the atm!")
    if log_in():
        # you will need to make this one yourself!
        main_menu()
    print("Exiting Program")

start_menu()

Whenever you can avoid nesting, you should. This makes it easier to read.

In this case you can clearly see logging in will open the main_menu (which you will have to write), and entering a correct pin is the only way to log in.


Codility Frog Jump solutions Problem details from Codility:

  A small frog wants to get to the other side of the road. The frog is currently located at position X and wants to get to a position greater than or equal to Y. The small frog always jumps a fixed distance, D.
  Count the minimal number of jumps that the small frog must perform to reach its target.

  Write a function:

def solution(X, Y, D)

  that, given three integers X, Y and D, returns the minimal number of jumps from position X to a position equal to or greater than Y.

Method 1:

def solution(X, Y, D):
    if (X == Y):
        jumps = 0
    elif Y-X % D == 0:
        jumps = (Y-X)/D
    else:
        jumps = ((Y-X)/D) + 1
    return jumps

Method 2:

def solution(X, Y, D):
    d = Y-X
    jumps=d/D
    rem=d%D
    if rem != 0:
        jumps+=1
    return jumps

I submitted the Method1 solution and received a 55% score. On the other hand, Method2 got 100%. It's not clear to me why Method2 is better than Method1.  Could someone please explain?

Method 1 sometimes gives wrong solution because it has a bug:

def solution(X, Y, D):
    if (X == Y):
        jumps = 0
    elif Y-X % D == 0:
        jumps = (Y-X)/D
    else:
        jumps = ((Y-X)/D) + 1
    return jumps

In the elif there, Y-X should have been within brackets, because % has higher precedence than - operator. In other words, Y - X % D is not the same as (Y - X) % D.

Other than that, they are practically equivalent. I would clean up the formatting and remove some obvious temporary variables, like this:

def solution(X, Y, D):
    jumps = (Y - X) / D
    if (Y - X) % D > 0:
        return jumps + 1
    return jumps

In particular, in the if instead of incrementing with jumps += 1, I return the value immediately, because I prefer to avoid mutating variables in general.


Encoding a string into base64 I'm trying to create a simple HTTP client. In order to make one that is standard compliant, I must be able to extract user information from a URI and convert it to base64 and send it via an authentication header to the server. I created this demo program to test my progress on this function I've made. Does this program seem sufficient? Is there any major flaws with it? You can see the demo here.

char *base64encode(char *input) {
    int len = strlen(input), padding = (len % 3)?3 - (len % 3):0, n, xlen = 0;
    size_t bitlen = len * 8;
    uint8_t *x = malloc(((int)(bitlen / 24) * 4) + ((padding)?4:0)), i, o;
    for (n = 0; n < len; n++) {
        switch (n % 3) {
            case 0:
                i = (uint8_t)(input[n] >> 2);
                x[xlen++] = i;
                break;
            case 1:
                i = (uint8_t)((input[n-1] & 3) << 4);
                o = (uint8_t)((input[n] & 240) >> 4);
                x[xlen++] = i | o;
                break;
            case 2:
                i = (uint8_t)((input[n-1] & 15) << 2);
                o = (uint8_t)((input[n] & 192) >> 6);
                x[xlen++] = i | o;
                i = (uint8_t)(input[n] & 63);
                x[xlen++] = i;
                break;
        }
        if ((len - n) == ((padding==2)?2:3) && padding > 0) {
            n++;
            break;
        }
    }
    if (padding == 2) {
        i = (uint8_t)(input[n] >> 2);
        x[xlen++] = i;
        i = (uint8_t)((input[n] & 3) << 4);
        x[xlen++] = i;
    } else if (padding == 1) {
        i = (uint8_t)(input[n] >> 2);
        x[xlen++] = i;
        i = (uint8_t)((input[n] & 3) << 4);
        o = (uint8_t)((input[n+1] & 240) >> 4);
        x[xlen++] = i | o;
        i = (uint8_t)((input[n+1] & 15) << 2);
        x[xlen++] = i;
    }
    const char *index = "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789+/";
    char *returnString = malloc((int)((bitlen / 24) + padding) * 4 + 1);
    for (n = 0; n < ((int)(bitlen / 24) * 4) + ((padding)?4:0) - padding; n++) {
        returnString[n] = index[x[n]];
    }
    free(x);
    for (;padding > 0; padding--) {
        returnString[n++] = '=';
    }
    returnString[n] = '\0';
    return returnString;
}

Bugs

I found a couple of bugs in your program.

Your end-of-string handling is off for input strings smaller than 3 bytes.  This is because your end-of-string check is too late to stop the first byte from being processed.  So for the input "a", your function returns "YA==" instead of "YQ==".  See below for a solution for this problem.
If the input string contains characters with the high bit set, such as what could happen with UTF-8 input, then your function handles that incorrectly.  The culprit is that you use signed chars, and do bit shifting with those signed chars.  Imagine that input[n] is 0x80 here:

        i = (uint8_t)(input[n] >> 2);

What you would like to be calculated for i is 0x20.  But what you will actually calculate for i is 0xe0.  Later you will use 0xe0 as an index into your string and it will overflow the bounds.  You can fix this by moving your cast to the right place:

        i = ((uint8_t)input[n] >> 2);

One pass instead of two

Right now you are building up an array x of indices and then you use a second pass to convert the x array into the base64 string.  It would be quicker and simpler to just build up returnString directly and get rid of the intermediary x array.  For example, instead of:

        case 0:
            i = (uint8_t)(input[n] >> 2);
            x[xlen++] = i;
            break;

You could write:

        case 0:
            i = (uint8_t)(input[n] >> 2);
            returnString[xlen++] = index[i];
            break;

Simplifying the loop termination check

Right now, you have a complicated check that looks like this:

    if ((len - n) == ((padding==2)?2:3) && padding > 0) {
        n++;
        break;
    }

As mentioned earlier, this check was also insufficient for strings smaller than 3 bytes.  A simpler way of doing this is just to modify len to round down to the nearest multiple of 3 before starting the loop.  This does exactly what you wanted and also handles smaller than 3 byte strings:

    len -= (len % 3);
    for (n = 0; n < len; n++) {
        switch (n % 3) {
            ...
        }
        // Complicated check removed.
    }

Getting rid of the switch

Once you have made len a multiple of three (above), you can actually get rid of the switch statement.  Instead of iterating over every byte of the input, you can now iterate by three bytes at a time and do all three switch cases one after the other.

Confusing bitmask constants

Your code is correct, but I had a hard time trying to figure out what some of your constants meant.  For example:

            o = (uint8_t)((input[n] & 240) >> 4);

would make a lot more sense if you used the hex value like this:

            o = (uint8_t)((input[n] & 0xf0) >> 4);

Now I know you are taking the top 4 bits and shifting them down.

Reusing common code

After the main loop when you handle the two possible padding cases, most of the code for the padding == 2 case is also done in the padding == 1 case.  You can combine some of the common code together so that you don't repeat the code twice.  This is a matter of preference though, so what you have is fine if you find it more readable.

Other things

You should check the return value of malloc.
Your input argument is unmodified so you can make it a const argument.
You don't really need both i and o although if you like them to be separate variables you can.

Putting it all together

Here's what your code would look like after all my suggested changes (and a lot of other tweaks that are stylistic/optional):

char *base64encode(const char *input)
{
    int      len      = strlen(input);
    int      leftover = len % 3;
    char    *ret      = malloc(((len/3) * 4) + ((leftover)?4:0) + 1);
    int      n        = 0;
    int      outlen   = 0;
    uint8_t  i        = 0;
    uint8_t *inp      = (uint8_t *) input;
    const char *index = "ABCDEFGHIJKLMNOPQRSTUVWXYZ"
                        "abcdefghijklmnopqrstuvwxyz"
                        "0123456789+/";

    if (ret == NULL)
        return NULL;

    // Convert each 3 bytes of input to 4 bytes of output.
    len -= leftover;
    for (n = 0; n < len; n+=3) {
        i = inp[n] >> 2;
        ret[outlen++] = index[i];

        i  = (inp[n]   & 0x03) << 4;
        i |= (inp[n+1] & 0xf0) >> 4;
        ret[outlen++] = index[i];

        i  = ((inp[n+1] & 0x0f) << 2);
        i |= ((inp[n+2] & 0xc0) >> 6);
        ret[outlen++] = index[i];

        i  = (inp[n+2] & 0x3f);
        ret[outlen++] = index[i];
    }

    // Handle leftover 1 or 2 bytes.
    if (leftover) {
        i = (inp[n] >> 2);
        ret[outlen++] = index[i];

        i = (inp[n]   & 0x03) << 4;
        if (leftover == 2) {
            i |= (inp[n+1] & 0xf0) >> 4;
            ret[outlen++] = index[i];

            i  = ((inp[n+1] & 0x0f) << 2);
        }
        ret[outlen++] = index[i];
        ret[outlen++] = '=';
        if (leftover == 1)
            ret[outlen++] = '=';
    }
    ret[outlen] = '\0';
    return ret;
}


Battleship Game in Java I wanted to make a very simply Battleship game in Java, any feedback is welcome.

Nevermind the getters / setters. Just assume they are there.

class Game {
    Board board = new Board();

    void playGame() {
        Scanner scanner = new Scanner(System.in);
        while (!board.allShipsSank()) {
            System.out.println("Enter missile locations, x and y.");
            int x = scanner.nextInt();
            int y = scanner.nextInt();
            board.shoot(x, y);
        }
    }
}

class Board {
    Set<Ship> ships = new HashSet<>();

    boolean allShipsSank() {
        for (Ship ship : ships)
            if (!ship.isSank())
                return false;
        return true;
    }

    void shoot(int x, int y) {
        for (Ship ship : ships) {
            if (ship.shipGeographic.orientation == ShipGeographic.Orientation.HORIZONTAL) {
                if (y == ship.shipGeographic.y && ship.shipGeographic.x <= x && x <= ship.shipGeographic.x + ship.len) {
                    ship.missileAt(x - ship.shipGeographic.x);
                    return;
                }
            }
            if (ship.shipGeographic.orientation == ShipGeographic.Orientation.VERTICAL) {
                if (x == ship.shipGeographic.x && ship.shipGeographic.y <= y && y <= ship.shipGeographic.y + ship.len) {
                    ship.missileAt(ship.shipGeographic.y + y);
                    return;
                }
            }
        }
    }
}

class ShipGeographic {
    enum Orientation {
        HORIZONTAL, VERTICAL;
    }

    int x;
    int y;
    ShipGeographic.Orientation orientation;
}

class Ship {
    // length of the ship
    int len;

    // Nodes where this ship has been hit
    Set<Integer> hit = new HashSet<>();

    // Where this Ship is
    ShipGeographic shipGeographic;

    Ship(int len) {
        this.len = len;
    }

    boolean isSank() {
        return hit.size() == len;
    }

    // For example, if ship is hit at head, parameter is 0
    // Missile at same node of ship has no effect
    void missileAt(int offsetFromLen) {
        if (hit.add(offsetFromLen)) {
            System.out.println("boom!");
            System.out.println(hit);
        }
    }
}

here is how I initialize and run the game:

class App {
    public static void main(String[] args) {
        Game game = new Game();

        // Just some sample data!
        {
            Ship ship = new Ship(3);
            ShipGeographic shipGeographic = new ShipGeographic();
            shipGeographic.orientation = ShipGeographic.Orientation.HORIZONTAL;
            shipGeographic.x = 1;
            shipGeographic.y = 1;

            ship.shipGeographic = shipGeographic;

            game.board.ships.add(ship);
        }

        game.playGame();
    }
}

A sample run:

Enter missile locations, x and y.
1
1
boom!
[0]
Enter missile locations, x and y.
5
5
Enter missile locations, x and y.
2
1
boom!
[0, 1]
Enter missile locations, x and y.
3
1
boom!
[0, 1, 2]

Process finished with exit code 0

Thanks for sharing your code.

what I like

you follow the Java Naming conventions
you resist to solve the problem based on an array

what I dislike

violation of encapsulation / information hiding

You access directly properties of an object and (even worse) their properties too like this:

x == ship.shipGeographic.x && ship.shipGeographic.y <= y

But the code using an object should not know anything about the internal implementation pf this object. The using code should only call (public) methods on on an object. This enables both: polymorphism and improving the internal implementation without affecting the calling code. The getters and setters you mention are not a solution to this problem.

The names for this "code smells" is feature envy: the calling code does something what should better be done inside the object by applying the "tell, don't ask" principle.

separation of concerns - intermix of user IO and business logic

In your code the Board class is responsible for the user IO and also does game logic. This reduces the  re-usability of the game logic. I.e.: you have to do a complete rewrite if you want to change the User interface to  graphics instead of a command line. You should learn about the Model-View-Controller concept (MVC) and its relatives MCP, MVVC and MVVM.

unnecessary mutability

Your ship objects are mutable, this means their properties could be changed at any time during the game. But in practice the properties never change after initialization. Therefore your should pass all the properties as constructor parameters and store them in final declared properties.

incomplete OO approach.

IMHO you stopped doing an OO approach half way.

Holding a list of ships which know their positions is a good first step. But you could walked that path further.

E.g. The list of ships could only hold unsunk ones. Then the check for the end of the game would change from a loop to a simple statement:

boolean allShipsSank() {
    return listOfShips.isEmpty();
}

Next possibility could be the "ship hit" logik.

For once the ship should know itself if being hit:

class Ship {
  //...
  boolean isHitBy(int x, int y) {
    // logic here
  }
  //...
}

but instead of doing a calculation here I'd go the "collection" approach again.

First we need to introduce another class:

class BordPosition{ // this is a DTO and has no business logik
   final int x;
   final int y;
   BordPosition(int x, int y){
     this.x = x;
     this.y = y;
   }
   // implement equals() and hascode()
}

and we need to enhance the Orientation enum:

enum Orientation {
    HORIZONTAL{
        public BordPosition translate(BordPosition startPoint, int stepsToMove){
           return new BordPosition(startPoint.x+stepsToMove, startPoint.y);
        }
    },
    VERTICAL{
        public BordPosition translate(BordPosition startPoint, int stepsToMove){
           return new BordPosition(startPoint.x, startPoint.y+stepsToMove);
        }
    };
    abstract public BordPosition translate(BordPosition startPoint, int stepsToMove);
}

then class Ship would change to this

class Ship {
  private final Collection<BordPosition> occupiedFields = new HashSet<>();
  private final Collection<BordPosition> hitFields = new HashSet<>();
  Ship(BordPosition startPoint, Orientation orientation, int size){
    for (int stepsToMove = 0; stepsToMove<size; stepsToMove++) {
        occupiedFields.add(orientation.translate(startPoint,stepsToMove));
    }
  }

  boolean isHitBy(BordPosition missleTarget) {
    if(occupiedFields.contains(missleTarget)){
      hitFields.add(missleTarget);
      if (hitFields.size()==occupiedFields.size()){
       // deregister from ship list via Listener pattern
      }
      return true;
    } else {
      return false;
    }
  }
  //...
}


Python class to abstract the iptables Linux firewall I've written the following Python class that fetches iptables rules using basic string processing in Linux. Am I doing this in the correct manner? I've tested the code and it's running fine.

I'm ultimately going to use this class in a GUI app that I'm going to write using Python + GObject.

#!/usr/bin/python
import sys, subprocess

class Fwall:
    currchain=""
    dichains={'INPUT':[],'OUTPUT':[],'FORWARD':[]}
    dipolicy=dict(INPUT=True,OUTPUT=True,FORWARD=True)
    dicolumns={}

def __init__(self):
    self.getrules()
    self.printrules()
    print "class initialized."

def getrules(self):
    s = subprocess.check_output("iptables --list --verbose",shell=True,stderr=subprocess.STDOUT)#.split('\n')
    print s
    for line in s.splitlines():
        if len(line)>0:
            self.parseline(line)

def parseline(self,line):
    line=line.strip()
    if line.startswith("Chain"): #this is the primary header line.
        self.currchain=line.split(' ')[1]
        allowed=not line.split('policy ')[1].startswith('DROP')
        self.dipolicy[self.currchain]=allowed
        #print "curr chain set to " + self.currchain
    else:
        items=line.split()
        if line.strip().startswith('pkts'): #this is the secondary header line, so fetch columns.
            if len(self.dicolumns)==0:
                for i,item in enumerate(items):
                    if len(item)>0:
                        self.dicolumns.setdefault(item,i)
                #print self.dicolumns
            else:
                return
        else:
            ditemp={}
            #print line
            self.dichains[self.currchain].append(ditemp)
            for k,v in self.dicolumns.iteritems():
                #idx= self.dicolumns[item]#,items[item]
                ditemp.setdefault(k,items[v])
                #print line
                #print k,v
                #print k,items[v]#,idx

def printrules(self):
    for k,v in self.dichains.iteritems():
        print k
        litemp=self.dichains[k]
        for item in litemp:
            print item

if __name__=="__main__":
    f=Fwall()

There are violations of PEP-8 throughout and there are no PEP-257 docstrings. It sounds petty, but these mean the difference between an easy read and a hard one.

The class name Fwall is terrible, if you mean Firewall call it that. If you mean that it is a container of FirewallRules then say so. Names are extremely important.

If getrules() is not part of the public class interface, convention suggests naming it _getrules which tells the reader that it is expected to be used only by the class itself. Having print statements is fine for debugging, but a well behaved library should assume that there is no stdout and return lists of strings in production code.

Long lines are hard to read, especially on StackExchange. Fortunately, splitting within parenthesis is natural and preferred, thus:

s = subprocess.check_output("iptables --list --verbose",
        shell=True, stderr=subprocess.STDOUT)

but sticking a comment marker in code will certainly cause misreadings. Where you have

s = subprocess.check_output(...)#.split('\n')

you should drop the #.split('\n'). Also, following PEP-8, put spaces after argument separating commas.

You use iteritems() unnecessarily as the standard iteration behavior of a dictionary is now assumed. Where you have for k,v in self.dicolumns.iteritems(): the line

for k, v in self.dicolumns:

is equivalent. To bang on the "names are important" drum, every dictionary has k and v, using more descriptive names would tell me what the meaning of the keys and values are. For that matter I have no idea was a dipolicy or dicolumns are, they might be related or parallel lists but I don't know. You tend to use if len(item)==0 when if item means the same thing.

I had to edit your entry because the class methods were not properly indented; this usually means that you are using a mix of tabs and spaces. Tell your editor to forget that the TAB character exists and to use only spaces, there are instructions for most editors on how to make it pretend that the TAB key behaves like you expect without putting literal \t characters in your source code.

The structure of parseline() is so deeply nested and has so many return points and un-elsed ifs that I really can't follow the logic of it. Read The Zen of Python and keep it wholly.

Finally, iptables is notoriously hairy. I strongly suggest that you use python-iptables or at least study its class structure. You might also want to look at Uncomplicated Firewall as it is designed to abstract iptables into something most humans can understand. It doesn't handle all possible uses, but it makes the typical cases comprehensible.


Trying to catch and throw exceptions if error message window is found I'm writing a test to validate that while clicking through a series of menu items, an error message pops up, and if it does, then I should throw an exception.

Now I wonder: Is it appropriate to use nested try-catch block here?

public static void main(String[] args) throws Exception{
        File file = new File("C:/Program Files/Selenium/IEDriverServer.exe");
        System.setProperty("webdriver.ie.driver", file.getAbsolutePath());
        driver.get("http://ap-apbase-d002/Ecx.Web/Account/Login/?ReturnUrl=%2fEcx.Web%2f");
        WebElement element;
        element = driver.findElement(By.id("username"));
        element.sendKeys("Jh");
        element = driver.findElement(By.id("password"));
        element.sendKeys("1");
        element.submit();
        WebElement homeFrame = (new WebDriverWait(driver, 30)).until(ExpectedConditions.presenceOfElementLocated(By.id("pnCenter_IFrame")));
        driver.switchTo().frame("pnCenter_IFrame");
        WebElement myHomeLink = (new WebDriverWait(driver, 10)).until(ExpectedConditions.presenceOfElementLocated(By.linkText("My Projects")));
        driver.findElement(By.linkText("My Projects")).click();
        driver.switchTo().defaultContent();
        driver.switchTo().frame("pnCenter_IFrame");
        driver.switchTo().defaultContent();
        //parent try block
        try{
        WebElement UniversalInboxLink = (new WebDriverWait(driver, 10)).until(ExpectedConditions.presenceOfElementLocated(By.linkText("Universal Inbox")));
        driver.findElement(By.linkText("Universal Inbox")).click();
        WebElement MyProjectsLink = (new WebDriverWait(driver, 10)).until(ExpectedConditions.presenceOfElementLocated(By.linkText("My Projects")));
        driver.findElement(By.linkText("My Projects")).click();
        WebElement AllProjectsLink = (new WebDriverWait(driver, 10)).until(ExpectedConditions.presenceOfElementLocated(By.linkText("All Projects")));
        driver.findElement(By.linkText("All Projects")).click();
        WebElement MyTeamLink = (new WebDriverWait(driver, 10)).until(ExpectedConditions.presenceOfElementLocated(By.linkText("My Team")));
        driver.findElement(By.linkText("My Team")).click();
        WebElement WOTTaxFormLink = (new WebDriverWait(driver, 10)).until(ExpectedConditions.presenceOfElementLocated(By.linkText("WOT Tax Form")));
        driver.findElement(By.linkText("WOT Tax Form")).click();
        WebElement MyDraftsLink = (new WebDriverWait(driver, 10)).until(ExpectedConditions.presenceOfElementLocated(By.linkText("My Drafts")));
        driver.findElement(By.linkText("My Drafts")).click();
        WebElement MyArchivesLink = (new WebDriverWait(driver, 10)).until(ExpectedConditions.presenceOfElementLocated(By.linkText("My Archives")));
        driver.findElement(By.linkText("My Archives")).click();
        WebElement RetrieveLink = (new WebDriverWait(driver, 10)).until(ExpectedConditions.presenceOfElementLocated(By.linkText("Retrieve")));
        driver.findElement(By.linkText("Retrieve")).click();
            //Child try block
            try{
        List <WebElement> AppbaseErrorWindow = driver.findElements(By.cssSelector(".x-window-header"));
        for (WebElement item : AppbaseErrorWindow) {
            if (item.getText().contains("Request Failure")) {
                //Do Nothing
            }
        }
            }catch (Exception e){
        System.out.println("Exception is found here: " + e);

            }
            System.exit(0);
            driver.quit();
    }
        catch(Exception e){
        System.out.println("All is OK.");
        }
        System.out.println("Normal behavior");
 }

The code that you submitted is not complete as it was missing the driver variable (Had to look up the type as I have not used Selenium).

private WebDriver driver;

First off move everything out of the main method even if you are going to only use one instance, whilst doing this you should also separate the code into single responsibility methods. This will improve the readability of the code and give the program a nicer "flow".

Methods you come to mind are:

private void init();
private void login(String username, String password);
private void clickElement(String element) throws Exception;
private void processWebElements(String header);
private void terminate();

init() will be:

private void init() {
    // The driver you want to use
    driver = new FirefoxDriver();
    File file = new File("C:/Program Files/Selenium/IEDriverServer.exe");
    System.setProperty("weObdriver.ie.driver", file.getAbsolutePath());
    driver.get("http://ap-apbase-d002/Ecx.Web/Account/Login/?ReturnUrl=%2fEcx.Web%2f");
}

login() will be:

private void login(String username, String password) {
    WebElement element;
    element = driver.findElement(By.id("username"));
    element.sendKeys(username);
    element = driver.findElement(By.id("password"));
    element.sendKeys(password);
    element.submit();

    WebElement homeFrame = (new WebDriverWait(driver, 30)).until(ExpectedConditions.presenceOfElementLocated(By.id("pnCenter_IFrame")));
    driver.switchTo().frame("pnCenter_IFrame");
    WebElement myHomeLink = (new WebDriverWait(driver, 10)).until(ExpectedConditions.presenceOfElementLocated(By.linkText("My Projects")));
    driver.findElement(By.linkText("My Projects")).click();
    driver.switchTo().defaultContent();
    driver.switchTo().frame("pnCenter_IFrame");
    driver.switchTo().defaultContent();
}

clickelement() will be:

private void clickElement(String element, int wait) throws Exception {
    WebElement universalInboxLink = (new WebDriverWait(driver, wait)).until(ExpectedConditions.presenceOfElementLocated(By.linkText(element)));
    driver.findElement(By.linkText(element)).click();
}

processWebElements() will be:

private void processWebElements(String header) {
    List<WebElement> AppbaseErrorWindow = driver.findElements(By.cssSelector(header));
    for (WebElement item : AppbaseErrorWindow) {
        if (item.getText().contains("Request Failure")) {
            //Do Nothing
        }
    }
}

finally terminate() will be:

private void terminate() {
    System.exit(0);
    driver.quit();
}

This way you can easily change a bit of the program without rewriting for every time you wanted to call do something. Your try-catch can also be unested as the try inside will only start when everything inside the outside try block is finished. Using the above functions my constructor would be as follows:

public WebAuto() {
    init();
    login("Jh", "1");

    try {
        clickElement("Universal Inbox", 10);
        clickElement("My Projects", 10);
        clickElement("All Projects", 10);
        clickElement("My Team", 10);
        clickElement("WOT Tax Form", 10);
        clickElement("My Drafts", 10);
        clickElement("My Archives", 10);
        clickElement("Retrieve", 10);
        terminate();
    } catch (Exception e) {
        System.out.println("All is OK.");
    }

    try {
        processWebElements(".x-window-header");
    } catch (Exception e) {
        System.out.println("Exception is found here: " + e);
    }
    System.out.println("Normal behavior");
}

Extra: You should set up constants for strings such as file locations.


Dictionary for locking on unique identifiers without leaking memory The following code is greatly inspired on this question and this answer. That answer works quite well for a specific case, but I was looking for a solution that:

could be used more generic (so I can put this in a base library and never have to think about it anymore).
got rid of the Find() and replace it with a O1 dictionary lookup. To be honest I doubt if this would have any performance effect, but it was tickling my OCDs.

I've tried to test it with running about 10 threads and superficially it seems to work. But as concurrency is hard to test and reason about - am I missing something? Any other obvious upgrades?

public class LockDictionary<TKey>
{
    object _RootLock = new object();
    Dictionary<TKey,ItemLock> _KeyToLock = new Dictionary<TKey,ItemLock>();

    public IDisposable Lock(TKey key)
    {
        ItemLock item_lock;

        lock  ( _RootLock )
        {
            item_lock = GetOrCreate(key);
            item_lock._Counter++;
        }

        Monitor.Enter(item_lock);

        return new LockCounter(this, key);
    }

    private ItemLock GetOrCreate(TKey key)
    {
        if ( !_KeyToLock.TryGetValue(key, out var item_lock) )
        {
            item_lock = new ItemLock();
            _KeyToLock.Add( key, item_lock );
        }

        return item_lock;
    }

    private void ReleaseLock(TKey key)
    {
        lock (_RootLock)
        {
            var item_lock = _KeyToLock[key];
            item_lock._Counter--;

            if ( item_lock._Counter == 0 )
                _KeyToLock.Remove(key);

            Monitor.Exit(item_lock);
        }
    }

    private class ItemLock
    {
        public object   _ItemLock   = new object();
        public int      _Counter    = 0;
    }

    private class LockCounter : IDisposable
    {
        LockDictionary<TKey> _Parent;
        TKey _Key;

        public LockCounter(LockDictionary<TKey> parent, TKey key)
        {
            _Parent     = parent;
            _Key        = key;
        }

        public void Dispose()
        {
            _Parent.ReleaseLock(_Key);
        }
    }
}

Update

To clarify; I'm looking for an implementation for the following:

// minimal definition boilerplate
// possibility of using other types as key, like string, long or a struct
static LockDictionary<int> _LockDictionary = new LockDictionary<int>();

// minimal boilerplate for having a separate lock per "groupId"
using ( _LockDictionary.Lock(groupId) )
{
     // can only be run once concurrently per unique groupId
}

ItemLock._ItemLock is not used anywhere. Also, as a side note, I'm not a fan of using _NamesLikeThis for public items
Using a single lock may not be the best idea performance-wise (though heavily scenario-dependent). Consider using CuncurrentDictionary (which of course brings all sorts of new propblems, but this is how concurrency works). Also, you may consider using a ReadWriteLockSlim, it might be more performant in some scenarios
ReleaseLock is not safe if not matched by a Lock call. Which might happen for example in case of a ThreadAbort. An ugly, but widespread way of dealing with it is placing the critical code to a finally block. This article from the SQL Server people might also be of interest: https://docs.microsoft.com/en-us/dotnet/framework/performance/reliability-best-practices


Is it okay to use multiple repositories from inside a single service implementation in Spring? I currently have two entities, Adress and Company. They both have DTOs to perform CRUD operations.

AddressCreateDto:

@Getter
@Setter
public class AddressCreateDto {

    @NotNull
    private String address1;

    private String address2;

    @NotNull
    private String city;

    @NotNull
    private String state;

    @NotNull
    private int zipCode;

    @JsonIgnore
    private final Long createdBy = 1L;

    @JsonIgnore
    private final Date createdDate = new Date();

    @JsonIgnore
    private final Long lastModifiedBy = 1L;

    @JsonIgnore
    private final Date lastModifiedDate = new Date();

}

CompanyCreateDto:

@Getter
@Setter
public class CompanyCreateDto {

    @NotNull
    private String name;

    @NotNull
    private AddressCreateDto address;

    @JsonIgnore
    private final Long createdBy = 1L;

    @JsonIgnore
    private final Date createdDate = new Date();

    @JsonIgnore
    private final Long lastModifiedBy = 1L;

    @JsonIgnore
    private final Date lastModifiedDate = new Date();

}

In my CompanyServiceImpl file, I am using both the CompanyRepository and the AddressRepository but I don't know if this is bad practice. The code works, but not sure if there is a better way of rewriting this so that it follows a specific set of conventions.

CompanyServiceImpl:

@Service
public class CompanyServiceImpl implements CompanyService {

    private final CompanyRepository companyRepo;

    private final AddressRepository addressRepo;

    @Autowired
    public CompanyServiceImpl(CompanyRepository companyRepo, AddressRepository addressRepo) {
        this.companyRepo = companyRepo;
        this.addressRepo = addressRepo;
    }

    @Override
    public CompanyReadDto create(CompanyCreateDto dto) {
        Address address = ObjectMapperUtils.map(dto.getAddress(), Address.class);
        address = addressRepo.save(address);

        Company company = ObjectMapperUtils.map(dto, Company.class);
        company.setAddress(address);

        return ObjectMapperUtils.map(companyRepo.save(company), CompanyReadDto.class);
    }

    ...

}

EDIT:

CompanyReadDto:

@Getter
@Setter
public class CompanyReadDto {

    @Id
    @NotNull
    private Long id;

    @NotNull
    private String name;

    @NotNull
    private AddressReadDto address;

}

AddressReadDto:

@Getter
@Setter
public class AddressReadDto {

    @Id
    @NotNull
    private Long id;

    @NotNull
    private String address1;

    private String address2;

    @NotNull
    private String city;

    @NotNull
    private String state;

    @NotNull
    private int zipCode;

}

Best practice to perform CRUD operations is to have this flow for an entity-

View --passing Dto--> Controller -- passing Dto/Entity--> Service --passing Entity--> Repo

In your example you can have both repositories inside a service as both entities ( company and address ) are associated.

Also, to fall in line you can save address entity along with country entity using cascade


Grid of radio buttons in Android I want to display multiple radio buttons in a layout with multiline style as an item inside a RecyclerView. Look at the code I provided below and suggest how I can refactor the code to be more efficient or optimized:

Layout in XML:

<RadioButton
    android:id="@+id/rb_rad1"
    android:layout_width="wrap_content"
    android:layout_height="wrap_content"
    android:layout_marginTop="@dimen/dp_10"
    android:checked="true"
    android:text="@string/text1"
    app:layout_constraintLeft_toLeftOf="parent"
    app:layout_constraintTop_toBottomOf="@+id/xyz" />

<RadioButton
    android:id="@+id/rb_rad2"
    android:layout_width="wrap_content"
    android:layout_height="wrap_content"
    android:layout_marginLeft="@dimen/dp_34"
    android:checked="false"
    android:text="@string/text2"
    app:layout_constraintBaseline_toBaselineOf="@+id/rb_rad1"
    app:layout_constraintLeft_toRightOf="@+id/rb_rad1" />

<RadioButton
    android:id="@+id/rb_rad3"
    android:layout_width="wrap_content"
    android:layout_height="wrap_content"
    android:checked="false"
    android:text="@string/text3"
    app:layout_constraintLeft_toLeftOf="@+id/rb_rad1"
    app:layout_constraintTop_toBottomOf="@+id/rb_rad1" />

<RadioButton
    android:id="@+id/rb_rad4"
    android:layout_width="wrap_content"
    android:layout_height="wrap_content"
    android:checked="false"
    android:text="@string/text4"
    app:layout_constraintLeft_toLeftOf="@+id/rb_rad2"
    app:layout_constraintTop_toBottomOf="@+id/rb_rad2" />

Inside the onBindViewHolder() method:

holder.binding.rbRad1.setOnClickListener(object : View.OnClickListener {
                override fun onClick(view: View?) {
                    holder.binding.rbRad2.isChecked = false
                    holder.binding.rbRad4.isChecked = false
                    holder.binding.rbRad3.isChecked = false
                    notifyDataSetChanged()
                }
            })

            holder.binding.rbRad3.setOnClickListener(object : View.OnClickListener {
                override fun onClick(view: View?) {
                    holder.binding.rbRad2.isChecked = false
                    holder.binding.rbRad4.isChecked = false
                    holder.binding.rbRad1.isChecked = false
                    notifyDataSetChanged()
                }
            })

            holder.binding.rbRad2.setOnClickListener(object : View.OnClickListener {
                override fun onClick(view: View?) {
                    holder.binding.rbRad3.isChecked = false
                    holder.binding.rbRad4.isChecked = false
                    holder.binding.rbRad1.isChecked = false
                    notifyDataSetChanged()
                }
            })

            holder.binding.rbRad4.setOnClickListener(object : View.OnClickListener {
                override fun onClick(view: View?) {
                    holder.binding.rbRad2.isChecked = false
                    holder.binding.rbRad3.isChecked = false
                    holder.binding.rbRad1.isChecked = false
                    notifyDataSetChanged()
                }
            })

In your layout XML file, keep the checked value of all RadioButton to false.

<RadioButton
    android:id="@+id/rb_rad1"
    android:layout_width="wrap_content"
    android:layout_height="wrap_content"
    android:layout_marginTop="@dimen/dp_10"
    android:checked="false"
    android:text="@string/text1"
    app:layout_constraintLeft_toLeftOf="parent"
    app:layout_constraintTop_toBottomOf="@+id/xyz" />
...

Inside onBindViewHolder() method, pass the clicked RadioButton to another method customRadioButtonOnClick where you'll check what value the passed RadioButton holds and based on that, set the checked values of that one or the other buttons to true or false as per your logic like this:

        rb_rad1.setOnClickListener {
            customRadioButtonOnClick(rb_rad1)
        }
        rb_rad2.setOnClickListener {
            customRadioButtonOnClick(rb_rad2)
        }
        rb_rad3.setOnClickListener {
            customRadioButtonOnClick(rb_rad3)
        }
        rb_rad4.setOnClickListener {
            customRadioButtonOnClick(rb_rad4)
        }

customRadioButtonOnClick

private fun customRadioButtonOnClick(radioButton: RadioButton) {
    when (radioButton) {
        rb_rad1 -> rb_rad1.isChecked = true
        rb_rad2 -> rb_rad2.isChecked = true
        rb_rad3 -> rb_rad3.isChecked = true
        rb_rad4 -> rb_rad4.isChecked = true
    }
}


Python. Refactoring a function to increase its capabilities I'm writing a bot, it turns out pretty big (1000 lines).
In code, I often use the "show profile" function. I noticed that I greatly complicate it by adding functionality with kwargs.
I am a newbie, I don’t know if I should create a class instead of a function, break it into different functions or continue in the same manner.

Now I need to add 2 features:

"show profile when creating for confirmation"
"Keyboard selection", if you want to show a group of profiles (the most important)

Basically, these problems arise because I make a bot using a module and in the conditions of a strict API.
If you have some remarks related to security, performance, etc - please, tell me them.

Sum up: I have three functions:

get_keyboard
show_profile
show_group_of_profiles

show_group_of_profiles must call show_profile, and show_profile must call get_keyboard. But show_group_of_profiles must call get_keyboard (via show_profile) with a specific argument, that in reality hardcoded in show_profile. I can pass this argument via kwargs, but it is a bad approach I think

def show_profile(user_id, profile_data=None, photo=None):

""" Profile it is just a message object with photo, text (caption) and (inline) keyboard (markup).
    Photo=None because While a recreating a new profile, old photos will be taken from DB,
    but you can't remove them until a user will approve a new profile (first missing feature).
    db_execute - manually created function, the short version of cursor.execute (pymysql)"""

    if not profile_data:
        profile_data = db_execute('SELECT goal, age, gender, country, city, comment FROM users WHERE user_id = %s',
                                  user_id, fetchone=True, get_tuple=True)
    else:
        profile_data = list(profile_data.values())

    if not photo:
        photo = db_execute('SELECT photo FROM photos WHERE user_id = %s and id = (SELECT MIN(id) FROM photos)',
                           user_id, fetchone=True)
        if not photo:
            photo = DEFAULT_PHOTO[0]

    username = bot.get_chat(user_id).username  # Get first and last name of user
    caption = f'Имя - {username}.\n'  # add link to user inside a message
    profile_data = {'Goal': profile_data[0], 'Agr': profile_data[1], 'Gender': profile_data[2],
                    'Country': profile_data[3], 'City': profile_data[4], 'Comment': profile_data[5]}

    # Remove None data from profile
    for key, value in dict(profile_data).items():  # Dict() to avoid 'dictionary changed size during iteration' error
        if value is None:
            del profile_data[key]
        else:
            caption += f'{key} - {value}\n'

    message = bot.send_photo(user_id, photo, caption=caption, parse_mode=ParseMode.HTML,
                             reply_markup=get_scrolling_profile_keyboard(user_id))
    return message  # To allow edits

def get_scrolling_photos_keyboard(shower_id):
    return InlineKeyboardMarkup([[
        InlineKeyboardButton('<', callback_data=f'back_photo {shower_id}'),
        InlineKeyboardButton('>', callback_data=f'next_photo {shower_id}')]])

def get_scrolling_profiles_keyboard(shower_id):

""" Add more buttons for scrolling, group of photos and group of profiles
(the profile is still just a message object with photo, text, and buttons)
(The second missing feature)"""

    keyboard_obj = get_scrolling_photos_keyboard(shower_id)
    buttons = keyboard_obj.inline_keyboard  # Get list from object
    buttons.append(InlineKeyboardButton('Back', callback_data=f'back_profile'))
    buttons.append(InlineKeyboardButton('Next', callback_data=f'next_profile'))
    return InlineKeyboardMarkup(buttons)

I can't comment much on the questions you asked about the functions that haven't been implemented yet, other than to say that passing an argument down through multiple function calls is perfectly fine.  If your code cleanly declares what arguments it expects and what each argument does it shouldn't be a problem.  I did my best to clean up the code you provided, mostly just fixing PEP8 style stuff and trying my best to fix the typing (although it's hard without having any of your dependencies to check against, I tried to tidy up stuff like having profile_data go from dict to list back to dict).  I also replaced the janky in-place dictionary modification with a comprehension.

from typing import Dict, Optional

def show_profile(
    user_id: str,
    profile_data: Optional[Dict[str, str]] = None,
    photo: Optional[str] = None
) -> Message:  # fixme: what's the actual type of this?
    """
    A profile is a message object with a photo, text (caption), and markup.
    If no profile data is specified it will be loaded from the db.
    If no photo is specified, it will be loaded from the db,
    or DEFAULT_PHOTO will be used.
    """
    # db_execute - manually created function,
    # the short version of cursor.execute (pymysql)"""
    if profile_data is None:
        goal, age, gender, country, city, comment = db_execute(
            'SELECT goal, age, gender, country, city, comment '
            'FROM users '
            'WHERE user_id = %s',
            user_id, fetchone=True, get_tuple=True
        )
        profile_data = {
            'Goal': goal,
            'Age': age,
            'Gender': gender
            'Country': country,
            'City': city,
            'Comment': comment
        }

    if photo is None:
        photo = db_execute(
            'SELECT photo '
            'FROM photos '
            'WHERE user_id = %s and id = (SELECT MIN(id) FROM photos)',
            user_id, fetchone=True
        ) or DEFAULT_PHOTO[0]

    # Get first and last name of the user.
    username = bot.get_chat(user_id).username

    # add link to user inside a message
    caption = f'Имя - {username}.\n'

    # Remove None data from profile
    profile_data = {k: v for k, v in profile_data.items() if v is not None}
    # Add it to the caption
    for k, v in profile_data.items():
        caption += f'{key} - {value}\n'

    return bot.send_photo(
        user_id,
        photo,
        caption=caption,
        parse_mode=ParseMode.HTML,
        reply_markup=get_scrolling_profile_keyboard(user_id)
    )

def get_scrolling_photos_keyboard(shower_id: str) -> InlineKeyboardMarkup:
    return InlineKeyboardMarkup([[
        InlineKeyboardButton('<', callback_data=f'back_photo {shower_id}'),
        InlineKeyboardButton('>', callback_data=f'next_photo {shower_id}')]])

def get_scrolling_profiles_keyboard(shower_id: str) -> InlineKeyboardMarkup:
    """
    Add more buttons for scrolling, group of photos and group of profiles
    (the profile is still just a message object with photo, text, and buttons)
    (The second missing feature)
    """
    keyboard_obj = get_scrolling_photos_keyboard(shower_id)
    buttons = keyboard_obj.inline_keyboard  # Get list from object
    buttons.append(InlineKeyboardButton('Back', callback_data=f'back_profile'))
    buttons.append(InlineKeyboardButton('Next', callback_data=f'next_profile'))
    return InlineKeyboardMarkup(buttons)


Finding all paths from a given graph I need to find all paths from a given graph. I can do that for now, however my recursive code is not efficient and my graphs are very complicated, hence I need a better algorithm.

def findLeaves(gdict):
    # takes graph and find its leaf nodes
    leaves = []
    for endNode in gdict.iterkeys():
        if not gdict[endNode]:
            leaves.append(endNode)
    return leaves

graphPaths = {}

def findPaths(gname, gdict, leave, path):
    # finds all noncycle paths
    if not gdict:
        return []
    temp = [node for node in gdict.iterkeys() if leave in gdict[node].keys() and node not in path]
    if temp:
        for node in temp:
            findPaths(gname, gdict, node, [node] + path)
    else:
        graphPaths[gname].append(path)

graph = {}
graph['graph'] = {0: {1: {}}, 1: {2: {}, 3: {}}, 2: {3: {}}, 3: {4: {}}, 4: {5: {}}, 5: {6: {}}, 6: {7: {}}, 7: {6: {}, 5: {}, 8: {}}, 8: {}}
graph['name'] = 'nameofthegraph'

# main
leaves = findLeaves(graph['graph'])
graphPaths['nameofthegraph'] = []

seenNodes = []
for leave in leaves:
    findPaths(graph['name'], graph['graph'], leave, [leave])

for path in graphPaths.values()[0]:
    print path

There is only one starting node, which makes things easier for recursive function. Every leaf needs to arrive there if a leaf is followed in reverse order.  The starting node is the one which does not have an incoming edge.

These structures are taken from pygraphviz, which simply shows the outgoing edges from any nodes. Keys are the nodes and values are outgoing edges to the nodes. However, when I have very complicated graphs as below, this code could not find all paths.

Is there any better algorithm that you can suggest? Or is there any way to optimize my algorithm for complicated graphs?

1. Introduction

Your goal (if I understand correctly, of generating all the execution paths in a state transition graph for a Java thread and then using those execution paths to test the code) seems unrealistic to me.

Even in an acyclic graph (or with the condition that you must not revisit a state) there may be exponentially many paths. In a graph with cycles (like any realistic state transition graph) there are infinitely many paths.

You cannot afford the time to generate all these path, let alone the time to run the test cases based on the paths: the best you can hope for is to intelligently (or randomly) sample the space of paths.

2. Detailed review

There are no docstrings. What do your functions do, what data structures do they act on, and how do I call them?
The singular of leaves is leaf in English (not leave).
Your program has code at the top level of the module. This makes it inconvenient to test the code from the interactive interpreter: each time you reload the module, the program runs. It would be better to put the top-level code into a function and call this function inside the guard if __name__ == '__main__':.
Your program relies on a global variable graphPaths. This makes it inconvenient to test the program repeatedly, as you have to ensure that this variable is reinitialized each time you want to run the test case. Instead of passing gname and appending to graphPaths[gname] inside findPaths, it would be better to pass the list as an argument to findPaths.
But it would be even better not to append paths to a list, but instead to generate them one at a time using Python's yield instruction. That would allow you to interleave the generation of the paths with their further processing, and would avoid attempting to store exponentially many paths in memory.
There's a global variable seenNodes which is not used.
There's no point in writing:

for endNode in gdict.iterkeys():

when you could achieve the same thing by writing:

for endNode in gdict:

This also has the advantage of being portable to Python 3.
The lines:

if not gdict:
    return []

are unnecessary. You can't call findPaths unless you can give it a vertex for the leave argument, which means that the graph must be non-empty.
The argument leave to findPaths is poorly named. On the first call to the function it is (what you call) a leaf, but thereafter it can be any vertex.
The leave argument to findPaths is always the first element of the argument path. So why not get it from there and avoid repeating yourself?
Your algorithm searches backwards through the graph from its sinks. This is inconvenient because in order to prepare the list temp of vertices that might appear on the path before the current vertex, you have to search the whole graph. It would be simpler to search forwards because then you could look up the neighbours directly.
Your choice of graph data structure is not one of the standard representations such as adjacency list (a map from a vertex to a list or set of its neighbouring vertices). It looks to me as though your representation is "map from vertex to map from neighbouring vertex to the empty map". Why did you choose this representation?

Instead of the representation:

{0: {1: {}}, 1: {2: {}, 3: {}}, 2: {3: {}}, 3: {4: {}}, 4: {5: {}}, 5: {6: {}}, 6: {7: {}}, 7: {6: {}, 5: {}, 8: {}}, 8: {}}

I would use:

{0: [1], 1: [2, 3], 2: [3], 3: [4], 4: [5], 5: [6], 6: [7], 7: [6, 5, 8], 8: []}

or the similar alternative with sets instead of lists.
Your test case is very small: it only has two paths. This means that it does not really exercise the code. See §4 below for some more thorough test cases.

3. Revised code

def paths(graph, v):
    """Generate the maximal cycle-free paths in graph starting at v.
    graph must be a mapping from vertices to collections of
    neighbouring vertices.

    >>> g = {1: [2, 3], 2: [3, 4], 3: [1], 4: []}
    >>> sorted(paths(g, 1))
    [[1, 2, 3], [1, 2, 4], [1, 3]]
    >>> sorted(paths(g, 3))
    [[3, 1, 2, 4]]

    """
    path = [v]                  # path traversed so far
    seen = {v}                  # set of vertices in path
    def search():
        dead_end = True
        for neighbour in graph[path[-1]]:
            if neighbour not in seen:
                dead_end = False
                seen.add(neighbour)
                path.append(neighbour)
                yield from search()
                path.pop()
                seen.remove(neighbour)
        if dead_end:
            yield list(path)
    yield from search()

Notes:

This uses the yield from statement and so requires Python 3.3 or later. If you have to use an earlier Python, you can rewrite yield from search() as:

for p in search():
    yield p

seen is a collection containing the same vertices as path, but in a set for efficient computation of neighbour not in seen. It would be nice to be able to use a single data structure here (an ordered set), but Python does not provide one. We might consider using the keys of an OrderedDict or Raymond Hettinger's ordered set recipe.
This implementation maintains the search state on the Python call stack (the current position in the iteration over the neighbouring vertices, and the value of dead_end) and so a large graph could exceed the recursion limit. In this case you could increase sys.setrecursionlimit, or maintain your own stack (for example, using the stack of iterators pattern).

4. Test cases

from itertools import chain, permutations
from unittest import TestCase

class PathTest(TestCase):
    def test_complete(self):
        # The maximal paths in a complete graph are the permutations
        # of the vertices.
        n = 8
        vertices = list(range(n))
        g = {v: vertices for v in vertices}
        found = sorted(chain.from_iterable(paths(g, i) for i in vertices))
        expected = map(list, permutations(vertices))
        for p, q in zip(found, expected):
            self.assertEqual(p, q)

    def test_binary(self):
        # Two alternatives at each of n places yields 2 ** n paths.
        n = 10
        g = {n * 2: [], n * 2 + 1: []}
        g.update({i: [i + 2, i + 3] for i in range(0, 2 * n, 2)})
        g.update({i: [i + 1, i + 2] for i in range(1, 2 * n, 2)})
        found = sorted(paths(g, 0))
        def expected():
            for i in range(2 ** n):
                p = []
                for j in reversed(range(0, 2 * n + 2, 2)):
                    p.append(j + i % 2)
                    i //= 2
                yield p[::-1]
        for p, q in zip(found, expected()):
            self.assertEqual(p, q)