Powershell Script for setting DHCP reservation description & host value I'm new to Powershell scripting and trying to write a script that will go through printer queues on a print server, get the print queue's port address, then compare the print queues port address to the DHCP reservation associated with the print queue port address on the server. If the port address of the print queue compares, it should then set the description of the DHCP reservation to reflect the print queue name and location. It should then also set the hostname option to be the print queue's name.

#Set DHCP reservation description and host value on server

#DHCP Server for site
$DHCPSvr = "Server1"

#DHCP Server Scope ID
$DHCPSvrScope = "10.224.148.0"

#Get Printer Management list of shared available printers
$PrinterObjects = Get-Printer -ComputerName Server1 * | Select-Object Name, PortName, Shared, Location |
                  Where-Object { ($_.Name -match "^.{6}$") -and ($_.PortName -match "IP*" -or $_.PortName -match '^10*') -and ($_.Shared -EQ $True)} |
                  Sort-Object -Property PortName -Descending

#Get DHCP Reservation list from server
$DHCPSvrReservations = Get-DhcpServerv4Reservation -ComputerName $DHCPSvr -ScopeId $DHCPSvrScope

foreach ($DHCPSvrReservation in $DHCPSvrReservations){
    foreach ($PrinterObject in $PrinterObjects)
       {
        if ($DHCPSvrReservation.IPAddress -eq $PrinterObjects.PortName)
            {
                {Set-DhcpServerv4Reservation -ComputerName $DHCPSvr -IPAddress $PrinterObjects.PortName -Description "$PrinterObjects.Name & $PrinterObjects.Location"}
                {Set-DhcpServerv4OptionValue -ScopeId $DHCPSvrScope -OptionId 12 $PrinterObjects.Name}
            }
    }
}

I'm a bit unsure of the following code within the IF statement, is this the right way to have the loop process the two commands?

{Set-DhcpServerv4Reservation -ComputerName $DHCPSvr -IPAddress $PrinterObjects.PortName -Description "$PrinterObjects.Name & $PrinterObjects.Location"}
{Set-DhcpServerv4OptionValue -ScopeId $DHCPSvrScope -OptionId 12 $PrinterObjects.Name}

Anyway, it would be good if someone could go over what I have at the moment and point out any issues with formatting and so on.

Very impressed with your formatting and spacing. Top notch to see that in practice. I see a couple areas that could be improved and I see at least 2 problems.
Variable Expansion in Strings
Variables will expand in double quotes easy. However you need to be using subexpressions in order to get the property portion to work correctly. You can also use the format operator -f but that is better used for more complicated string.
... -Description "$($PrinterObjects.Name) & $($PrinterObjects.Location)"

If this code is in use already check existing descriptions for dependencies.
Efficient and Reliable conditions

Where-Object { ($_.Name -match "^.{6}$") -and ($_.PortName -match "IP*" -or $_.PortName -match '^10*') -and ($_.Shared -EQ $True)}

The first clause looks like you are just trying to find Name's that are exactly 6 characters long? Just use the length property all strings have instead. No need for regex: $_.Name.Length -eq 6).
In the second clause you are using regex as well and while I doubt it will cause conflicts you need to be aware you would match IP as well as IPPPPPPPPPPP. -like "IP*" or the string method .StartsWith("IP") would work just as well. Note that StartsWith() is case sensitive.
Similarily with $_.PortName -match '^10*' if you are just trying to find ones where the first octet is 10 you will also match ones that are 100 or 1000000 since the greedy quantifier is in use here. Solution is the same as the last one. -like "10.*" or the string method .StartsWith("10."). Notice I added periods. That may or not be an issue for you in production.
Possible trade off of readabilty but for the last clause here (($_.Shared -EQ $True)) you don't need to use -eq. PowerShell will evaluate $_.Shared as its own expression. If it is a non-zero length string or non-zero integer it will evaluate to true.
Curly Braces
You are slightly inconsistent in your foreach blocks. Some have the opening brace after the block and some have it on the following line. I would pick on standard and stick with it. I personally favour the former.
foreach ($DHCPSvrReservation in $DHCPSvrReservations){
    # Do stuff
}

Same goes for your if and other similar blocks and constructs.
Your Set-DhcpServerv4Reservation and Set-DhcpServerv4OptionValue dont need to be surrounded by curly braces. They should function the same without them.
Using if when Where would do
You act on all $printerobject's that have a matching reservation. You loop though is still running through the $printerobject's that don't match. True, you don't do anything with them but you could move that condition into your loop then you can drop the if
foreach ($DHCPSvrReservation in $DHCPSvrReservations){
    foreach ($PrinterObject in ($PrinterObjects | Where-Object{$DHCPSvrReservation.IPAddress -eq $_.PortName})){
            Set-DhcpServerv4Reservation -ComputerName $DHCPSvr -IPAddress $PrinterObjects.PortName -Description "$($PrinterObjects.Name) & $($PrinterObjects.Location)"
            Set-DhcpServerv4OptionValue -ScopeId $DHCPSvrScope -OptionId 12 $PrinterObjects.Name
    }
}

That should work but you should test first.


Writing a particular column of a two-dimension array to a worksheet I had some great feedback on my code previously here and I was hoping that I could get some further help with writing a column of the array to the worksheet.

I have an example here where I write one column of my array to the worksheet with a loop, but I'm wondering if there wouldn't be a more efficient way to do this:

Option Explicit
Option Base 1

Public Sub arrayMatch()

    Dim firstArray As Variant
    With ThisWorkbook.Worksheets(1)
        firstArray = .Range("A1", .Range("A1").End(xlDown).End(xlToRight))
    End With
    ReDim Preserve firstArray(UBound(firstArray, 1), UBound(firstArray, 2) + 1)

    Dim secondArray As Variant
    With ThisWorkbook.Worksheets(2)
        secondArray = .Range("A1", .Range("A1").End(xlDown).End(xlToRight))
    End With

    Dim rowFirstArray As Long
    Dim rowSecondArray As Long
    For rowFirstArray = 1 To UBound(firstArray,1)
        For rowSecondArray = 1 To UBound(secondArray,1)
            If firstArray(rowFirstArray, 3) = secondArray(rowSecondArray, 1) Then
                firstArray(rowFirstArray, 4) = secondArray(rowSecondArray, 2)
                Exit For
            End If
        Next rowSecondArray
    Next rowFirstArray
    Erase secondArray

    For rowFirstArray = 1 To UBound(firstArray, 1)
        ThisWorkbook.Worksheets(1).Cells(rowFirstArray, 4).Value = firstArray(rowFirstArray, 4)
    Next rowFirstArray
    Erase firstArray

End Sub

I wouldn't use

Option Base 1

It's completely unneeded, just change your loops to go from LBound to UBound. Remember, it's always better to stick with defaults otherwise you need to explain why you're doing it. Much easier -

For rowFirstArray = LBound(firstArray, 1) To UBound(firstArray, 1)

What's this for:

Dim previousSetting As Boolean
previousSetting = Application.DisplayAlerts
Application.DisplayAlerts = False
tempWS.Delete
Application.DisplayAlerts = previousSetting

You're turning it off no matter what anyway, there's no reason to store the setting. Why not just put it into an If block instead?

If Application.DisplayAlerts Then
    Application.DisplayAlerts = False
    tempWS.Delete
    Application.DisplayAlerts = True
    Else: tempWS.Delete
End If

I'm not sure what this is doing -

ReDim Preserve firstArray(UBound(firstArray, 1), UBound(firstArray, 2) + 1)

Usually you need to say 1 to UBound(...) for a dimension. But, you can't redimension the first dimension in a non- one dimensional array. So I'm not sure what is supposed to happen here.

Don't often see the Erase method

Erase secondArray

More commonly you set the array to nothing if you need to clean it up, but usually you won't need to clean it up.

Your With blocks aren't really needed

With ThisWorkbook.Worksheets(1)
    firstArray = .Range("A1", .Range("A1").End(xlDown).End(xlToRight))
End With

Can just be

firstArray = Sheet1.Range("A1", Sheet1.Range("A1").End(xlDown).End(xlToRight))

But I think what you mean is

firstArray = Sheet1.Range(Sheet1.Cells(1,1),Sheet1.Cells(lastRow, lastColumn))

What it's currently doing is

Range("A1") to (Rows.Count, 1)

The .End(xlToRight) isn't doing anything excepting the width of column A.

Dim rowFirstArray As Long
Dim rowSecondArray As Long
For rowFirstArray = 1 To UBound(firstArray,1)
    For rowSecondArray = 1 To UBound(secondArray,1)
        If firstArray(rowFirstArray, 3) = secondArray(rowSecondArray, 1) Then
            firstArray(rowFirstArray, 4) = secondArray(rowSecondArray, 2)

Is only needed if your two arrays are different sizes. Are they different sizes?


Opposite access to enums Consider I have many enums (Java) that looks somewhat like this:

@AllArgsConstructor
@Getter
public enum PopularPeriod {
  MONTHLY  ("MON"),
  BIMONTHLY ("BMN"),
  QUARTERLY  ("QTR"),
  SEMI_ANNUALLY  ("SMA"),
  ANNUALLY  ("ANL");

  private final String value;
}

Now, a usual use, I'd think, would be to get the enum object using its value (the string) - there's an input you're parsing and you want to turn the string into some nice objects that make the code more readable.

So I added this to the enum above -

public static PopularPeriod fromString(String value) {
  for (PopularPeriod pp : PopularPeriod.values()) {
    if (pp.getValue().equalsIgnoreCase(value)) return pp;
  }

  return null;
}

(don't mind the returning of null the casing).
This works quite good.
But if you have dozens of those enums - you need to write it for each one.

I tried approaches like adding something like this to a utility class -

public static <T extends Enum<T>> T safeEnumByValue(Class<T> enumType, Object value) { ... }

but there I don't know of the private field (value).

So I though of "implementing" an interface instead (as enum may not extend base classes), and make the search as a default method in the interface.

public interface IEnum<T> {
  T getValue();
}

@AllArgsConstructor
@Getter
public enum PopularPeriod implements IEnum<String> { ... }

and adding a default method in the interface - but that's impossible as I don't have the PopularPeriod.values() there to work with.

In the end, I combined the two approaches.
I have this method in a utility class:

public static <T extends IEnum> T safeEnumByValue(Class<T> enumType, Object value) {
  for (T enumConstant : enumType.getEnumConstants()) {
    if (enumConstant.getValue().equals(value)) return enumConstant;
  }

  return null;
}

Just got there.
It works.
I thought maybe I'd delete this question,but I would love to hear if there's any elegant way to do it.

There may be a more fundamental problem here beyond converting a string to enum.

By adding a fromString(String) into the enum and defining the format specific strings in the enum constructor, you are coupling the enum tightly to the data format and making it an integral part of the input parser. Your enum now has two responsibilities, being clearly in violation of the single responsibility principle

Instead you should implement a format specific converter that converts the string used in transport to the enum used in computing and make the enum as bare as possible.


Generic callback object, but I need the type parameter inside methods Inside my android app, I currently have methods that look like below code. Since they all need a callback object that basically does the same thing and I would like to try to eliminate the duplicated code seen below.

The object that gets posted via the mBus.post(response); statement needs to be the specific type.

@Subscribe
public void onLeave(LeaveRequest event) {
    mRailsApi.leave(event.getEmail(), event.getPassword(),
            new Callback<LeaveResponse>() {
                @Override
                public void failure(RetrofitError arg0) {
                    LeaveResponse response = new LeaveResponse();
                    response.setSuccessful(false);
                    mBus.post(response);
                }

                @Override
                public void success(LeaveResponse leaveResponse,
                        Response arg1) {
                    // need to create one since the api just returns a
                    // header with no body , hence the response is null
                    LeaveResponse response = new LeaveResponse();
                    response.setSuccessful(true);
                    mBus.post(response);
                }
            });
}

@Subscribe
public void onGetUsers(GetUsersRequest event) {
    mRailsApi.getUsers(mApp.getToken(), new Callback<GetUsersResponse>() {
        @Override
        public void failure(RetrofitError arg0) {
            GetUsersResponse response = (GetUsersResponse) arg0.getBody();
            response.setSuccessful(false);
            mBus.post(response);
        }

        @Override
        public void success(GetUsersResponse getUsersResponse, Response arg1) {
            getUsersResponse.setSuccessful(true);
            mBus.post(getUsersResponse);
        }
    });
}

Here is what I have come up with so far.  It seems to work but I am wondering if there is a better solution.  One thing that bothers me is, that I have both the type parameter for the class and I am passing in the class to the constructor.  It seems like I should not need to pass in the same info in two different ways.

The method calls become this:

    @Subscribe
    public void onLeave(LeaveRequest event) {
        System.out.println("inside api repo - making leave request");
        LeaveResponse response = new LeaveResponse();
        mRailsApi.leave(event.getEmail(), event.getPassword(),
                new RailsApiCallback<LeaveResponse>(mBus, LeaveResponse.class ));
    }

And this is the callback class :

public class RailsApiCallback<T extends BaseResponse> implements Callback<T> {

    private Bus mBus;
    private Class mType;

    public RailsApiCallback(Bus bus, Class type) {
        super();
        mBus = bus;
        mType = type;
    }

    @Override
    public void failure(RetrofitError retrofitError) {
        T response = (T) retrofitError.getBody();
        response.setSuccessful(false);
        mBus.post(mType.cast(response));
    }

    @Override
    public void success(T convertedResponse, Response rawResponse) {
        T response = null;
        try {
            response = (T) (convertedResponse != null ? convertedResponse : mType.newInstance());
        } catch (InstantiationException e) {
            e.printStackTrace();
        } catch (IllegalAccessException e) {
            e.printStackTrace();
        }
        response.setSuccessful(true);
        mBus.post(mType.cast(response));
    }
}

One thing that bothers me is, that I have both the type parameter for the class and I am passing in the class to the constructor. It seems like I should not need to pass in the same info in two different ways.

The reason for why you have to do that is because of Type Erasure. Simply put, the generic class is only known during compile-time.

This call is unnecessary in your constructor: super();
All your fields should be marked with final.
Class mType; should be Class<T> mType;
I believe the mType.cast is unnecessary here:

mBus.post(mType.cast(response));

simply mBus.post(response); is enough. The eventbus will automatically detect the class by calling obj.getClass() and invoke the appropriate listeners.

In fact, your response variable is already declared as T response; so I don't see what good casting it will do.


Getting user quota stats I have the following function to collect quota information for the given user and return the information on a "ready to use" array:

/* Get User Quota Information
 *
 * Prepare and array with the user's account disk space information
 * by ascertaining the amount of space available on its quota,
 * the used space from that quota and free space remaining.
 *
 * @param string $user The system user name
 *
 * @return array
 */
function getUserQuotaInfo($user) {

  $quota = exec("quota -s -u ".$user);            // query server

  $quota = preg_replace("/[\s-]+/", ' ', $quota); // clear spaces

  $arr = explode(' ', $quota);

  $freeSpace = str_replace('M', '', $arr[3]) - str_replace('M', '', $arr[2]);

  return array(
    "total" => $arr[3],
    "used"  => $arr[2],
    "free"  => $freeSpace.'M'
  );
}

The function is working well and giving the expected results, but there's a lot going on to manipulate the information in order to have it ready to present to the user.

Is there any optimization that can be done as to reduce the possibilities of failure on this function as well as simplifying it?

I think you should use escapeshellarg to secure that $user does not do damage through your exec call. Also, you should check if your $arr actually has the required data.

As for optimization, replacing multiple spaces and dashes with single spaces and then exploding such strings seems to me slower than just splitting the string right away, using preg_split:

$arr = preg_split("/[\s-]+/", $quota);

However, you should still check that count($arr) > 3.

I must say that I am a bit surprised to see this work for you. That means that the user under which PHP is running has the permission to see other users' quota. IMO, web server should not be allowed as much.

A better way to do this is to dump all users' quotas in some text file (i.e., using repquota command; see an example on how to use it here), via a cron job run as root, and then make the script that only parses that file.

Or, quite similarly, your cron job can make a file with all the data, i.e., create quota-data.php like this:

<?php

$quota_data = array(
  'zuul' => array(
    'total' => ...,
    'used'  => ...,
    'free'  => ...,
  ),
  ...
);

?>

which you then just include in your script and use the data.

Which of these two approaches is better depends on how often the data is read from the web. If rarely, the text file approach is better, so that parsing is done only when really needed; if often, the second approach is better, as it parses the data only once per update.


Pairwise summation in Haskell, take two Here's my second try at pairwise summation in Haskell:
pairwiseSum :: (Num a) => [a] -> a
pairwiseSum [] = 0
pairwiseSum [x] = x
pairwiseSum x = pairwiseSum (sumPairs x)

sumPairs :: (Num a) => [a] -> [a]
sumPairs [] = []
sumPairs [x] = [x]
sumPairs (x:y:xs) = ((x + y) : sumPairs xs)

I ran both versions on pairwiseSum (map exp [1,0.9999..(-56.0)]), and this one's noticeably faster. Is it well written?
ETA: here are some runs comparing pairwiseSum with sum :
*Main> pairwiseSum (map exp [1,0.9999..(-56.0)])
27184.177448160026
*Main> sum (map exp [1,0.9999..(-56.0)])
27184.177448147617
*Main> pairwiseSum (map exp [1,0.99999..(-56.0)])
271829.5419903211
*Main> sum (map exp [1,0.99999..(-56.0)])
271829.54198831413

The previous version of pairwiseSum gave 27184.177448160022 as the sum of the first list. On the second, I counted to 13 while pairwiseSum ran and to 11 while sum ran.

Linking the other version of pairwiseSum here Pairwise summation in Haskell
That version is an O(nlogn) algorithm, while this one is linear [ie O(n)]. In fact this version is probably the best you can do in Haskell with lists. It's reminiscient of a fold, which is the natural operation on a list data structure.
The previous version has a bunch of wasted effort as the lists are recursively split. Each time it's split the algorithm has to traverse the whole array. So it's perfectly reasonable that the first version is slower


MVP win forms application In a module of a win forms application I have a hierarchy of classes like BaseRecovery, LoanRecovery and FineRecovery. Both LoanRecovery and FineRecovery inherits from BaseRecovery. All these models have one view called RecoveryForm where you can enter/view loans and fines of employees. and I'm planing to use single Presenter class called RecoveryPresenter which inherits from BasePresenter. We have a DataService Class for db transactions

LoanRecovery class looks like this...

Class LoanRecovery : BaseRecovery
{
    public string LoanID {get;set;}
    public DateTime IssueDate {get;set;}
    public Decimal Amount {get;set;}
    .
    .
    .
}

So I'd do the following in Programe.cs.

IBaseRcovery recovery=null;
IRecoveryForm recoveryForm = new RecoveryForm();
IDataService dataService = new DataService();

BasePresenter presenter = new RecoveryPresenter( recovery, recoveryForm, dataService );
presenter.Show(); // This will display the form recoveryForm

In the presenter I would say

public RecoveryPresenter( IBaseRecover model, IRecoveryForm view, IDataService dataService )
{
    this._Model = model;
    this._View = view;
    this._DataService = dataService;
    WireUpViewEvents();

}

Now lets say if I need to give a loan, I would run SetModelPropertisFromView() method in the BasePresenter class which use reflection. But before that I should pass an instance of LoanRecovery class (i.e. _Model) to that method together with _View. Like wise we can do same thing for all children classes as follows...

    public void Issue()
    {
        if (_View.Type == "Loan")
        {
            _Model = new LoanRecovery();
            SetModelPropertiesFromView(_Model, _View, _DataService);
            _dataService.InsertLoan(_Model); //Error
        }

        if (_View.Type == "Fine")
        {
            _Model = new FineRecovery();
            SetModelPropertiesFromView(_Model, _View, _DataService);
            _DataService.InsertFine(_Model); //Error
        }

        if (_View.Type == "Insurance")
        {
            _Model = new InsuranceRecovery();
            SetModelPropertiesFromView(_Model, _View, _DataService);
            _DataService.InsertFine(_Model); //Error
        }

    }

Everything is fine until the last line of the above if blocks. The issue is the data access method in the DataService class requires a child instance not a base class instance.

  public void InsertLoan( LoanRecovery loan)
    {
        using (SqlConnection sqlConnection = new SqlConnection(db.GetConnectionString))
        {
            SqlCommand sqlCommand = new SqlCommand("BEGIN INSERT INTO recovery ;
            sqlCommand.Parameters.Add("@empID", SqlDbType.Char).Value = loan.EmployeeID;
            sqlCommand.Parameters.Add("@loanType", SqlDbType.VarChar).Value = loan.Type;
            sqlCommand.Parameters.Add("@loanAmount", SqlDbType.Decimal).Value = loan.FullAmount;
            sqlCommand.Parameters.Add("@loanDuration", SqlDbType.Int).Value = loan.Duration;
            sqlConnection.Open();
            sqlCommand.ExecuteNonQuery();
        }
    }

So I solved the problem like this

    public void Issue()
    {
        if (_View.Type == "Loan")
        {
            LoanRecovery loanModel = new LoanRecovery(); //Creates a child instance
            SetModelPropertiesFromView(loanModel, _View, _DataService);
            _DataService.InsertLoan(loanModel);
        }
    }

Now it's working but my worry is that I'm not using the injected instance through the constructor rather newing a child object which creates a dependency. Can anybody propose a better design to solve this issue please?

There are a number of typos in your code blocks, that lead me to believe the code wasn't taken from the working code in your IDE. I will assume the following:

IBaseRcovery is really IBaseRecovery
RecoveryPresenter( IBaseRecover model, ... is really RecoveryPresenter(IBaseRecovery model, ...
SqlCommand sqlCommand = new SqlCommand("BEGIN INSERT INTO recovery ; is really SqlCommand sqlCommand = new SqlCommand("BEGIN INSERT INTO recovery ...");

That said, the naming feels awkward with an underscore followed by a capital letter:

this._Model = model;
this._View = view;
this._DataService = dataService;

I would rather see this:

_model = model;
_view = view;
_dataService = dataService;

Or that:

this.model = model;
this.view = view;
this.dataService = dataService;

But not a mixture of both.

The raison d'être of the underscore prefix is to avoid having to use the this qualifier; if you prefix a field name with an underscore and use this, you're cluttering up your code.

Also convention is camelCase (i.e. lowercase first letter) for private fields, local variables and parameters. PascalCase (i.e. uppercase first letter) is for type and member names (classes, interfaces, delegates, enums, events, methods, etc.).

You can declare a variable without initializing it. When it's a reference type, it's set to null until you assign it a reference. Hence, this is perfectly redundant:

IBaseRecovery recovery = null;

You could just do this:

IBaseRecovery recovery;

Looks like generics could come in handy here:

public RecoveryPresenter(IBaseRecovery model, IRecoveryForm view, IDataService dataService)

Can turn into a constructor for a generic class:

public class Presenter<TView, TModel>
    where TView : IView
    where TModel : class
{
    private TModel _model;
    private TView _view;
    private IRepository<TModel> _repository;

    public Presenter(TModel model, TView view, IRepository<TModel> repository)
    {
        _model = model;
        _view = view;
        _repository = repository;
    }

    /* ... */
}

If IBaseRecovery is only a marker interface with no members, get rid of it, it smells.

The key here, is IRepository<TModel>: your DataService has too many responsibilities, its interface will keep getting modified as you add more features to your application (i.e. as you need to deal with more database tables), and that's not very cleanly extensible.

The IRepository<TModel> interface could look something like this:

public interface IRepository<TModel> where TModel : class
{
    void SelectAll();
    void GetById(int id);
    void Insert(TModel model);
    void Update(TModel model);
    void Delete(TModel model);
}

Look into the repository pattern for more information. The idea is that you'll have a family of specialized classes, all presenting the same interface, that perform similar operations on different parts of the underlying database:

public class LoanRepository : IRepository<Loan>

public class FineRepository : IRepository<Fine>

...

You can even derive all these classes from an abstract ReposityBase class (non-generic), and implement the common code only once (i.e. connection set-up & disposal, for example).

Now, if your presenter is going to be dealing with more than a single database table / repository, you have a problem; that problem is solved with a unit of work, which the presenter can depend on:

public class Presenter<TView>
    where TView : IView
{
    private IUnifOfWork _uow;
    private TView _view;

    public Presenter(IUnitOfWork uow, TView view)
    {
        _uow = uow;
        _view = view;
    }

    /* ... */
}

Repository and Unit-of-Work patterns usually go hand-in-hand; here is an example of an implementation I've written in VBA recently. It's another language and an earlier data access technology, but you'll find many similarities - look at the pattern, not the code itself.

And/or read up on repository+UoW design patterns; adding these to your arsenal should greatly simplify your life.


Reference type and constructors Code in Java but should be readable also for c#...

Let's assume I have a class with some reference types. Example:

class MyClass {
   private int uniqueId;
   private double doubleValue;
   private MyReferenceType refType1;
   // ...

   public MyClass(int id, double doubleValue, MyReferenceType refType){
      this.id = id;
      this.doubleValue = doubleValue;
      this.refType1 = refType;  // seems dangerous - I want to create new object with the same state instead of adding a reference to an existing one...
   }
   // ...
}

The question is what approach of constructing such an object do you prefer:

Option 1
In constructor:

public MyClass(int id, double doubleValue, MyReferenceType refType){
      // ...
      this.refType1 = new MyReferenceType(refType); // MyReferenceType has some "copy" constructor
}

In code:

MyClass mc = new MyClass(1, 1.0, someRefTypeInstance);

or

Option 2:
In constructor:

public MyClass(int id, double doubleValue, MyReferenceType refType){
      // ...
      this.refType1 = refType;
   }

In code:

MyClass mc = new MyClass(1, 1.0, new MyReferenceType(someRefTypeInstance));

Which approach do you find more reasonable and you use in practice?

Basically I use option 1. The usage of constructor is initialize the properties and creating an object in memory.  Value types sets the default values (int = 0 etc), reference types sets  null. If you want use own default values, you must use own constructor. This is option 1 in your post. In my opinion this is the safest and most logical option.


Adding some config to a juniper device I wrote this script to add some config to a Juniper device.  It works well but the whole user input validation section (yes or no section) seems a little messy.

Is there a better way I could have done it?

from jnpr.junos import Device
from jnpr.junos.utils.config import Config
import getpass

#Grab credentials
username = raw_input("Enter your username:")
password = getpass.getpass("Enter your password:")

#Confirmation of commands
commands = open("commands" , "r")
commands_to_commit = commands.read()
print ""
print "The commands you are about to commit are: "
print commands_to_commit
print ""
print "Do you want to continue to run these commands?"
confirm = raw_input("Y or N: ")
    if confirm in ['n','no','N','NO','No']:
            print "Exiting..."
            quit()
while confirm not in ['y','Y','yes',"Yes",'YES','n','N','NO','No','no']:
    print "Invalid Choice, Try again"
    confirm = raw_input("Y or N: ")
    if confirm in ['n','no','N','NO','No']:
            print "Exiting..."
            quit()
    elif confirm in ['y','Y','yes',"Yes",'YES']:
            continue

#Open a file called swichlist which has a list of devices to modify
with open('switchlist') as infile:
    for host in infile:
            try:
                    print "Working on:", host,
                    #Connect to devices in switchlist
                    dev = Device(host=host.strip(),  user=username, password=password)
                    dev.open()
                    cu = Config(dev)
                    #Looks for a file named commands with the list of commands to run
                    incmd = open('commands')
                    set_cmd = incmd.read()
                    cu.load(set_cmd, format="set")
                    cu.commit()
                    dev.close()
                    print "Completed:", host
            except Exception,e: print "Error:", e
            continue

The confirmation prompt is a self-contained chunk of code with a specific purpose.  It deserves to be extracted into a function:

def confirm(prompt='Y or N: ', yes='YES', no='NO'):
    while True:
        response = raw_input(prompt).upper()
        if yes.startswith(response) and not no.startswith(response):
            return True
        elif no.startswith(response) and not yes.startswith(response):
            return False
        print "Invalid choice. Try again."

(Technically, this is more lenient than your original code, in that it accepts strings like 'yE' as a yes.  I think that's OK.)

Then, in your main program, you can write:

print "Do you want to continue to run these commands?"
if not confirm():
    print "Exiting..."
    quit()

Any call to open() should almost certainly be done using a with block.  You wrote with open('switchlist') as infile, which is good.  However, you also wrote commands = open("commands", "r") and incmd = open('commands'), neither of which you close properly.  But why are you calling incmd = open('commands') at all, when it's just supposed to be commands_to_commit?


Custom error/fatal error and exception handler in PHP Introduction

I wrote three functions that aims to log errors and exceptions as well as show a user a generic error page. Would appreciate some feedback on it with regards to the PSR standards and if I have left out anything of value.

Memory issue:

I am aware that the memory issue has not been addressed in this code, i.e. if you run out of memory the errors will not be logged. However, this isn't an issue I'm looking to solve at the moment.

Code:

<?php
  // General error handling
  error_reporting(-1);
  ini_set('display_errors', 0);
  ini_set('log_errors', 1);

  // Custom error handler
  function errorHandler($type, $str, $file, $line) {
    // Sets path to error log
    $errorLog = "/home/ubuntu/errors.log";
    // Generic error structure
    $msg = $str . ' in ' . $file . ' on line ' . $line;
    // Checks error severity
    switch ($type) {
      // Notices
      case E_NOTICE:
      case E_USER_NOTICE:
      case E_DEPRECATED:
      case E_USER_DEPRECATED:
      case E_STRICT:
        // Logs notices to file
        $notice = 'NOTICE ' . $msg;
        file_put_contents($errorLog, $notice . PHP_EOL, FILE_APPEND);
        break;
      // Warnings
      case E_WARNING:
      case E_USER_WARNING:
        // Logs warnings to file
        $warning = 'WARNING ' . $msg;
        file_put_contents($errorLog, $warning . PHP_EOL, FILE_APPEND);
        break;
      // Unknown errors
      default:
        // Sets header and redirect user to generic error page
        header('HTTP/1.1 500 Internal Server Error', true, 500);
        require_once('500.php');
        // Terminates the script
        exit;
    }
  }

  // Custom exception handler
  function exceptionHandler($e) {
    // Sets path to exception log
    $exceptionLog = "/home/ubuntu/exception.log";
    // Logs exceptions to file
    file_put_contents($exceptionLog, $e->__toString() . PHP_EOL, FILE_APPEND);
    // Sets header and redirect user to generic error page
    header('HTTP/1.1 500 Internal Server Error', true, 500);
    require_once('500.php');
    // Terminates the script
    exit;
  }

  // Logs on shutdown caused by an error
  function fatalErrorHandler() {
    // Gets the last occured error
    $error = error_get_last();
    // Check if error exists
    if (!empty($error) && in_array($error['type'], array(E_ERROR, E_USER_ERROR))) {
      // Sets path to error log
      $fatalErrorLog = "/home/ubuntu/errors.log";
      // Generic fatal error structure
      $fatal = 'FATAL  ' . $error['message'] . ' in ' . $error['file'] . ' on line ' . $error['line'];
      // Logs fatal error to file
      file_put_contents($fatalErrorLog, $fatal . PHP_EOL, FILE_APPEND);
      // Sets header and redirect user to generic error page
      header('HTTP/1.1 500 Internal Server Error', true, 500);
      require_once('500.php');
      // Terminates the script
      exit;
    }
  }

  // Sets custom handler for errors, exceptions and fatal errors
  set_error_handler('errorHandler');
  set_exception_handler('exceptionHandler');
  register_shutdown_function('fatalErrorHandler');
?>

The first thing which comes to my mind when reading your code is how concurrent requests with errors can tamper with the written data. You can aqquire an exclusive lock on the file for the duration of the writing with the flag LOCK_EX (see file_put_contents documentation).

I would also make sure the file 500.php actually exists. You could also avoid using require_once since the script is terminated afterwards and the possibility for another include is non-existent.

I assume the comments are because the code is posted here, since none of them provide extra information your code doesn't provide. Otherwise I think your code look fine. The variables have meaningful names and the indentation is good.

Happy coding!


A program to evenly divide pizzas I wrote a program to evenly divide pizzas.
And I tried to stylize it as a console.
I will be grateful for tips on improving my code.

Here is a sample image of the program after a user has entered values:

$(function(){
 $('.secondQuestion').hide();
 $('#peopleInput').focus();

 //Three functions for outputting the result
 const firstOutput = function(a, b){
  if(b>1){
   $('#firstStr').text(a + " people with " + b + " pizzas")
  } else {
   $('#firstStr').text(a + " people with " + b + " pizza")
  }
 }
 const piece = (a, b) => Math.floor((b * 8) / a);
 const secondOutput = function(a){
  if(a > 1){
   $('#secondStr').text("Each person gets " + a + " pieces of pizza")
  } else {
   $('#secondStr').text("Each person gets " + a + " piece of pizza")
  }
 }
 const leftover = (a, b) => (b * 8) % a;
 const thirdOutput = function(a){
  if(a > 1){
   $('#thirdStr').text("There are " + a + " leftover pieces")
  } else {
   $('#thirdStr').text("There is " + a + " leftover pieces")
  }
 }

 //Action after pressing the enter key on the first line
 $('#peopleInput').bind("enterKey", function(e){
  $('.secondQuestion').show();
  $('#pizzaInput').focus();
 });
 $('#peopleInput').keyup(function(e){
  if(e.keyCode == 13){$(this).trigger("enterKey")}
 });

 //Action after pressing the enter key on the second line
 $('#pizzaInput').bind("enterKey", function(e){
  $('#peopleInput').focus();
  const people = $('#peopleInput').val();
  const pizza = $('#pizzaInput').val();
  firstOutput(people, pizza);
  setTimeout(function(){secondOutput(piece(people, pizza)); }, 300);
  setTimeout(function(){thirdOutput(leftover(people, pizza)); }, 600);
 });
 $('#pizzaInput').keyup(function(e){
  if(e.keyCode == 13){$(this).trigger("enterKey")}
 });
});
body{
 background: #000;
 color: #fff;
 font-family: 'Inconsolata', monospace;
}

h2{
 font-weight: normal;
}

.output{
 margin-top: 10px;
}

p{
 margin: 0 0 5px 0;
}
input {
 border: none;
    background-color: #000;
    width: 50px;
    color: #fff;
    padding-left: 5px;
    margin-bottom: 5px;
}
input:focus{
 outline-color: #000;
}

main{
 position: absolute;
    left: 200px;
}
input::-webkit-outer-spin-button,
input::-webkit-inner-spin-button {
    -webkit-appearance: none;
    margin: 0;
}
<link href="https://fonts.googleapis.com/css?family=Inconsolata:400,700" rel="stylesheet">

Pizza Party >

  How many people?
  <input type="number" class="form-control" id="peopleInput" value="" name="" min="0" oninput="validity.valid||(value='');">

  How many pizzas do you have?
  <input type="number" class="form-control" id="pizzaInput" value="" name="" min="0" oninput="validity.valid||(value='');">

Feedback

I like the style, timing of events, and how the number input spinners are hidden. It really looks like a console application! Plus, pizza is my favorite food!*

Suggestions

Cache DOM references

As was mentioned in answers to your other question (like the retirement calculator), any DOM element lookups that happen multiple time should be reduced to a single lookup that is stored in a variable (or a constant using const). So in your code, it would be wise to cache $('#peopleInput');, $('#pizzaInput'), etc...

$(function(){
    $('.secondQuestion').hide();
    const peopleInput = $('#peopleInput');
    peopleInput.focus();
    const pizzaInput = $('#pizzaInput');

Don't repeat yourself

This principle comes to play here with the following blocks:

$('#peopleInput').keyup(function(e){
    if(e.keyCode == 13){$(this).trigger("enterKey")}
});

and

$('#pizzaInput').keyup(function(e){
    if(e.keyCode == 13){$(this).trigger("enterKey")}
});

These could be combined to a single CSS Selector:

$('#peopleInput, #pizzaInput').keyup(function(e){
    if(e.keyCode == 13){$(this).trigger("enterKey")}
});

Since those are the only input elements on the page, it could be simplified to simply $('input'), but that might not be a good habit to get into, especially if you go on to larger applications.

Naming

Some of the names are misleading. For example: piece is actually a function that returns the number of pieces, so something like pieceDivision or getNumerOfPieces would be more appropriate.

Templating

Like I mentioned in my answer to your question about the retirement calculator, using templates might allow the code that sets the text to be simplified...

Altered Code

With the suggestions above, the code could be modified to be more like this:

$(function() {
  $('.secondQuestion').hide();
  const peopleInput = $('#peopleInput');
  peopleInput.focus(); //$('#peopleInput').focus();
  const pizzaInput = $('#pizzaInput');

  //Three functions for outputting the result
  const firstOutput = function(a, b) {
    if (b > 1) {
      $('#firstStr').text(a + " people with " + b + " pizzas")
    } else {
      $('#firstStr').text(a + " people with " + b + " pizza")
    }
  }
  const getNumberOfPiecesPerPerson = (a, b) => Math.floor((b * 8) / a);
  const secondOutput = function(a) {
    if (a > 1) {
      $('#secondStr').text("Each person gets " + a + " pieces of pizza")
    } else {
      $('#secondStr').text("Each person gets " + a + " piece of pizza")
    }
  }
  const getNumberOfLeftoverPieces = (a, b) => (b * 8) % a;
  const thirdOutput = function(a) {
    if (a > 1) {
      $('#thirdStr').text("There are " + a + " leftover pieces")
    } else {
      $('#thirdStr').text("There is " + a + " leftover pieces")
    }
  }

  //Action after pressing the enter key on the first line
  peopleInput.bind("enterKey", function(e) {
    $('.secondQuestion').show();
    pizzaInput.focus();
  });
  //combine $('#peopleInput') and $('#pizzaInput') - could also be $('#peopleInput, #pizzaInput')
  $('input').keyup(function(e) {
    if (e.keyCode == 13) {
      $(this).trigger("enterKey")
    }
  });

  //Action after pressing the enter key on the second line
  pizzaInput.bind("enterKey", function(e) {
    peopleInput.focus();
    const numPeople = peopleInput.val();
    const numPizzas = pizzaInput.val();
    firstOutput(numPeople, numPizzas);
    setTimeout(function() {
      secondOutput(getNumberOfPiecesPerPerson(numPeople, numPizzas));
    }, 300);
    setTimeout(function() {
      thirdOutput(getNumberOfLeftoverPieces(numPeople, numPizzas));
    }, 600);
  });
});
body {
  background: #000;
  color: #fff;
  font-family: 'Inconsolata', monospace;
}

h2 {
  font-weight: normal;
}

.output {
  margin-top: 10px;
}

p {
  margin: 0 0 5px 0;
}

input {
  border: none;
  background-color: #000;
  width: 50px;
  color: #fff;
  padding-left: 5px;
  margin-bottom: 5px;
}

input:focus {
  outline-color: #000;
}

main {
  position: absolute;
  left: 200px;
}

input::-webkit-outer-spin-button,
input::-webkit-inner-spin-button {
  -webkit-appearance: none;
  margin: 0;
}
<link href="https://fonts.googleapis.com/css?family=Inconsolata:400,700" rel="stylesheet">

Pizza Party >

      How many people?
      <input type="number" class="form-control" id="peopleInput" value="" name="" min="0" oninput="validity.valid||(value='');">

      How many pizzas do you have?
      <input type="number" class="form-control" id="pizzaInput" value="" name="" min="0" oninput="validity.valid||(value='');">

*not that it means the code is worth more, but at least it is interesting!


Scraping web data with Python 3 Below is a portion of the code i have written to scrape the bikesales.com.au website for details of bikes for sales (The full code is here). This finds all the 'href' attributes on each search page and tries to request the html for each href corresponding to each bike for sale. My code works correctly, however I had to add some retry attempts with exponential backoff to avoid the following error:

ConnectionResetError(10054, 'An existing connection was forcibly closed by the remote host', None, 10054, None)

The code works correctly, however I would like to avoid the backoff approach if possible.

from requests import get
from requests.exceptions import RequestException
from contextlib import closing
from bs4 import BeautifulSoup

def get_html_content(url, multiplier=1):
    """
    Retrieve the contents of the url.
    """
    # Be a responisble scraper.
    # The multiplier is used to exponentially increase the delay when there are several attempts at connecting to the url
    time.sleep(2*multiplier)

    # Get the html from the url
    try:
        with closing(get(url)) as resp:
            content_type = resp.headers['Content-Type'].lower()
            if is_good_response(resp):
                return resp.content
            else:
                # Unable to get the url response
                return None

    except RequestException as e:
        print("Error during requests to {0} : {1}".format(url, str(e)))

if __name__ == '__main__':

    baseUrl = 'https://www.bikesales.com.au/'
    url = 'https://www.bikesales.com.au/bikes/?q=Service%3D%5BBikesales%5D'

    content = get_html_content(url)
    html = BeautifulSoup(content, 'html.parser')
    BikeList = html.findAll("a", {"class": "item-link-container"})

    # Cycle through the list of bikes on each search page.
    for bike in BikeList:

        # Get the URL for each bike.
        individualBikeURL = bike.attrs['href']
        BikeContent = get_html_content(baseUrl+individualBikeURL)

        # Reset the miltipler for each new url
        multiplier = 1

        ## occasionally the connection is lost, so try again.
        ## Im not sure why the connection is lost, i might be that the site is trying to guard against scraping software.

        # If initial attempt to connect to the url was unsuccessful, try again with an increasing delay
        while (BikeContent == None):
            # Limit the exponential delay to 16x
            if (multiplier < 16):
                multiplier *= 2
            BikeContent = get_html_content(baseUrl+individualBikeURL,multiplier)

My question is, Is there something that i am missing in the implementation of the request? or, is this just a result of the site denying scraping tools?

I assume is_good_response is just checking for a 200 response code.
Merge is_good_response, get_html_content and the insides of your for-loop in your main together.

This makes the main code:

from requests import get
from bs4 import BeautifulSoup

if __name__ == '__main__':
    baseUrl = 'https://www.bikesales.com.au/'
    url = 'https://www.bikesales.com.au/bikes/?q=Service%3D%5BBikesales%5D'

    content = get_html_content(url)
    html = BeautifulSoup(content, 'html.parser')
    BikeList = html.findAll("a", {"class": "item-link-container"})

    for bike in bike_list:
        individualBikeURL = bike.attrs['href']
        bike_content = get_bike(baseUrl+individualBikeURL)

Where we will be focusing on:

def get_bike(url):
    multiplier = 1
    while (BikeContent == None):
        time.sleep(2*multiplier)
        try:
            with closing(get(url)) as resp:
                content_type = resp.headers['Content-Type'].lower()
                if 200 <= resp.status_code < 300:
                    return resp.content
        except RequestException as e:
            print("Error during requests to {0} : {1}".format(url, str(e)))
        if (multiplier < 16):
            multiplier *= 2
    return None

Allow a retry argument. Retry should also act differently on different values:

None - Don't retry.
-1 - Retry infinatly.
n - Retry until \$2^n\$.
iterator - loop through for the delays

We can also add another function to work the same way your previous code did.
You shouldn't need to use contextlib.closing, as Response.close "should not normally need to be called explicitly."
You don't need content_type in get_bike.
You should use *args and **kwargs so you can use requests.gets arguments if you ever need to.
You can allow this to work with post and other request methods if you take the method as a parameter.

import itertools
import collections.abc

import requests.exceptions

def request(method, retry=None, *args, **kwargs):
    if retry is None:
        retry = iter()
    elif retry == -1:
        retry = (2**i for i in itertools.count())
    elif isinstance(retry, int):
        retry = (2**i for i in range(retry))
    elif isinstance(retry, collections.abc.Iterable):
        pass
    else:
        raise ValueError('Unknown retry {retry}'.format(retry=retry))

    for sleep in itertools.chain([0], retry):
        if sleep:
            time.sleep(sleep)
        try:
            resp = method(*args, **kwargs)
            if 200 <= resp.status_code < 300:
                return resp.content
        except requests.exceptions.RequestException as e:
            print('Error during requests to {0} : {1}'.format(url, str(e)))
    return None

def bike_retrys():
    for i in range(5):
        yield 2**i
    while True:
        yield 16

To improve the rest of the code:

Use snake case.
Constants should be in upper snake case.
Use the above code.
Use import requests, rather than from requests import get.
You can make a little helper function to call request, so usage is cleaner.

import requests
from bs4 import BeautifulSoup

def get_bike(*args, **kwargs):
    return request(requests.get, bike_retrys(), *args, **kwargs)

if __name__ == '__main__':
    BASE_URL = 'https://www.bikesales.com.au/'
    url = 'https://www.bikesales.com.au/bikes/?q=Service%3D%5BBikesales%5D'

    content = get_bike(url)
    html = BeautifulSoup(content, 'html.parser')
    bike_list = html.findAll("a", {"class": "item-link-container"})

    for bike in bike_list:
        bike_content = get_bike(BASE_URL + bike.attrs['href'])


Controls to dump key/value pairs Are there alternative (better) ways to factor out the common parts other than my use of abstract base class?  I find myself following this pattern lately and it makes sense when I code it but sometimes leads to code that is particularly susceptible to obfuscation as time goes by (i.e. I come back to it later and have a hard time remembering how it worked). I am curious to know if it looks good and readable or if maybe it's over-optimized or has other issues.

public abstract class DumpKeyValuePairs : Control
{
    static string XsltPath = "~/Controls/DumpKeys.xslt";
    protected override void OnLoad(EventArgs e)
    {
        this.Controls.Add(new Xml {
            DocumentContent = new XElement("keys",
                                    from key in this.Keys
                                    select new XElement("key",
                                                new XAttribute("name", key),
                                                this[key])).ToString(),
            TransformSource = DumpKeyValuePairs.XsltPath,
        });
        base.OnLoad(e);
    }
    abstract protected IEnumerable<string> Keys { get; }
    abstract protected string this[string index] { get; }
}

public class DumpServerVariables : DumpKeyValuePairs
{
    protected override IEnumerable<string> Keys
    {
        get { return HttpContext.Current.Request.ServerVariables.AllKeys; }
    }

    protected override string this[string index]
    {
        get { return HttpContext.Current.Request.ServerVariables[index]; }
    }
}

public class DumpSessionVariables : DumpKeyValuePairs
{
    protected override string this[string index]
    {
        get { return HttpContext.Current.Session[index].ToString(); }
    }

    protected override IEnumerable<string> Keys
    {
        get { return HttpContext.Current.Session.Cast<string>(); }
    }
}

XSLT:

<?xml version="1.0" encoding="utf-8"?>
<xsl:stylesheet version="1.0" xmlns:xsl="http://www.w3.org/1999/XSL/Transform">
  <xsl:template match="keys">

      <xsl:apply-templates select="key" />

  </xsl:template>
  <xsl:template match="key">

        <xsl:value-of select="@name" />

          <xsl:value-of select="." />

  </xsl:template>
</xsl:stylesheet>

This bit is a little too much to digest:

this.Controls.Add(new Xml {
    DocumentContent = new XElement("keys",
                            from key in this.Keys
                            select new XElement("key",
                                        new XAttribute("name", key),
                                        this[key])).ToString(),
    TransformSource = DumpKeyValuePairs.XsltPath,
});

The trailing comma is annoying, but the LINQ query syntax is a little bit overboard. The instruction is doing too many things.

Wouldn't this be clearer?

Controls.Add(xml);

There's no need to qualify XsltPath with the DumpKeyValuePairs type, even if the field is static - you're in the same type anyway. Actually, this:

static string XsltPath = "~/Controls/DumpKeys.xslt";

Should really be this:

private static readonly string XsltPath = "~/Controls/DumpKeys.xslt";

Now, in order to turn that LINQ jam into a simple Controls.Add(xml); instruction, things need to happen.

var xml = new Xml
{
    DocumentContent = CreateDocumentContent(),
    TransformSource = XsltPath
};

The CreateDocumentContent() method could be where that LINQ query lives.

private string CreateDocumentContent()
{
    return new XElement("keys", Keys.Select(CreateKeyElement)).ToString();
}

And that CreateKeyElement method group, extracted to its own one-liner method again, makes it pretty clear what's going on:

private XElement CreateKeyElement(string key)
{
    return new XElement("key", new XAttribute("name", key), this[key]);
}

Cramming all these functions into a single instruction is making maintenance much harder than it needs to be.

abstract protected string this[string index] { get; }

Maybe it's just me, but in my mind an index is an int that specifies the position of an item in a collection. What you really have here is a string name for accessing the values in some dictionary; correctly naming the indexer parameter can avoid confusion here.


"Nice Angles" - Code Eval challenge Out of fun, I solved "Nice Angles" challenge on CodeEval in C# 4.0:

  CHALLENGE DESCRIPTION:

  Write a program that outputs the value of angle, reducing its
  fractional part to minutes and seconds.

  INPUT SAMPLE:

  The first argument is a path to a file that contains the values of
  angles with their decimal fractions:

330.39991833
0.001
14.64530319
0.25
254.16991217

  OUTPUT SAMPLE:

  Print to stdout values of angles with their fractional parts reduced
  to minutes and seconds.

  The whole and fractional parts are separated by period, minutes are
  separated by apostrophe, seconds by double quotes. The values of
  minutes and seconds are shown as two numbers (with leading zeros if
  needed).

 330.23'59"
 0.00'03"
 14.38'43"
 0.15'00"
 254.10'11"

This is the code I used to solve the problem:

public static void ConvertToMinSec(string path)
        {
            using (StreamReader reader = File.OpenText(path))
            {

                while (!reader.EndOfStream)
                {
                    var degreeValues = reader.ReadLine();
                    if (!string.IsNullOrWhiteSpace(degreeValues))
                    {
                        var totalVal = degreeValues.Split('.');
                        if (totalVal.Length == 1)
                        {
                            var temp = totalVal[0] + ".0";
                            totalVal = temp.Split('.');
                        }
                        var intPart = totalVal[0];
                        var decimalPart =  "0." + totalVal[1];
                        decimal minutes;
                        decimal.TryParse(decimalPart, out minutes);

                        minutes = decimal.Multiply(minutes, 60);
                        var minutePart = (int)minutes;
                        var remainingDecimal = minutes.ToString().Split('.');
                        if (remainingDecimal.Length == 1)
                        {
                            var tempDecimal = remainingDecimal[0] + ".0";
                            remainingDecimal = tempDecimal.Split('.');
                        }

                        decimal seconds;
                        decimal.TryParse("0." + remainingDecimal[1], out seconds);
                        seconds = seconds*60;
                        var secondsPart = (int) seconds;
                        var report = string.Format("{0}"+"."+"{1:00}"+"'"+"{2:00}"+'"', intPart,
                            minutePart, secondsPart);

                        Console.WriteLine(report);
                    }
                }
            }

At the heart of the solution, there should be a function that converts decimal degrees into a d°mm'ss" string.  Assuming that the input is in a reasonable domain (between 0 and 360, for example), the conversion algorithm should be simple and regular.  Above all, it should be purely mathematical at this point — you shouldn't be trying to do any string parsing.

Your format string had lots of unnecessary concatenation.

public static string ToDegMinSec(double dec)
{
    int degrees = (int)dec;
    dec = 60 * (dec - degrees);
    int minutes = (int)dec;
    dec = 60 * (dec - minutes);
    int seconds = (int)dec;
    return string.Format("{0}.{1:00}'{2:00}\"", degrees, minutes, seconds);
}


Ruby parallel assignment I recently wrote the following code:

def freq_progress
    user_flashcards = input_flashcards.includes(:term)
    terms_unicos = user_flashcards.map(&:term).compact.uniq

    freq_1, freq_2, freq_3, freq_4, freq_5 = 0, 0, 0, 0, 0
    terms_unicos.each do |term|
      case term.ranking
      when nil
        next
      when 1..100
        freq_1 += 1
      when 101..300
        freq_2 += 1
      when 301..1000
        freq_3 += 1
      when 1001..2000
        freq_4 += 1
      when 2001..5000
        freq_5 += 1
      else
        next
      end
    end
    {
        f1: freq_1,
        f2: freq_2,
        f3: freq_3,
        f4: freq_4,
        f5: freq_5
    }
  end

In the code review, our technical manager told me that parallel assignment is considered bad practice.

In the Ruby style guide, it says:

  Avoid the use of parallel assignment for defining variables. Parallel assignment is allowed when it is the return of a method call, used with the splat operator, or when used to swap variable assignment. Parallel assignment is less readable than separate assignment.

However, I figure that this would apply mostly to situations where each variable is set to a different value, and that my particular use case (setting all values to 0, the same "boring" value) would potentially be an exception to the readability rule.

In other words, it seems that this would emphasize the "equal" nature of the 5 variables.

This may be a matter of opinion, but since the Ruby style guide recommends against it, I’d love to hear what Rubyists more experienced than I have to say about this.

As well as any opportunities for refactoring!

Thanks, Michael

p.s. this question was originally posted on StackOverflow, but I was told to close the question and move it to Code Review.

Yes, it is generally considered a bad practice because it is considered confusing. Though as you point out it is fairly unlikely to confuse because everything is being set to zeros and the alternative is five lines of repeated code.

However I find myself asking why you don't use an array. i.e.

freqs = [0, 0, 0, 0, 0]
or
freqs = Array.new(5, 0)

or else just define define the final hash:

def freq_progress
    user_flashcards = input_flashcards.includes(:term)
    terms_unicos = user_flashcards.map(&:term).compact.uniq

    result = { f1: 0, f2: 0, f3: 0, f4: 0, f5: 0}
    terms_unicos.each do |term|
      case term.ranking
      when 1..100
        result[:f1] += 1
      when 101..300
        result[:f2]+= 1
      when 301..1000
        result[:f3] += 1
      when 1001..2000
        result[:f4] += 1
      when 2001..5000
        result[:f5] += 1
      end
    end

    result
  end


API factory for a realtime chat site I was told I'm using factories wrong, but I'm not so sure. Any ideas? (I know I should be using GET; I've only just noticed that)

app.factory('apiFactory', ['$http', function($http){
    var apiFactory = {};

    apiFactory.initUser = function(){
        return  $http({
            method: "POST",
            url: "api/new_user.php",
            headers: {"Content-Type": "application/x-www-form-urlencoded"}
        });
    };

    apiFactory.hostOrClient = function(session){
        return $http({
            method: "POST",
            url: "api/sessionhostclient.php",
            data: $.param({"session":session}),
            headers:{"Content-Type": "application/x-www-form-urlencoded"}
        });
    };

    apiFactory.hasPlayerJoined = function(session){
        return $http({
            method: "POST",
            url: "api/sessionstatus.php",
            data: $.param({"session":session}),
            headers: {"Content-Type": "application/x-www-form-urlencoded"}
        });
    };

    apiFactory.sendMessage = function(session, message){
        return $http({
            method: "POST",
            url: "api/addmessage.php",
            data: $.param({"message":message, "session":session}),
            headers: {"Content-Type": "application/x-www-form-urlencoded"}
        });
    };

    apiFactory.checkMessages = function(session, id){
        return $http({
            method: "POST",
            url: "api/getmessages.php",
            data: $.param({"session":session, "id":id}),
            headers: {"Content-Type": "application/x-www-form-urlencoded"}
        });
    };

    apiFactory.getAllMessages = function(session) {
        if(session == null){
            return;
        }

        return $http({
            method:"POST",
            url:"api/getallmessages.php",
            data: $.param({"session":session}),
            headers:{"Content-Type": "application/x-www-form-urlencoded"}
        });
    } ;

    apiFactory.getMessagesFrom = function(session, id){
        return $http({
            method: "POST",
            url: "api/getmessages.php",
            data: $.param({"session":session, "id":id}),
            headers: {"Content-Type": "application/x-www-form-urlencoded"}
        });
    };

    return apiFactory;
}]);

James Heald said:

  I was told I'm using factories wrong, but I'm not so sure.

The people telling you that are technically correct. A "factory" is an object or method whose sole purpose for existence is to create other objects. Factories are generally used when you want to create a concrete sub type, but return either an interface or a common parent type.

From Wikipedia: Factory (object-oriented programming)

  In class-based programming, a factory is an abstraction of a constructor of a class, while in prototype-based programming a factory is an abstraction of a prototype object. A constructor is concrete in that it creates objects as instances of a single class, and by a specified process (class instantiation), while a factory can create objects by instantiating various classes, or by using other allocation schemes such as an object pool. A prototype object is concrete in that it is used to create objects by being cloned, while a factory can create objects by cloning various prototypes, or by other allocation schemes.

Your apiFactory object is technically returning "new" objects, but since it is making web service calls, you've actually created a "service object".

More info: AngularJS Services.

Really the problem is the name "apiFactory" because the apiFactory variable itself is not a factory. It is a service. A more appropriate name might be ChatService.

The function that returns apiFactory is actually the factory, whereas apiFactory is actually the service.


Cast an object to Decimal? or Int? etc but leave it as null if it is null In a case like this:

object A1 = null;
Decimal? B = Convert.ToDecimal(A1);
object A2 = null;
int? C = Convert.ToInt32(A2);

Note that in case where A1 or A2 have values, they appear to be of type double.

B and C will be 0 instead of null. A1 actually comes from Excel via the interop and (Decimal?)A1 throws an error: Specified cast is not valid. yet Convert.ToDecimal(A1) works. The problem is that if A1 is null, I want B to also be null

So at the moment I'm solving it like this:

Func<object, Decimal?> convertOrNullDecimal = x => x == null ? (Decimal?)null : Convert.ToDecimal(x);
Func<object, int?> convertOrNullInt32 = x => x == null ? (int?)null : Convert.ToInt32(x);

B = convertOrNullDecimal(A1);
C = convertOrNullDecimal(A2);

Is this a reasonable way to go? Or can I refactor that into a single function? It's possible I'll have to add other numeric data types later on...

I tried it like this:

public static Decimal? convertOrNull(object x)
{
    return x == null ? (Decimal?)null : Convert.ToDecimal(x);
}
public static int? convertOrNull(object x)
{
    return x == null ? (int?)null : Convert.ToInt32(x);
}

but then the calls become ambiguous. I guess you can't overload based on output variable type...

Also note that if I try B = A1 as Decimal?, then if A1 had a numerical value, B is still null because it looks like Excel passed numeric cells as doubles and empty cells as null

If you know that the object is always either double or null, then that's exactly what the double? type can represent and you can convert to that using a cast:

object obj = ...;
double? doubleValue = (double?)obj;

And once you have double?, you can cast it to decimal? and it will work both for the case when the value is null and when isn't:

decimal? decimalValue = (decimal?)doubleValue;

Or, on a single line:

decimal? decimalValue = (decimal?)(double?)obj;

Apart from being clear, this approach is also likely going to be pretty efficient.


Very simple login script review I'm relatively new to PHP and have recently created a small website for friends and family to upload and share photos, protected by a very simple login system.

While I'm aware what I'm doing isn't best practice and you'd never use this in a "published" site, I'd like to use this as learning experience to improve my coding, and would appreciate any pointers as to best practice and efficiency/security improvements.

The website contains no sensitive information and the login was purely to prevent unwanted uploading of files. The username isn't checked against anything and is only used to log who's uploaded a file. The password is shared for all users.

This script, login.php, is included at the top of each page and checks if the user is logged in - if not then redirect to homepage where the login form is displayed. If already logged in, then display some internal links instead of form.

if(!isset($_SESSION)){ session_start(); }

/***********************************************************/
//check if home page
if (/*code to check if outside root directory*/) }{$notHomePage = true;}

if($notHomePage and ($_SESSION['allow'] == false or !isset($_SESSION['allow'])) ) {
    $_SESSION['error'] = 'Please enter your name and password!';
    header('Location: index.php');
    exit();
   }

/***********************************************************/
function readCookie() {
    if (isset($_COOKIE["username"])) {
        $name = $_COOKIE["username"];
    }
    return $name;
}

/***********************************************************/
function loginForm() {
    if($_SESSION['allow']) {
        //display internal links and logout link
        echo "
Logout
";
    }
    else {
        if(isset($_SESSION['error'])) {
            echo "
" . $_SESSION['error'] . "
";
            unset($_SESSION['error']);
        }

        $name = readCookie();
        echo "<form method=\"post\" action=\"login.php\">
            <input type=\"text\" value=\"$name\" name=\"name\">Name
            <input type=\"password\" name=\"password\">Password
            <button type=\"submit\" name=\"submit\">Login</button>
        </form>";
    }
}

/***********************************************************/
function login() {
    $days = 30;
    if(trim($_POST['name']) == "" || trim($_POST['password']) == "") {
        $_SESSION['error'] = 'Please enter your name and password!';
        header('Location: index.php');
        exit();
    }

    $name = $_POST['name'];
    $password = $_POST['password'];

    $pass = "password"; //passsword goes here

    if ($password != $pass) {
        $_SESSION['error'] = 'Please enter the correct password!';
        header('Location: index.php');
        exit();
    }
    else {
        $_SESSION['allow'] = true;
        header('Location: index.php');
    }

    if($days) $expire = time()+60*60*24*$days;
    setcookie("username", $name, $expire,"/");
}
/***********************************************************/
if($_SERVER['REQUEST_METHOD'] == "POST") {
   login();
}
if($_SERVER['QUERY_STRING'] == "logout") {
    $_SESSION['allow'] = false;
    header("Location: index.php");
    exit;
}

?>

I'd like to think security as a thing that at first refuses everything, and after that permits something specific. Your code does the opposite at the moment. It checks that if the user is NOT logged in, and if the condition does not pass, the user is permitted to view the page.

I'd do the opposite. I would first check that if the user IS logged in, and only after that permit the user to view the page if the condition pass. It is more strict when it comes to potential bugs, which means that if something goes wrong, the user will not be permitted anyhow.

Second thing, you really do not want to tell the browser the user name in the cookie you set. Instead you should hash it with some secret salt if it really is necessary to send it to the browser. Remember, every detail you send to the browser is a potential security risk that someone (man-in-the-middle attack, viruses, other malware) might abuse.

And then, if that is a php script you include at the top of every page, why do you include the login form there? It seems to me that you only display it when you are on index.php. It is not needed elsewhere. Of course this is not a big issue, but it's the tiny little seed of bloated software that will grow in time if not treated carefully. And by this I mean that if you begin to add lots and lots of thing that is being used in only one place, it will make the script run so much slower.

Also, I see a potential bug there. If you ever wanted to add a POST form (say, a guestbook or chat or shoutbox or your own forum) on your website, you will trigger the login() function every time when ANY of those POST forms get submitted. The login() function will not then find the correct data in the POST variable, and will refuse to display the page.


Listing blog posts, each of which may belong to multiple categories I have 3 tables like this:

CREATE TABLE IF NOT EXISTS blog_post (
  id int(11) NOT NULL AUTO_INCREMENT PRIMARY KEY,
  title varchar(100) NOT NULL,
  image varchar(250),
  content TEXT NOT NULL,
  created DATETIME NOT NULL DEFAULT CURRENT_TIMESTAMP,
  edited DATETIME ON UPDATE CURRENT_TIMESTAMP
) ENGINE=InnoDB DEFAULT CHARSET=utf8 DEFAULT COLLATE utf8_general_ci;

CREATE TABLE IF NOT EXISTS blog_category (
  id int(11) NOT NULL AUTO_INCREMENT PRIMARY KEY,
  name varchar(100) NOT NULL UNIQUE,
  icon varchar(250) NOT NULL
) ENGINE=InnoDB DEFAULT CHARSET=utf8 DEFAULT COLLATE utf8_general_ci;

CREATE TABLE IF NOT EXISTS blog_post_blog_category (
  blog_post_id int(11) NOT NULL,
  blog_category_id int(11) NOT NULL,
  CONSTRAINT blog_post_blog_category_pkey PRIMARY KEY (blog_post_id, blog_category_id),
  CONSTRAINT blog_post_blog_category_fkey FOREIGN KEY (blog_post_id) REFERENCES blog_post (id)
    ON DELETE CASCADE
    ON UPDATE CASCADE,
  CONSTRAINT blog_category_blog_post_fkey FOREIGN KEY (blog_category_id) REFERENCES blog_category (id)
    ON DELETE CASCADE
    ON UPDATE CASCADE
) ENGINE=INNODB CHARACTER SET ascii COLLATE ascii_general_ci;

So basically one blog post can be part of many categories and one category can be part of many posts, pretty straight forward, my question is what is best way to get list of all posts with their categories. As result I'm expecting array of posts and I want every post to have array of categories like this.

{
  "4": {
    "id": "4",
    "title": "first post",
    "image": "https://image.flaticon.com/icons/svg/102/102833.svg",
    "content": "this should be some content",
    "created": "2017-09-24 16:21:28",
    "edited": null,
    "categories": [{
      "id": 1,
      "icon": 'www.url.com',
      "name": 'category name'
    }, {
      "id": 5,
      "icon": 'www.url1.com',
      "name": 'category6 name'
    }]
  },
  "6": {
    "id": "6",
    "title": "second post",
    "image": "https://image.flaticon.com/icons/svg/102/102833.svg",
    "content": "this is some text from post",
    "created": "2017-09-24 16:29:53",
    "edited": null,
    "categories": [{
      "id": 5,
      "icon": 'www.url.com',
      "name": 'category name'
    }, {
      "id": 3,
      "icon": 'www.url1.com',
      "name": 'category6 name'
    }]
  }
}

This is how i did it, is this correct and good solution i feel like i'm missing something and i'm pretty sure this could be done way better?
Also i would like to get this list with correct indexes starting from 0 and not array with IDs like i have in my solution.

$app->get('/blog/posts', function (Request $request, Response $response, array $args) {
    $QUERY = 'SELECT bp.id, bp.title, bc.name FROM blog_post bp
              LEFT JOIN blog_post_blog_category bpbc ON bpbc.blog_post_id = bp.id
              LEFT JOIN blog_category bc ON bc.id = bpbc.blog_category_id ORDER BY bp.id';

    $result = R::getAll( $QUERY );
    $resultMap = array();

    foreach ($result as $value) {
        if(empty($resultMap[$value['id']])) {
            unset($value['name']);
            $resultMap[$value['id']] = $value;
            $resultMap[$value['id']]['categories'] = array();
        }
    }

    foreach ($result as $value) {
        $category = array();
        $category['name'] = $value['name'];
        array_push($resultMap[$value['id']]['categories'], $category);
    }

    echo json_encode($resultMap);
});

I am not sure why you are using 2 foreach loops, you should be able to combine them.

foreach ($result as $value) {
    if(empty($resultMap[$value['id']])) {
        unset( $value['name'] );
        $resultMap[ $value['id'] ] = $value;
        $resultMap[$value['id']]['categories'] = array();
    }

    $resultMap[$value['id']]['categories'][] = array(
        'name' => $value['name'],
    );
}


Beginner Tic Tac Toe Game for Python I am a beginner programmer and would like some feedback on this Tic Tac Toe game I made on Python. Any feedback would be appreciated, as I really want to get better at programming and pick up good habits. Here is my code. Something I would add is try statements for invalid user input, but this is not important as I would like feedback on how to optimise this code. I would also love feedback on the check_if_win() function as I believe there might be a better way of doing this. Thank you!

"""Tic Tac Toe"""

__author__ = "Enrique Gonzalez Aretos"

EMPTY = " "
PLAYER_1 = "X"
PLAYER_2 = "O"

# Create Board
def create_board():
    """
    Function which creates an empty game board
    :return: <list<list<str>>> Returns the game board
    """
    return [
        [EMPTY, EMPTY, EMPTY],
        [EMPTY, EMPTY, EMPTY],
        [EMPTY, EMPTY, EMPTY]
    ]
# Show board
def show_board(board):
    """
    Function which prints the board for the user
    :param board: <list<list<str>>> The Tic Tac Toe board
    :return: <str> A clean-looking version of the board
    """
    for x in board:
        # Find better way to print this
        print("|".join(x))

# Place X on spot
def place_player(board, active_player, row, column):
    """
    Function which places the player into the board
    :param board:
    :param active_player:
    :param row:
    :param column:
    :return:
    """
    if board[row-1][column-1] == EMPTY:
        board[row-1][column-1] = active_player

# Check if someone has won
def check_if_win(board, active_player):
    """
    Function which checks if the active player has won
    :param board: <list<list<str>>> Tic Tac Toe Board
    :param active_player: <str> The current active player
    :return: <bool> Return True or False
    """
    # Making lists which will store the diagonals and columns of baord
    diagonal1 = []
    diagonal2 = []
    column1 = []
    column2 = []
    column3 = []
    # Diagonal counter is used to index the different values in each row
    diagonal_counter = 1
    for row in board:
        # When each row is checked, the different indexes are organised
        # into the column and diagonal lists
        column1.append(row[0])
        column2.append(row[1])
        column3.append(row[2])
        diagonal1.append(row[diagonal_counter-1])
        diagonal2.append(row[-diagonal_counter])
        diagonal_counter += 1
        if "".join(row) == active_player * 3:
            return True

    # If statement which checks if any list is full of the active
    # player's symbol
    if "".join(column1) == active_player * 3 or "".join(column2) ==
    active_player * 3 or "".join(column3) == active_player * 3:
        return True
    elif "".join(diagonal1) == active_player * 3 or "".join(diagonal2)
    == active_player * 3:
        return True
    else:
        return False

def swap_player(active_player):
    if active_player == PLAYER_1:
        return PLAYER_2
    else:
        return PLAYER_1

def main():
    # Creates board and assigns starting player
    board = create_board()
    active_player = PLAYER_1
    while True:
        show_board(board)
        # Ask for player input
        row = int(input("\nIt is {}'s turn play. Choose a row (1-3):
              ".format(active_player)))
        column = int(input("Choose a column: "))
        place_player(board, active_player, row, column)
        # Checks if player has won
        if check_if_win(board, active_player) is True:
            show_board(board)
            print("\n{} won!".format(active_player))
            break
        active_player = swap_player(active_player)

if __name__ == "__main__":
    main()

create_board()

You explicitly return a hard-coded board, using 5 lines of code.  This could be written as one line, using list multiplication and list comprehension:

return [ [EMPTY]*3 for _ in range(3) ]

[EMPTY]*3 creates a list of 3 EMPTY values: [EMPTY, EMPTY, EMPTY], and the outer [ ... for _ in range(3) ] repeats that operation 3 times, producing the required board.  (As pointed out in the comments, using list multiplication for the outer loop doesn’t produce 3 unique rows, but 3 references to the same row.)

show_board()

This function does not return anything, so certainly does not :return: <str> A clean-looking version of the board

place_player()

Despite the comment above the function, this method can be used to place an "O" on a spot on the board.

Suggestion: indicate somehow if the move is invalid (beyond row 1..3 or column 1..3, or if the spot is not empty, such as by raising an exception or returning a success/failure code.

check_if_win()

This function is using active_player * 3 in a total of six places.  Instead of repeating this expression six times, you could assign the result to a local variable, and test against that variable instead:

win_pattern = active_player * 3
# ...
for row in board:
   # ...
   if "".join(row) == win_pattern:
      return True
# ... etc ...

But a winning pattern isn't actually a string; it is a list of cells each containing the active_player symbol: ['X', 'X', 'X'].  You've just used "".join(...) to convert the list into a string.  Why create the string when you can just check for equality with the list?

win_pattern = [ active_player ] * 3
# ...
for row in board:
   # ...
   if row == win_pattern:
       return True

if column1 == win_pattern or column2 == win_pattern or column3 == win_pattern:
    return True
elif diagonal1 == win_pattern or diagonal2 == win_pattern:
    return True
else:
    return False

But this code is still quite verbose.  There is a pythonic way of testing if any item in a list of items matches a condition.  It uses the any(iterable) call; if any of the iterable items is True, the resulting any(...) expression evaluates to True.  Using this to test for any of the rows of the board matching the win_pattern is straight forward:

if any(row == win_pattern for row in board):
    return True

Your for row in board: loop also constructs the column1, column2, column3, diagonal1 and diagonal2 lists.  Whenever you have a set of variables with the same prefix name and numeric suffixes, you aught to ask yourself if you could use a list (for example, column[0], column[1], and column[2]) or a loop.  Let's start by making a single column in a loop:

column = []
for row in board:
    column.append(row[col])

If col was 0, this would make your column1 variable from your code.  List comprehension can reduce this to one line:

column = [ row[col] for row in board ]

We actually only need the column variable for just the test against win_pattern, so we could actually eliminate it and perform the test directly:

if [ row[col] for row in board ] == win_pattern:
   return True

Doing that in a loop, for all 3 col values completes the test for a win in any column:

for col in range(3):
    if [ row[col] for row in board ] == win_pattern:
       return True

But above, we replaced a for x in y: and an inner if condition: with an any(...) statement; we can do the same thing here!

if any( [ row[col] for row in board ] == win_pattern for col in range(3)):
    return True

We can also do this for the forward diagonal:

if [ board[i][i] for i in range(3) ] == win_pattern:
    return True

And the reverse diagonal:

if [ board[i][2-i] for i in range(3) ] == win_pattern:
    return True

This looks a lot shorter, and simpler:

win_pattern = [ active_player ] * 3

if any(row == win_pattern for row in board):
    return True

if any( [ row[col] for row in board ] == win_pattern for col in range(3)):
    return True

if [ board[i][i] for i in range(3) ] == win_pattern:
    return True

if [ board[i][2-i] for i in range(3) ] == win_pattern:
    return True

return False

... but I don't quite like it yet.  We can get rid of the win_pattern list, and all the [ ... for x in y ] list creation, using something similar to any(...): the all(...) function.  It returns True when all elements are True.  So instead of

if [ row[col] for row in board ] == [ active_player ] * 3:

which creates a list of 3 items on the left-hand-side and creates a list of 3 items on the right hand side and tests if the lists are the same, we'll test if the first element on the left-hand-side is the same as the first element on the right-hand-side, and ditto for the second elements, and ditto for the third elements.  Of course, all 3 elements on the right-hand-side are the same element.  So the all(...) statement will look like:

if all(row[col] == active_player for row in board):

Again, that is testing one particular column index: col.  If we want to check if any column has the winning pattern, we'll wrap this in an any call:

if any( all(row[col] == active_player for row in board) for col in range(3)):
    return True

So, changing the implementation slightly, to exploit the duality nature of rows and columns, you might write this method as:

def check_if_win(board, active_player):

    if any( all(board[r][c] == active_player for c in range(3)) for r in range(3)):
        return True

    if any( all(board[r][c] == active_player for r in range(3)) for c in range(3)):
        return True

    if all(board[i][i] == active_player for i in range(3)):
        return True

    if all(board[i][2-i] == active_player for i in range(3)):
        return True

    return False

main()

if check_if_win(board, active_player) is True:

could be written without the is True portion:

if check_if_win(board, active_player):

Asking for user input, and converting it to an integer int(input(...)) can lead to ValueError if the user enter bad values, such as "fish".  You should check for and catch exceptions.  Moreover, you should range-check the input.  A user could enter 42 -1 for the row/column.  Finally, the user could enter a valid, legal board position which is already used.  If the user enters bad values (either not valid integers, or out-of-bound values, or already taken locations), the program should complain and ask the user to re-enter the row column input.

Bug: Finally, the game could end in a Tie, which is currently not handled.


Search for solutions to quartic diophantine equation I wrote this code to find the variables that will satisfy the diophantine equation (i.e. a^4 + b^4 + c^4 = d^4), for all variables belong to: (0 , 430000), and it takes forever to find the answer (I left the code running for two hours and got nothing). I know that this is caused by the algorithm or the approach I took, but I really couldn't think of any better solution...

here's the code:

#include <iostream>
using namespace std;
void main()
{
    //Here I use double because max value for int variables is too small for what I need.
    double n = 0, LHS, RHS, valA, valB, valC, valD;
    double *arrA, *arrB, *arrC, *arrD;
    arrA = new double[430000]; //array to store all the powers of 4 for var A
    arrB = new double[430000]; //array to store all the powers of 4 for var B
    arrC = new double[430000]; //array to store all the powers of 4 for var C
    arrD = new double[430000]; //array to store all the powers of 4 for var D
    for (int count = 0; count < 430000; count++)
    {
        arrA[count] = n*n*n*n;
        arrB[count] = n*n*n*n;
        arrC[count] = n*n*n*n;
        arrD[count] = n*n*n*n;
        n++;
    }
    //this loop is a counter, it increments "b" by 1 when "a" reaches max value, and so on for each variable...
    /*
    N.B:
    The variables that satisfy the equation should be the following:
    a = 95800
    b = 217519
    c = 414560
    d = 422481
    Plug it in as initializers for the variables to check that the program is working fine.
    */
    int a = 0, b = 0, c = 0, d = 0;
    while (d < 430000)
    {
        if (a == 430000)
        {
            a = 0;
            b++;
        }
        if (b == 430000)
        {
            a = 0;
            b = 0;
            c++;
        }
        if (c == 430000)
        {
            a = 0;
            b = 0;
            c = 0;
            d++;
        }
        if (d == 430000)
        {
            break;
        }
        LHS = arrA[a] + arrB[b] + arrC[c];
        RHS = arrD[d];
        if (LHS == RHS && (arrA[a] != arrB[b] != arrC[c] != arrD[d]))
        {
            valA = a; valB = b; valC = c; valD = d;
            break;
        }
        a++;
    }

    if (d == 430000){
        cout << "Couldn't find an answer! \n" }
    else
    {
        cout << "value of A is:  " << valA << endl;
        cout << "value of B is:  " << valB << endl;
        cout << "value of C is:  " << valC << endl;
        cout << "value of D is:  " << valD << endl;
    }
}

If anyone could tell me how to speed up the code or could think of a better algorithm. There's nothing wrong (IMO) in the code's syntax or logic, but it is very very slow...

N.B: here's a link to an article regarding the equation for further reading:
https://www.jstor.org/stable/2008781?seq=1#page_scan_tab_contents

I'm using a 2013 macbook pro "running windows 10", (intel i5 2.4GHz, 8GB RAM), MS Visual Studio 2017...

This looks like a bug:

if (LHS == RHS && (arrA[a] != arrB[b] != arrC[c] != arrD[d]))

The operator != returns a boolean result. Its Associativity is also left to write. so the above is equivalent to:

if (LHS == RHS && (((arrA[a] != arrB[b]) != arrC[c]) != arrD[d]))

So:

arrA[a]         != arrB[b]     =>  true or false
(true or false) != arrC[c]     =>  true or false  (so usually true)
true            != arrD[d]     =>  true or false  (so usually true)

LHS == RHS && true             =>  Looking for LHS == RHS

This is a bad way of writing a nested loop:

while (d < 430000)
{
    if (a == 430000)
    {
        a = 0;
        b++;
    }
    if (b == 430000)
    {
        a = 0;
        b = 0;
        c++;
    }
    if (c == 430000)
    {
        a = 0;
        b = 0;
        c = 0;
        d++;
    }
    if (d == 430000)
    {
        break;
    }
    LHS = arrA[a] + arrB[b] + arrC[c];
    RHS = arrD[d];
    // stuff
}

Just write:

for(int a = 0; a < 430000; ++a) {
    for(int b = 0; b < 430000; ++b) {
        for(int c = 0; c < 430000; ++c) {
            for(int d = 0; d < 430000; ++d) {
                LHS = arrA[a] + arrB[b] + arrC[c];
                RHS = arrD[d];
                // stuff
            }
        }
    }
}

There is no point looping over all the values.

if a^4 + b^4 + c^4 > 430000^4 then there is no point in running through the d loop as you will never find the value.

There is no point in checking all values of d. If you must use a search arrD then do a binary search of the array. This will vastly reduce the number of items checked in the innermost loop. Alternatively just find the cube root of the result using a maths function.

 double root = std::pow(LHS, 1.0/4.0);
 double base = std::floor(root);
 if (floor * floor * floor * floor == LHS) {
     break;
 }

I am not convinced that calculating and saving all the powered values is going to save time. Also  you don't need to save the same result four times. Rather than have arrA and arrB and arrC and arrD have a single array arr. It is the index into the array that is important.

Also don't manually allocate this space with new use a vector<double>.

The question is a memory lookup faster than or slower than doing 4 multiplications (or callling std::pow(x, 4)). That is something you will need to time and find out.


Counting the occurrences of bank account numbers for SPOJ challenge I am solving a problem on SPOJ and am currently unsatisfied with my submission time, given that my fastest time is 0.28s (which ranks me 123/5037) and the leading submission is 0.04s.

The problem is pretty simple: Given a list of bank account numbers, print the bank account number along with the frequency at which it is repeated.

  Input

  t [the number of tests <= 5]
  n [the number of accounts<= 100 000]

  [list of accounts]
  [empty line]
  [next test cases]

  Output

  [sorted list of accounts with the number of repeated accounts]
  [empty line]
  [other results]

Example:

Input:
2
6
03 10103538 2222 1233 6160 0142
03 10103538 2222 1233 6160 0141
30 10103538 2222 1233 6160 0141
30 10103538 2222 1233 6160 0142
30 10103538 2222 1233 6160 0141
30 10103538 2222 1233 6160 0142

5
30 10103538 2222 1233 6160 0144
30 10103538 2222 1233 6160 0142
30 10103538 2222 1233 6160 0145
30 10103538 2222 1233 6160 0146
30 10103538 2222 1233 6160 0143

Output:
03 10103538 2222 1233 6160 0141 1
03 10103538 2222 1233 6160 0142 1
30 10103538 2222 1233 6160 0141 2
30 10103538 2222 1233 6160 0142 2

30 10103538 2222 1233 6160 0142 1
30 10103538 2222 1233 6160 0143 1
30 10103538 2222 1233 6160 0144 1
30 10103538 2222 1233 6160 0145 1
30 10103538 2222 1233 6160 0146 1

In order to solve this, I am using a map where the key is the bank account and the value is the frequency of repetition. However, I am first loading the bank accounts into a vector, and I am pretty sure this is one area where I am losing efficiency. I have tried to avoid using the vector, but my efficiency is greatly reduced.

What are some things that I can do to either improve my algorithm, or perhaps increase my I/O speed?

#include <iostream>
#include <map>
#include <string>
#include <vector>

int main() {
  std::ios_base::sync_with_stdio(false);
  std::cin.tie(NULL);
  int t;
  std::cin >> t;
  while (t--) {
    int n, i;
    std::cin >> n;
    std::cin.ignore();
    std::vector<std::string> accounts(n);
    for (i = 0; i < n; ++i) {
      std::getline(std::cin, accounts[i]);
    }
    std::cout << "\n";
    std::map<std::string, int> count;
    for (i = 0; i < accounts.size(); ++i) {
      auto map_it(count.find(accounts[i]));
      if (map_it != count.end()) {
        map_it->second++;
      } else {
        count[accounts[i]] = 1;
      }
    }
    for (auto it = count.begin(); it != count.end(); ++it) {
      std::cout << it->first << it->second << "\n";
    }
    if (t != 0) {
      std::string blank;
      std::getline(std::cin, blank);
    }
  }
  return 0;
}

One area of inefficiency is here:

for (i = 0; i < n; ++i) {
  std::getline(std::cin, accounts[i]);
}
std::cout << "\n";
std::map<std::string, int> count;
for (i = 0; i < accounts.size(); ++i) {
  auto map_it(count.find(accounts[i]));
  if (map_it != count.end()) {
    map_it->second++;
  } else {
    count[accounts[i]] = 1;
  }
}

You're reading each account number, storing them then going back through the data to get the counts.  The map class allows for a key that doesn't exist to create an entry on the fly.  Using this will eliminate the second loop:

std::string temp = "";
std::map<std::string, int> count;
for (i = 0; i < n; ++i) {
  std::getline(std::cin, temp);
  count[temp]++;
}
std::cout << "\n";


Creating a new item or updating an existing item on page load When the page loads, it checks to see if the user is trying to update an existing book or create a new book.

Something about my code just doesn't seem right.  It feels cumbersome and not very logical. Could someone put me out of my misery and let me know that I wrote this bit ok?

Public Property modID As Int32

Protected Sub Page_Load(ByVal sender As Object, ByVal e As System.EventArgs) Handles Me.Load
Dim bookID = Request.QueryString("bookID")

If Not String.IsNullOrWhiteSpace(bookID) Then
        newBook = False
    Else
        newBook = True
        Create()   'create a new book
        bookID = modID.ToString
    End If

    If Not newBook Then
        commitData()   'saves data on form
    End If

    If Not IsPostBack And Not newBook Then
        Page.DataBind()
        populateControls()    'populate my dropdown lists and radio/checkbox controls
        populateDatasets()    'load datasets to be used later
    End If

End Sub

Public Sub Create()
    'create a new book
    newBook = False
    Using dbcon As New SqlConnection(DBUtilities.ConnectionStringDefault)
        dbcon.Open()
        Dim cmd As New SqlCommand(CreateSQL, dbcon)
        cmd.Parameters.Add(New SqlParameter("@authorID", currentUser))
        modID = DirectCast(cmd.ExecuteScalar(), Int32)
    End Using
    navmod = NavbookFactory.Getbook(CInt(modID))
    populateNewBook()
End Sub

The only real issue I can see (it's been a while since I worked in VB, so there may be other things) is using a global value to store the ID of the created record. Make your Create() sub a function and return the ID instead of storing it in the property; that will remove a potential source of errors.

Other than that, naming is always important. Create doesn't say much, maybe name it CreateNewBook. Also, what do navmod or modID mean? Generally be more explicit in your naming, it will help others (and you as well a bit later on) understand your code.

This may not exactly compile, as my VB days are a bit in the past as mentioned, but I think you'll get the idea:

Protected Sub Page_Load(ByVal sender As Object, ByVal e As System.EventArgs) Handles Me.Load
    Dim bookID = Request.QueryString("bookID")

    If Not String.IsNullOrWhiteSpace(bookID) Then
        newBook = False
    Else
        newBook = True
        bookID = CreateNewBook()   'create a new book
    End If

    If Not newBook Then
        commitData()   'saves data on form
    End If

    If Not IsPostBack And Not newBook Then
        Page.DataBind()
        populateControls()    'populate my dropdown lists and radio/checkbox controls
        populateDatasets()    'load datasets to be used later
    End If
End Sub

Public Function CreateNewBook() As String
    'create a new book
    Using dbcon As New SqlConnection(DBUtilities.ConnectionStringDefault)
        dbcon.Open()
        Dim cmd As New SqlCommand(CreateSQL, dbcon)
        cmd.Parameters.Add(New SqlParameter("@authorID", currentUser))
        modID = DirectCast(cmd.ExecuteScalar(), Int32)
    End Using
    navmod = NavbookFactory.Getbook(CInt(modID))
    populateNewBook()

    CreateNewBook = modID.ToString()
End Function

Now that I think of it, the modID property I just eradicated may have been used by other methods of yours. If at all possible, try to always pass those values around to any methods that need them. You want to have as little "global" state as possible, because that increases the risk of side effects - and such bugs are often pretty hard to track down and fix.

Also, if you need to work in VB (if you don't, my advice would be to consider switching to C#, as it's a much clearer and stricter language, you can often get much better help because many advanced programmers shun VB, and most example code nowadays will also be in C#), always call parameterless methods (such as ToString()) with parentheses anyway - method calls that don't look like method calls are always a pain, and it gets worse if that code should ever have to transition to another language.


Simple spinlock for C using ASM This is my second attempt to make simple lock using extended assembly.
See https://stackoverflow.com/questions/37241553/locks-around-memory-manipulation-via-inline-assembly/37246263

The code:

static inline void atomic_open(volatile int *gate)
{
    asm volatile (
        "jmp check\n"
        "wait:\n"                      // Set spawning point.
        "pause\n"                      // Stroke your beard. (If you have one.)
        "check:\n"
        "cmp %[open], %[gate]\n"       // Check if gate is open.
        "jne wait\n"                   // If it isn't open, wait. If it is open, go through the gate.
        "lock xadd %[lock], %[gate]\n" // In case you where not alone entering the gate, at least try to be the first one to put the lock.
        "cmp %[lock], %[open]\n"       // Check to see you where the first one who put the lock.
        // "If not, consider thug life..."
        "jne wait\n"                   // If you didn't win, respawn and try again later.
        : [gate] "=m" (*gate)
        : [lock] "r" (1), [open] "r" (0)
    );
}

static inline void atomic_close(volatile int *gate)
{
    asm volatile (
        "pause\n"
        "lock xchg %[lock], %[gate]\n"
        : [gate] "=m" (*gate)
        : [lock] "r" (0)
    );
}

// Usage, example.
volatile int atomic_gate_memory = 0;
void *mymalloc(size_t size)
{
    atomic_open(&atomic_gate_memory);
    void *ptr = malloc(size);
    atomic_close(&atomic_gate_memory);
    return ptr;
}

The question: Will atomic_[open/close] make mymalloc both threadsafe and reentrant?

If no, what is wrong?
If yes, it is still wrong; isn't it?... Give me a good rant about what to consider, what is missing or about better approach. If you want to suggest libraries, please restrict your self to C. I am not experienced enough to bind C++ stuff to other languages, so I often can't use the good stuff over there :'(

Op's code has a possible race condition between the lock and compare.

"lock xadd %[lock], %[gate]\n"
"cmp %[lock], %[open]\n"

OP identified: lock is not certainly made a shared variable.