Rock Papers Scissors in Python I am a newbie to Python and have started object-oriented programming recently. I have implemented a "Rock Paper Scissors" application in OOP. I would like for you to evaluate my code, and tell me where can I improve my code and how I can better organize the functionality.

import random

class RockPaperScissors(object):
    def setUserOption(self,val):
        self.__option = val

    def getValueFromList(self,option):
        l = ['rock','scissors','paper']
        return l[option]

    def __getRandomValue(self,max):
        val = random.randint(1,max)
        return self.getValueFromList(val-1)

    def __getResult(self,t):
        self.__d = {('rock','scissors'):'rock breaks scissors - User has won',('rock','paper'):'rock is captured by paper - User has won',('scissors','paper'):'Scissors cut paper - User has won',
                    ('scissors','rock'):'rock breaks scissors - Computer won',('paper','rock'):'rock is captured by paper - Computer won',('paper','scissors'):'Scissors cut paper - Computer won'}
        return self.__d[t]

    def __computeResult(self,computerOption):
        if computerOption == self.__option:
            return 'The user and computer choose the same option'
        else:
            return self.__getResult((self.__option,computerOption))

    def printResult(self):
        computerOption = self.__getRandomValue(3)
        print 'User choice: ',self.__option
        print 'Computer choice: ',computerOption
        print self.__computeResult(computerOption)

if __name__ == "__main__":
    print 'Welcome to rock paper scissors'
    print '1. Rock, 2. Paper, 3. Scissor'
    val = int(raw_input('Enter your choice Number: '))
    if val >=1 and val <= 3:
        obj = RockPaperScissors()
        obj.setUserOption(obj.getValueFromList(val-1))
        obj.printResult()
    else:
        raise ValueError('You are supposed to enter the choice given in the menu')

These answers are fantastic, but seem to be focusing on one side of your question.  I'm going to focus on object orientation.

I reviewed your code with a couple basic concepts in mind, namely polymorphism and encapsulation.

Polymorphism

Where are your objects?  A rock, paper, and scissors could all be objects, could they not?  And, most importantly, they are all the same kind of object.  They are all "pieces" of the game.

Here's a pseudo-example:

class Rock inherits from Element
    type = 'rock'
    def compare(Element)
        if type == 'paper'
            return 'LOSE'
        elsif type == 'scissors'
            return 'WIN'
        else
            return 'TIE'

Although this is a potential solution, my example is going to go in a different direction simply for the fact that if you wanted to add more elements then you'd have to touch the existing code (re: Chris' example of RPSLS).

Encapsulation

A great programming muscle to exercise is encapsulation.  The two main areas, in this example, are to hide the user interface from the game code.

The user interface shouldn't care about the inner-workings of the game.  The game shouldn't care how it's displayed.  What if you wanted to change the interface to something more graphical?  Right now, your code relies on the console to know about the pieces of the game and how it works.

Example

So, let's return to Chris' example again.  The elements of this game are something that may change quite often.  How can you handle this to make future programming easier?

One solution is to store the "data" of the game elsewhere and dynamically create objects.

The following is Python code to materialize the ideas I've written about here.

Here's code for an abstracted piece of the game, an element:

# The goal for an element is to just know when it wins, when it loses, and how to figure that out.
class Element:
    _name = ""
    _wins = {}
    _loses = {}

    def get_name(self):
        return self._name

    def add_win(self, losingElementName, action):
        self._wins[losingElementName] = action

    def add_loss(self, winningElementName, action):
        self._loses[winningElementName] = action

    def compare(self, element):
        if element.get_name() in self._wins.keys():
            return self._name + " " + self._wins[element.get_name()] + " " + element.get_name()
        elif element.get_name() in self._loses.keys():
            return None
        else:
            return "Tie"

    def __init__(self, name):
        self._name = name
        self._wins = {}
        self._loses = {}

Games have players:

# The player's only responsibility is to make a selection from a given set. Whether it be computer or human.
class Player:
    _type = ''
    _selection = ''

    def make_selection(self, arrayOfOptions):
        index = -1
        if (self._type == 'Computer'):
            index = random.randint(0, len(arrayOfOptions) - 1)
        else:
            index = int(raw_input('Enter the number of your selection: ')) - 1
        self._selection = arrayOfOptions[index]
        return self._type + ' selected ' + self._selection + '.'

    def get_selection(self):
        return self._selection

    def __init__(self, playerType):
        self._type = playerType
        self._selection = ''

Game code:

# A game should have players, game pieces, and know what to do with them.
class PlayGame:
    _player1 = Player('Human')
    _player2 = Player('Computer')

    _elements = {}

    def print_result(self, element1, element2):
        val = element1.compare(element2)
        if (val != None):
            print "YOU WIN! (" + val + ")" # win or tie
        else:
            print "You lose. (" + element2.compare(element1) + ")"

    def fire_when_ready(self):
        counter = 1
        for e in self._elements.keys():
            print str(counter) + ". " + e
            counter = counter + 1
        print ""
        print "Shoot!"
        print ""

        print self._player1.make_selection(self._elements.keys())
        print self._player2.make_selection(self._elements.keys())

        element1 = self._elements[self._player1.get_selection()]
        element2 = self._elements[self._player2.get_selection()]

        self.print_result(element1, element2)

    def load_element(self, elementName1, action, elementName2):
        winningElementObject = None
        newElementObject = None

        if (elementName1 in self._elements):
            winningElementObject = self._elements[elementName1]
            winningElementObject.add_win(elementName2, action)
        else:
            newElementObject = Element(elementName1)
            newElementObject.add_win(elementName2, action)
            self._elements[elementName1] = newElementObject

        if (elementName2 in self._elements):
            losingElementObject = self._elements[elementName2]
            losingElementObject.add_loss(elementName1, action)
        else:
            newElementObject = Element(elementName2)
            newElementObject.add_loss(elementName1, action)
            self._elements[elementName2] = newElementObject

    def __init__(self, filepath):
        # get elements from data storage
        f = open(filepath)
        for line in f:
            data = line.split(' ')
            self.load_element(data[0], data[1], data[2].replace('\n', ''))

The console code, the user interface:

if __name__ == "__main__":
    print "Welcome"
    game = PlayGame('data.txt')
    print ""
    print "Get ready!"
    print ""
    game.fire_when_ready()

And the data file, data.txt:

scissors cut paper
paper covers rock
rock crushes lizard
lizard poisons spock
spock smashes scissors
scissors decapitates lizard
lizard eats paper
paper disproves spock
spock vaporizes rock
rock crushes scissors

The goal for my answer was to show you how you can use some object oriented concepts in solving your problem.  In doing so I also left some unresolved problems.  One big one is that the player and game objects are still coupled to the user interface.  One way to nicely resolve this would be through the use of delegates.


Access function to return "number of working days until date" for form I need a function I can put on a form which does:

Calculates number of working days until a given date
Excludes weekends
Excludes any dates in a "holiday" table

This is replacing something which looks like:

 =DateDiff("d",Now(),[DateEstimatedDesignStartBy])

with

 =getWorkingDaysUntilDate([DateEstimatedDesignStartBy])

The below is a all within a single module I am using to do this.

I am reading in the entire table of holidays and caching it, and then iterating until teh required date to find the number of days and then caching this result as well.

The net result is a much faster function than having to do the calculation for each time I do anything.

Any feedback would be great. I have a lot of poor naming but am not really sure what would be good...

It seems a bit slower than what it is replacing.

Option Compare Database
Option Explicit

Private m_HolidayLibrary As Scripting.Dictionary
Private m_WorkingDaysUntilDate As Scripting.Dictionary

Public Function getWorkingDaysUntilDate(p_date As Date) As Integer

    If getWorkindDaysUntilDateDict.exists(p_date) = False Then
        getWorkindDaysUntilDateDict.Add p_date, calculateWorkingDaysUntilDate(p_date)
    End If

    getWorkingDaysUntilDate = getWorkindDaysUntilDateDict(p_date)
End Function
Private Function getWorkindDaysUntilDateDict() As Scripting.Dictionary

    If m_WorkingDaysUntilDate Is Nothing Then
        Set m_WorkingDaysUntilDate = New Scripting.Dictionary

    End If

    Set getWorkindDaysUntilDateDict = m_WorkingDaysUntilDate

End Function
Private Function calculateWorkingDaysUntilDate(p_date As Date) As Integer

    Dim iteratedDate As Date
    iteratedDate = Date
    calculateWorkingDaysUntilDate = 0

    Do Until iteratedDate >= p_date
        calculateWorkingDaysUntilDate = calculateWorkingDaysUntilDate + 1

        Do
            iteratedDate = DateAdd("d", 1, iteratedDate)
            If isDateValidAdd(iteratedDate) Then Exit Do
        Loop

    Loop

End Function

Private Function isDateValidAdd(p_date As Date) As Boolean
    isDateValidAdd = True

    If getHolidayLibrary.exists(p_date) Then
        isDateValidAdd = False
    'saturdays
    ElseIf Weekday(p_date, vbSaturday) = 1 Then
        isDateValidAdd = False
    'sunday
    ElseIf Weekday(p_date, vbSaturday) = 2 Then
        isDateValidAdd = False
    End If

End Function

Private Function getHolidayLibrary() As Scripting.Dictionary

    If m_HolidayLibrary Is Nothing Then
        Set m_HolidayLibrary = createHolidayLibrary
    End If

    Set getHolidayLibrary = m_HolidayLibrary

End Function
Private Function createHolidayLibrary() As Scripting.Dictionary

   Dim rs As ADODB.Recordset
   Set rs = New ADODB.Recordset
   Set createHolidayLibrary = New Scripting.Dictionary

   rs.Open "eHolidays", CurrentProject.Connection, , adLockReadOnly

   Do Until rs.EOF
        createHolidayLibrary.Add CDate(rs.fields("Holiday")), rs.fields("HolidaysID")
        rs.MoveNext
   Loop

End Function

Don't compare a Boolean expression with a Boolean value to make.. a Boolean expression - the syntax for If blocks goes If {Boolean Expression} Then - therefore, = False is unnecessary here:

If getWorkindDaysUntilDateDict.exists(p_date) = False Then

The IsDateValid function can be simplified:

isDateValidAdd = True

If getHolidayLibrary.exists(p_date) Then
    isDateValidAdd = False
'saturdays
ElseIf Weekday(p_date, vbSaturday) = 1 Then
    isDateValidAdd = False
'sunday
ElseIf Weekday(p_date, vbSaturday) = 2 Then
    isDateValidAdd = False
End If

Is logically equivalent to :

Dim wkDayForDate Integer
wkDayForDate = Weekday(p_date, vbSaturday)

IsDateValidAdd = Not (GetHolidayLibrary.Exists(p_date) Or wkDayForDate = 1 Or wkDayForDate = 2)

Now because VBA will not short-circuit, one may argue that the long-winded If..ElseIf block will perform better... but comparing {Integer} = {value} isn't exactly an expensive operation, so I wouldn't worry about not short-circuiting.

If anything you may want to avoid accessing GetHolidayLibrary when Weekday(p_date, vbSaturday) is 1 or 2, so this:

Dim wkDayForDate Integer
wkDayForDate = Weekday(p_date, vbSaturday)

If Not (wkDayForDate = 1 Or wkDayForDate = 2) Then
    IsDateValidAdd = Not GetHolidayLibrary.Exists(p_date)
End If

Might just be the best compromise.

"Is date valid add" isn't exactly a great name. Assuming it means "can this date be added?", it could be renamed to CanAddDate, or perhaps IsWorkday.

Procedure names should be PascalCase, and I would drop the p_ prefix for parameters and the m_ prefix for fields.

Private Function getHolidayLibrary() As Scripting.Dictionary

    If m_HolidayLibrary Is Nothing Then
        Set m_HolidayLibrary = createHolidayLibrary
    End If

    Set getHolidayLibrary = m_HolidayLibrary

End Function

This looks like you're trying to "encapsulate" the dictionary with a getter... but in the same scope as the rest. You could have a class responsible for this encapsulation, and then the module wouldn't have to care about whether the holidays are loaded; CreateHolidayLibrary (which is really LoadHolidays or, in the case of a hypothetical HolidayLibrary class, simply Load or Refresh) would belong there as well.


Unnecessary unit test? I have this method in my service layer

    public ModuleResponse GetModules(ModuleRequest request)
    {
        var response = new ModuleResponse(request.RequestId);
        try
        {
            response.Modules = Mapper.ToDataTransferObjects(ModuleDao.GetModules());
            return response;
        }
        catch (Exception ex)
        {
            Log.Error(ex);
            response.Acknowledge = AcknowledgeType.Failure;
            response.Message = "An error occured.";
            return response;
        }
    }

I've written a unit test like this

    [Fact]
    public void GetModulesTest()
    {
        //Arrange
        var mockModuleDao = Mock.Create<IModuleDao>();
        var mockLog = Mock.Create<ILog>();
        var mockAuditDao = Mock.Create<IAuditDao>();

        var moduleList = new List<ModuleItem>
        {
            new ModuleItem {Id = 100, Category = "User Accounts", Feature = "Users"},
            new ModuleItem {Id = 101, Category = "User Accounts", Feature = "Roles Permissions"}
        };

        mockModuleDao.Arrange(dao => dao.GetModules()).Returns(moduleList);

        IUserManagementService userService = new UserManagementService(mockModuleDao, mockLog, mockAuditDao);

        var request = new ModuleRequest().Prepare();

        //Act
        var actualResponse = userService.GetModules(request);

        //Assert
        Assert.Equal(AcknowledgeType.Success, actualResponse.Acknowledge);
        Assert.Equal(2, actualResponse.Modules.Count);
    }

My question is... are such methods worth testing? I have a whole other bunch of retrieve methods that just return me lists of data.

Are there any other ways to improve the unit test in this particular example?

The first assert basically checks if the method has been executed without any error.

Is the 2nd Assert of my test redundant?

In my opinion No. There is hardly any behaviour at all. As soon you say "I have this method in my service layer" it make it much more obvious. Service layer provide communication and pretty much should orchestrate things. That's it.

Note that Unit tests are good but have it unnecessary written they become a maintenance issue.

Test your code which has behaviour from the customer's requirements point of view.

Typically you would have an acceptance type test or an integration test to cover service layer and other layers as well.


Printing Brown numbers I have written a program to print Brown numbers from Brocard's problem.
I want to improve the code.

  Brocard's problem is a problem in mathematics that asks to find
  integer values of n and m for which

n ! + 1 = m^2

  where n! is the factorial.

  Pairs of the numbers (n, m) that solve Brocard's problem are called
  Brown numbers. There are only three known pairs of Brown numbers:

(4,5), (5,11), and (7,71).

#include <iostream>

unsigned int factorial(unsigned int num)
{
  unsigned int result = 1;
  for(unsigned int i = 1; i <= num; i++)
  {
    result = result*i;
  }
  return result;
}

void print_brown_numbers(int limit)
{
  for(unsigned int i = 2; i <= limit; i++)
  {
    for(unsigned int j = 1; j < i; j++)
    {
      if((i * i) == (factorial(j) + 1))
      {
        std::cout << i << " " << j << " are brown numbers\n";
      }
    }
  }
}

int main()
{
  unsigned int max_limit;
  std::cout << "Enter the maximum limit for you want to test \n";
  std::cin >> max_limit;
  print_brown_numbers(max_limit);
}

This is a pretty good brute-force way to find these numbers. It will obviously be slow because of it's brute-force, but it's easy to read and understand, which is always a plus! Here are some ways you could improve it.
Don't Try All Combinations
Once you've calculated a factorial, it's pretty easy to add 1 to it and determine if it's a perfect square. I would write a function called is_perfect_square() that looks like this:
bool is_perfect_square(const int x)
{
    double square = static_cast<double>(x);
    double root = sqrt(square);
    return root == std::round(root);
}

Now you can simply do this:
void print_brown_numbers(int limit)
{
    for (unsigned int i = 2; i <= limit; ++i)
    {
        unsigned int fact = factorial(i);
        if (is_perfect_square (fact + 1))
        {
            std::cout << sqrt(fact + 1) << " " << i << " are brown numbers\n";
        }
    }
}

Note that in my version, the limit is the limit of the factorial, not the square, so it works slightly differently.
Error Handling
Most compilers will use 32-bits for an int or an unsigned int these days. Factorials can get very large very quickly. 12! is the largest factorial a 32-bit int or unsigned int can hold. You should check your inputs to make sure you won't get an overflow and let the user enter a different one if it would.
Types
You're mixing signed and unsigned types. While there's nothing wrong with your math, you're leaving 1 bit (so half the range - 2 billion values) on the table for the square. It will never be negative, so you should just make it unsigned to begin with. (Or make the factorial signed. Either way is fine given the maximum values.)


ReactJS Calling an API and rendering results I am currently learning JS Frameworks, right now exploring React and wanted to make sure I keep everything in order, as in: the logic of working with React, code quality, any shortcomings, good/bad practices etc?

The code renders a simple Search panel that will allow to call an API, get the results (if any) and render the results on the page.

import React from "react";
import "./App.css";
import axios from "axios";

var Style = {
  marginRight: "22px"
};

var Style2 = {
  display: "none"
};

const API_CALL = "xxx";

class SearchForm extends React.Component {
  constructor(props) {
    super(props);
    this.state = {
      value: "",
      errorValue: "",
      countryCode: "",
      VATNumber: "",
      valid: "",
      name: "",
      address: "",
      isLoading: false,
      isSubmitted: false
    };

    this.handleChange = this.handleChange.bind(this);
    this.handleSubmit = this.handleSubmit.bind(this);
  }

  renderField() {
    return (
      <form onSubmit={this.handleSubmit}>
        Search VAT:
        <input type="text" onChange={this.handleChange} style={Style} />
        <input type="submit" value="Submit" />
      </form>
    );
  }

  render() {
    return (

Reference: EE100247019

        {this.renderField()}

        <Loader loading={this.state.isLoading} />
        {this.state.isSubmitted && (
          <Result
            country={this.state.countryCode}
            number={this.state.VATNumber}
            name={this.state.name}
            address={this.state.address}
            error={this.state.errorValue}
            valid={this.state.valid}
            loading={this.state.isLoading}
          />
        )}

    );
  }

  handleChange(event) {
    this.setState({ value: event.target.value.trim() });
  }

  handleSubmit(event) {
    this.setState({ isLoading: true, isSubmitted: false });
    Style2 = {
      listStyleType: "none",
      textAlign: "left",
      display: "block",
      border: "1px solid white",
      marginTop: "50px"
    };
    axios
      .get(API_CALL + this.state.value)
      .then(res =>
        this.setState({
          countryCode: res.data.CountryCode,
          VATNumber: res.data.VATNumber,
          name: res.data.Name,
          address: res.data.Address,
          valid: res.data.Valid,
          isLoading: false,
          isSubmitted: true
        })
      )
      .catch(error =>
        this.setState({
          valid: false,
          errorValue: this.state.value,
          isLoading: false,
          isSubmitted: true
        })
      );

    event.preventDefault();
  }
}

function Loader(props) {
  if (!props.loading) {
    return null;
  }
  return
 Loading ...
;
}

function Result(props) {
  if (!props.valid) {
    return (

        Invalid value "{props.error}"

        Please enter valid VAT Number

    );
  } else {
    return (

          Country code:
          {props.country}

          VAT Number:
          {props.number}

          Product name:
          {props.name}

        Address:
        {props.address}

    );
  }
}

export default SearchForm;

Here is the code after refactor some changes that i think should be good to follow:

use let instead of var.
I have used style2 as a state as its changing.
Used arrow function instead of bind.
Prefer destructuring.

import React from "react";
import "./App.css";
import axios from "axios";

let Style = {
  marginRight: "22px"
};

const API_CALL = "xxx";

class SearchForm extends React.Component {
  constructor(props) {
    super(props);
    this.state = {
      value: "",
      errorValue: "",
      countryCode: "",
      VATNumber: "",
      valid: false,
      name: "",
      address: "",
      isLoading: false,
      isSubmitted: false,
      Style2: { display: "none" }
    };
  }

  renderField() {
    return (
      <form onSubmit={this.handleSubmit}>
        Search VAT:
        <input type="text" onChange={this.handleChange} style={Style} />
        <input type="submit" value="Submit" />
      </form>
    );
  }

  render() {
    const {
      Style2 = {},
      countryCode = "",
      VATNumber = "",
      valid = false,
      name = "",
      address = "",
      isLoading = false,
      isSubmitted = false,
      errorValue = ""
    } = this.state;
    return (

Reference: EE100247019

        {this.renderField()}

        <Loader loading={isLoading} />
        {isSubmitted && (
          <Result
            country={countryCode}
            number={VATNumber}
            name={name}
            address={address}
            error={errorValue}
            valid={valid}
            loading={isLoading}
            style={Style2}
          />
        )}

    );
  }

  handleChange = event => this.setState({ value: event.target.value.trim() });

  handleSubmit = event => {
    this.setState(({ Style2 }) => ({
      isLoading: true,
      isSubmitted: false,
      Style2: {
        ...Style2,
        listStyleType: "none",
        textAlign: "left",
        display: "block",
        border: "1px solid white",
        marginTop: "50px"
      }
    }));
    axios
      .get(API_CALL + this.state.value)
      .then(
        ({
          data: {
            CountryCode = "",
            VATNumber = "",
            Name = "",
            Address = "",
            Valid = false
          } = {}
        } = {}) =>
          this.setState({
            countryCode: CountryCode,
            VATNumber: VATNumber,
            name: Name,
            address: Address,
            valid: Valid,
            isLoading: false,
            isSubmitted: true
          })
      )
      .catch(error =>
        this.setState({
          valid: false,
          errorValue: this.state.value,
          isLoading: false,
          isSubmitted: true
        })
      );

    event.preventDefault();
  };
}

const Loader = ({ loading = false } = {}) =>
  loading ? null :
 Loading ...
;

const Result = ({
  valid = false,
  error = "",
  style = {},
  country = "",
  number = "",
  name = "",
  address = ""
} = {}) => {
  if (!valid) {
    return (

        Invalid value "{error}"

        Please enter valid VAT Number

    );
  } else {
    return (

          Country code:
          {country}

          VAT Number:
          {number}

          Product name:
          {name}

        Address:
        {address}

    );
  }
}

export default SearchForm;


Calculating the circumference and area of a circle This simple program uses Scanner to obtain a radius from the user, then calculates and outputs the circumference and area.

Questions:

Is it customary in Java to place a function call inside of a println(), or is it better to put the returned value into a separate variable and then display that?
Should I consider implementing input-validation for practice as this is a toy program?
Am I correct in making the calculation functions static?

import java.util.Scanner;
import java.lang.Math;

public class Circle {

    public static double getCircumference(double radius) {
        return 2 * Math.PI * radius;
    }

    public static double getArea(double radius) {
        return Math.PI * radius * radius;
    }

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

        double radius = sc.nextDouble();

        System.out.println("Circumference: ");
        System.out.println(getCircumference(radius));
        System.out.println("Area: ");
        System.out.println(getArea(radius));

        sc.close();
    }
}

Sample output:

5.5
Circumference:
34.55751918948772
Area:
95.03317777109123

Is it customary in Java to place a function call inside of a println(), or is it better to put the returned value into a separate variable and then display that?

This is something I would say is primarily opinion based. For this little app, I think it's fine to put the function method call inside a println.

When it comes to more complex things, it is usually better to do the calculation separately from the output (or most importantly logging), because you don't want your program behavior to change when you remove a System.out.println or logging statement.

  Should I consider implementing input-validation for practice as this is a toy program?

I don't see any need to use input-validation when you're using nextInt on the Scanner. Perhaps you'd want to avoid a negative size, and either throw an exception or show an error message to the user, but that's probably it.

  Am I correct in making the calculation functions static?

Yes, absolutely. I see no reason for why they should not be static. Given the usefulness of them, it is a good idea to have them public as well (which you have).

Feature-request: You're currently not prompting for any input which might make the user confused. A simple System.out.println("Enter radius:"); would be nice to have.

Overall, your code looks perfectly fine. A minor minor minor nitpick is that you're writing one space more than necessary here:

System.out.println("Circumference: ");
System.out.println("Area: ");

Another minor nitpick is that your Scanner variable could be named either scanner or input instead. (Even though sc is a quite normal variable name for a Scanner) #perfectionist-naming.


Contains method in lambda expression I use this method to extract table name and field name from formula.

public List<string> ExtractFieldsFromFormula(string formula)
{
    List<string> formulas = new List<string>();
    formulas.AddRange(formula.Split(new char[] {'{', '}'}).Where(f=>!f.Contains("+") && !f.Contains("-")&&!f.Contains("/")&&!f.Contains("*")&& !f.Contains("(")&& !f.Contains(")") && f.Trim()!=String.Empty));
    return formulas;
}

my formula is like this:

  "{City.a}+5*(2/{City.b})"

and result of method is a list :

City.a
City.b

how can i write this code better than this?!!

And there's always a place for a regular expression (especially when searching for a pattern in a string of characters ;-)).

using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Text.RegularExpressions;

namespace ConsoleApplication9
{
    class Program
    {
        static void Main(string[] args)
        {
            string pattern = @"\{(?<expr>[a-zA-Z0-9.]+?)\}";
            string formula = @"{City.a}+5*(2/{City.b})";

            var matches = Regex.Matches(formula, pattern).OfType<Match>();
            foreach (var match in matches)
            {
                Console.WriteLine(match.Groups["expr"]);
            }
            // prints
            // City.a
            // City.b

            string splitPattern = @"\{(?<object>[a-zA-Z0-9]+?)\.(?<field>[a-zA-Z0-9]+?)\}";

            matches = Regex.Matches(formula, splitPattern).OfType<Match>();
            foreach (var match in matches)
            {
                Console.WriteLine("Object: {0}   Field: {1}", match.Groups["object"], match.Groups["field"]);
            }

            // prints
            // Object: City   Field: a
            // Object: City   Field: b
        }
    }
}


Addition operators for length units I'm writing some code that helps me deal with units better, by having a user defined type for things like Metres, Centimetres and Millimetres. Here is the class declaration for Metres:

template <typename StorageType>
class Metres : public Unit<StorageType>
{
public:
    static Metres fromMetres (StorageType metres) noexcept;

    Metres operator+ (Metres rhs) const noexcept;
    Metres operator+ (Centimetres<StorageType> rhs) const noexcept;
    Metres operator+ (Millimetres<StorageType> rhs) const noexcept;

protected:
    using Unit<StorageType>::Unit;
};

However, when writing the definitions of the operator+ functions, I find myself repeating Metres<StorageType> quite a lot:

template <typename StorageType>
Metres<StorageType> Metres<StorageType>::fromMetres (StorageType metres) noexcept
{ return { metres }; }

template <typename StorageType>
Metres<StorageType> Metres<StorageType>::operator+ (Millimetres<StorageType> rhs) const noexcept
{ return { this->getRawValue() + rhs.getRawValue() / StorageType (1000) }; }

template <typename StorageType>
Metres<StorageType> Metres<StorageType>::operator+ (Centimetres<StorageType> rhs) const noexcept
{ return { this->getRawValue() + rhs.getRawValue() / StorageType (100) }; }

template <typename StorageType>
Metres<StorageType> Metres<StorageType>::operator+ (Metres<StorageType> rhs) const noexcept
{ return { this->getRawValue() + rhs.getRawValue() }; }

I can't define the functions in the class declaration, as the types Centimetres and Millimetres forward declared. If I define these classes beforehand, I end up with the same problem in a different class.

I feel like this could be done better. How can I reduce the amount of visual noise in these function definitions?

Note:
If there are any rules in C++ that allow the omission of <StorageType> in certain places, quotes from the standard are much appreciated.

The obvious possibility would be to just define the functions inside the class definition:

template <typename StorageType>
class Metres : public Unit<StorageType> {
public:
    static Metres fromMetres(StorageType metres) noexcept { return metres; }

    Metres operator+ (Metres rhs) const noexcept {
        return getRawValue() + rhs.getRawValue();
    }

    Metres operator+ (Centimetres<StorageType> rhs) const noexcept {
        getRawValue() + rhs.getRawValue() / StorageType(100);
    }

    Metres operator+ (Millimetres<StorageType> rhs) const noexcept {
        getRawValue() + rhs.getRawValue() / StorageType(1000);
    }

protected:
    using Unit<StorageType>::Unit;
};

The problem (as I'd see it, anyway) stemmed from two facts: these are templates, so you needed to include the template parameters, and the bodies of the functions in question are so tiny, they basically got lost in the noise.

Both of these point to the fact that (in this case) including the function definitions inline in the class definition gains a great deal, and loses nothing.


To update or to delete? That is the Query I'm currently developing a method which performs an update of some data; much like the following simplified logic:

boolean updateRequired = (currentValue == null && newValue != null);
boolean deleteRequired = (currentValue != null && newValue == null);

if(updateRequired || deleteRequired) {

    // ...

}

Which probably is fine enough by itself. However, I can't loose the feeling that I'm somehow re-inventing the wheel. The above is just simplified, current/new-Value are strings if that matters.

Is there any way to make this more neat?

Depending on your use case, I recommend two options. Both of them are based on a separate function for checking things....

At its simplest, have a function:

private static final boolean needToDoSomething(String currentValue, String newValue) {
    return currentValue == null && newValue != null || currentValue != null && newValue == null;
}

Then you can just have:

if (needToDoSomething(currentValue, newValue)) {
    ....
}

If you need to do something more special than just the 'needs modification' test, for example, you need to distinguish inside the if block between these conditions, then I would recommend an Enum with a static method, for example:

public enum EditState {
    ADDED, DELETED, MODIFIED, UNCHANGED;

    public static getState(String currentValue, String newValue) {
        if (currentValue == newValue) {
            // this covers null == null too
            return UNCHANGED;
        }
        if (currentValue != null && newValue == null) {
            return DELETED;
        }
        if (newValue != null && currentValue == null) {
            return ADDED;
        }
        return currentValue.equals(newValue) ? UNCHANGED : MODIFIED;
    }

    public boolean isStateChanging() {
        return this != UNCHANGED;
    }
}

Then, with this Enum class, you can be more specific:

EditState state = EditState.getState(currentValue, newValue);

switch (state) {
    case UNCHANGED :
        .....
    case DELETED :
        .....
    ....
}


A command line reader buffering the commands in Java Until now, all my command readers expect the entire command on a single line, so that it may be read with the call scanner.nextLine(). This code here caches the input line and after each Enter attempts to build command(s) from the currently stored lines. If no commands may be built, the previous lines remain in the line list, and the entire program expects more input.

Here is what I have:

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

public class CommandLineReader {

    private static final String COMMAND_SEPARATOR = ";";
    private static final String QUIT_COMMAND = "quit";
    private static final String PROMPT = "> ";

    public static void main(String[] args) {
        Scanner scanner = new Scanner(System.in);
        StringBuffer stringBuffer = new StringBuffer();
        List<String> lineList = new ArrayList<>();

        main_loop:
        while (true) {
            System.out.print(PROMPT);
            String inputLine = scanner.nextLine();
            lineList.add(inputLine);
            stringBuffer.delete(0, stringBuffer.length());

            for (String line : lineList) {
                stringBuffer.append(line.trim()).append(' ');
            }

            String bufferContent = stringBuffer.toString().trim();
            String[] bufferCommands = bufferContent.split(COMMAND_SEPARATOR);
            boolean lastCommandTerminated =
                    bufferContent.endsWith(COMMAND_SEPARATOR);

            if (!lastCommandTerminated && bufferCommands.length < 2) {
                continue;
            }

            if (lastCommandTerminated) {
                for (String bufferCommand : bufferCommands) {
                    String command = bufferCommand.trim();
                    System.out.println(command + COMMAND_SEPARATOR);

                    if (command.equals(QUIT_COMMAND)) {
                        break main_loop;
                    }
                }

                lineList.clear();
            } else {
                for (int i = 0; i < bufferCommands.length - 1; ++i) {
                    String command = bufferCommands[i].trim();
                    System.out.println(command + COMMAND_SEPARATOR);

                    if (command.equals(QUIT_COMMAND)) {
                        break main_loop;
                    }
                }

                lineList.clear();
                // Return the last unfinished command:
                lineList.add(bufferCommands[bufferCommands.length - 1]);
            }
        }

        System.out.println("Bye!");
        scanner.close();
    }
}

A possible session might look like this:

> yargh
> aye
> captain; abc; def; hello
yargh aye captain;
abc;
def;
> world
> ; quit;
hello world;
quit;
Bye!

My primary critique concern is usability: Can it be improved?

Here are some suggestions for improving the code:

Use a StringBuilder instead of a StringBuffer. StringBuilder is equivalent to StringBuffer, with the exception that it is not safe for multithreading, which makes it a bit faster. In your program, no other thread can ever have access to stringBuffer, because it is a local variable and you never pass it to a method or constructor, so you don't need synchronization on it.
Declare stringBuffer inside the while loop. It is not needed outside it and reset at the beginning of every loop iteration. So declaring it inside the loop reduces its scope to the smallest extent necessary, which makes the code clearer.
Replace this line:

String bufferContent = stringBuffer.toString().trim();

with this line:

String bufferContent = stringBuffer.deleteCharAt(stringBuffer.length() - 1).toString();

Since for (String line : lineList) will execute at least once (you manually added a line to lineList, so you can be sure it contains at least one element), stringBuffer is guaranteed to end with a single space character and have no whitespace directly preceding it, so the purpose of trim() is essentially to remove this single space character. The first line above, however, creates two String objects, one of which is immediately discarded after the line and only serves as an intermittent String to call trim() on, while the second line only creates one String object, that which is stored in bufferContent. Note that the second line will also modify stringBuffer, while the first line does not. However, since stringBuffer is never accessed again afterwards, this does not matter.

In fact, an even more efficient approach would be to never let the final space character be appended in the first place by modifying the for (String line : lineList) loop to something like this:

for (Iterator<String> lineListIterator = lineList.iterator(); lineListIterator.hasNext(); ) {
    stringBuffer.append(lineListIterator.next().trim());
    if (lineListIterator.hasNext()) {
        stringBuffer.append(' ');
    }
}

Then you can just call:

String bufferContent = stringBuffer.toString();

This might be more verbose than your original loop combined with the subsequent removal of the space character, but it is also more explicit, and thus maybe easier to read despite the increased verbosity.
Replace this construct:

while (true) {
    // ...
    if (!lastCommandTerminated && bufferCommands.length < 2) {
        continue;
    }

    if (lastCommandTerminated) {
        // ...
    } else {
        // ...
    }
}

with this construct:

while (true) {
    // ...
    if (lastCommandTerminated) {
        // ...
    } else if (bufferCommands.length >= 2) {
        // ...
    }
}

That way, you convey the exact same message, but with less code, and also, the while loop's control flow is easier to infer from the code structure inside it alone, because you have one continue statement less.
Exterminate duplicate code in the two if clauses by filtering out the common behavior. The only differences are that, in the second if clause, the last element of bufferCommands is not iterated over but added to lineList after lineList is cleared. A way to represent this logic more concisely might be replacing the two if clauses with one larger if clause:

if (lastCommandTerminated || bufferCommands.length >= 2) {
    for (int i = 0;
            i < bufferCommands.length - (lastCommandTerminated ? 0 : 1);
            ++i) {
        String command = bufferCommands[i].trim();
        System.out.println(command + COMMAND_SEPARATOR);

        if (command.equals(QUIT_COMMAND)) {
            break main_loop;
        }
    }

    lineList.clear();
    if (!lastCommandTerminated) {
        lineList.add(bufferCommands[bufferCommands.length - 1]);
    }
}


Null replacement - Is this LINQ readable? This is the original version of code written by my coworker that replaces every null cells with an empty string.

for (int i = 0; i < dGV_factory.Rows.Count; i++)
{

       this.dGV_factory["dGV_factory_groupID", i].Value
             = this.dGV_factory["dGV_factory_groupID", i].Value ?? "";
       this.dGV_factory["dGV_factory_groupID", i].Value
             = this.dGV_factory["dGV_factory_groupID", i].Value ?? "";
       this.dGV_factory["dGV_factory_groupID", i].Value
             = this.dGV_factory["dGV_factory_groupID", i].Value ?? "";
       this.dGV_factory["UE", i].Value
             = this.dGV_factory["UE", i].Value ?? "";
                ...
}

My instant reaction: Ugh.

Being a shameless fanatic of LINQ, I decided to rewrite the code using LINQ.

foreach(DataGridViewRow row in dGV_factory.Rows)
{
      //Replace every null cells with an empty string
      row.Cells.Cast<DataGridViewCell>().ToList().ForEach(cell => cell.Value = cell.Value ?? "");
}

However, I'm wondering if this code is less readable than the above version, even though it is definitely compact.

In my eyes, it's definitely readable as I'm comfortable with LINQ but my coworkers have not even heard of LINQ.

Which is a better change? Should I keep my LINQ version or unroll LINQ to two foreach loops?

List.ForEach might have its uses, but I wouldn't use it here. It's not "wrong" per se, but a classic foreach loop

is at least as easy to read as your LINQ expression,
does not require the explicit Cast<...>(),
does not require ToList().

If you put it on one line, it is actually shorter than your LINQ-based solution.

foreach(DataGridViewRow row in dGV_factory.Rows)
{
      //Replace every null cells with an empty string
      foreach (DataGridViewCell cell in row.Cells) cell.Value = cell.Value ?? "";
}

Although, for readability, I'd prefer:

foreach (DataGridViewRow row in dGV_factory.Rows)
{
      foreach (DataGridViewCell cell in row.Cells)
      {
          cell.Value = cell.Value ?? "";
      }
}

(Of course, this is still a great improvement over your co-worker's code.)


Skeleton for a text-based RPG I'm new to python and created this small program. It's basically a skeleton for a simple text-based RPG. The program is one menu that allows you to:

Create a Character
View Character Stats/Attributes
Load Character
Battle
Save
Quit

I'm looking for constructive criticism from any angle, be that if I comment too much, if I'm not comment to specific, if my variables are to implicit, or any other flaws in my programming. Perhaps a better way for a menu but this time using tuples or dictionaries. Perhaps you noticed a redundancy in my code.

I'd also appreciate if someone can give me some direction if my goal is to make a top-side RPG. Should I use tkinter? PyGame? Tmxlib? Or should I just focus on the programming aspect for now until I'm a bit more experienced before tackling a library?

# Pickle for object serialization, OS to check if character data file exist
import pickle
import os

class Character(object):
    def __init__(self, name, exp):
        self.name = name
        self.exp = exp
        self.lvl = self.exp/100

    # Simple character statistic display using the new format technique instead of %
    def stats(self):
        print ("Character stats: {}, you are level {} with {} experience points!".format(self.name,self.lvl,self.exp))

    # Simple battle program, has one paramter for the amount of EXP
    # self.lvl is necessary to update the self.lvl variable with the actual level
    def battle(self,exp):
        self.exp = self.exp + exp
        self.lvl = self.exp/100
        print ("Successful battle! {} experienced gained!".format(exp))
        self.stats()

# Not sure what to comment for starting variables? "Global variables?" "Initial variables"?
# sets menu on loop and player sees the main menu
loop_menu = True
menu_choice = "main"

# error prevention
new_character = None
fileCheck = "charData"

# menu_display
# 1 = menu with input
# "new" = input
menu_display = 1

# menu_error
# None = no error yet
# 1 = invalid input received
menu_error = None

while loop_menu == True:
    if menu_display == 1:
        print("New Character, Stats, Load, Battle, Save or Quit?")
        menu_choice = (input("Input your choice: ")).lower()
        menu_display = 0

    if menu_display == "new" and menu_error == None:
        menu_choice = (input("What would you like to do now?: ")).lower()

    if menu_display == "new" and menu_error == 1:
        menu_choice = (input("Please type a real answer, what now?: ")).lower()
        menu_error = None

    # Text Formatting
    print ("\n", end="")

    if menu_choice == "new":
        chardata = ["Foo", 300]
        new_character = Character(*chardata)
        print ("New character created!")
        new_character.stats()

    elif menu_choice == "stats":
        if new_character == None:
            print ("Sorry, you can't check your stats, you haven't made a new/loaded a character yet!")

        else:
            new_character.stats()

    elif menu_choice == "load":
        if os.path.exists(fileCheck):
            print("Load successful!")
            inFile = open("charData", "rb")
            new_character = pickle.load(inFile)
            inFile.close()

        else:
            print("You can't load yet! Data file not created!")
            print("You can use the \"save\" feature after you've created a character to automatically create a save file!")

    elif menu_choice == "battle":
        if new_character == None:
            print("You can't battle, you haven't made a character")

        else:
            new_character.battle(100)

    # Saving character to data file
    elif menu_choice == "save":
        print ("Character saved!")
        outFile = open("charData", "wb")
        pickle.dump(new_character, outFile)
        outFile.close()

    elif menu_choice == "quit":
        print ("Quitting program...")
        break

    else:
        menu_error = 1

    # Text Formatting
    print ("\n", end="")

    # Skips the instructions and goes straight to the input
    menu_display = "new"

Design considerations
There are a few higher level design considerations that will make your life a lot easier if you decide to make a more complicated game or go to a GUI approach.
Input/output
One thing I see is that you don't have a clear separation of the input/output and the game logic. This will make it considerably harder to move to say a GUI approach in the future.
For example:
class Character(object):
    def battle(self,exp):
        self.exp = self.exp + exp
        self.lvl = self.exp/100
        print ("Successful battle! {} experienced gained!".format(exp))
        self.stats()

You have 2 things going on here that really should be separate. You have the battle aspect that updates information about the character and you have some output handling. As it currently stands there is an implicit assumption in parts of your code that the game is a terminal based game because of where the input/output occurs.
These types of situations are much more nicely handled if you split the input/output out from the game logic.
So in this case:
new_character.battle(100)

Might become
old_exp, old_lvl = new_character.exp, new_character.exp
new_character.battle(100)
print ("Successful battle! {} experienced gained!".format(new_character.exp - old_exp))

Now if you move to a GUI approach you just change the print here to something else. This is a lot easier than having to track down the print statements scattered throughout the code.
Lack of functions
There's a noticeable lack of functions here. An especially good example is the menu handler, with the large change of else-if statements:
if menu_choice == "new":
    chardata = ["Foo", 300]
    new_character = Character(*chardata)
    print ("New character created!")
    new_character.stats()

I would start breaking out things like this into functions:
def create_new_character(character_data):
    """Create a new character"""
    new_character = Character(*chardata)
    new_character.stats()
    return new_character

if menu_choice = "new":
    new_character = create_new_character(["Foo", 300])
    print ("New character created!")

Again notice how I am separating out the input/output here from the other functionality. Step 1: create the character then step 2: deal with the output. I would do a similar refactoring with other cases in this chain of else-if's.
Mutable global state
This can turn into a particularly nasty maintenance burden as your codebase grows in size. It makes the flow of the code harder to follow and reason about. This is because having global state that can be change from anywhere makes it much harder to track down which parts of the code can change other parts of the code.
I would strongly recommend creating a class that manages the state of the game which contains the main game loop. This will pay off whenever you have issues you have to debug.
Python usage
In some places you are comparing against None and other "truthiness" values. This is not a recommend practice.
For example
while loop_menu == True:

if menu_display == "new" and menu_error == None:

can be written as:
while loop_menu:

if menu_display == "new" and not menu_error:

I notice you quit the program by breaking out of the main game loop. If there is code after the thread loop you might not immediately quit here. If your intent is to quite immediately you can use something like:
elif menu_choice == "quit":
    print ("Quitting program...")
    import sys
    sys.exit()

This guarantees your code exits here.


User Registration compress to use less querry, CPU posible I want to find out if there are any improvements I can make to this code. How can I make it better, faster and use less queries? Or is it fine the way I wrote it?

<?php if ( $user_ID ) : ?>
<?php
global $user_identity;
?>

Bine ai revenit<?php echo $user_identity; ?> !

<?php
global $current_user;
$current_user = wp_get_current_user();
?>

Utilizatori|Profil|<a href="<?php echo wp_logout_url( get_bloginfo('url') ); ?>" title="Logout">Logout

<?php
$current_user = wp_get_current_user();
if ( ($current_user instanceof WP_User) ) {
    echo get_avatar( $current_user->user_email, 40 );
}
?>

<?php else : ?>

<form name="loginform" id="loginform" action="<?php echo get_option('siteurl'); ?>/wp-login.php" method="post">
<input value="Utilizator" class="input" type="text" size="10" tabindex="10" name="log" id="user_login" onfocus="if (this.value == 'Utilizator') {this.value = '';}" onblur="if (this.value == '') {this.value = 'Utilizator';}" />
<input value="Parola" class="input" type="password" size="10" tabindex="20" name="pwd" id="user_pass" onfocus="if (this.value == 'Parola') {this.value = '';}" onblur="if (this.value == '') {this.value = 'Parola';}" />
<input name="wp-submit" id="wp-submit" value="Login" tabindex="100" type="submit">

Recuperare parolaInregistrare<input name="rememberme" id="rememberme" value="forever" tabindex="90" type="checkbox"> Retine parola?

<input name="redirect_to" value="<?php echo get_option('siteurl'); ?>" type="hidden">
<input name="testcookie" value="1" type="hidden">
</form>

<?php endif; ?>

Ok, a few things of note. Not really related to perfomance, but readability. To be fair though these are just details for better readability,  your code is not so bad.

This code is not expensive process wise, I was curious as to where $User_id comes from, because that is where the query you are worried about comes into play, but I trust your tables are properly indexed and output data is properly escaped.

On line 12-14,  I would put that php code up at top, all in a single php block.
This would separate the php from the html and make for better readability.

Also, same thing for the php block on line 21.  You call once again $current_user = wp_get_current_user(); , but you already have $current_user from line 15.

I would take this block to the top, and change the echo get_avatar() call to :

$avatar = "";
if ( ($current_user instanceof WP_User) ) {
  $avatar =  get_avatar( $current_user->user_email, 40 );
}

By initialising the variable (best practice), in your html "template code" below you can get away with just:

if ($avatar) { print $avatar; }

This habit leaves minimal php in the html and thus separates the logic from presentation.

I assume indentation was lost when you pasted, so I will not patronize you on this.

Hope this helps, happy coding.


Can someone improve this code? public bool CheckPlace(string State, string City)
{
    TestDataContext data = new TestDataContext();
    bool IsKochi = new bool();
    if (!string.IsNullOrEmpty(State) && !string.IsNullOrEmpty(City))
    {
        var Place = (from a in data.Places where a.State == State || a.City == City select a.Place).ToList();
        if (Place != null)
        {
            foreach (var placeName in Place)
            {
                if (placeName.ToString().ToLower() == "kochi")
                {
                    IsKochi = true;
                    break;
                }
                else
                {
                    IsKochi = false;
                }
            }
        }
    }

    return IsKochi;
}

public bool CheckPlace(string State, string City)
{
    if (string.IsNullOrEmpty(State) || string.IsNullOrEmpty(City))
    {
        return false;
    }

    using (var data = new TestDataContext())
    {
        var ret = data.Places.Where(a => (a.State == State || a.City == City)).Select(a => a.Place).ToList().Any(x => x.ToString().ToLower() == "kochi");

        return ret;
    }
}

If the Place have a string property what can be use at the "is kochi" check then use that in the Any() and leave out the .ToList().

Instantiating the TestDataContext() in this method is bad (can not test the method) please inject it as method or class constructor parameter (you can inject a lambda for instantiating it also).

(Syntax errors maybe exists in my code [powered by Notepad].)


Generating a PANDAS DataFrame of simulated coin tosses It's taking my machine quite a long time to execute 1 billion (1st loop x 10, 2nd loop x 1000, 3rd loop x 100,000) instructions. Suggestions for performance enhancements? Sources of potential concern:

global variables slower than local variables?
using for loops instead of something like map?
overhead for class instantiation?
overhead of storing results in a dictionary before making a pandas frame?

from numpy import random
from pandas import DataFrame, concat

class coin(object):

    HEADS = 1
    TAILS = 0
    FLIP_TIMES = 10

    def __init__(self):
        self.frequency = self.flip10()

    def flip10(self):
        choices = [coin.HEADS, coin.TAILS]
        record = []
        for i in xrange(coin.FLIP_TIMES):
            record.append(random.choice(choices))
        return sum(record) / float(coin.FLIP_TIMES)

class trial(object):

    NUM_COINS = 1000
    DEFAULT_VAL = 11.0 / 10.0
    C_RAND_IDX = random.randint(0, NUM_COINS)

    def __init__(self):
        self.results = self.flip1000()

    def flip1000(self):
        v_rand = trial.DEFAULT_VAL
        v_first = trial.DEFAULT_VAL
        v_min = trial.DEFAULT_VAL
        for i in xrange(trial.NUM_COINS):

            # grab frequency
            v_i = coin().frequency

            # hold on to pocket minimum frequency
            if v_i < v_min:
                v_min = v_i

            # get random frequency
            if i == trial.C_RAND_IDX:
                v_rand = v_i

            # get first
            if i == 0:
                v_first = v_i

        # some error checking to make sure they don't
        # still have their default values at this point
        # would be great
        return {'rand': v_rand, 'first': v_first, 'min': v_min}

data = DataFrame([trial().results for i in xrange(100000)])

You are in for a treat, first of all a style review of existing code, followed by a code review of your current code, followed by a code refactoring, and topped of with a performance review of current solutions. All a part of the experience here at Code Review for free! Sit back, and enjoy. :-)

Style review

Actually it seems like you've done some coding before, and so most of the style is quite good. There are however some issues, and they are all related to naming:

Class names is suggested to be CamelCase – And neither coin or trial is good class names. When looking at the function they provide better names could possibly be CoinFlip and RepeatedCoinFlip
Don't use abbreviations in any names – What is it with v_rand or C_RAND_IDX and most other variable names? When you have to guess what the variable is, then it is a bad choice of variable name
Don't let a function name contain the parameter  – It is quite clear that both the flip10 and flip1000 are named after something which should be a parameter. This is not dynamic, and doesn't easily extend. What if you wanted the flip10 to flip it a hundred times?
Bonus point for using xrange in Python 2.7

Code review

There are several code smells related to your code, sadly enough, which affects both readability as well as efficiency of your code. Lets address these:

Letting the __init__ do all the work – When you can't do anything with the class except what is done through the initialisation of the class, you should reconsider whether you are on the right path or not. In most cases this should be written as a simple function. If insisting on using class, you should strip down the initialisation, and let methods do the actual work
Initialisation of C_RAND_IDX should most likely be in __init__ – I'm guessing, but I'm assuming that this should change for every instantation of the trial class. In this case, it should not be a constant, or it should be set within the __init__ method
Hardcoding of parameters – Having methods like flip10 and flip1000 is going against what we have methods for. They should really have a parameter allowing for this number to change. You could have a default parameter, but it should be possible to change it
Calculating coin frequency using arrays and float calculation is waste of memory and calculation power – In your current code there is no reason to store every coin toss in an array when you all you need is the sum of all coin tosses (which represent the tail coin tosses). You could easily sum it directly. (Taking it all the way, you could even do a random.randit(0, coin.FLIP_TIMES) instead of the sum :) )
Simplify coin toss – To rebuild the choices = [coin.HEADS, coin.TAILS] and use random.choice() seems like waisted energy. Why not simply use random.randint(0, 2) which will give the same result?
Why use classes, when methods suffice? – As already touched upon, your scenario doesn't seem to be a good case for classes as they only provide one single return in both cases. This could most likely be better handled in methods directly.
Avoid extensive float calculations, if possible – Aritmethic with floats are more expensive then int arithmetic. Int comparision is also cheaper than float comparison. In other words, why compute the frequency as a float when the integer is just as good. You can always return the frequency as a float in the return statement. Which would reduce from 100 000 times * 1000 * 10 float operations, to 100 000 * 3 float operations. In total that is 333 333 less float division operations (and comparison and so on). That should be noticeable.

Code refactor

Lets give a pure methods implementation (before comparing some of these methods).

import random

def coin_flip(flip_count):
    """Returns how mains tails, 1, out flip_count coin flips"""
    return sum(random.randint(0, 2) for _ in xrange(flip_count))

def fake_coin_flip(flip_count):
    """Returns a random tails count"""
    return random.randint(0, flip_count+1)

def repeated_coin_flip(repetitions=1000,
                       flip_count=10,
                       coin_flip_function=coin_flip):
    """Repeat some frequencies from a large repetition of coin flip sequences.

    Repeat <repetitions> of <flip_count> coin flips. Out of these return three
    frequencies, namely the first, the minimum and a random frequency.
    """

    minimum_count = repetitions + 1
    random_index = random.randint(0, repetitions)

    for i in xrange(repetitions):
        count_of_tails = coin_flip_function(flip_count)
        #print('coin_flip: {}'.format(count_of_tails))
        if count_of_tails < minimum_count:
            minimum_count = count_of_tails

        if i == random_index:
            random_count = count_of_tails

        if i == 0:
            first_count = count_of_tails

    flip_count_as_float = float(flip_count)
    return {'rand': random_count / flip_count_as_float,
            'first': first_count / flip_count_as_float,
            'min': minimum_count / flip_count_as_float }

Performance review

I've timed four solution based on answers so far: Original solution by compguy24, solution by SuperBiasedMan, and two variations of mine solution. In order to time them I've done a few adaptations, and only focus on the build of the data list. Here is the interesting part of the performance setup:

# Helper function to simulate the need of the full data set
# Could print out the first two, and last two elements
def print_some_data(data):
    """Print the two first and two last data elements"""
    for start_element in data[:2]:
        print('    first: {:,.4f},   min: {:,.4f},   rand: {:,.4f}'.format(
              start_element['first'], start_element['min'], start_element['rand']))

    print('    ... skipping loads of items ...')

    for end_element in data[-2:]:
        print('    first: {:,.4f},   min: {:,.4f},   rand: {:,.4f}'.format(
              end_element['first'], end_element['min'], end_element['rand']))

# After renaming classes to Coin & Trial
def compguy24_solution(datasize=1000, print_data = True):
    print('    Generating {} elements'.format(datasize))
    data = [Trial().results for i in xrange(datasize)]
    if print_data:
        print_some_data(data)

def SuperBiasedMan_solution(datasize=1000, print_data=True):
    print('    Generating {} elements'.format(datasize))
    data = [trial() for _ in xrange(datasize)]
    if print_data:
        print_some_data(data)

def holroy_solution(datasize=1000, print_data=True):
    print('    Generating {} elements'.format(datasize))
    data = [repeated_coin_flip(1000, 10) for _ in xrange(datasize)]
    if print_data:
        print_some_data(data)

# Same as previous, but faking the coin_flip :-D
def holroy_solution_v2(datasize=1000, print_data=True):
    print('    Generating {} elements'.format(datasize))
    data = [repeated_coin_flip(1000, 10, fake_coin_flip) for _ in xrange(datasize)]
    if print_data:
        print_some_data(data)

def main():
    test_case = "from {0} import {1}; {1}({2}, False)"

    for test_function in ('compguy24_solution',
                          'SuperBiasedMan_solution',
                          'holroy_solution',
                          'holroy_solution_v2',):

        print ('\nTesting {}'.format(test_function))

        datasize = 1000
        print('    execution time: {:,.4f} seconds'.format(
              timeit.timeit(test_case.format(__name__, test_function, datasize),
                                             number=1)))

if __name__ == '__main__':
   main()

Updated: I did a few test runs to compare the different solutions, before doing a final test run of 100 000 times (which I let run over night). With a 1000 samples I found the following: Original code (58.6 seconds) and the version by SuperBiasedMan (56.5 seconds) runs in around a minute, whilst my version using int comparison and simpler random function runs in only 2.8 seconds (about 20 times faster). ( And if faking the coin flip, it only takes 0.5 seconds to complete it. :-) )

But here are the ultimate test for a 100 000 times run on my computer:

Testing compguy24_solution
    Generating 100000 elements
    execution time: 5,600.1455 seconds

Testing SuperBiasedMan_solution
    Generating 100000 elements
    execution time: 5,565.2747 seconds

Testing holroy_solution
    Generating 100000 elements
    execution time: 277.3780 seconds

Testing holroy_solution_v2
    Generating 100000 elements
    execution time: 43.8776 seconds

That is the original and the solution by SuperBiasedMan took around 1.5 hours to complete, whilst my solution took 4.5 minutes. (The fake coin_flip variant under a minute). I think it is somewhat clear which solution I would prefer! Sorry guys! :-D


Wow that's a big integer! What's its largest prime factor? I see that there are a few other people who have tackled Project Euler Problem #3. I hope you're not all sick of that question yet. I've not taken a look at those yet (purposely), but am about to now.

I feel like I did a pretty good job on this one, but you tell me. How can I further improve this code? I'm particularly interested in how it could be made even faster, but it seems to already be a pretty quick implementation. My first version took over an hour, this one runs almost instantly. I'm also interested in how I could have used a List<T> instead of an array, if you feel that would have been a better data structure.

  The prime factors of 13195 are 5, 7, 13 and 29.

  What is the largest prime factor of the number 600851475143 ?

Numbers.cs - 79 Lines

There are three functions: IsPrime, IsFactor, and PrimeFactors.

namespace Challenges
{
    public class Numbers
    {
        public static Boolean IsPrime(Int64 number)
        {
            if (number < 2)
            {
                return false;
            }

            if (number == 2)
            {
                return true;
            }

            if (number % 2 == 0)
            {
                return false;
            }

            double sqrtOfNumber = Math.Sqrt(number);

            for (var index = 3; index <= sqrtOfNumber; index += 2) //skip even numbers
            {
                if (number % index == 0)
                {
                    return false;
                }
            }

            return true;

        }

        public static bool IsFactor(Int64 dividend, Int64 divisor)
        {
            return (dividend % divisor == 0);
        }

        public static Int64[] PrimeFactors(Int64 number)
        {
            Int64[] results = { };

            Int64 maxDivisor = number / 2;
            int index = 0;
            for (Int64 divisor = 2; divisor < maxDivisor; divisor++ )
            {
                if (IsFactor(number,divisor) && IsPrime(divisor))
                {
                    Array.Resize(ref results, results.Length + 1);
                    results[index] = divisor;
                    index++;

                    Int64 factor = number / divisor;
                    if (IsPrime(factor))
                    {
                        Array.Resize(ref results, results.Length + 1);
                        results[index] = factor;
                        break;
                    }
                    else
                    {
                        Int64[] recursiveResults = PrimeFactors(factor);
                        Array.Resize(ref results, results.Length + recursiveResults.Length);
                        for (int i = 0; i < recursiveResults.Length; i++)
                        {
                            results[index] = recursiveResults[i];
                            index++;
                        }
                        break;
                    }
                }
            }

            return results;
        }
    }
}

Console Program

To prove it works, you can check my answer.

namespace Challenges
{
    class ProjectEuler3Program
    {
        static void Main(string[] args)
        {
            const Int64 verybigint = 600851475143;
            Int64[] factors = Numbers.PrimeFactors(verybigint);

            Console.WriteLine((factors.Max()));

            Console.WriteLine();
            Console.WriteLine("Press enter to close...");
            Console.ReadLine();
        }
    }
}

What a fascinating solution!  It seems that for every smart decision you made, you also threw in a poor decision or two.

Smart decision: In PrimeFactors(), you start testing divisors from small to large, rather than large to small.

Poor decision: You used if (IsFactor(number,divisor) … rather than while (IsFactor(number,divisor)).  Using if instead of while disastrously complicates the program.

Smart decision: In IsPrime(), the index loop skips even numbers.  (Why is the variable named index?)

Poor decision: The divisor loop in PrimeFactors() doesn't skip even numbers.

Smart decision: In IsPrime() the index loop goes up to Math.Sqrt(number).

Poor decision: In PrimeFactors(), the divisor loop goes all the way up to number / 2.

Smart decision: You defined an IsFactor() function for readability, and used it in PrimeFactors().

Poor decision: You didn't use IsFactor() in IsPrime().

Smart decision: In PrimeFactors(), whenever you find a successful divisor, you continue the calculation based on factor = number / divisor rather than number.

Poor decision: You continue to factor factor by recursion instead of looping.  Worse, the way you used recursion requires you to append the results to the results array.

Poor decision: In the innermost if-else of PrimeFactors()…

if (IsPrime(factor))
{
    …
} else {
    Int64[] recursiveResults = PrimeFactors(factor);
    …
}

the call to PrimeFactors() repeats a lot of throwaway work that was done by IsPrime().

Overall, though, the fundamental issue is the if-vs.-while distinction.  Had you chosen while, then you could…

enumerate the successful divisors in nondecreasing order.  Currently, you keep a list of all prime factors and find its maximum.  If you could obtain the divisors in order, then you wouldn't need to bother with keeping a list at all.
eliminate IsPrime() altogether.  Every successful divisor will necessarily be prime since you will have factored out all smaller divisors already.


Making lists symmetrical by merging elements My code is based around the following problem:

http://orac.amt.edu.au/cgi-bin/train/problem.pl?set=aio16int&problemid=903

It's about making a series of plots of land symmetrical by knocking down fences, but out of context of this problem, the code: Takes a list, makes it symmetrical by adding elements together, and prints / writes the number of changes needed to achieve this.

My main concern is speed: the code works fine but it is taking too long to solve cases with long lists. (up to 100,000 elements, where each element is an integer up to 10,000) Any feedback welcomed!

infile = open("farmin.txt", "r")
outfile = open("farmout.txt", "w")
n = int(infile.readline())
plots = infile.readline().split()
infile.close()
plots = [int(i) for i in plots]
fencescut = 0
leftpos = 0
rightpos = 0

def mergeplots (direc, n1, n2):
    global plots
    if direc == "l":
        plots[n1] = plots[n1] + plots.pop(n2)
    else:
        plots[len(plots) - 1 - n1] = plots[len(plots) - 1 - n1] + plots.pop(len(plots) - 1 - n2)

while leftpos <= len(plots)/2 and rightpos <= len(plots)/2:
    if plots[len(plots) - 1 - rightpos] - plots[leftpos] > 0: # left fence needs to be cut
        fencescut += 1
        mergeplots("l",leftpos, leftpos + 1)
    elif plots[len(plots) - 1 - rightpos] - plots[leftpos] < 0: #right fence needs to be cut
        fencescut += 1
        mergeplots('r', rightpos, rightpos + 1)
    else: # no fence needs to be cut
        leftpos += 1
        rightpos += 1
outfile.write(str(fencescut))
outfile.close()

It is better to read input and output files using the with keyword because:

  It is good practice to use the with keyword when dealing with file
  objects. This has the advantage that the file is properly closed after
  its suite finishes, even if an exception is raised on the way. It is
  also much shorter than writing equivalent try-finally blocks:

with open('farmin.txt', 'r') as input:
    # Process the code you need here.


Counting unival subtrees I have written some code to solve the following interview question. Please advise how it can be improved. Thanks in advance.
A unival tree (which stands for "universal value") is a tree where all nodes under it have the same value. Given the root to a binary tree, count the number of unival subtrees.
For example, the following tree has 5 unival subtrees:
    0
   / \
  1   0
     / \
    1   0
   / \
  1   1

Implementation:
#include <stdio.h>
#include <stdlib.h>
#include <stdbool.h>

typedef struct stTree
{
    struct stTree * left;
    struct stTree * right;
    int value;
}

stTree;

stTree* createNode(int value)
{
    stTree *node = malloc(sizeof *node);
    node->left = NULL;
    node->right = NULL;
    node->value = value;

    return node;
}

bool isTreeUniv(stTree *node)
{
    bool flag = true;

    if (!node)
        return false;

    if (node->right && node->right->value != node->value)
    {
        flag = false;
    }

    if (node->left && node->left->value != node->value)
    {
        flag = false;
    }

    return flag;
}

stTree* insertRight(stTree *currNode, int value)
{
    stTree *node = malloc(sizeof *node);

    currNode->right = node;
    node->left = NULL;
    node->right = NULL;
    node->value = value;
    return node;
}

stTree* insertLeft(stTree *currNode, int value)
{
    stTree *node = malloc(sizeof *node);

    currNode->left = node;
    node->left = NULL;
    node->right = NULL;
    node->value = value;
    return node;
}

unsigned int uTreeCount = 0;
void countUnivSubT(stTree *Node)
{
    if (isTreeUniv(Node))
        uTreeCount++;

    if (Node->left)
        countUnivSubT(Node->left);

    if (Node->right)
        countUnivSubT(Node->right);

}

int main(void)
{
    //build a tree
    stTree *rootNode = createNode(0);
    insertLeft(rootNode, 1);
    insertRight(rootNode, 0);

    insertLeft(rootNode->right, 1);
    insertRight(rootNode->right, 0);

    insertLeft(rootNode->right->left, 1);
    insertRight(rootNode->right->left, 1);

    countUnivSubT(rootNode);
    printf("total universal subree: %u\n", uTreeCount);

}

The one red flag is uTreeCount. This should not be a global, and in fact it is easy to rephrase your countUnivSubT to be fully re-entrant: have it return an integer, and do addition within the body, something like
unsigned countUnivSubT(stTree *Node)
{
    unsigned int uTreeCount = isTreeUniv(Node);

    if (Node->left)
        uTreeCount += countUnivSubT(Node->left);

    if (Node->right)
        uTreeCout += countUnivSubT(Node->right);

    return uTreeCount;
}

That said, you have an inner null check, so this can actually reduce to
unsigned countUnivSubT(stTree *Node)
{
    if (!Node) return 0;

    return isTreeUniv(Node)
        + countUnivSubT(Node->left)
        + countUnivSubT(Node->right);
}


Tower of Hanoi (without recursion) I came across an interesting method of solution for the Tower of Hanoi puzzle
and wrote a short version of it as a programming exercise.

The program produces the correct results but I have two questions.

First is there simpler way to write the alternating step of determining the
only valid move which does not involve the smallest disk.

Second when I try to make the two primary routines (move smallest disk and
make alternating move) into functions the handling of variables becomes
unwieldy.

/* tower.c

   Tower of Hanoi -- mechanical solution

   Arrange the three rods to form a triangle.

   Alternate between moving the smallest disk and making the only valid move
   which does not involve the smallest disk.

   The following rule will make sure that the tower of disks end up on the
   destination rod: if the number of disks in the puzzle is an odd number
   always move the smallest disk counter-clockwise around the triangle; if
   the number of disks in the puzzle is an even number always move the
   smallest disk clockwise around the triangle.
*/

#include <stdio.h>
#include <stdbool.h>

int destinationCount (int array[], int numberOfElements)
{
    int count = 0, i;

    for ( i = 1; i <= numberOfElements; ++i )
        if ( array[i] == 3 )
            ++count;

    return count;
}

int main (void)
{
    int numberOfDisks, i, smallestDir, moveCount = 0;
    bool everyOtherMove = false;
    int rodFrom, rodTo, disk;
    int topDisk[4];
    int temp;

    printf ("\nTower of Hanoi puzzle\n");

    printf ("\nnumber of disks? ");
    scanf ("%i", &numberOfDisks);

    int rod[numberOfDisks + 1];

    // all disks start on rod 1

    for ( i = 1; i <= numberOfDisks; ++i )
        rod[i] = 1;

    // set direction to move smallest disk

    if ( (numberOfDisks & 1) == 0 )
        smallestDir = 1;
    else
        smallestDir = -1;

    printf("\nsolution\n\n");

    do {

        ++moveCount;

        if ( ! everyOtherMove ) {

            // move smallest disk

            rodFrom = rod[1];

            rodTo = rodFrom + smallestDir;
            if ( rodTo > 3 )
                rodTo = 1;
            if ( rodTo < 1 )
                rodTo = 3;

            disk = 1;

        }
        else {

            // make only valid move not involving the smallest disk

            // find disk at the top of each rod

            for ( i = 1; i <= 3; ++i )
                topDisk[i] = numberOfDisks + 1;

            for ( i = numberOfDisks; i >= 1; --i )
                topDisk[rod[i]] = i;

            // find which disk to move

            switch ( rod[1] )
            {
                case 1:
                    rodFrom = 2;
                    rodTo = 3;
                    break;
                case 2:
                    rodFrom = 1;
                    rodTo = 3;
                    break;
                case 3:
                    rodFrom = 1;
                    rodTo = 2;
                   break;
                default:
                    printf ("error");
                    break;
            }

            if ( topDisk[rodFrom] > topDisk[rodTo] ) {
                // swap values
                temp = rodFrom;
                rodFrom = rodTo;
                rodTo = temp;
            }

            disk = topDisk[rodFrom];

        }

        printf ("%i: disk %i rod %c to rod %c\n", moveCount, disk,
                 rodFrom + 64, rodTo + 64);

        // move disk

        rod[disk] = rodTo;

        // toggle everyOtherMove

        everyOtherMove = ! everyOtherMove;

    }
    while ( destinationCount (rod, numberOfDisks) != numberOfDisks );

    return 0;
}

Remove the everyOtherMove flag variable

You use it only once to control the logic of your program:

if ( ! everyOtherMove ) {

and this usage may be substitued with a evenness check of the moveCount variable that you would have anyway.

Do not declare vars so much before using them

At line 36 temp is declared as an int, at line 113 temp is used for the first time. How can the reader remember the type of temp 77 lines later? Declare it just before using it.

Loop variables should be declared inside the loop statement as C99 allows it.

Use ternary when it clearly simplifies

if ( (numberOfDisks & 1) == 0 )
    smallestDir = 1;
else
    smallestDir = -1;

Becomes

int smallestDir = (numberOfDisks & 1) == 0 ? 1 : -1

Much shorter and without the smallestDir = repetition.

Using % instead of bitwise operations to check evenness would be a further improvement.

Reduce main (both code and vertical whitespace)

I introduce this helper function:

int wrap_around(int min, int max, int value)
{
    return value < min ? max : (value > max ? min : value);
}

(Please note that it could also be written with if conditionals, I wrote it like this just because of my familiarity with ternary).

The first if branch now is:

    if (moveCount % 2 == 0) {
        // move smallest disk
        rodFrom = rod[1];
        rodTo = wrap_around(1, 3, rodFrom + smallestDir);
        disk = 1;
    }

while before it was:

    if ( ! everyOtherMove ) {

        // move smallest disk

        rodFrom = rod[1];

        rodTo = rodFrom + smallestDir;
        if ( rodTo > 3 )
            rodTo = 1;
        if ( rodTo < 1 )
            rodTo = 3;

        disk = 1;

    }

The same concept is expressed in much less space, and this is a good attribute in my view because:

Some logic is modularized in other functions, the reader gets a more abstract overview.
If all code is compacted this way an overall view of the program becomes possible helping understanding.

Compacting the else clause

    else {

        // make only valid move not involving the smallest disk

        // find disk at the top of each rod

        for ( i = 1; i <= 3; ++i )
            topDisk[i] = numberOfDisks + 1;

        for ( i = numberOfDisks; i >= 1; --i )
            topDisk[rod[i]] = i;

        // find which disk to move

        switch ( rod[1] )
        {
            case 1:
                rodFrom = 2;
                rodTo = 3;
                break;
            case 2:
                rodFrom = 1;
                rodTo = 3;
                break;
            case 3:
                rodFrom = 1;
                rodTo = 2;
               break;
            default:
                printf ("error");
                break;
        }

        if ( topDisk[rodFrom] > topDisk[rodTo] ) {
            // swap values
            temp = rodFrom;
            rodFrom = rodTo;
            rodTo = temp;
        }

        disk = topDisk[rodFrom];

    }

I do not fully understand the uppermost two for loops but both the switch and the body of the if ( topDisk[rodFrom] > topDisk[rodTo] ) statement are performing very clear, specific tasks, so:

    else {
        // make only valid move not involving the smallest disk
        // find disk at the top of each rod
        for ( i = 1; i <= 3; ++i )
            topDisk[i] = numberOfDisks + 1;
        for ( i = numberOfDisks; i >= 1; --i )
            topDisk[rod[i]] = i;

        // find which disk to move
        find_start_and_destination(rod[1], *rodFrom, *rodTo);
        if ( topDisk[rodFrom] > topDisk[rodTo] ) {
            SWAP(rodFrom, rodTo);
        }
        disk = topDisk[rodFrom];

    }

I just removed the unnecessary blanklines (blanklines should separate logically separated blocks of code, not each line / statement), and used a function to incorporate the switch and a macro to swap variables. The function must use pointers because two values may not be returned from a function in C, but I think the modularization is still an advantage.


Small DSL: Converting JSON to MSSQL query Scenario

Once upon a time, there was this human (ahem, me, ahem) who tried (and succeeded) to build a small program which would convert JSON to a MSSQL query using Python 3.x.

How does it work?

The user makes a POST request with a JSON (in the code, I've used a JSON file) which looks like this:

{
  "endpoint": "rfc",
  "expression": [
    {
      "field": "first_name",
      "operator": "EQ",
      "value": "Dixie"
    },
    "AND",
    [
      {
        "field": "last_name",
        "operator": "NEQ",
        "value": "Smith"
      },
      "OR",
      {
        "field": "middle_name",
        "operator": "EQ",
        "value": "Sam"
      }
    ]
  ],
  "limit": "2"
}

Now, the key point in this story is the expression which is being translated to a query, using recursion, as follows:

def process(data):
    """
    :param data: JSON Object (dict).
    :return: where clause (str) built from data
    """
    where_clause = ""
    if isinstance(data, list):
        for part in data:
            if part not in LOGICAL_OPERATORS:
                where_clause += " ({}) ".format(process(part))
            else:
                where_clause += process(part)
    elif isinstance(data, dict):
        where_clause += " {} {} '{}'".format(data["field"], COMPARISON_OPERATORS[data["operator"]], data["value"])
    elif isinstance(data, str):
        return data
    return where_clause

The entire code, is as follows:

import json

LOGICAL_OPERATORS = ("AND", "OR")

COMPARISON_OPERATORS = {
    "LT": "<",
    "GT": ">",
    "LTE": "<=",
    "GTE": ">=",
    "EQ": "=",
    "NEQ": "!="
}

def get_data(file):
    """
    :param file: JSON file.
    :return: data as a json object (dict).
    """
    with open(file) as data_file:
        return json.load(data_file)

def process(data):
    """
    :param data: JSON Object (dict).
    :return: where clause (str) built from data
    """
    where_clause = ""
    if isinstance(data, list):
        for part in data:
            if part not in LOGICAL_OPERATORS:
                where_clause += " ({}) ".format(process(part))
            else:
                where_clause += process(part)
    elif isinstance(data, dict):
        where_clause += " {} {} '{}'".format(data["field"], COMPARISON_OPERATORS[data["operator"]], data["value"])
    elif isinstance(data, str):
        return data
    return where_clause

def main():
    expression = get_data("other.json")["expression"]
    where_clause = process(expression)

    return "SELECT * FROM table WHERE {}".format(where_clause)

if __name__ == '__main__':
    print(main())

Questions:

Will the recursive function scale good on a larger expression? Now, that expression can go in depth as long as the memory goes. Should I add a limit to it?
Did I miss something? Maybe an edge case?
Is there an already builtin in out there for this?

Escaping and SQL injections
One of the major problems in the presented code is that the code is vulnerable to SQL injection attacks and does not properly sanitize, validate and escape the field values.
For example, if a last_name contains a single quote, for example O'Reilly:
{
    "field": "last_name",
    "operator": "NEQ",
    "value": "O'Reilly"
},

The generated query would be:
SELECT * FROM table WHERE  ( first_name = 'Dixie') AND ( ( last_name != 'O'Reilly') OR ( middle_name = 'Sam') )

which is syntactically incorrect query and would raise an error if executed (which may be a sign of not properly sanitized inputs for a potential "bad" user).
Then, we can try:
{
  "endpoint": "rfc",
  "expression": [
    {
      "field": "first_name",
      "operator": "EQ",
      "value": "' or '1' = '1"
    }
  ],
  "limit": "2"
}

which will result into:
SELECT * FROM table WHERE  ( first_name = '' or '1' = '1')

which will match everything in the table.
Possible to solution to the problem
A possible solution to the problem would be to use parameterized queries and named placeholders where names would come from the field parameter (assuming it's unique). Something along these lines (please test thoroughly):
def process(data, parameters={}):
    """
    :param data: JSON Object (dict).
    :param parameters: dict.
    :return: where clause (str) built from data
    """
    where_clause = ""
    if isinstance(data, list):
        for part in data:
            if part not in LOGICAL_OPERATORS:
                where_clause += " ({}) ".format(process(part, parameters))
            else:
                where_clause += process(part, parameters)
    elif isinstance(data, dict):
        where_clause += " {} {} %({})s ".format(data["field"], COMPARISON_OPERATORS[data["operator"]], data["field"])
        parameters[data["field"]] = data["value"]
    elif isinstance(data, str):
        return data
    return where_clause

def main():
    expression = get_data("other.json")["expression"]
    parameters = {}
    where_clause = process(expression, parameters)

    return "SELECT * FROM table WHERE {}".format(where_clause), parameters

Is there an already builtin in out there for this?

I would look into using ORMs like SQLAlchemy to generate queries. This way you would not have to deal with SQL at all, working with Python abstraction layer only.


Grouping objects that have the same attributes A list of objects needs to be grouped.

The list is iterated and checked with other objects in the same list except itself. The method People.isAttributeEqual() checks if two objects can be grouped.

The list returns a list of two unique objects with the same attributes.

I am trying to optimize the method People.groupSimilar(). All other details given here are just snippets.

The code below works but looks clunky and non optimized. Is there a way to optimize and use lambda instead?

public class People
{

  private String name;
  private String age;
  private String address;

  public People(final String name, final String age, final String address)
  {
    this.name = name;
    this.age = age;
    this.address = address;
  }

  public boolean isAttributeEqual(final People dupPeop)
  {
    return this.address.equals(dupPeop.address) && this.age.equals(dupPeop.age) && this.name.equals(dupPeop.name);
  }

  public static void main(final String[] args)
  {

    List<People> asList = Arrays.asList(new People("Sri", "28", "TN"), new People("Sri", "28", "TN"), new People("Sri", "28", "TN"),
        new People("Pri", "28", "TN"));

    List<People> groupSimilar = groupSimilar(asList);

  }

  public static List<People> groupSimilar(final List<People> people)
  {
    List<People> duplicatePeople = new ArrayList<>(people);
    for (Iterator<People> iterator = duplicatePeople.iterator(); iterator.hasNext();)
    {
      People people2 = iterator.next();
      for (People orignalPeople : people)
      {
        if (!orignalPeople.equals(people2) && orignalPeople.isAttributeEqual(people2))
        {
          iterator.remove();
          people2 = iterator.next();
        }
      }
    }
    return duplicatePeople;
  }
}

public class People

The class is named People, but it should probably be called Person. Every instance of the class is another Person, not another People.

private String name;
private String age;
private String address;

It might be worth to consider splitting up the name attribute into a firstName and lastName, because otherwise it could be difficult to compare two peoples' last names, for example.

Representing age as a String is not the best fit. Age is usually a whole number, i.e. an integer, so it would be better to use int instead. Then you could also more easily compare ages by e.g. calculating the age difference between two people. Strings should be used for things that are actual text or at least sequences of characters.

Even better than having an age attribute at all could be to have a dateOfBirth instead, represented by the Java type Date. Age is just an implicit attribute of a person that could change any day, and so instead of saving it in a variable you can calculate it from the date of birth and the current date whenever you need.

I'd say that representing address as a String is fine, but could be worth getting its own class with street, zipCode etc., then saving a reference to an Address inside the Person.

public boolean isAttributeEqual(final People dupPeop)

isAttributeEqual does not tell you which attribute it checks. The singular name implies that one attribute is being checked, but what the method actually does is compare all attributes. There is a convention for that in Java, which is to override the equals method which the class inherits from Object.

List<People> asList = Arrays.asList(new People("Sri", "28", "TN"), new People("Sri", "28", "TN"), new People("Sri", "28", "TN"),
    new People("Pri", "28", "TN"));

List<People> groupSimilar = groupSimilar(asList);

It can be more readable if a variable name states what it contains, independently from how it came to be. asList is the operation that creates the list, but that will not be relevant anymore if you use the variable later on in the code. So, assuming the rename of the class that I recommended earlier, you could have something like this:

List<Person> persons

or

List<Person> people

Similarly, groupSimilar groups similar people, but the result is a list of similar people:

List<Person> similarPeople = groupSimilar(people);

or

List<Person> similarlyGroupedPeople = groupSimilar(people);

Also, it seems to me that what groupSimilar does is to find the people that are duplicated in the same list, rather than actually grouping similar people, so I would rename the variable and the method to this:

List<Person> duplicates = findDuplicates(people);

  public static List<People> groupSimilar(final List<People> people)
  {
    List<People> duplicatePeople = new ArrayList<>(people);
    for (Iterator<People> iterator = duplicatePeople.iterator(); iterator.hasNext();)
    {
      People people2 = iterator.next();
      for (People orignalPeople : people)
      {
        if (!orignalPeople.equals(people2) && orignalPeople.isAttributeEqual(people2))
        {
          iterator.remove();
          people2 = iterator.next();
        }
      }
    }
    return duplicatePeople;
  }

Rather than manually controlling the iterator, i.e. checking hasNext() and advancing it with next(), I would use a simple for-each loop for the outer loop, just like your inner loop already is.

Since you are using the default implementation for equals which is inherited from Object, it is equivalent to a reference equality check, which can be done on reference types using the == operator (!= for reference inequality).

So, assuming that you replaced your isAttributeEqual with an overriding implementation of equals, and use ==/!= instead of the default equals, I would rewrite the method like this:

public static List<People> findDuplicates(final List<People> people) {
    List<People> duplicates = new ArrayList<>();

    for (Person person : people) {
        for (Person person2 : people) {
            if (person != person2 && person.equals(person2) {
                duplicates.add(person);
            }
        }
    }

    return duplicates;
}

The code also becomes, in my view, more easily understandable by only adding the people that are verified as duplicates, rather than declaring all as duplicates and removing those that are not.


Detect cycle in undirected graph Below is my code to detect cycle in an undirected graph. Could anyone please review and let me know if I have taken care of all the cases?

The basic idea is to use DFS (non-recursive using stack) and check if already visited vertices are found in the stack.

bool dfsDetectCycle(int vertex, int visited[10]) {
    stack<int> s;

    s.push(vertex);

    while (!s.empty()) {
        int np_vertex = s.top();
        s.pop();
        // visit while poping and check visited
        if (visited[np_vertex]) {
            // if already visited means there is a cycle
            return true;
        }
        visited[np_vertex] = 1;
        cout << np_vertex << "\t";

        // Graph with Adjacency Matrix
        if (!isAdjList) {
            for (int j = 0; j < v; ++j)
                // below !visited[j] is needed to exclude previously visited vertices as well as to exclude parent node of currently in progress vertex
                if (adjMatrix[np_vertex][j] && !visited[j]) {
                    s.push(j);
                }
        }
        // Graph with Adjacency List
        else {
            vector<int>::iterator it = adjList[np_vertex].begin();
            for (; it != adjList[np_vertex].end(); ++it)
                if (!visited[*it])
                    s.push(*it);
        }
    }
    return false;
}

bool hasCycle() {
    if (empty())
        return false;
    int visited[10] = {0};
    for (int i = 0; i < v; ++i) {
        if (!visited[i]) {
            if (dfsDetectCycle(i, visited))
                return true;
        }
    }
    return false;
}

I think your implementation of the logic is fine. But the variable "isAdjList" should be defined for each of the graph for which you are going to detect cycles . Make sure that you have either an adjacent list or an adjacent matrix for a particular graph and not both.