Sudoku solver using backtracting and depth-first-search I've recently finished my first ever C program. I implemented a Sudoku solver that uses backtracking.

Does my code look like a proper C-program? Are there any recommendations for improving my code?

#include <stdio.h>

int valid(int[][9], int, int, int);
int solve(int[][9]);
int find_empty_cell(int[][9], int *, int *);

int main() {
  int puzzle[9][9] = {{1, 7, 4, 0, 9, 0, 6, 0, 0},
                      {0, 0, 0, 0, 3, 8, 1, 5, 7},
                      {5, 3, 0, 7, 0, 1, 0, 0, 4},
                      {0, 0, 7, 3, 4, 9, 8, 0, 0},
                      {8, 4, 0, 5, 0, 0, 3, 6, 0},
                      {3, 0, 5, 0, 0, 6, 4, 7, 0},
                      {2, 8, 6, 9, 0, 0, 0, 0, 1},
                      {0, 0, 0, 6, 2, 7, 0, 3, 8},
                      {0, 5, 3, 0, 8, 0, 0, 9, 6}};
  int row = 0;
  int column = 0;

  if (solve(puzzle)) {
    printf("\n+-----+-----+-----+\n");
    for (int x = 0; x < 9; ++x) {
      for (int y = 0; y < 9; ++y) printf("|%d", puzzle[x][y]);
      printf("|\n");
      if (x % 3 == 2) printf("\n+-----+-----+-----+\n");
    }
  }
  else {
    printf("\n\nNO SOLUTION FOUND\n\n");
  }

  return 0;
}

int valid(int puzzle[][9], int row, int column, int guess) {
  int corner_x = row / 3 * 3;
  int corner_y = column / 3 * 3;

  for (int x = 0; x < 9; ++x) {
    if (puzzle[row][x] == guess) return 0;
    if (puzzle[x][column] == guess) return 0;
    if (puzzle[corner_x + (x % 3)][corner_y + (x / 3)] == guess) return 0;
  }
  return 1;
}

int find_empty_cell(int puzzle[][9], int *row, int *column) {
  for (int x = 0; x < 9; x++) {
    for (int y = 0; y < 9; y++) {
      if (!puzzle[x][y]) {
        *row = x;
        *column = y;

        return 1;
      }
    }
  }
  return 0;
}

int solve(int puzzle[][9]) {
  int row;
  int column;

  if(!find_empty_cell(puzzle, &row, &column)) return 1;

  for (int guess = 1; guess < 10; guess++) {
    if (valid(puzzle, row, column, guess)) {
      puzzle[row][column] = guess;

      if(solve(puzzle)) return 1;
      puzzle[row][column] = 0;
    }
  }
  return 0;
}

Overall it looks fine for a beginner-level program, you keep the functions small and separate different parts of the logic between them, which is good.

int valid(int[][9], int, int, int); You should make a habit of naming your parameters even during function declaration. Ideally the function definition is a copy/paste of the declaration.
(Minor remark) It would be clearer to write int puzzle[9][9] in the parameter list, even though it makes no practical difference. It gives self-documenting code, however.
But you should entirely avoid "magic numbers" like 9 in your code. You should have #define SUDOKU_SIZE 9 or such instead. This goes for all the divide by 3 statements too, it's not clear to the reader where the "magic number" 3 came from. This is where you would need to place a comment, since the code doesn't speak for itself.
(Major issue) Your indention style is exotic and therefore wouldn't be tolerated in a professional setting. For example the line following an if or for etc should be placed on a line of its own, and also indented (2 or 4 spaces are ok):

for (int y = 0; y < 9; ++y)
  printf("|%d", puzzle[x][y]);

For mission-critical programs, this style isn't tolerated either, but you need to always use { } compound statements even if there is just a single line.
(Minor remark) int puzzle[9][9] This could perhaps have been declared const, so you have one read-only table of the original data, and place the solution in a different matrix. You could then also have implemented const correctness for functions that do not modify this matrix.
(Minor remark) Coding style preference - avoid using the ! for operands that aren't boolean. Instead of !puzzle[x][y], I'd write puzzle[x][y]==0 since the purpose is to check if something is 0, not false.
Instead of returning 1/0 type int, you should be using true/false and bool from stdbool.h.
(Minor remark) This int main() is obsolete style in C. You should be using int main(void) instead. (C and C++ are different here)
(Minor remark) Your functions are pretty short so I'd say it is ok to return from multiple places inside them. For more complex functions, multiple returns from a function is sometimes frowned upon and it's preferred to only have a single return at the end. This is no black/white rule though. I say avoid multiple return statements unless it actually makes the code more readable. In your current program it's fine.
The matrix print could be made a function of its own, to be called from main().


Retrieval of the corners of a mask I have trained a polygon detector neural network to recognize the mask of "quadrilateral" (the mask generates curvy lines so it's not exactly a quadrilateral). I would like to get the corners of the quadrilateral.

I believe the best approach is to get the points in the mask that are closest to the corners of the image. First question is, are these valid assumptions? Second question is, is this the best approach?

Top-Left is minimum distance between (0,0) and mask.
Top-Right is minimum distance between (width, 0) and mask.
Bottom-Left is minimum distance between (0, height) and mask.
Bottom-Right is minimum distance between (width, height) and mask.

The last question is, is my implementation slow? The Neural network generates the mask in .7 seconds, but it's taking my loop ~2 seconds to find the corners. Can this be sped up?

def predict(self,img):
    # Read image
    image = img
    height,width,channels=img.shape

    # Detect objects
    r = self.model.detect([image], verbose=0)[0]
    mask=r['masks']
    print(mask)
    x1=0
    x2=0
    x3=0
    x4=0
    y1=0
    y2=0
    y3=0
    y4=0
    minDistanceTopLeft=999999
    minDistanceTopRight=999999
    minDistanceBottomLeft=999999
    minDistanceBottomRight=999999
    xAverage=0.0
    yAverage=0.0
    for x in range(0, len(mask)):
        for y in range(0, len(mask[x])):
            if(mask[x][y]):
                distToTopLeft=(x-0)*(x-0)+(y-0)*(y-0)
                if(distToTopLeft<minDistanceTopLeft):
                    minDistanceTopLeft=distToTopLeft
                    x1=x
                    y1=y
                distToTopRight=(x-width)*(x-width)+(y-0)*(y-0)
                if(distToTopRight<minDistanceTopRight):
                    minDistanceTopRight=distToTopRight
                    x2=x
                    y2=y
                distToBottomLeft=(x-0)*(x-0)+(y-height)*(y-height)
                if(distToBottomLeft<minDistanceBottomLeft):
                    minDistanceBottomLeft=distToBottomLeft
                    x4=x
                    y4=y
                distToBottomRight=(x-width)*(x-width)+(y-height)*(y-height)
                if(distToBottomRight<minDistanceBottomRight):
                    minDistanceBottomRight=distToBottomRight
                    x3=x
                    y3=y
    toReturn=np.array([x1, y1, x2, y2, x3, y3, x4, y4, 1])
    return [toReturn.tolist()]

Mask is a numpy array of booleans:

[[[False]
  [False]
  [False]
  ...
  [False]
  [False]
  [False]]

 [[False]
  [False]
  [False]
  ...
  [False]
  [False]
  [False]]

 [[False]
  [False]
  [False]
  ...
  [False]
  [False]
  [False]]

  ...

 [[False]
  [False]
  [False]
  ...
  [False]
  [False]
  [False]]

 [[False]
  [False]
  [False]
  ...
  [False]
  [False]
  [False]]

 [[False]
  [False]
  [False]
  ...
  [False]
  [False]
  [False]]]

assorted findings

your code does not document what predict() accomplishes
• I don't even get how the name predict is telling/helpful
• your code documents neither the approach chosen nor alternatives disregarded
comparing a cheaper monotone function of Euclidean distance: nice
• naming the variables without fussing that it's equivalent Euclidean at the end of the day rather than equal to or sum/Manhattan or max: nice, again
camelCase is not pythonic
initial value for minDistances should be height*height+width*width+1
the approach visits each and every element of mask
no mask[x].index(True) (careful with mask[x].reverse().index(True))
the squares get computed time and again
looks especially off with x
code for the four pairs looks repetitive
naming too, come to think of it
the mask example is useless for showing one value, only

context provided is lacking: what is get the corners of [not-exactly-]quadrilateral?

alternative approaches to find closest to the [image] corners

start from the middle
when you find an element set, you still have to inspect all the element closer to the corner, up to the corner itself
just complicates iteration
proceed in order of increasing distance from the corners
+: you find elements set close to the corner early on
  you don't need to look any further for that corner
-: even with at least one element set, there may be more than two visits on average (solitary True in one corner)


Data Structures BFS Breadth First Search algorithm improvement written in python I have written a python program that calculates the traversal of BFS, but it seems inefficient to me for few reasons like

I have to say, how many nodes there will be at the start
I'm not satisfied with the node assigning technique

How to make it more efficient that is my question?

from __future__ import print_function

class graph(object):

    def __init__(self, n):
        #initiating a 2d array for the graph
        self.graph = [[] for y in range(n)]

        #al the alphabet characters ['A', 'B', 'C', 'D', 'E', 'F', 'G', 'H', 'I', 'J', 'K', 'L',
        #                            'M', 'N', 'O', 'P', 'Q', 'R', 'S', 'T', 'U', 'V', 'W', 'X', 'Y', 'Z']
        self.alphab = [chr(97+i) for i in range(26)]

    def addnode(self,u,v):
        #populating the 2d array with the graph
        self.graph[self.alphab.index(u)].append(self.alphab.index(v))

    #printing the graph if needed
    def printgraph(self):
        print('\nthe graph structure is {}\n'.format(self.graph))

    def bfs(self, node):

        node = self.alphab.index(node)
        print('\nstarting from node {} the traverse is '.format(self.alphab[node]))
        queue = []
        visited = [None for i in range(len(self.graph))]
        parent = [None for i in range(len(self.graph))]
        distance = [0 for i in range(len(self.graph))]
        queue.append(node)
        visited[node] = True

        while len(queue) != 0:
            node = queue[0]
            del queue[0]
            for i in self.graph[node]:
                if visited[i] == None:
                    queue.append(i)
                    visited[i] = True
                    distance[i] = distance[node] + 1
                    parent[i] = node

        #converting them back to ascii when printing using the list we created
            print('--->', self.alphab[node],)
        print('\nthe distance structure is {}\n\nand the parents structure is {}'.format(distance, [ self.alphab[i] if i != None else i for i in parent ]),)

def main():
    #How many nodes there will be initiating with the constructor
    se = graph(4)

    """
        a----b.
        |  .
        |.
        c----d """

    se.addnode('a', 'b')
    se.addnode('a', 'c')
    se.addnode('b', 'c')
    se.addnode('c', 'a')
    se.addnode('c', 'd')
    se.addnode('d', 'd')

    se.printgraph()
    se.bfs('a')

if __name__ == '__main__':
    main()

the output for starting from node 'a'

the graph structure is [[1, 2], [2], [0, 3], [3]]

starting from node a the traverse is
---> a
---> b
---> c
---> d

the distance structure is [0, 1, 1, 2]

and the parents structure is [None, 'a', 'a', 'c']

The very first thing that caught my eye was the del queue[0] which is a \$O(n)\$ operation since it requires for the whole list to be moved.

There is a quick win here - use collections.deque() which has a .popleft() method operating at \$O(1)\$:

queue = deque([node])

while queue:
    node = queue.popleft()

    for i in self.graph[node]:
        if visited[i] is None:
            queue.append(i)

            visited[i] = True
            distance[i] = distance[node] + 1
            parent[i] = node

where deque is imported this way:

from collections import deque

Also note the use of while queue: instead of a less efficient and less readable while len(queue) != 0:.

There is a number of code style issues as well:

look for the PEP8 violations, in particular:

class name should start with a capital letter - Graph instead of graph

use proper documentation strings instead of regular comments before the method definitions

watch for the spaces around operators and the use of blank lines

_ (underscore) variable name is agreed to be used a throwaway variable name, e.g.:

self.graph = [[] for _ in range(n)]

instead of:

self.graph = [[] for y in range(n)]


Implement integer exponentiation faster than repeated multiplication The task:

  Implement integer exponentiation. That is, implement the pow(x, y)
  function, where x and y are integers and returns x^y.

  Do this faster than the naive method of repeated multiplication.

  For example, pow(2, 10) should return 1024.

Solution 1

function pow(x, y) {
  if (!y) { return 1; }
  let tmp = res = x;
  for (let i = 1; i < y; i++) {
    for (let j = 1; j < x; j++) {
      tmp += res;
    }
    res = tmp;
  }
  return res;
}

console.log(pow(2, 10));

I can't claim the following solution to be originally by me. I read it up and implemented it the way I understood. I'm also not sure whether Solution 1 nor Solution 2 is "faster than (...) repeated multiplication".

Solution 2 Also known as "Exponentiation by squaring"

function pow2(x, y) {
  if (!y) { return 1; }
  if (y % 2) {
    return x * pow2(x, y - 1);
  }
  const p = pow2(x, y/2);
  return p * p;
}

console.log(pow2(2,10));

The solution 1 is still a repeated multiplication. The only difference is that the multiplication is implemented as a repeated addition:

    for (let j = 1; j < x; j++) {
        tmp += res;
    }

is a very long way to say

    tmp = res * x;

and obviously it doesn't make the code any faster.

The second solution is almost correct. It fails for negative y (negative is still integer!), and the handling of pow2(0, 0) is questionable.

Besides that, try to avoid the recursion; recursive calls are expensive, and the goal of the exercise is to achieve the maximal speed. There does exist a true honest-to-goodness iterative algorithm.


Refactor method by removing else condition Given the below code is there a way to eliminate the else clause from the below method?
In my workplace else clause is considered evil because it somehow decreases the readability of the code.
public boolean validateAndSetDateRelatedSessionAttributes(String startDateFromParam, String endDateFromParam, HttpSession session) throws ParseException {
    boolean isDateRangeValid = true;
    final boolean requestContainsCustomDate = requestContainsCustomDate(startDateFromParam, endDateFromParam);

    if(requestContainsCustomDate || sessionContainsCustomDate(session)) {
        if(requestContainsCustomDate) {
            if(!isDateRangeValid(startDateFromParam, endDateFromParam)) {
                isDateRangeValid = false;
                startDateFromParam = session.getAttribute(BACalendarConstant.START_DATE).toString();
                endDateFromParam = session.getAttribute(BACalendarConstant.END_DATE).toString();
            }
        } else {
            startDateFromParam = session.getAttribute(BACalendarConstant.START_DATE).toString();
            endDateFromParam = session.getAttribute(BACalendarConstant.END_DATE).toString();
        }
        setSessionAttributes(startDateFromParam, endDateFromParam, session);
    }
    return isDateRangeValid;
}

"Else is evil"? That's a very peculiar point of view!
But if you really want to avoid it, you can look at early return, provided that isn't also regarded as evil - some people are very firm on the "single entry, single exit" principle.
Gilbert Le Blanc's code takes a similar approach to me, but in avoiding early return, he risks duplicated code execution, which may not be a problem here, but worries me a little.
The code below is not checked in any detail, but shows the idea, though it does duplicate code.
public boolean validateAndSetDateRelatedSessionAttributes(String startDateFromParam, String endDateFromParam, HttpSession session) throws ParseException {

  if (requestContainsCustomDate(startDateFromParam, endDateFromParam)) {
    boolean isDateRangeValid = isDateRangeValid(startDateFromParam, endDateFromParam);
    if (!isDateRangeValid) {
      startDateFromParam = session.getAttribute(BACalendarConstant.START_DATE).toString();
      endDateFromParam = session.getAttribute(BACalendarConstant.END_DATE).toString();
    }
    setSessionAttributes(startDateFromParam, endDateFromParam, session);
    return isDateRangeValid;
  }

  if (sessionContainsCustomDate(session)) {
    startDateFromParam = session.getAttribute(BACalendarConstant.START_DATE).toString();
    endDateFromParam = session.getAttribute(BACalendarConstant.END_DATE).toString();
    setSessionAttributes(startDateFromParam, endDateFromParam, session);
  }

  return true;
}


Simple Generic output for Deserializer Lately I have been learning about serialization so I decided to write a little helper class for my application to make it easy to use this feature in multiple places. I actually mixed some generics in the code and want to hear your feedback on my overall approach.

SerializationService.cs

public class SerializationService
{
    /// <summary>
    /// This method Serializes the given Obj into an XML file.
    /// </summary>
    /// <param name="myObject">The obj to be serialized.</param>
    /// <param name="fileFullPath">The file name with the full
    /// path for the output serialized XML file.</param>
    public static void SerializeToFile(Object myObject, string fileFullPath)
    {
        var xmlDoc = new XmlDocument();
        var xmlSerializer = new XmlSerializer(myObject.GetType());
        using (var fs = new FileStream(fileFullPath, FileMode.Create))
        {
            var writer = new XmlTextWriter(fs, Encoding.Unicode);
            xmlSerializer.Serialize(writer, myObject);
            writer.Close();
        }
    }

    /// <summary>
    /// This method deserializes the XML file into an Obj.
    /// </summary>
    /// <param name="fuleFullPath">The XML file to read.</param>
    /// <returns>Deserialized obj of a type T.</returns>
    public static T DeserializeToObj<T>(string fuleFullPath)
    {
        var xmlSerializer = new XmlSerializer(typeof(T));
        using (var reader = new FileStream(fuleFullPath, FileMode.Open))
        {
            return (T) xmlSerializer.Deserialize(reader);
        }
    }
}

And here is how I use it in my code:

Serialization

SerializationService.SerializeToFile(
    _partRepository.GetParts().Where(p => p.IsIgnored).ToList(),
    fileFullPath);

De-serialization

var ignoredParts = SerializationService.DeserializeToObj<List<Part>>(ignoredPartsXml);

Here are a few remarks. I'm not perfect or write perfect code but I hope these tips will help write better code:

Naming consistency:

It would be easier to understand if the names of your two methods had opposing names. For example: SerializeToFile and DeserializeFromFile. It's obvious that deserialization will return an object, omit that in the name.

Also, if you're using the same type/kind of parameter or variable in one method, give it the same name in the other method.

Truly generic:

You should make your serialize method generic too:

public static void SerializeToFile<T>(T obj, string fullPath)

In the point Type constraint a bit further you'll see why.

Scoping variables:

You should define variables as close to their usage as possible. In your case you can instantiate the XmlSerializer class inside the using statement since you don't need it outside the statement. Example:

using (var stream = new FileStream(fullPath, FileMode.Open))
{
    var xmlSerializer = new XmlSerializer(typeof(T));
}

Checking for the file:

In both methods an exception can be thrown in certain situations. In the serialize method, if the path doesn't exist you'll get a DirectoryNotFoundException and in the deserialize method you'll get a FileNotFoundException if the file doesn't exist. This can easily be solved:

//Check for directory in serialize:
var directoryPath = Path.GetDirectory(fileFullPath);
var directoryExists = Directory.Exists(directoryPath);

//Check for file in deserialize
var fileExists = File.Exists(fileFullPath);

You should decide whether you want positive or negative evaluation on this. For example, only perform the method if the checks are positive or throw an argument exception if the checks are negative. That is up to you.

Also, like Heslacher stated: what if the file already exists? As for now, the content of the file will be overwritten by the new content.

using statement:

Following:

using (var fs = new FileStream(fileFullPath, FileMode.Create))
{
    var writer = new XmlTextWriter(fs, Encoding.Unicode);
    xmlSerializer.Serialize(writer, myObject);
    writer.Close();
}

can be replaced by:

using (var stream = new FileStream(fullPath, FileMode.Create))
using (var writer = new XmlTextWriter(stream, Encoding.Unicode))
{
    var xmlSerializer = new XmlSerializer(obj.GetType());
    xmlSerializer.Serialize(writer, obj);
}

Type constraint:

Remember my point for making the serialize method generic too? For now, you don't validate the incoming and outgoing object. What if I pass an instance of a class that has no public parameterless constructor? During runtime I'll get an InvalidOperationException with a message like:

  ... cannot be serialized because it does not have a parameterless constructor.

The solution is to place type constraints on your methods. Definition from MSDN:

  When you define a generic class, you can apply restrictions to the kinds of types that client code can use for type arguments when it instantiates your class, these restrictions are called constraints.

You should use the class and new() type constraints. Here's their definition:

class: the type argument must be a reference type; this applies also to any class, interface, delegate, or array type.
new(): the type argument must have a public parameterless constructor. When used together with other constraints, the new() constraint must be specified last.

Now you'll get a compile-time exception when trying to serialize an instance of a clas without a public parameterless constructor. You can read more on this here: Constraints on Type Parameters (C# Programming Guide).

public static void SerializeToFile<T>(T obj, string fullPath) where T : class, new()
{
    //method body
}

public static T DeserializeFromFile<T>(string fullPath) where T : class, new()
{
    //method body
}

Final code:

With all this applied, your code might look like following:

public class SerializationService
{
    public static void SerializeToFile<T>(T obj, string fullPath) where T : class, new()
    {
        if(Directory.Exists(Path.GetDirectoryName(fullPath)))
        {
            using (var stream = new FileStream(fullPath, FileMode.Create))
            using (var writer = new XmlTextWriter(stream, Encoding.Unicode))
            {
                var xmlSerializer = new XmlSerializer(obj.GetType());
                xmlSerializer.Serialize(writer, obj);
            }
        }
    }

    public static T DeserializeFromFile<T>(string fullPath) where T : class, new()
    {
        if(File.Exists(fullPath))
        {
            using (var stream = new FileStream(fullPath, FileMode.Open))
            {
                var xmlSerializer = new XmlSerializer(typeof(T));
                return (T) xmlSerializer.Deserialize(stream);
            }
        }
        return default(T);
    }
}


Wrapper class for system dialog to allow for dependency injection The following class is a wrapper for Microsoft.Win32.SaveFileDialog. I've implemented the usage of Microsoft.Win32.SaveFileDialog this way because I'm using the ISaveFileDialog interface as a dependency throughout my code base.

public class SaveFileDialog : ISaveFileDialog
{
    public bool Save(string content, string suggestedFileExtension = null, string suggestedFileExtensionName = null, string suggestedFileName = null)
    {
        string filter = "All Files|*.*";

        if (suggestedFileExtension != null)
        {
            filter = $"{suggestedFileExtensionName ?? string.Empty}|*{suggestedFileExtension}|" + filter;
        }

        Microsoft.Win32.SaveFileDialog dlg = new Microsoft.Win32.SaveFileDialog
        {
            FileName = suggestedFileName ?? string.Empty,
            DefaultExt = suggestedFileExtension ?? string.Empty,
            Filter = filter
        };

        switch (dlg.ShowDialog())
        {
            case true:
                return WriteFile(dlg.FileName, content);
            default:
                return false;
        }
    }

    private bool WriteFile(string filePath, string content)
    {
        try
        {
            File.WriteAllText(filePath, content);
            return true;
        }
        catch (PathTooLongException)
        {
            return false;
        }
        catch (DirectoryNotFoundException)
        {
            return false;
        }
        catch (IOException)
        {
            return false;
        }
        catch (UnauthorizedAccessException)
        {
            return false;
        }
        catch (System.Security.SecurityException)
        {
            return false;
        }
    }
}

While this works, I'm struggling to refactor this class to allow for unit testing as currently there's no way to mock/stub the Microsoft.Win32.SaveFileDialog - have I missed a trick here? Or is there genuinely no nice way to unit test this and I should be relying on integration tests?

I know that I could refactor the catch blocks to either be

catch (Exception)
{
    return false;
}

Or even to be

catch (Exception ex) when (ex is PathTooLongException
                        || ex is DirectoryNotFoundException
                        ........ )
{
    return false;
}

But I would like to keep them the way that they are because I would like to avoid catching all exceptions and I'm using a Visual Studio plugin to help me manage exceptions and it complains when using the second option.

Thanks for any help!

unit tests

You can use fakes (stubs vs shims) to mock third-party classes. It is not as lightweight as your regular mocks, but it does the job.

exception handling

catch (Exception)
{
    return false;
}

Why don't you do something like Microsoft tends to do often in their framework? You make some internal convenience method IsCriticalException that checks whether to rethrow the exception or swallow it.

catch (Exception ex)
{
    if (IsCriticalException(ex)) throw;
    return false;
}

clean code

Use var when you can.

  Microsoft.Win32.SaveFileDialog dlg = new
  Microsoft.Win32.SaveFileDialog

 var dlg = new Microsoft.Win32.SaveFileDialog


Performing multiple substring replacements on a string I have code like the following, which performs a series of replacements on a string, to convert it to its final form.

public String convertString(String string) {

String convertedString = string;

if( string.contains("something")) {
    convertedString = convertStringA(string);

    return convertedString;
} else if (string.contains("something else")) {
    convertedString = convertStringB(string);

    return convertedString;
} ...

// many more else if blocks

else if (string.contains("something else")) {
    convertedString = convertStringXXX(string);

    return convertedString;
} ...

}

How can I do it better?

Your Algorithm is a little bit too abstract to provide a one size fits all solution.

What does it mean, when a string contains "something" or "something else"?
Are they completely different strings like "I know (something)" and "He gave me (something else)"? Or are there similarities like:
"Peter knows (something)" and "Joe knows (something else)". So "knows" would be the keyword after which your phrase occurs. "Contains" is somehow a little bit unprecise.

If it is relativly easy to extract the substring you need -perhaps with a regular expression- you could use a HashMap to provide the correct converter with a simple call with the phrase as the key.

public class Converters {

  HashMap<String, Converter> converters=new HashMap<String, Converter>();

  public Converters(){
      // Fill Hashmap here
  }

  public Converter getConverterFor(String phrase){
      if (!converters.containsKey(phrase)) throw new NoConverterFoundException();
      return converters(phrase);
  }

}

Converter is here an interface, which all of your converters have to implement.
This solution provides easy maintenance - a new Converter needs only one new entry in the HashMap. And no unreadable if-else-whattheeckyouwantelse-blocks anymore.

But this is only one possible solution. A better solution requires more information on your phrases.


Find smallest number of squares that sum to a number Leetcode problem 279 “Perfect Squares” is:

  Given a positive integer n, find the least number of perfect square numbers (for example, 1, 4, 9, 16, ...)

I wrote the following solution:

def numSquares(self, n):
        """
        :type n: int
        :rtype: int
        """
        sqs = [0]
        nums = {}
        for x in range(1,n+1):
            nums[x] = 1
        for i in nums:
            if (i * i) in nums:
                sqs.append(i * i)
        T = [x for x in range(n+1)]
        for i in range(1,len(sqs)):
            for j in range(1,n+1):
                if sqs[i] <= j:
                    T[j] = min(T[j],math.floor(j/sqs[i]) + T[j%sqs[i]])
        return T[n]

I wrote a dynamic-programming solution, storing results in a 1d array, but it seemed that due to the way I was originally calculating the perfect squares, the solution was timing out. I then modified the solution and came up with the one above, but the code is still causing a time out error when run against an input of 4635 for n.

How could this solution be optimized to avoid this timeout error?

Not an review, but an extended comment:

I don't know how this solution can be optimized. The expected solution is radically different, and is based on a couple of theorems from number theory.

You should check whether the number is

A perfect square (the answer is obviously 1), or
is a sum of 2 squares (the answer is obviously 2), or
not a Legendre's number, that is \$n = 4^a(8b + 7)\$ (the answer is 3)

Otherwise, the answer is 4.


DB2 statement to update a header file if no corresponding record exists I have written below SQL in an RPGLE program. Intent is to update the header file (TC400F) if no corresponding records exist in detail file (TC401F). Are there better ways of doing this?

Exec SQL UPDATE  TC400F
            SET  T40STS = '05',
                 T40OFL = '1'
          WHERE  T40SID = :K#T41SID AND
                 T40PID = :K#T41PID AND
     NOT EXISTS  (SELECT * FROM TC401F WHERE
                 T41SID = :K#T41SID AND
                 T41PID = :K#T41PID );

Not really but you can simplify your query like this (variable are not replaced 2x)

Exec SQL UPDATE  TC400F f1
         SET  (f1.T40STS, f1.T40OFL) = ('05', '1')
         WHERE  (f1.T40SID, f1.T40PID) = (:K#T41SID, :K#T41PID) AND
         NOT EXISTS
         (SELECT * FROM TC401F f2 WHERE (f1.T40SID, f1.T40PID)=(f2.T41SID, f2.T41PID));


Writing a bitmap image from C++ Here's my function that creates a bitmap file from an array of pixels. It can write a bitmap both with transparency and without transparency. Please review my code and give constructive criticism on how to increase the overall quality of it. :)

typedef unsigned char BYTE;

inline int bmpEncoder(const std::string &location,
    const unsigned __int32 &width, const unsigned __int32 &height,
    const std::vector<BYTE> &buffer,
    const bool &hasAlphaChannel = true) {

    std::ofstream fout(location, std::ios::out | std::ios::binary);

    if (fout.fail()) {
        return 0;
    }

    //Padding
    const unsigned __int8 padding = hasAlphaChannel ? 0 : (4 - (width * 3) % 4) % 4;

    //Bitmap file header.
    const char signature[2] = { 'B', 'M' };
    const unsigned __int32 fileSize = buffer.size() * sizeof(BYTE) + padding * (height - 1) + 14 + 124;
    const unsigned __int32 offset = 14 + 124;

    //Bitmap information header file
    const unsigned __int32 DIBSize = 124;
    const __int32 bitmapWidth = width;
    const __int32 bitmapHeight = height;
    const unsigned __int16 numPlanes = 1;
    const unsigned __int16 bitsPerPixel = (hasAlphaChannel) ? 32 : 24;
    const unsigned __int32 compressionMethod = (hasAlphaChannel) ? 3 : 0; //BI_RGB = 0, BI_BITFIELDS = 3
    const unsigned __int32 bitmapSize = buffer.size() * sizeof(BYTE);
    const __int32 horizontalResolution = 2834;
    const __int32 verticalResolution = 2834;
    const unsigned __int32 numColors = 0;
    const unsigned __int32 impColorCount = 0;
    const unsigned __int32 redBitmask = (hasAlphaChannel) ? 0x0000FF00 : 0; //ARGB32 pixel format
    const unsigned __int32 greenBitmask = (hasAlphaChannel) ? 0x00FF0000 : 0;
    const unsigned __int32 blueBitmask = (hasAlphaChannel) ? 0xFF000000 : 0;
    const unsigned __int32 alphaBitmask = (hasAlphaChannel) ? 0x000000FF : 0;

    //Writing the file header and information header to the file
    std::vector<BYTE> header(offset, 0);
    header[0] = signature[0];
    header[1] = signature[1];

#define HEADERS(i, variableName)    header[i] = variableName; header[i+1] = variableName >> 8; header[i+2] = variableName >> 16; header[i+3] = variableName >> 24;

    HEADERS(2, fileSize);
    HEADERS(6, 0);
    HEADERS(10, offset);
    HEADERS(14, DIBSize);
    HEADERS(18, bitmapWidth);
    HEADERS(22, bitmapHeight);

    header[26] = (BYTE)numPlanes;
    header[27] = (BYTE)(numPlanes >> 8);
    header[28] = (BYTE)bitsPerPixel;
    header[29] = (BYTE)(bitsPerPixel >> 8);

    HEADERS(30, compressionMethod);
    HEADERS(34, bitmapSize);
    HEADERS(38, horizontalResolution);
    HEADERS(42, verticalResolution);
    HEADERS(46, numColors);
    HEADERS(50, impColorCount);
    HEADERS(54, redBitmask);
    HEADERS(58, greenBitmask);
    HEADERS(62, blueBitmask);
    HEADERS(66, alphaBitmask);

#undef HEADERS

    fout.write((char *)header.data(), sizeof(BYTE) * header.size());

    //Writing the pixel array
    const unsigned __int32 bWidth = bitsPerPixel / 8 * width;

    for (int i = height - 1; i >= 0; i--) {
        std::vector<BYTE> row(buffer.begin() + i * bWidth, buffer.begin() + i * bWidth + bWidth);
        fout.write((char *)row.data(), row.size() * sizeof(BYTE));
        fout.seekp(padding * sizeof(BYTE), std::ios::cur);
    }

    fout.close();
    return 1;
}

Not bad! Let's go through it line-by-line.

typedef unsigned char BYTE;

This is fine, but just so you know, starting from C++17, there is a std::byte that could replace this. If you're not using C++17 yet, fine, but if this is code is for C++17 or better, that's something you can take advantage of.

inline int bmpEncoder(const std::string &location,
    const unsigned __int32 &width, const unsigned __int32 &height,
    const std::vector<BYTE> &buffer,
    const bool &hasAlphaChannel = true) {

I'm not sure that the inline specifier serves any purpose here. Compilers generally ignore inline suggestions anyway, but in this case I can't see this entire huge function being something you'd want to inline. If you added it to get a header-only library function, that makes sense, but this doesn't really seem like a candidate for a header-only library.

You use __int32 and similar types all over the code, but were you aware there are standard versions? Just include <cstdint> and you get access to the fixed-width integer types. Instead of const unsigned __int32, you'd do std::uint32_t.

I don't really see the benefit of taking 32-bit integers by reference. They're basically cost-free to copy, so you might as well use them by value.

Taking the image data by const std::vector<BYTE>& is a good idea... but you can do even better! You see, if you take your image data by const std::vector<BYTE>&, that means the image data must be in a std::vector<BYTE>. Maybe that's usually the case... but why restrict the options? Ultimately, to use fout.write(), you just need the image data to be in a contiguous array. A std::vector<BYTE> works... but so does a std::unique_ptr<BYTE[]>, or an std::array<BYTE, N>, or third-party container types.

So rather than taking a std::vector<BYTE>, you could just take a const BYTE* and std::size_t pair. Even better, you could use a gsl::span (which is probably going to be in C++20 as std::span).

Finally, there's no reason to take a bool argument by const-ref. Just take it by value.

Oh, also, you only seem to be using the return value as a true/false thing to denote errors. You could do that with a bool, but you should look into better ways to report errors, like std::error_code or exceptions (or the proposed std::expected).

So here's what all that put together looks like:

int bmpEncoder(const std::string &location,
    std::uint32 width, std::uint32_t height,
    const BYTE* buffer, std::size_t buffer_size,
    bool hasAlphaChannel = true) {

Or with span:

int bmpEncoder(const std::string &location,
    std::uint32 width, std::uint32_t height,
    std::span<const BYTE> buffer,
    bool hasAlphaChannel = true) {

The first thing you do in the function is open the output file, then double-check that it's open. But that's the last error-checking you do. If there's any problems writing the output after that, they just get forgotten, so this function could return 1 even though the output never got written.

You have multiple options for how to deal with this. The simplest is to just not bother to do any error checking until the end of the function - just open the file, and write everything to it, then at the end return bool{fout};. That will return 1 if everything worked and all your image data was written to the file (you should probably flush it beforehand), 0 otherwise. If there's an error early in the process, you will waste time spitting bytes into the ether, but no big deal - no harm will be done except time wasted, and the error will be detected.

Another option is to turn exceptions on in your output stream. If any error occurs, output will stop immediately. You can either catch the exception and return 0, or let the exception propagate if you prefer.

So the next bit is the biggest part of the function. What you do here is set up all your data in local variables... then copy all those local variables to a buffer... then write the buffer to the output file. That all seems a bit unnecessary.

Here's what I recommend instead. Create a set of functions that write binary values to the output stream. For example

inline std::ostream& write_binary_8bit(std::ostream& out, char c) {
    return out.put(c);
}

inline std::ostream& write_binary_8bit(std::ostream& out, char const* c, std::size_t n) {
    return out.write(c, n);
}

inline std::ostream& write_binary_8bit(std::ostream& out, BYTE const* c, std::size_t n) {
    return write_binary_8bit(out, reinterpret_cast<char const*>(c), n);
}

inline std::ostream& write_binary_16bit(std::ostream& out, std::uint16_t v) {
    return out.put(v & 0xFFu).put((v >> 8) & 0xFFu);
}

inline std::ostream& write_binary_16bit(std::ostream& out, std::int16_t v) {
    return write_binary_16bit(static_cast<std::uint16_t>(v));
}

// And so on for whatever types you need...

Then your function code just becomes:

write_binary_8bit(fout, "BM", 2);   // signature
write_binary_32bit(fout, buffer.size() + padding * (height - 1) + 14 + 124; // file size
write_binary_32bit(fout, 14 + 124); // offset

write_binary_32bit(fout, 124);      // DIBSize
write_binary_32bit(fout, width);    // width
write_binary_32bit(fout, height);   // height
write_binary_16bit(fout, 1);        // numPlanes
write_binary_16bit(fout, (hasAlphaChannel) ? 32 : 24);    // bitsPerPixel
// and so on

Now you can get rid of the header vector and the macro.

Then it's on to writing the actual image data.

for (int i = height - 1; i >= 0; i--) {
    std::vector<BYTE> row(buffer.begin() + i * bWidth, buffer.begin() + i * bWidth + bWidth);
    fout.write((char *)row.data(), row.size() * sizeof(BYTE));
    fout.seekp(padding * sizeof(BYTE), std::ios::cur);
}

What you're doing here is that for each row, you're creating a new vector, copying the row data into it, then writing the vector. Why the extra steps? All you really want to do is write the row data. So just write it directly. No need for the extra vector.

for (int i = height - 1; i >= 0; i--) {
    write_binary_8bit(buffer.begin() + i * bWidth, bWidth);
    fout.seekp(padding, std::ios::cur);
}

One thing you might want to do here is some error checking, though. You need to be sure that the image data buffer isn't truncated - that it actually does hold width×height pixels, or you might be reading (and writing to a file) random memory data. That's how security disasters like Heartbleed happen.

And finally:

fout.close();

This is unnecessary. The file will close itself automatically.

What you might want to do, though, is:

return bool{fout.flush()};

That will flush everything buffered to disk (hopefully), and check the error status of the stream.

In summary, my major recommendations are:

use standard types (instead of __int32)
avoid unnecessary buffering by writing data directly (rather than creating a buffer, copying the data to the buffer, then writing the buffer)
get rid of the preprocessor macro (and unnecessary buffer) by using helper funcions
consider better error checking


C linked list implementation For the following C linked list implementation, please give me some suggestions for improved efficiency, style and design. This implementation is not 100% complete, please give some suggestions for additional features. Notice the data type for the list is void*, I want it to be as generic as possible.

accList.h:

#ifndef ACCLIST_H
#define ACCLIST_H

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

struct accListNode                 //the nodes of a linked-list for any data type
{
  void *data;                     //generic pointer to any data type
  struct accListNode *next;       //the next node in the list
};

struct accList                    //a linked-list consisting of accListNodes
{
  struct accListNode *head;
  struct accListNode *tail;
  int size;
};

void accList_allocate(struct accList *theList);           //allocate the accList and set to NULL
void appendToEnd(void *data, struct accList *theList);    //append data to the end of the accList
void removeData(void *data, struct accList *theList);         //removes data from accList

#endif

accList.c:

#include "accList.h"
#include "cmpsc311.h"

void accList_allocate(struct accList *theList)    //allocate and initialize to NULL values
{
   theList = Malloc(sizeof(struct accList));
   theList->head = NULL;
   theList->tail = NULL;
   theList->size = 0;
}

void appendToEnd(void *data, struct accList *theList)
{
  struct accListNode *newNode = Malloc(sizeof(struct accListNode));
  newNode->data = data;
  newNode->next = NULL;
  if(theList->head == NULL)    //the list is empty
  {
    theList->head = theList->tail = newNode;
  }
  else                        //the list is not empty
  {
    theList->tail->next = newNode;
    theList->tail = newNode;
  }
}

void removeData(void *data, struct accList *theList)
{
  if(theList->head == NULL)                                 //the list is empty
    return;
  else if(theList->head == theList->tail)                   //there is one element in the list
  {
    free(theList->head);
    theList->head = NULL;
  }
  else if(data == theList->head->data)                     //the element to be removed is the head
  {
    struct accListNode *temp = theList->head->next;
    free(theList->head);
    theList->head = temp;
  }
  else if(data == theList->tail->data)                     //the element to be removed is the tail
  {
    struct accListNode *cur;
    struct accListNode *prev = NULL;
    for(cur = theList->head; cur->next != NULL; prev = cur, cur = cur->next);
    free(theList->tail);
    prev->next = NULL;
    theList->tail = prev;
  }
  else                                                     //any other node
  {
    struct accListNode *prev = NULL;
    struct accListNode *cur;
    for(cur = theList->head; cur != NULL; prev = cur, cur = cur->next)
    {
      if(cur->data == data)   //this is the node we must free
      {
        prev->next = cur->next;
        free(cur);
      }
    }
  }
}

I think the remove function needs the most work. I did post that function on this site, however I think I needed to include the whole implementation to get the best feedback. Will this work on an embedded system?

Several problems I see:

accList_allocate

accList_allocate doesn't do what you probably think. It creates a new accList, yes, but this new one will not be visible to the caller (and thus, will be lost as memory leakage). If you want to initialize the pointer passed to it, use a pointer-to-pointer:

void accList_allocate(struct accList **outList)    //allocate and initialize to NULL values
{
   struct accList* theList = Malloc(sizeof(struct accList));
   theList->head = NULL;
   theList->tail = NULL;
   theList->size = 0;
   *outList = theList;
}

Also, you should really check the return value of Malloc. Especially on embedded systems, there's a real possibility of running out of memory. Let the caller handle the error, but tell him that something's wrong.

appendToEnd

Same as with accList_allocate, check Malloc's return value and return some kind of error code if it's NULL. What you're writing there is library code that should always handle errors gracefully and let the caller decide whether to crash and burn or recover and try again.

removeData

Please refer to my answer in your other thread.


Move function for a game I have a move function in this game that I'm making.  It works fine, but I'd like to reduce its size a bit.

function move(moveTo, direction) {

    switch (direction) {
        case "right":
            $('#player').animate({ left: "+=" + scale }, "slow", function () {
                $("#player").appendTo("#" + moveTo);$('#player').attr("style", "");
            });break;
        case "left":
            $('#player').animate({ left: "-=" + scale }, "slow", function () {
                $("#player").appendTo("#" + moveTo);$('#player').attr("style", "");
            });break;
        case "up":
            $('#player').animate({ top: "-=" + (9 * (scale / 10)) }, "slow", function () {
                $("#player").appendTo("#" + moveTo);$('#player').attr("style", "");
            });break;
        case "down":
            $('#player').animate({ top: "+=" + (9 * (scale / 10)) }, "slow", function () {
               $("#player").appendTo("#" + moveTo);$('#player').attr("style", "");
            });break;
    }
}

I always execute the same commands:

$("#player").appendTo("#" + moveTo);

and

$('#player').attr("style", "");

I can't put it after the switch statement because I want to wait until the animate is finished.  Is there a way to do this other than making a function? I also want this type of thing for some other code, so a bit less case specific would be better.

I would abstract the animation logic into a high order function and the possible directions in an object, then just lookup the corresponding function for each direction, in other words:

function move(to, dir) {

  function animate(props) {
    return function() {
      $('#player').animate(props, 'slow', function() {
        $(this).attr('style', '').appendTo('#'+ to);
      });
    }
  }

  var dirs = {
    right: animate({ left: '+='+ scale }),
    left: animate({ left: '-='+ scale }),
    up: animate({ top: '-='+ (9*scale/10) }),
    down: animate({ top: '+=' + (9*scale/10) })
  };

  if (dir in dirs) dirs[dir]();
}

I fixed a few other things. You can use $(this) to refer to the element being animated in the callback and chain those methods.

Then (9*(scale/10)) is effectively the same as 9*scale/10; associative operators.

You probably need to abstract even more if you add more code; ideally you'd want separate all your logic from your DOM manipulation and create a clear data structure that you can reuse. Also think about using classes instead of IDs. Classes are more reusable and lead to simpler code.


Sending messages for processing I have a class which is responsible for waiting until a message is added to a message list and then sending it off to get processed.

withdrawMessages waits for a message. I wait for a total of 2 minutes for a message to appear. I do this to keep the connection alive on my browser.
depositMessage adds a message to the message list.

The process flow I am aiming for is:

Deposit a message into the list
Wait is notified and the whole list gets passed to the processor object (IUpdater)

If processMessages is about to get executed and a new message is added to the list I want to make sure this new message is also sent when calling responseUpdater.sendResponse

I need it to be thread-safe so when responseUpdater.sendResponse(messageList); is actually getting processed, then nothing else can be added to the list. It needs to wait until sendResponse is completed before adding it.

sendResponse basically sends the list to browser then the browser sends back a request to call withdrawMessages. So if a new message is added when sendResponse is getting executed, it has to wait until withdrawMessages is called again. At that point it will process the messages.

public class MessagePusher {

    private static final int TWO_MINUTE_WAIT = 120 * 1000;
    private volatile boolean notified = false;
    private final ArrayList<String> messageList = new ArrayList<String>();

    private void waitOnMessages(int duration) throws InterruptedException {
        messageList.wait(duration);
    }

    private void notifyWaitingThreads() {
        if (messageList.size() > 0) {
            notified = true;
            this.messageList.notify();
        }
    }

    public void depositMessage(String message) {
        synchronized (this.messageList) {
            this.messageList.add(message);
            notifyWaitingThreads();
        }
    }

    //IUpdater just processes the messages
    public void withdrawMessages(IUpdater updater) {
        boolean processRequired = false;
        synchronized (messageList) {
            //gets lock
            boolean timeout = false;
            try {
                //releases lock
                waitOnMessages(TWO_MINUTE_WAIT);
                //gets lock again
                if ((!notified)) {
                    timeout = true;
                }
            } catch (InterruptedException e) {
                ShipsLog.out.asWarning("Unexpected exception! Nested exception is:\n" + e);
            }
            if (notified || timeout) {
                notified = false;
                processRequired = true;
            }
        }
        if (processRequired) {
            processMessages(updater);
        }
    }

    public void processMessages(IUpdater responseUpdater) {
            synchronized (messageList) {
                responseUpdater.sendResponse(messageList);
                messageList.clear();
            }
    }
}

It looks like you only have one lock, so you can't have deadlock; but for future reference, you can add deadlock detection to the program, see for example this blog post but there are many other options available.
You may want to replace the arraylist with a ConcurrentLinkedQueue, this will let you remove the synchronized block from depositMessage and may let you remove the synchronized block from withdrawMessages and processMessages depending on how responseUpdater.sendReponse is implemented.  In general, try to use lock-free data structures when possible - they generally scale better, and you can't heave deadlock if you don't have any locks.
ConTest is a tool for detecting multithreading problems e.g. race conditions.  Java Pathfinder is a much more heavyweight approach to this.
For a large project, you may want to use a library like Akka or JMS to manage concurrency.


Generating simple and complex passwords I am pretty new to Python and I would like some tips and criticism on my formatting style/code and what can be better organized. I am a bit shaky on how classes work, but I think that it can be used here to make it more simple.

from tkinter import *
from tkinter import ttk
import random

#frame setup
main = Tk()
frame = ttk.Frame(main)
button_frame = ttk.Frame(frame)
text_frame = ttk.Frame(frame)
text = Text((text_frame), borderwidth=10, height=8, width=20,    relief="sunken",)

def complex_password(): #complex password function
    password2 = []
    password2.append(random.choice((capital)))
    count = 0
    x = ""
    while count != 12:
        roll = random.randint(1,4)
        if roll == 1 or roll == 2:
            password2.append(random.choice((letters)))
            count +=1
        if roll == 3 or roll == 4:
            number = random.randint(0,9)
            number = str(number)
            password2.append(number)
            count +=1
        if count == 12:
            password2 = x.join(password2)
            text.insert(INSERT, password2)
            text.insert(INSERT, "\n")

def simple_password(): #simple password function
    y = ""
    password1 = []
    password1.append(random.choice((words)))
    password1.append(random.choice((words)))
    number = random.randint(1,99)
    num = str(number)
    password1.append(num)
    password1 = y.join(password1)
    text.insert(INSERT, password1)
    text.insert(INSERT, "\n")

def clear_text(): #clear txt box function
    text.delete(1.0, END)

#buttons
simple = ttk.Button(button_frame, text="Simple", command=simple_password)
complex = ttk.Button(button_frame, text="Complex", command=complex_password)
clear = ttk.Button(button_frame, text="Clear", command=clear_text)

#buttons grids
simple.grid(column=2, row=1)
complex.grid(column=1, row=1)
clear.grid(column=3, row=1)
text.grid()

#frame grids
frame.grid(column=1, row=2)
text_frame.grid(column=1, row=2)
button_frame.grid(column=1, row=1)

#misc settings
for child in frame.winfo_children(): child.grid_configure(padx=5, pady=10)
main.title("Password Gen")
main.resizable(width=FALSE, height=FALSE)
main.geometry("238x230")

words = ['Dog', 'Cat', 'Mouse', 'Fire', 'Ice', 'Basket', 'Tree', 'Tiger',
     'Lion', 'Flash','Super', 'Light', 'Zoom','Speed', 'Pants', 'Shirt',
     'Hat', 'Suit', 'Berry', 'Yogurt', 'Epic', 'Keyboard', 'Toe', 'Car',
     'Truck', 'Bike', 'Motor', 'Hammer', 'Pizza', 'Heart', 'Arm','Joint',
     'Saw', 'New', 'Carrots', 'Baby', 'Kiss', 'Backspace',  'Enter', 'Alt',
     'Print', "Down", 'Up', 'Question', 'Rain', 'Forest','Red', 'Orange',
     'Yellow', 'Green', 'Blue', 'Purple', 'Brown', 'Black', 'Indigo', 'Grey',
     'Shadow', 'Eye', 'Brick', 'Twig', 'Gangster', 'Thug', 'Chains', 'Gold',
     'Silver', 'Bronze', 'Platinum', 'Titanium', 'Exploding', 'Ladybug', 'Grass',
     'Monkey', 'Rhino', 'Comma', 'Hair', 'Shark', 'Fish']
letters = ['a', 'b', 'c', 'd', 'e', 'f', 'g', 'h', 'i', 'j', 'k', 'l', 'm',
       'n','o', 'p', 'q', 'r', 's', 't', 'u', 'v', 'w', 'x', 'y', 'z']
capital = ['A', 'B', 'C', 'D', 'E', 'F', 'G', 'H', 'I', 'J', 'K', 'L', 'M',
       'N','O', 'P', 'Q', 'R', 'S', 'T', 'U', 'V', 'W', 'X', 'Y', 'Z']

main.mainloop() #end of GUI

I prefer

import tkinter as tk

and I can see that tk.Button, tk.INSERT, tk.FALSE are not my local variables/classes.

You can use string.ascii_lowercase instead of letters list, and string.ascii_uppercase instead of capital.

You don't have to convert string.ascii_lowercase and string.ascii_uppercase into list.

https://docs.python.org/3/library/string.html

password1 and password2 are local variables so you can use one name password in both functions.

for loop put in two lines - to make it more readable.

for child in frame.winfo_children():
    child.grid_configure(padx=5, pady=10)

In comments you can put one space after #  - to make it more readable.
You can put one space after every , .

You can use "" directly with join

"".join(password1)

You don't need ()

around text_frame in

tk.Text((text_frame), ...

# OK
tk.Text(text_frame, ...

around capitol in

choice((capital))

# OK
choice(capital)

around letters in

choice((letters))

# OK
choice(letters)

You can add empty lines in functions to separate some part of functions

In complex_password you can randint(1, 2) instead randint(1, 4) because it gives the same probability - 50% for letter and 50% for number.

In complex_password you can use for loop instead while and don't need variable count.

In complex_password you don't need if count == 12 because this part can be outside while/for loop.

You can organize code this way

import tkinter as tk
from tkinter import ttk
import random
import string

# === constants === (UPPER_CASE names)

    # empty

# === classes === (CamelCase names)

    # empty

# === functions === (lower_case names)

def complex_password(): #complex password function
    password = []

    password.append(random.choice(capital))

    for __ in range(12):
        roll = random.randint(1, 2)

        if roll == 1:
            password.append(random.choice(letters))
        else:
            number = random.randint(0, 9)
            number = str(number)
            password.append(number)

    password = "".join(password)

    text.insert(tk.INSERT, password)
    text.insert(tk.INSERT, "\n")

def simple_password(): #simple password function
    password = []

    password.append(random.choice(words))
    password.append(random.choice(words))

    number = random.randint(1, 99)
    password.append(str(number))

    password = "".join(password)

    text.insert(tk.INSERT, password)
    text.insert(tk.INSERT, "\n")

def clear_text(): #clear txt box function
    text.delete(1.0, tk.END)

# === main ===

# --- some variables ---

words = [
    'Dog', 'Cat', 'Mouse', 'Fire', 'Ice', 'Basket', 'Tree', 'Tiger',
     'Lion', 'Flash','Super', 'Light', 'Zoom','Speed', 'Pants', 'Shirt',
     'Hat', 'Suit', 'Berry', 'Yogurt', 'Epic', 'Keyboard', 'Toe', 'Car',
     'Truck', 'Bike', 'Motor', 'Hammer', 'Pizza', 'Heart', 'Arm','Joint',
     'Saw', 'New', 'Carrots', 'Baby', 'Kiss', 'Backspace',  'Enter', 'Alt',
     'Print', "Down", 'Up', 'Question', 'Rain', 'Forest','Red', 'Orange',
     'Yellow', 'Green', 'Blue', 'Purple', 'Brown', 'Black', 'Indigo', 'Grey',
     'Shadow', 'Eye', 'Brick', 'Twig', 'Gangster', 'Thug', 'Chains', 'Gold',
     'Silver', 'Bronze', 'Platinum', 'Titanium', 'Exploding', 'Ladybug', 'Grass',
     'Monkey', 'Rhino', 'Comma', 'Hair', 'Shark', 'Fish'
]

letters = string.ascii_lowercase
capital = string.ascii_uppercase

# --- init ---

main = tk.Tk()

main.title("Password Gen")
main.resizable(width=tk.FALSE, height=tk.FALSE)
main.geometry("238x230")

# --- objects ---

frame = ttk.Frame(main)
button_frame = ttk.Frame(frame)
text_frame = ttk.Frame(frame)
text = tk.Text(text_frame, borderwidth=10, height=8, width=20, relief="sunken")

# buttons
simple = ttk.Button(button_frame, text="Simple", command=simple_password)
complex = ttk.Button(button_frame, text="Complex", command=complex_password)
clear = ttk.Button(button_frame, text="Clear", command=clear_text)

# buttons grids
simple.grid(column=2, row=1)
complex.grid(column=1, row=1)
clear.grid(column=3, row=1)
text.grid()

# frame grids
frame.grid(column=1, row=2)
text_frame.grid(column=1, row=2)
button_frame.grid(column=1, row=1)

# misc settings
for child in frame.winfo_children():
    child.grid_configure(padx=5, pady=10)

# --- start the engine ---

main.mainloop()


C++ wrapper class for array of OpenGL buffer objects -- just the constructors A similar question was asked here, and I'm trying to do the same thing. But

I'm trying a different approach by deriving from std::array, and
This is a very focused question about constructors only.

Here is my gl_wrap.h:
// header guards snipped

#include <array>
#include <algorithm>
#include <cstring>
#include <GL/gl.h>

namespace gl_wrap {

// aliases for style consistency and to mark as parts of gl_wrap
using gl_name = GLuint;
using gl_enum = GLenum;

template<size_t N>
class buffer_objects : public std::array<gl_name, N> {
public:
    buffer_objects() noexcept;
    ~buffer_objects();
    buffer_objects(const buffer_objects&) = delete;
    buffer_objects operator=(const buffer_objects&) = delete;
    buffer_objects(buffer_objects&& from) noexcept;
    buffer_objects& operator=(buffer_objects&& from) noexcept;
};

template<size_t N>
buffer_objects<N>::buffer_objects() noexcept
{
    glGenBuffers(N, this->data());
}

template<size_t N>
buffer_objects<N>::~buffer_objects()
{
    glDeleteBuffers(N, this->data());
}

template<size_t N>
buffer_objects<N>::buffer_objects(buffer_objects<N>&& from) noexcept
    : std::array<gl_name, N>(std::move(from))
{
    memset(from.data(), 0, N * sizeof(gl_name));
}

template<size_t N>
buffer_objects<N>& buffer_objects<N>::operator=(buffer_objects<N>&& from)
    noexcept
{
    std::array<gl_name, N>::operator=(std::move(from));
    memset(from.data(), 0, N * sizeof(gl_name));
    return *this;
}

}
// namespace gl_wrap

Some specific questions I have about this code/approach are, if applicable,

Error handling design decision: should the constructors throw, or should I leave error checking, if desired, to the caller?
Design decision: is it a good idea to use templates here? Will this lead to problematic code bloat if I'm using a lot of different-sized buffer_objects, or is it efficient? Can I evaluate this through profiling?
Are the memcpys worth it to put the rvalues in a valid non-owning state, or can I treat the rvalues as non-owning leftovers?
Am I properly using the base class's move constructor?

Some specific questions I have about this code/approach are, if applicable,

OK.

Error handling design decision: should the constructors throw, or should I leave error checking, if desired, to the caller?

Either the object is correctly initialized and ready to use or it should throw. Two stage initialization (construct then check) is a bad idea as it leaves it open to the user to do the correct thing. You should make sure your object can not be abused not relie on the user to not abuse you.

Design decision: is it a good idea to use templates here?

If you are going to use std::array you don't really have a choice.

Will this lead to problematic code bloat if I'm using a lot of different-sized buffer_objects, or is it efficient? Can I evaluate this through profiling?

Potentially this will lead to methods be compiled for different types. But this is compiler dependent and some modern compiler can optimize this out. But what are you comparing it too?

Are the memcpys worth it to put the rvalues in a valid non-owning state, or can I treat the rvalues as non-owning leftovers?

I think you need to null out the src values. Otherwise the destructor is going to release the names. Alternatively you can store more state about if the object is valid and make the call to glDeleteBuffers() conditional on the object being in a valid state.
But I also think there is a bug here. You have just copied over the values. But you did not release the values on the dst side before the copy so you have lost the names stored there.
Remember that a std::array does not have its own move (as the data is local to the object not dynamically allocated). It moves the underlying objects between containers.

Am I properly using the base class's move constructor?

Yes it seems ligit.
Suggestions.
I would make the std::array a member rather inherit from it.
I would do:
#ifndef THORSANVIL_GL_WRAPPER_H
#define THORSANVIL_GL_WRAPPER_H

#include <GL/gl.h>
#include <array>
#include <algorithm>
#include <cstring>

namespace ThorsAnvil::GL {

template<size_t N>
class BufferObjects
{
    std::array<GLuint, N>  buffer;
    bool                   valid;
    public:
        BufferObjects() noexcept;
       ~BufferObjects();

        // Note: These are non copyable objects.
        //       Deleting the copy operations.
        BufferObjects(BufferObjects const&)           = delete;
        BufferObjects operator=(BufferObjects const&) = delete;

        // Note: The move is as expensive as a copy operation.
        //       But we are doing a logical move as you
        //       can not have two objects with the same resource.
        BufferObjects(BufferObjects&& from)            noexcept;
        BufferObjects& operator=(BufferObjects&& from) noexcept;

        // Reset an invalid object.
        // Note: If object is valid no action.
        void reset();

};

template<size_t N>
BufferObjects<N>::BufferObjects() noexcept
    : valid(false)
{
    reset();
}

template<size_t N>
BufferObjects<N>::~BufferObjects()
{
    if (valid) {
        glDeleteBuffers(N, buffer.data());
    }
}

template<size_t N>
BufferObjects<N>::BufferObjects(BufferObjects<N>&& from) noexcept
{
    // Move the resources from the other object.
    std::move(std::begin(from.buffer), std::end(from.buffer), std::begin(buffer));

    // Maintain the correct valid states
    // The rhs is no longer in a valid state and has no resources.
    valid = from.valid;
    from.valid = false;
}

template<size_t N>
BufferObjects<N>& BufferObjects<N>::operator=(BufferObjects<N>&& from)
    noexcept
{
    // The standard copy and swap not efficient.
    // So we should do a test for self assignment
    if (this != &from)
    {
        // Destroy then re-create this object.
        // Call destructor and then placement new to use
        // the move copy constructor code to set this value.
        ~BufferObjects();
        new (this) BufferObjects(std::move(from));
    }
    return *this;
}

template<size_t N>
void BufferObjects::reset()
{
    if (!valid) {
        glGenBuffers(N, buffer.data());
        valid = true;
    }
}


Updating user information When I realized that the code below has a major problem in what it'll do (it'll just update the database with information currently in the database), I was told that there are a lot of mistakes.

Please point out the mistakes to me, suggest improvements and how to avoid the mistakes in the future.

public User Get(int UserID)
{
    string sql = "select * from users where userid = :id";

    List<MySqlParameter> args = new List<MySqlParameter>();
    args.Add(new MySqlParameter("id", UserID));

    MySqlCommand cmd = new MySqlCommand(sql, db.conn);
    cmd.Parameters.AddRange(args.ToArray());

    db.createConnection();
    MySqlDataReader dr = cmd.ExecuteReader();
    db.closeConnection();

    User user = new User();

    if (dr.HasRows)
    {
        dr.Read();

        user.EmailAddress = (string)dr["email"];
        user.Password = (string)dr["passcode"];
    }

    return user;
}

public void  Update(int UserID)
{
    User user = Get(UserID);

    string sql = "update users set email = :email, passcode = :pass where userid = :id";

    List<MySqlParameter> args = new List<MySqlParameter>();
    args.Add(new MySqlParameter(":email", user.EmailAddress));
    args.Add(new MySqlParameter(":pass", user.Password));
    args.Add(new MySqlParameter("id", UserID));

    MySqlCommand cmd = new MySqlCommand(sql, db.conn);
    cmd.Parameters.AddRange(args.ToArray());

    db.createConnection();
    cmd.ExecuteNonQuery();
    db.closeConnection();
}

I'm not sure why you're using your own built classes to access the database when .net has a very complete set of classes that can be used for data access.

Just looking at your syntax, I see the same functionality, so to me that is a waste of development time.

Also, it seems while I don't have access to your classes locally, I've changed them to use the included .net classes.

EDIT: Changed dr.GetString("") to dr.GetString(dr.GetOrdinal(""))

using System.Data.SqlClient;

public User Get(int userID)
{
    const string sql = "select * from users where userid = @id";
    const string connnectionString = "<your connection string>";

    // Using will dispose of the connection when the inner statements
    // have completed executing.  The dispose actually closes the connection.
    // The class has to implement the IDisposable interface.
    // var is a strongly typed variable, mostly used to keep the code easy to read.
    using(var connection = new SqlConnection(connnectionString))
    {
        connection.Open();

        // Make sure the SqlCommand object is disposed
        using (var cmd = new SqlCommand(sql, connection))
        {
            // No need to use a separate list to add parameters, just
            // add them directly to the command.
            cmd.Parameters.Add(new SqlParameter("@id", userID));

            var dr = cmd.ExecuteReader();
            var user = new User();

            // Read returns a boolean if it reads a record.
            if (dr.Read())
            {
                user.EmailAddress = dr.GetString(dr.GetOrdinal("email"));
                user.Password = dr.GetString(dr.GetOrdinal("passcode"));
            }
            return user;
        }
    }
}

public void Update(int userID)
{
    const string sql = "update users set email = @email, passcode = @pass where userid = @id";
    const string connnectionString = "<your connection string>";

    var user = Get(userID);

    using (var connection = new SqlConnection(connnectionString))
    {
        connection.Open();

        using (var cmd = new SqlCommand(sql, connection))
        {
            cmd.Parameters.Add(new SqlParameter("@email", user.EmailAddress));
            cmd.Parameters.Add(new SqlParameter("@pass", user.Password));
            cmd.Parameters.Add(new SqlParameter("@id", userID));

            cmd.ExecuteNonQuery();
        }
    }
}


Determine objects to delete from a database I have the following method which determines which cars I need to delete from the DB.

private List<CarDTO> BuildCarsToDelete(IList<CarDTO> newCars, IList<CarDTO> existingCars)
{
    var missingCars = new List<CarDTO>();

    var cars = newCars.Select(c => c.CarId);
    var newCarIds = new HashSet<int>(cars);

    foreach (var car in existingCars)
    {
        //If there are no new cars then it had some and they have been removed
        if (newCars.Count() == 0)
        {
            missingCars.Add(car);
        }
        else
        {
            if (!newCarIds.Contains(car.CarId))
            {
                missingCars.Add(car);
            }
        }
    }

    return missingCars;
}

This works as expecting - but if I want to achieve the same functionality for Customers or Apartments of other DTOs I will be copying a pasting the code but only changing the variable names and the Type of DTO around - is there a nicer way possible using generics which would keep the algorithm and logic as it is but allow me to use on any DTO?

There is an easier way to find the cars to delete. You just need to use the Except extension.

var carsToDelete = existingCars.Except(newCars);

In order for this to work you have two choices:

you can either implement the IEquatable for each DTO or
you can implement a common interface for the Id and a custom IEqualityComparer<IYourInterface> and use it with the Except extension.


A library for interacting with JPL Horizons I'm writing a tiny Python library that fetches data from Horizons, JPL's solar system data and ephemeris computation service. An overview of their batch CGI is here. While the script is simple, I would like to make it as clear and standardized as possible.
The idea is to initialize the HorizonsRequest class with a location for the observer (center), target (COMMAND), datetime, and the the data you want (quantities). The response can be retrieved as either a complete list of the lines sent back or as a dictionary of the requested values.
Example request
A request for the angle of Jupiter (599), its distance, and light travel time in relation to Earth (399) at the current time. Horizons users can find these numeric ids using the web-interface.
request = horizons.HorizonsRequest("399", "599", datetime.utcnow(), "2,20,21")
request.send()

dictionary = request.get_dictionary()
for key in dictionary:
    print(key, ":", dictionary[key])

Library
import sys
import urllib.request
import math
from datetime import datetime, timedelta

def get_julian_datetime(date):
    # Ensure correct format
    if not isinstance(date, datetime):
        raise TypeError('Invalid type for parameter "date" - expecting datetime')
    elif date.year < 1801 or date.year > 2099:
        raise ValueError('Datetime must be between year 1801 and 2099')

    # Perform the calculation
    julian_datetime = 367 * date.year - int((7 * (date.year + int((date.month + 9)
    / 12.0))) / 4.0) + int((275 * date.month) / 9.0) + date.day + 1721013.5
    + (date.hour + date.minute / 60.0 + date.second / math.pow(60,2)) / 24.0
    - 0.5 * math.copysign(1, 100 * date.year + date.month - 190002.5) + 0.5

    return julian_datetime

class HorizonsRequest:
    def __init__(self, center, target, datetime, quantities):
        self.keys = {
            "CENTER": center,
            "COMMAND": target,
            "QUANTITIES": quantities,
            "TLIST": str(get_julian_datetime(datetime)),
            "MAKE_EPHEM": "YES",
            "OBJ_DATA": "NO",
            "TABLE_TYPE": "OBSERVER",
            "CAL_FORMAT": "BOTH",
            "ANG_FORMAT": "DEG",
            "CSV_FORMAT": "YES"
        }
        self.response = []
        self.dictionary = {}

    def set_key(self, key, value):
        self.keys[key] = str(value)

    def delete_key(self, key):
        self.keys.pop(key, None)

    def send(self):
        # create request
        request = "https://ssd.jpl.nasa.gov/horizons_batch.cgi?batch=1"
        for key in self.keys:
            request += "&" + key + "='" + self.keys[key] + "'"

        # send request
        file = urllib.request.urlopen(request)
        for line in file:
            self.response.append(line.decode('utf-8').replace('\n', ''))

    def get_dictionary(self):
        keys = []
        values = []

        # locate data position in response
        i = 0
        while i < len(self.response):
            if "$$SOE" in self.response[i]:
                keys = self.response[i - 2].split(",")
                values = self.response[i + 1].split(",")
            i += 1

        # create dictionary
        i = 0
        while i < len(keys):
            self.dictionary[keys[i].strip()] = values[i].strip()
            i += 1

        return self.dictionary

    def get_response(self):
        return self.response

Any feedback would be greatly appreciated.

Use Requests
urllib is kind of a pain to use. Use Requests instead. Among other things, all of this code:
    for key in self.keys:
        request += "&" + key + "='" + self.keys[key] + "'"

will go away. Requests has query string formatting from a dictionary.
Julian conversion
It's doubtful that you need to be doing this yourself. Find a library such as https://pypi.org/project/julian/ .
Combined-inequality syntax
date.year < 1801 or date.year > 2099

should be
not (1800 < date.year < 2100)

Squaring
math.pow(60,2)

should just be
60**2

Late serialization
I would recommend that HorizonsRequest.keys (which, by the way, is not only a collection of keys - it's a dictionary, so it's misnamed) not be constructed until it's needed in send. Nor should you expose arbitrary set_key / delete_key. In theory you should understand enough about the request format that information to create it is stored in more strongly-typed variables that can receive better validation. For instance, do not store
"OBJ_DATA": "NO",

as a class member; instead store
obj_data: bool = False


Web Crawler (A Tour of Go #71) So I was trying to do this exercise from Tour of Go. I managed to get it working. I am not at all sure that it is correctly concurrent or idiomatic (I started learning Go, like, 4 hours ago).

I would really appreciate feedback about my solution. Here's the relevant meat of the code. Full code available below.

type urlToFetch struct {
  Url   string
  Depth int
}

// Group stuff together, so that it's easier to pass around.
type fetchContext struct {
  Queue   chan urlToFetch
  History map[string]int
  Quit    chan int
  Fetcher Fetcher
}

func Crawl(url string, depth int, fetcher Fetcher) {
  context := fetchContext{
    Queue:   make(chan urlToFetch, 500),
    History: make(map[string]int),
    Quit:    make(chan int, 50),
    Fetcher: fetcher,
  }

  go fetchOne(&context)
  context.Queue <- urlToFetch{url, depth}

  <-context.Quit
}

func fetchOne(ctx *fetchContext) {
  timeout := time.After(2000 * time.Millisecond)

  select {
  case utf := <-ctx.Queue:
    if utf.Depth > 0 {
      body, urls, err := ctx.Fetcher.Fetch(utf.Url)
      ctx.History[utf.Url] = 1

      if err != nil {
        fmt.Println(err)
        return
      }
      fmt.Printf("found: %s %q\n", utf.Url, body)

      for _, u := range urls {
        go fetchOne(ctx)
        if _, ok := ctx.History[u]; !ok {
          ctx.Queue <- urlToFetch{u, utf.Depth - 1}
        } else {
        }
      }
    }
  case <-timeout:
    ctx.Quit <- 1
  }
}

The full source is at: http://pastie.org/8443147

There are two things I see.

Don't store a reference to a time out. I think the count down starts when After() is called.

timeout := time.After(2000 * time.Millisecond)
...
case <- timeout:

Most code I've seen writes it like this.

case <- time.After(2000 * time.Millisecond):

Use a lock when accessing a map from multiple goroutines because maps aren't thread safe.

Better solution:

func Crawl(url string, depth int, fetcher Fetcher) {
    m := map[string]bool{url: true}
    var mx sync.Mutex
    var wg sync.WaitGroup
    var c2 func(string, int)
    c2 = func(url string, depth int) {
        defer wg.Done()
        if depth <= 0 {
            return
        }
        body, urls, err := fetcher.Fetch(url)
        if err != nil {
            fmt.Println(err)
            return
        }
        fmt.Printf("found: %s %q\n", url, body)
        mx.Lock()
        for _, u := range urls {
            if !m[u] {
                m[u] = true
                wg.Add(1)
                go c2(u, depth-1)
            }
        }
        mx.Unlock()
    }
    wg.Add(1)
    c2(url, depth)
    wg.Wait()
}

Useful link:

https://stackoverflow.com/questions/12224962/exercise-web-crawler-concurrency-not-working


Segment Intersection — Failing on Unknown Edge Cases I was recently at a programming competition. One of the problems was to determine whether two line segments are parallel, intersect, or do not intersect.

For all of the test cases I ran, my solution worked. However, when I submitted it, it was rejected (but no other information was given). I suspect there is either some edge case I am completely missing... or perhaps the judges messed up.

Input is entered as follows:

ax ay bx by cx cy dx dy

where the first segment is given by $$(a_x, a_y), (b_x, b_y)$$ and the second by $$(c_x, c_y), (d_x, d_y)$$

Some other rules/assumptions:

We may assume that no segment has length zero; that is, each segment has distinct endpoints.
Segments may share endpoint(s) with the other segment. Endpoints are included in the segment and thus are able to intersect (this also means that a segment whose endpoint lies within the other segment will intersect)
Lines which both intersect and are parallel (either partial or complete overlap) should be considered parallel.
All coordinates are integers on the open interval (-1000, 1000).

Here's a screenshot of the problem. Note that I'm ignoring the multiple tests option because I highly doubt that was the issue.

import java.util.Scanner;

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

        int ax = in.nextInt();
        int ay = in.nextInt();
        int bx = in.nextInt();
        int by = in.nextInt();
        int cx = in.nextInt();
        int cy = in.nextInt();
        int dx = in.nextInt();
        int dy = in.nextInt();

        in.close();

        /*
         * Cross multiply to check equal slopes This takes care of
         * +/- infinity as well as zero
         */
        if ((by - ay) * (dx - cx) == (dy - cy) * (bx - ax))
        {
            System.out.println("PARALLEL");
        }
        else
        {
            double m1 = (double) (by - ay) / (bx - ax);
            double m2 = (double) (dy - cy) / (dx - cx);

            // Find point of intersection
            double x, y;
            if (Double.isInfinite(m1))
            {
                x = ax;
                y = m2 * (x - cx) + cy;
            }
            else if (Double.isInfinite(m2))
            {
                x = cx;
                y = m1 * (x - ax) + ay;
            }
            else
            {
                x = (m1 * ax - ay - m2 * cx + cy) / (m1 - m2);
                y = m1 * (x - ax) + ay;
            }

            // Check bounds
            if (((x >= ax && x <= bx) || (x <= ax && x >= bx))
                    && ((y >= ay && y <= by) || (y <= ay && y >= by))
                    && ((x >= cx && x <= dx) || (x <= cx && x >= dx))
                    && ((y >= cy && y <= dy) || (y <= cy && y >= dy)))
            {
                System.out.println("INTERSECT");
            }
            else
            {
                System.out.println("DO NOT INTERSECT");
            }
        }

    }
}

And some test cases:

Vertical Horizontal, Intersect

0 0 0 5 -1 2 1 2
INTERSECT

Vertical Horizontal, No Intersect

0 0 0 5 1 2 4 2
DO NOT INTERSECT

Horizontal Horizontal, Parallel

0 0 5 0 1 1 5 1
PARALLEL

Vertical Regular, Intersect

0 0 0 5 1 0 -1 4
INTERSECT

Vertical Regular, No Intersect

0 0 0 5 1 0 4 6
DO NOT INTERSECT

Horizontal Regular, Intersect

4 4 7 4 4 3 6 5
INTERSECT

Horizontal Regular, No Intersect

2 2 8 2 3 3 9 6
do not intersect

Regular Regular, Intersect

2 1 4 3 1 2 5 1
INTERSECT

Regular Regular, Parallel

1 2 5 6 3 2 5 4
PARALLEL

Regular Regular, No Intersect

1 2 5 6 3 2 8 5
DO NOT INTERSECT

Let me know if anything else needs to be clarified (or any other test cases).

Failure cases

I ran your program and was able to find an endless number of failures.  My technique was to have one line touch the origin (0,0) and then generate all lines that pass directly through the origin by simply choosing mirrored coordinates.  I was hoping to find roundoff errors and I did.  Here are just a small sampling of failure cases, all of which print "Do not intersect" instead of "intersect":

0 0 0 5 7 29 -7 -29
0 0 0 5 11 -30 -11 30
0 0 0 5 11 -15 -11 15
0 0 0 5 11 25 -11 -25
0 0 0 5 13 -30 -13 30
0 0 0 5 13 -15 -13 15
0 0 0 5 13 27 -13 -27
0 0 0 5 14 29 -14 -29
0 0 0 5 19 21 -19 -21
0 0 0 5 21 23 -21 -23
0 0 0 5 21 27 -21 -27
0 0 0 5 22 -30 -22 30
0 0 0 5 22 -15 -22 15
0 0 0 5 22 25 -22 -25
0 0 0 5 23 -26 -23 26
0 0 0 5 23 -13 -23 13
0 0 0 5 23 27 -23 -27
0 0 0 5 25 -29 -25 29
0 0 0 5 25 7 -25 -7
0 0 0 5 25 14 -25 -14
0 0 0 5 25 28 -25 -28
0 0 0 5 26 -30 -26 30
0 0 0 5 26 -15 -26 15
0 0 0 5 26 27 -26 -27
0 0 0 5 27 29 -27 -29
0 0 0 5 28 29 -28 -29
0 0 0 5 29 15 -29 -15

Floating point?

My personal opinion about solving this problem correctly is that if your coordinates are constrained to be within -1000..1000, then you can do the whole problem using integers and not use floating point at all.

What you would need to do is create a fraction class that holds a numerator and denominator.  The fraction class needs to be able to: multiply, divide, add, subtract, compare (i.e. everything you are doing with your doubles).  Since the range of coordinates is so small, you shouldn't have a problem with overflow.  By doing this you will be safe from roundoff errors.


Is this a 100% safe from SQL Injection and will work correctly for any input? $a=$_GET["a"];
$b=$_GET["b"];
$a=str_replace("%", "\%", $a);
$b=str_replace("%", "\%", $b);
$sql="SELECT * FROM table ";
$sql.="WHERE ColA LIKE :txtA AND ColB LIKE :txtB";
$query = $db->prepare($sql);
$query->bindValue(':txtA', '%'.$a.'%', PDO::PARAM_STR);
$query->bindValue(':txtB', '%'.$b.'%', PDO::PARAM_STR);
$query->execute();

I want to give user ability to search anything. Should I filter more characters? Will it be 100% safe and work as it should do for searching pieces of text in database?

Edit:
ColA and ColB are strings, just text (phrase and tags in search engine)

I want to give user ability to search anything.

It looks like you've achieved that, however, I'd use $_POST to reduce the likelihood that your application will be taken offline by users opening multiple tabs or search engine spiders hitting URLs.

  Will it be 100% safe and work as it should do for searching pieces of text in database?

Looks good for SQL injection.

As to whether it "should do" - have you considered generating the query dynamically to allow for tokenized search on inputs like "foo bar"? (i.e. ColB LIKE :token1 OR ColB LIKE :token2)


Bounded spigot algorithms for e and pi in Python I've written bounded spigot algorithms(Spigot Algorithm) for e and pi. I would like to understand if there is anything I can do to make this more efficient. I've done things already like move the carry over check to the very end, precompute values etc.
You run the functions and pass it the number of digits you want i.e. print(pispig(1000))`
The spigot algorithm for computing functions you can apply a Horner schema to is one of the coolest things I have ever learned.
def espig(n):
    estr = '2.'
    (multq, asum, carover) = (0, 0, 0)
    prec = 10 ** n
    (remainders, tsum) = ([], [])
    b = [x for x in range(2, n + 1)]
    init = [1 for x in range(2, n + 1)]
    x10 = [prec * x for x in init]
    while True:
        for (x, y) in zip(reversed(b), x10):
            asum = carover + y
            remainders.insert(0, asum % x)
            tsum.insert(0, asum)
            multq = asum // x
            carover = multq
        estr += str(int(tsum[0] // 2))
        x10 = [prec * x for x in list(reversed(remainders))]
        (carover, asum) = (0, 0)
        (remainders, tsum) = ([], [])
        if len(estr) > n:
            break
    return estr

def pispig(n):
    e = n
    xfact = 10 ** e
    n = int(round(3.4 * n))
    prec = n // 3.4
    (pilist, remainders, tsum) = ([], [], [])
    (multq, asum, carover, counter) = (0, 0, 0, 0)
    a = [x for x in range(n)]
    b = [2 * x + 1 for x in range(n)]
    init = [2 for x in range(1, n + 1)]
    x10 = [xfact * x for x in init]
    while True:
        for (A, B, R) in zip(reversed(a), reversed(b), reversed(x10)):
            asum = carover + R
            remainders.insert(0, asum % B)
            tsum.insert(0, asum)
            multq = asum // B
            carover = multq * A
        pilist.append(tsum[0] // 10)
        remainders[0] = tsum[0] % xfact
        x10 = [10 * x for x in list(remainders)]
        (carover, asum) = (0, 0)
        (remainders, tsum) = ([], [])
        if len(''.join([str(x) for x in pilist])) > prec:
            break
    for D in reversed(pilist):
        counter = 0
        if D >= xfact:
            pilist[pilist.index(D) - 1] += 1
            pilist[pilist.index(D)] = pilist[pilist.index(D)] % xfact
            counter += 1
    return str('3.' + ''.join(str(x) for x in pilist[1:]))

About the first function. If you are so inclined, apply these ideas to the second one too.
Generally I'd love to see more descriptive variable names, comments, a docstring.
I tried to stick to your algorithm/implementation as much as possible, i.e. I'm just playing the role of a linter here.
That being said.
Why
while True:
  ....
  if len(estr) > n:
     break

instead of
while len(estr) <= n:
  ....

?
Why
        asum = carover + y
        remainders.insert(0, asum % x)
        tsum.insert(0, asum)
        multq = asum // x
        carover = multq

instead of
        asum = carover + y
        tsum.insert(0, asum)
        divx, modx = divmod(asum, x)
        remainders.insert(0, modx)
        carover = divx

?
Why not move
    (carover, asum) = (0, 0)
    (remainders, tsum) = ([], [])

to the start of the loop, so that you can remove these from before the loop.
Why not write e.g.
carover, asum = 0, 0

instead, and lose the parens?
Why build an inner list in
x10 = [prec * x for x in list(reversed(remainders))]

instead of just using the iterator
x10 = [prec * x for x in reversed(remainders)]

?
Why cast to int after integer division? You shouldn't need the int in the following:
int(tsum[0] // 2)

Why reverse b, remainders? Why not construct them so that they are "reversed" by default:
b = list(range(n, 1, -1))

remainders.append(asum % x)

To maintain symmetry, let us do the same to tsum:
estr += str(tsum[-1] // 2)

tsum.append(asum)

Why do we need multq = 0? Why do we need multq at all?
You don't use tsum, only its first value. I don't think there's a need to build b either.
OK, so far we have something like the following:
def espig(n):
    estr = '2.'
    prec = 10 ** n
    ys = [1 for _ in range(2, n + 1)]

    while len(estr) <= n:
        carover, asum, remainders = 0, 0, []

        for x, y in zip(range(n, 1, -1), ys):
            asum = carover + prec * y
            carover, remainder = divmod(asum, x)
            remainders.append(remainder)

        ys = remainders
        estr += str(asum // 2)

    return estr


Query for stddev of number of stars per chatroom message I am learning SQL today, so I decided to practice by writing some queries for The Nineteenth Byte Data Explorer. This is a PostgreSQL database about messages from The Nineteenth Byte chatroom, created by El'endia Starman. The schema can be found at the bottom of the page.

The code functions correctly (as far as I can tell), but it could surely look better. In particular, I'm interested in knowing more elegant ways to store the result of an expression to be used later in the query. Right now, I'm creating a table containing one value using a WITH clause and then SELECTing * from the table each time I want to reference the number.

The purpose of the code is to, given an SE chat user ID, find the mean and standard deviation of the number of stars on messages posted by that user, among those which have at least one star.

The code can be tested here.

WITH SE_Chat_ID_to_search AS ( SELECT
169713   -- your query here
AS VAL),

internal_id as (
   SELECT max(id)
   FROM "transcriptAnalyzer_user"
   WHERE uid = (SELECT * from SE_Chat_ID_to_search)
),

stats as (
   SELECT AVG(stars) as mean, COUNT(*) as num_starred
   FROM "transcriptAnalyzer_message"
   WHERE user_id = (SELECT * FROM internal_id) AND stars > 0
),

sum_diff_squared as (
   SELECT sum(power((SELECT mean from stats) - stars, 2.)) as sds
   FROM "transcriptAnalyzer_message"
   WHERE user_id = (SELECT * FROM internal_id) AND stars > 0
)

SELECT mean, power(sds / num_starred, 0.5) as stddev from stats, sum_diff_squared

To answer your primary question directly, in certain SQL engines (for instance, Microsoft SQL Server and MySQL) you can declare query variables. Using plain SQL with Postgres, that is not possible.

You would either need to use plpgsql, which I'm pretty sure can't be done on 19B Data Explorer, or, using plain SQL, use WITH table expressions as you have.

Granted, it's not pretty, but otherwise works fine.

For a start, I would recommend to copy your query into a SQL formatter to format the code more appropriately; this makes it easier to then improve the code.

When using WITH tables, naming things goes a long way. I would first give this value a name (and remove the comment, please, or write a useful one):

WITH SE_Chat_ID_to_search
     AS (SELECT
        --Enter the user's SE chat user ID here:
        169713 AS id),

Which makes it easier to reference later and avoid doing SELECT * FROM SE_Chat_ID_to_search. I would also give a name such as uid to the value obtained within internal_id expression, for the same reason:

 internal_id
 AS (SELECT Max(id) AS uid
     FROM   "transcriptAnalyzer_user"
     WHERE  uid = (SELECT id
                   FROM   SE_Chat_ID_to_search)),

I would also carry that user ID into your other WITH table expressions, so that they can be joined properly (more on that in a moment), which will require the addition of a GROUP BY clause (or perhaps an aggregate function on it such as MAX(), same result though intention is less clear):

 stats
 AS (SELECT user_id,
            Avg(stars) AS mean,
            Count(*)   AS num_starred
     FROM   "transcriptAnalyzer_message"
     WHERE  user_id = (SELECT uid
                       FROM   internal_id)
        AND stars > 0
     GROUP  BY user_id),
 sum_diff_squared
 AS (SELECT user_id,
            Sum(Power((SELECT mean
                       FROM   stats) - stars, 2.)) AS sds
     FROM   "transcriptAnalyzer_message"
     WHERE  user_id = (SELECT uid
                       FROM   internal_id)
        AND stars > 0
     GROUP  BY user_id)

That brings us to the query which brings those tables together into the result set:

SELECT mean,
       power(sds / num_starred, 0.5) as stddev
from stats,
     sum_diff_squared

I mentioned joining tables properly. What you did above is an implicit join without a join condition, which effectively results in a Cartesian join (i.e. a Cartesian product of all rows of each table).

In your case, the impact was not felt because both tables return a single row; however, in reality, a Cartesian join is not only extremely slow, but more often also, unless done with careful intent, wrong.

There are 2 ways we can change this to a proper, non-Cartesian join. The first is by using the older (pre-ANSI92) implicit join syntax:

SELECT s.mean,
       Power(sds.sds / s.num_starred, 0.5) AS stddev
FROM   stats AS s,
       sum_diff_squared AS sds
WHERE  s.user_id = sds.user_id

Or by using the more modern explicit join syntax, which I always prefer due to its greater clarity:

SELECT s.mean,
       Power(sds.sds / s.num_starred, 0.5) AS stddev
FROM   stats AS s
JOIN   sum_diff_squared AS sds
       ON s.user_id = sds.user_id

The revised query returns identical results.